Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[David Hildenbrand <david@redhat.com>]

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