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

On 8/24/26 11:29, Vernon Yang wrote:
> From: Vernon Yang <[email protected]>
> 
> 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: [email protected]
> Signed-off-by: Vernon Yang <[email protected]>
> ---
>  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;
>  	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);

Why not set new_pfn once after successful alloc_charge_folio()?




-- 
Cheers,

David