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

[Lorenzo Stoakes <lstoakes@gmail.com>]

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.

>     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
>