Re: [PATCH v3 3/3] mm: khugepaged: fix folio is used after folio_put/unlock()

[Lorenzo Stoakes (ARM)]

On Wed, Aug 26, 2026 at 09:09:16AM +0100, Lorenzo Stoakes (ARM) wrote:
> On Mon, Aug 24, 2026 at 05:29:35PM +0800, Vernon Yang wrote:
> > From: Vernon Yang <yanglincheng@xxxxxxxxxx>
> >
> > On the rollback path, folio_put() has already dropped the last reference
> > of new_folio. On the success path, new_folio is already unlocked and can
> > be freed concurrently. The trace_mm_khugepaged_collapse_file() is left
> > with a dangling folio pointer.
> >
> > So using the folio_pfn() before dropping the reference, closing
> > use-after-free window.
> >
> > Fixes: 4c9473e87e75 ("mm/khugepaged: add tracepoint to collapse_file()")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Vernon Yang <yanglincheng@xxxxxxxxxx>
> > ---
> >  include/trace/events/huge_memory.h | 6 +++---
> >  mm/khugepaged.c                    | 5 ++++-
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> > index fa828967e1fb..5fb4d92cfd84 100644
> > --- a/include/trace/events/huge_memory.h
> > +++ b/include/trace/events/huge_memory.h
> > @@ -211,10 +211,10 @@ TRACE_EVENT(mm_khugepaged_scan_file,
> >  );
> >
> >  TRACE_EVENT(mm_khugepaged_collapse_file,
> > -	TP_PROTO(struct mm_struct *mm, struct folio *new_folio, pgoff_t index,
> > +	TP_PROTO(struct mm_struct *mm, unsigned long new_pfn, pgoff_t index,
> >  			unsigned long addr, bool is_shmem, struct file *file,
> >  			int nr, int result),
> > -	TP_ARGS(mm, new_folio, index, addr, is_shmem, file, nr, result),
> > +	TP_ARGS(mm, new_pfn, index, addr, is_shmem, file, nr, result),
> >  	TP_STRUCT__entry(
> >  		__field(struct mm_struct *, mm)
> >  		__field(unsigned long, hpfn)
> > @@ -228,7 +228,7 @@ TRACE_EVENT(mm_khugepaged_collapse_file,
> >
> >  	TP_fast_assign(
> >  		__entry->mm = mm;
> > -		__entry->hpfn = new_folio ? folio_pfn(new_folio) : -1;
> > +		__entry->hpfn = new_pfn;
> >  		__entry->index = index;
> >  		__entry->addr = addr;
> >  		__entry->is_shmem = is_shmem;
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 4e0fca5942dd..24347f1a94ae 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -2254,6 +2254,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> >  	struct address_space *mapping = file->f_mapping;
> >  	struct page *dst;
> >  	struct folio *folio, *tmp, *new_folio;
> > +	unsigned long new_pfn = -1;
>
> Nitty but:
>
> Could we use pfn_t? I don't love that we are inconsistent with that.

(As before disregard, I was somehow behind on the times + pre-morning-coffee
confused :)

>
> Also the general pattern for pfn names is pfn_xxx so pfn_new instead?

Also since we're using xxx_pfn elsewhere here new_pfn is fine.

Therefore, with David's suggestion applied:

Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

>
> >  	pgoff_t index = 0, end = start + HPAGE_PMD_NR;
> >  	LIST_HEAD(pagelist);
> >  	XA_STATE_ORDER(xas, &mapping->i_pages, start, HPAGE_PMD_ORDER);
> > @@ -2633,6 +2634,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> >  	retract_page_tables(mapping, start);
> >  	if (cc && !cc->is_khugepaged)
> >  		result = SCAN_PTE_MAPPED_HUGEPAGE;
> > +	new_pfn = folio_pfn(new_folio);
> >  	folio_unlock(new_folio);
> >
> >  	/*
> > @@ -2671,12 +2673,13 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> >  	}
> >
> >  	new_folio->mapping = NULL;
> > +	new_pfn = folio_pfn(new_folio);
> >
> >  	folio_unlock(new_folio);
> >  	folio_put(new_folio);
> >  out:
> >  	VM_BUG_ON(!list_empty(&pagelist));
> > -	trace_mm_khugepaged_collapse_file(mm, new_folio, index, addr, is_shmem, file, HPAGE_PMD_NR, result);
> > +	trace_mm_khugepaged_collapse_file(mm, new_pfn, index, addr, is_shmem, file, HPAGE_PMD_NR, result);
> >  	return result;
> >  }
> >
> > --
> > 2.53.0
> >
>
> --
> Cheers, Lorenzo

--
Cheers, Lorenzo