Re: [PATCH] smaps: fix BUG_ON in smaps_hugetlb_range

[Michal Hocko <mhocko@suse.com>]

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