Re: [bug report] mm/gup: migrate device coherent pages when pinning instead of failing

[Alistair Popple <apopple@nvidia.com>]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hello Alistair Popple,
>
> The patch b05a79d4377f: "mm/gup: migrate device coherent pages when
> pinning instead of failing" from Jul 15, 2022, leads to the following
> Smatch static checker warning:
>
> 	mm/migrate_device.c:842 migrate_device_coherent_page()
> 	warn: duplicate check 'src_pfn & (1 << 1)' (previous on line 832)
>
> mm/migrate_device.c
>     810 int migrate_device_coherent_page(struct page *page)
>     811 {
>     812         unsigned long src_pfn, dst_pfn = 0;
>     813         struct migrate_vma args;
>     814         struct page *dpage;
>     815
>     816         WARN_ON_ONCE(PageCompound(page));
>     817
>     818         lock_page(page);
>     819         src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
>     820         args.src = &src_pfn;
>     821         args.dst = &dst_pfn;
>     822         args.cpages = 1;
>     823         args.npages = 1;
>     824         args.vma = NULL;
>     825
>     826         /*
>     827          * We don't have a VMA and don't need to walk the page tables to find
>     828          * the source page. So call migrate_vma_unmap() directly to unmap the
>     829          * page as migrate_vma_setup() will fail if args.vma == NULL.
>     830          */
>     831         migrate_vma_unmap(&args);
>     832         if (!(src_pfn & MIGRATE_PFN_MIGRATE))
>     833                 return -EBUSY;
>
> Return here.
>
>     834
>     835         dpage = alloc_page(GFP_USER | __GFP_NOWARN);
>     836         if (dpage) {
>     837                 lock_page(dpage);
>     838                 dst_pfn = migrate_pfn(page_to_pfn(dpage));
>     839         }
>     840
>     841         migrate_vma_pages(&args);
> --> 842         if (src_pfn & MIGRATE_PFN_MIGRATE)
>     843                 copy_highpage(dpage, page);
>
> No need to check again.  Was dst_pfn intended?

No. The check is necessary and correct. migrate_vma_pages() may clear
MIGRATE_PFN_MIGRATE from src_pfn if the page could not be migrated for
some reason.

>     844         migrate_vma_finalize(&args);
>     845
>     846         if (src_pfn & MIGRATE_PFN_MIGRATE)
>     847                 return 0;
>
> Same check again.

I don't see the problem with this?

 - Alistair

>     848         return -EBUSY;
>     849 }
>
> regards,
> dan carpenter