Re: [PATCH] mm/migrate_device: Do not access pgmap for non zone device pages

[Alistair Popple <apopple@nvidia.com>]

On Thu, Feb 20, 2025 at 11:38:10PM +1100, Balbir Singh wrote:
> On 2/20/25 23:05, David Hildenbrand wrote:
> > On 20.02.25 12:58, Balbir Singh wrote:
> >> On 2/20/25 22:48, David Hildenbrand wrote:
> >>> On 20.02.25 00:13, Balbir Singh wrote:
> >>>> page_pgmap() is referenced before checking if the page is a zone
> >>>> device page and this triggers the warning in page_pgmap(). Refactor
> >>>> the code to use the helper function after relevant checks.
> >>>>
> >>>> Cc: Alistair Popple <apopple@nvidia.com>
> >>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> >>>> Cc: David Hildenbrand <david@redhat.com>
> >>>> Cc: Dan Williams <dan.j.williams@intel.com>
> >>>>
> >>>> Signed-off-by: Balbir Singh <balbirs@nvidia.com>
> >>>> ---
> >>>>
> >>>> Fixes: 7f1cfd71153b ("mm: allow compound zone device pages") on
> >>>> mm-unstable
> >>>
> >>> Is there actually something broken? At least for now, reading folio->pgmap should just work, although it might be garbage.
> >>>
> >>
> >> It triggers the VM_WARN_ON_ONCE_PAGE
> >>
> >> static inline struct dev_pagemap *page_pgmap(const struct page *page)
> >> {
> >>     VM_WARN_ON_ONCE_PAGE(!is_zone_device_page(page), page);
> >>     return page_folio(page)->pgmap;
> >> }
> >>
> >> Nothing is broken, because the code below has checks for is_device_coherent_page(),
> >> but in general I think the WARN_ON is correct because it warns us against garbage
> >> and it's propagation if the correct checks are not in place.
> > 
> > Ah! Now I read your "triggers the warning in page_pgmap()" in the description.
> > 
> > It's usually a good idea to just include the splat you observed, if you did, and call it "triggers the VM_WARN_ON_ONCE_PAGE".
> 
> Sure
> 
> > 
> > The "Fixes:" should go above the "---" in that case.
> 
> I was not sure if the commit-ids in mm-unstable are persistent, so I put it
> below the --- to avoid confusion

They are not, but I'm unsure what the process is here. I guess either I roll
this fix into my series and repost or maybe Andrew takes it as is a separate fix
on top of my series?

Either way the fix looks good, so with the code style comment below fixed feel
free to add:

Reviewed-by: Alistair Popple <apopple@nvidia.com>

> > 
> >>
> >>
> >>>>
> >>>>    mm/migrate_device.c | 13 ++++++++-----
> >>>>    1 file changed, 8 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> >>>> index 6771893d4601..e0bf771edb6f 100644
> >>>> --- a/mm/migrate_device.c
> >>>> +++ b/mm/migrate_device.c
> >>>> @@ -153,14 +153,17 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> >>>>                    goto next;
> >>>>                }
> >>>>                page = vm_normal_page(migrate->vma, addr, pte);
> >>>> -            pgmap = page_pgmap(page);
> >>>>                if (page && !is_zone_device_page(page) &&
> >>>>                    !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> >>>>                    goto next;
> >>>> -            else if (page && is_device_coherent_page(page) &&
> >>>> -                (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> >>>> -                 pgmap->owner != migrate->pgmap_owner))
> >>>> -                goto next;
> >>>> +            else if (page && is_device_coherent_page(page)) {
> >>>> +                pgmap = page_pgmap(page);
> >>>> +
> >>>> +                if (!(migrate->flags &
> >>>> +                    MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> >>>> +                    pgmap->owner != migrate->pgmap_owner)
> >>>> +                    goto next;
> >>>> +            }
> >>>
> >>> Coding style wants you to use
> >>>
> >>> if () {
> >>>
> >>> } else if {
> >>>
> >>> }
> >>>
> >>> Not
> >>>
> >>> if ()
> >>> else if {
> >>>
> >>> }
> >>>
> >>
> >> Ack, checkpatch.pl missed it, but agreed
> >>
> >>>
> >>> Something simpler might be
> >>>
> >>> page_pgmap(page)->owner != migrate->pgmap_owner
> >>>
> >>
> >> Yep, I had that and dropped it, the four clauses made it feel that it might
> >> benefit from a split.
> > 
> > Right, the mixture of && and || is confusing.
> > 
> > 
> > So with the {}
> > 
> > Acked-by: David Hildenbrand <david@redhat.com>
> > 
> > Thanks!
> > 
> 
> Thanks, I can respin it with the {} as needed.
> 
> Balbir