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

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

[Balbir Singh]

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

 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;
+			}
 			mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
 			mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
 		}
-- 
2.48.1

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

[David Hildenbrand]

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.

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

}


Something simpler might be

page_pgmap(page)->owner != migrate->pgmap_owner

>   			mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
>   			mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
>   		}


-- 
Cheers,

David / dhildenb

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

[Balbir Singh]

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.


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

>>               mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
>>               mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
>>           }
> 
> 

Thanks for the review!
Balbir

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

[David Hildenbrand]

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

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

[Balbir Singh]

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

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

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

[Alistair Popple]

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

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

[David Hildenbrand]

On 20.02.25 23:45, Alistair Popple wrote:
> 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?

Ah, I completely ignored that this is not in mm/mm-stable yet.

Yes, that should best just be squashed into the original commit as long 
as it is not in mm/mm-stable.

-- 
Cheers,

David / dhildenb