[PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Jinjiang Tu]

smaps_hugetlb_range() handles the pte without holdling ptl, and may be
concurrenct with migration, leaing to BUG_ON in pfn_swap_entry_to_page().
The race is as follows.

smaps_hugetlb_range              migrate_pages
  huge_ptep_get
                                   remove_migration_ptes
				   folio_unlock
  pfn_swap_entry_folio
    BUG_ON

To fix it, hold ptl lock in smaps_hugetlb_range().

Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields to /proc/PID/smaps")
Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
---
 fs/proc/task_mmu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 751479eb128f..0102ab3aaec1 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
 {
 	struct mem_size_stats *mss = walk->private;
 	struct vm_area_struct *vma = walk->vma;
-	pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
 	struct folio *folio = NULL;
 	bool present = false;
+	spinlock_t *ptl;
+	pte_t ptent;
 
+	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
+	ptent = huge_ptep_get(walk->mm, addr, pte);
 	if (pte_present(ptent)) {
 		folio = page_folio(pte_page(ptent));
 		present = true;
@@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
 		else
 			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
 	}
+	spin_unlock(ptl);
 	return 0;
 }
 #else
-- 
2.43.0

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Dev Jain]

On 21/07/25 1:44 pm, Jinjiang Tu wrote:
> smaps_hugetlb_range() handles the pte without holdling ptl, and may be
> concurrenct with migration, leaing to BUG_ON in pfn_swap_entry_to_page().
> The race is as follows.
>
> smaps_hugetlb_range              migrate_pages
>    huge_ptep_get
>                                     remove_migration_ptes
>                                  folio_unlock
>    pfn_swap_entry_folio
>      BUG_ON
>
> To fix it, hold ptl lock in smaps_hugetlb_range().
>
> Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields to /proc/PID/smaps")
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
>   fs/proc/task_mmu.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 751479eb128f..0102ab3aaec1 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>   {
>       struct mem_size_stats *mss = walk->private;
>       struct vm_area_struct *vma = walk->vma;
> -     pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
>       struct folio *folio = NULL;
>       bool present = false;
> +     spinlock_t *ptl;
> +     pte_t ptent;
>
> +     ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
> +     ptent = huge_ptep_get(walk->mm, addr, pte);
>       if (pte_present(ptent)) {
>               folio = page_folio(pte_page(ptent));
>               present = true;
> @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>               else
>                       mss->private_hugetlb += huge_page_size(hstate_vma(vma));
>       }
> +     spin_unlock(ptl);
>       return 0;
>   }
>   #else

LGTM but will wait for others...

the subject line at the very least should be
"fix race between smaps and migration"; or,
reading smaps_pte_range it doesn't jump
at me why one would run smaps_hugetlb_range without
the PTL, it can be "take PTL in smaps_hugetlb_range".
The current subject line doesn't sound right,
"fixing BUG_ON" means nothing :)

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[David Hildenbrand]

On 21.07.25 10:14, Jinjiang Tu wrote:
> smaps_hugetlb_range() handles the pte without holdling ptl, and may be
> concurrenct with migration, leaing to BUG_ON in pfn_swap_entry_to_page().
> The race is as follows.
> 
> smaps_hugetlb_range              migrate_pages
>    huge_ptep_get
>                                     remove_migration_ptes
> 				   folio_unlock
>    pfn_swap_entry_folio
>      BUG_ON
> 
> To fix it, hold ptl lock in smaps_hugetlb_range().
> 
> Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields to /proc/PID/smaps")
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
>   fs/proc/task_mmu.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 751479eb128f..0102ab3aaec1 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>   {
>   	struct mem_size_stats *mss = walk->private;
>   	struct vm_area_struct *vma = walk->vma;
> -	pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
>   	struct folio *folio = NULL;
>   	bool present = false;
> +	spinlock_t *ptl;
> +	pte_t ptent;
>   
> +	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
> +	ptent = huge_ptep_get(walk->mm, addr, pte);
>   	if (pte_present(ptent)) {
>   		folio = page_folio(pte_page(ptent));
>   		present = true;
> @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>   		else
>   			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
>   	}
> +	spin_unlock(ptl);
>   	return 0;
>   }
>   #else


Heh, I stumbled over that code many times and wondered "why don't we 
need the PTL here -- I'm  sure it's fine because otherwise we would be 
getting reports.".

In pagewalk code we only hold the vma lock -- see walk_hugetlb_range().

So I think we should just grab the PTL in all these walkers.

What about pagemap_hugetlb_range, pagemap_scan_hugetlb_entry, 
gather_hugetlb_stats?

I think we should not make exceptions here and just handle it like we 
would have handled ordinary PTEs/PMDs.

-- 
Cheers,

David / dhildenb

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Michal Hocko]

On Mon 21-07-25 11:29:52, David Hildenbrand wrote:
> On 21.07.25 10:14, Jinjiang Tu wrote:
> > smaps_hugetlb_range() handles the pte without holdling ptl, and may be
> > concurrenct with migration, leaing to BUG_ON in pfn_swap_entry_to_page().
> > The race is as follows.
> > 
> > smaps_hugetlb_range              migrate_pages
> >    huge_ptep_get
> >                                     remove_migration_ptes
> > 				   folio_unlock
> >    pfn_swap_entry_folio
> >      BUG_ON
> > 
> > To fix it, hold ptl lock in smaps_hugetlb_range().
> > 
> > Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields to /proc/PID/smaps")
> > Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> > ---
> >   fs/proc/task_mmu.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 751479eb128f..0102ab3aaec1 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
> >   {
> >   	struct mem_size_stats *mss = walk->private;
> >   	struct vm_area_struct *vma = walk->vma;
> > -	pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
> >   	struct folio *folio = NULL;
> >   	bool present = false;
> > +	spinlock_t *ptl;
> > +	pte_t ptent;
> > +	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
> > +	ptent = huge_ptep_get(walk->mm, addr, pte);
> >   	if (pte_present(ptent)) {
> >   		folio = page_folio(pte_page(ptent));
> >   		present = true;
> > @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
> >   		else
> >   			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> >   	}
> > +	spin_unlock(ptl);
> >   	return 0;
> >   }
> >   #else
> 
> 
> Heh, I stumbled over that code many times and wondered "why don't we need
> the PTL here -- I'm  sure it's fine because otherwise we would be getting
> reports.".
> 
> In pagewalk code we only hold the vma lock -- see walk_hugetlb_range().
> 
> So I think we should just grab the PTL in all these walkers.

I believe the reason that we try to avoid taking the lock in these paths
is that they are userspace accessible and we do not want to expose them
to users. I think it would be good to try to rework the code to not
require the lock even if we get imprecise numbers. We cannot trigger any
oops of course and that is a clear bug here. Can we achieve the fix
without taking the lock?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[David Hildenbrand]

On 21.07.25 11:35, Michal Hocko wrote:
> On Mon 21-07-25 11:29:52, David Hildenbrand wrote:
>> On 21.07.25 10:14, Jinjiang Tu wrote:
>>> smaps_hugetlb_range() handles the pte without holdling ptl, and may be
>>> concurrenct with migration, leaing to BUG_ON in pfn_swap_entry_to_page().
>>> The race is as follows.
>>>
>>> smaps_hugetlb_range              migrate_pages
>>>     huge_ptep_get
>>>                                      remove_migration_ptes
>>> 				   folio_unlock
>>>     pfn_swap_entry_folio
>>>       BUG_ON
>>>
>>> To fix it, hold ptl lock in smaps_hugetlb_range().
>>>
>>> Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields to /proc/PID/smaps")
>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>>> ---
>>>    fs/proc/task_mmu.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 751479eb128f..0102ab3aaec1 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>>>    {
>>>    	struct mem_size_stats *mss = walk->private;
>>>    	struct vm_area_struct *vma = walk->vma;
>>> -	pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
>>>    	struct folio *folio = NULL;
>>>    	bool present = false;
>>> +	spinlock_t *ptl;
>>> +	pte_t ptent;
>>> +	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
>>> +	ptent = huge_ptep_get(walk->mm, addr, pte);
>>>    	if (pte_present(ptent)) {
>>>    		folio = page_folio(pte_page(ptent));
>>>    		present = true;
>>> @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
>>>    		else
>>>    			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
>>>    	}
>>> +	spin_unlock(ptl);
>>>    	return 0;
>>>    }
>>>    #else
>>
>>
>> Heh, I stumbled over that code many times and wondered "why don't we need
>> the PTL here -- I'm  sure it's fine because otherwise we would be getting
>> reports.".
>>
>> In pagewalk code we only hold the vma lock -- see walk_hugetlb_range().
>>
>> So I think we should just grab the PTL in all these walkers.
> 
> I believe the reason that we try to avoid taking the lock in these paths
> is that they are userspace accessible and we do not want to expose them
> to users. I think it would be good to try to rework the code to not
> require the lock even if we get imprecise numbers. We cannot trigger any
> oops of course and that is a clear bug here. Can we achieve the fix
> without taking the lock?

We grab PTLs whenever we walk page tables, except in hugetlb. So I much 
rather want that changed?

-- 
Cheers,

David / dhildenb

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Michal Hocko]

On Mon 21-07-25 11:41:18, David Hildenbrand wrote:
> On 21.07.25 11:35, Michal Hocko wrote:
> > On Mon 21-07-25 11:29:52, David Hildenbrand wrote:
[...]
> > > Heh, I stumbled over that code many times and wondered "why don't we need
> > > the PTL here -- I'm  sure it's fine because otherwise we would be getting
> > > reports.".
> > > 
> > > In pagewalk code we only hold the vma lock -- see walk_hugetlb_range().
> > > 
> > > So I think we should just grab the PTL in all these walkers.
> > 
> > I believe the reason that we try to avoid taking the lock in these paths
> > is that they are userspace accessible and we do not want to expose them
> > to users. I think it would be good to try to rework the code to not
> > require the lock even if we get imprecise numbers. We cannot trigger any
> > oops of course and that is a clear bug here. Can we achieve the fix
> > without taking the lock?
> 
> We grab PTLs whenever we walk page tables, except in hugetlb. So I much
> rather want that changed?

OK, fair enough. If hugetlb is the only one odd here and we are already
living with the ptl already exposed then let's go with a lock here as
well.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Jinjiang Tu]

在 2025/7/21 17:29, David Hildenbrand 写道:
> On 21.07.25 10:14, Jinjiang Tu wrote:
>> smaps_hugetlb_range() handles the pte without holdling ptl, and may be
>> concurrenct with migration, leaing to BUG_ON in 
>> pfn_swap_entry_to_page().
>> The race is as follows.
>>
>> smaps_hugetlb_range              migrate_pages
>>    huge_ptep_get
>>                                     remove_migration_ptes
>>                    folio_unlock
>>    pfn_swap_entry_folio
>>      BUG_ON
>>
>> To fix it, hold ptl lock in smaps_hugetlb_range().
>>
>> Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields 
>> to /proc/PID/smaps")
>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ---
>>   fs/proc/task_mmu.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 751479eb128f..0102ab3aaec1 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, 
>> unsigned long hmask,
>>   {
>>       struct mem_size_stats *mss = walk->private;
>>       struct vm_area_struct *vma = walk->vma;
>> -    pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
>>       struct folio *folio = NULL;
>>       bool present = false;
>> +    spinlock_t *ptl;
>> +    pte_t ptent;
>>   +    ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
>> +    ptent = huge_ptep_get(walk->mm, addr, pte);
>>       if (pte_present(ptent)) {
>>           folio = page_folio(pte_page(ptent));
>>           present = true;
>> @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, 
>> unsigned long hmask,
>>           els
>>               mss->private_hugetlb += huge_page_size(hstate_vma(vma));
>>       }
>> +    spin_unlock(ptl);
>>       return 0;
>>   }
>>   #else
>
>
> Heh, I stumbled over that code many times and wondered "why don't we 
> need the PTL here -- I'm  sure it's fine because otherwise we would be 
> getting reports.".
>
> In pagewalk code we only hold the vma lock -- see walk_hugetlb_range().
>
> So I think we should just grab the PTL in all these walkers.
>
> What about pagemap_hugetlb_range, pagemap_scan_hugetlb_entry, 
> gather_hugetlb_stats?
> I think we should not make exceptions here and just handle it like we 
> would have handled ordinary PTEs/PMDs.
>
Glanced at all .hugetlb_entry callbacks, most of them operates the folio 
whose pfn stored in the pte, holding ptl is safe.

Especially, prot_none_hugetlb_entry() updates the pmd without holding 
ptl, it relies on mmap write lock to avoid concurrenct update. But it 
looks difficult to understand.

Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Jinjiang Tu]

在 2025/7/21 17:11, Dev Jain 写道:
>
> On 21/07/25 1:44 pm, Jinjiang Tu wrote:
>> smaps_hugetlb_range() handles the pte without holdling ptl, and may be
>> concurrenct with migration, leaing to BUG_ON in 
>> pfn_swap_entry_to_page().
>> The race is as follows.
>>
>> smaps_hugetlb_range              migrate_pages
>>    huge_ptep_get
>>                                     remove_migration_ptes
>>                                  folio_unlock
>>    pfn_swap_entry_folio
>>      BUG_ON
>>
>> To fix it, hold ptl lock in smaps_hugetlb_range().
>>
>> Fixes: 25ee01a2fca0 ("mm: hugetlb: proc: add hugetlb-related fields 
>> to /proc/PID/smaps")
>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ---
>>   fs/proc/task_mmu.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 751479eb128f..0102ab3aaec1 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1020,10 +1020,13 @@ static int smaps_hugetlb_range(pte_t *pte, 
>> unsigned long hmask,
>>   {
>>       struct mem_size_stats *mss = walk->private;
>>       struct vm_area_struct *vma = walk->vma;
>> -     pte_t ptent = huge_ptep_get(walk->mm, addr, pte);
>>       struct folio *folio = NULL;
>>       bool present = false;
>> +     spinlock_t *ptl;
>> +     pte_t ptent;
>>
>> +     ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
>> +     ptent = huge_ptep_get(walk->mm, addr, pte);
>>       if (pte_present(ptent)) {
>>               folio = page_folio(pte_page(ptent));
>>               present = true;
>> @@ -1042,6 +1045,7 @@ static int smaps_hugetlb_range(pte_t *pte, 
>> unsigned long hmask,
>>               else
>>                       mss->private_hugetlb += 
>> huge_page_size(hstate_vma(vma));
>>       }
>> +     spin_unlock(ptl);
>>       return 0;
>>   }
>>   #else
>
> LGTM but will wait for others...
>
> the subject line at the very least should be
> "fix race between smaps and migration"; or,
> reading smaps_pte_range it doesn't jump
> at me why one would run smaps_hugetlb_range without
> the PTL, it can be "take PTL in smaps_hugetlb_range".
> The current subject line doesn't sound right,
> "fixing BUG_ON" means nothing :)
Thanks, I will send v2 to update it after the disscussion finishes.
>
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose 
> the contents to any other person, use it for any purpose, or store or 
> copy the information in any medium. Thank you.
>