Re: [bug report] mm/gup: remove vmas parameter from get_user_pages_remote()

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Wed, May 17, 2023 at 12:54:00PM +0100, Lorenzo Stoakes wrote:
> On Wed, May 17, 2023 at 02:45:22PM +0300, Dan Carpenter wrote:
> > Hello Lorenzo Stoakes,
> >
> > The patch eca1a00155df: "mm/gup: remove vmas parameter from
> > get_user_pages_remote()" from May 14, 2023, leads to the following
> > Smatch static checker warning:
> >
> > 	mm/memory.c:5617 __access_remote_vm()
> > 	error: uninitialized symbol 'vma'.
> >
> > mm/memory.c
> >     5590 int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> >     5591                        int len, unsigned int gup_flags)
> >     5592 {
> >     5593         void *old_buf = buf;
> >     5594         int write = gup_flags & FOLL_WRITE;
> >     5595
> >     5596         if (mmap_read_lock_killable(mm))
> >     5597                 return 0;
> >     5598
> >     5599         /* ignore errors, just check how much was successfully transferred */
> >     5600         while (len) {
> >     5601                 int bytes, offset;
> >     5602                 void *maddr;
> >     5603                 struct vm_area_struct *vma;
> >     5604                 struct page *page = get_user_page_vma_remote(mm, addr,
> >     5605                                                              gup_flags, &vma);
> >     5606
> >     5607                 if (IS_ERR_OR_NULL(page)) {
> >
> > If page is either an error pointer or NULL then
> >
> >     5608                         int ret = 0;
> >     5609
> >     5610 #ifndef CONFIG_HAVE_IOREMAP_PROT
> >     5611                         break;
> >     5612 #else
> >     5613                         /*
> >     5614                          * Check if this is a VM_IO | VM_PFNMAP VMA, which
> >     5615                          * we can access using slightly different code.
> >     5616                          */
> > --> 5617                         if (!vma)
> >
> > that means vma is unitialized.
> >
>
> Ack yeah you're right, this is a product of carrying over the code with a
> wrapper that behaves slightly differently.
>
> I'll fix this + roll in the -fix patch stuff in a new respin tonight.

Fixed in [0].

[0]:https://lore.kernel.org/all/d20128c849ecdbf4dd01cc828fcec32127ed939a.1684350871.git.lstoakes@gmail.com/


>
> >     5618                                 break;
> >     5619                         if (vma->vm_ops && vma->vm_ops->access)
> >     5620                                 ret = vma->vm_ops->access(vma, addr, buf,
> >     5621                                                           len, write);
> >     5622                         if (ret <= 0)
> >     5623                                 break;
> >     5624                         bytes = ret;
> >     5625 #endif
> >     5626                 } else {
> >     5627                         bytes = len;
> >     5628                         offset = addr & (PAGE_SIZE-1);
> >     5629                         if (bytes > PAGE_SIZE-offset)
> >     5630                                 bytes = PAGE_SIZE-offset;
> >     5631
> >     5632                         maddr = kmap(page);
> >     5633                         if (write) {
> >     5634                                 copy_to_user_page(vma, page, addr,
> >     5635                                                   maddr + offset, buf, bytes);
> >     5636                                 set_page_dirty_lock(page);
> >     5637                         } else {
> >     5638                                 copy_from_user_page(vma, page, addr,
> >     5639                                                     buf, maddr + offset, bytes);
> >     5640                         }
> >     5641                         kunmap(page);
> >     5642                         put_page(page);
> >     5643                 }
> >     5644                 len -= bytes;
> >     5645                 buf += bytes;
> >     5646                 addr += bytes;
> >     5647         }
> >     5648         mmap_read_unlock(mm);
> >     5649
> >     5650         return buf - old_buf;
> >     5651 }
> >
> > regards,
> > dan carpenter
> >
>