Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[David Hildenbrand <david@redhat.com>]

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