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

[David Hildenbrand <david@redhat.com>]

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

The "Fixes:" should go above the "---" in that case.

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

-- 
Cheers,

David / dhildenb