Re: [PATCH v2 2/3] mm: khugepaged: fix folio is used after pte_unmap_unlock()

[David Hildenbrand (Arm)]

On 8/17/26 18:11, Lorenzo Stoakes (ARM) wrote:
> On Sat, Aug 15, 2026 at 01:19:23PM +0800, Vernon Yang wrote:
>> From: Vernon Yang <yanglincheng@xxxxxxxxxx>
>>
>> After the page table lock has dropped, the folio can be freed
>> concurrently. The trace_mm_khugepaged_scan_pmd() is left with
>> a dangling folio pointer.
>>
>> So using the folio_pfn() before dropping the page table lock,
>> closing use-after-free window.
>>
>> Fixes: 7d2eba0557c1 ("mm: add tracepoint for scanning pages")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Vernon Yang <yanglincheng@xxxxxxxxxx>
>> ---
>>  include/trace/events/huge_memory.h | 6 +++---
>>  mm/khugepaged.c                    | 4 +++-
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
>> index d3572d4ef453..5dc71d292f47 100644
>> --- a/include/trace/events/huge_memory.h
>> +++ b/include/trace/events/huge_memory.h
>> @@ -55,10 +55,10 @@ SCAN_STATUS
>>
>>  TRACE_EVENT(mm_khugepaged_scan_pmd,
>>
>> -	TP_PROTO(struct mm_struct *mm, struct folio *folio,
>> +	TP_PROTO(struct mm_struct *mm, unsigned long pfn,
>>  		 int referenced, int none_or_zero, int status, int unmapped),
>>
>> -	TP_ARGS(mm, folio, referenced, none_or_zero, status, unmapped),
>> +	TP_ARGS(mm, pfn, referenced, none_or_zero, status, unmapped),
>>
>>  	TP_STRUCT__entry(
>>  		__field(struct mm_struct *, mm)
>> @@ -71,7 +71,7 @@ TRACE_EVENT(mm_khugepaged_scan_pmd,
>>
>>  	TP_fast_assign(
>>  		__entry->mm = mm;
>> -		__entry->pfn = folio ? folio_pfn(folio) : -1;
>> +		__entry->pfn = pfn;
>>  		__entry->referenced = referenced;
>>  		__entry->none_or_zero = none_or_zero;
>>  		__entry->status = status;
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index e7830761d3a2..7c8c48577408 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1603,6 +1603,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
>>  	enum scan_result result = SCAN_FAIL;
>>  	struct page *page = NULL;
>>  	struct folio *folio = NULL;
>> +	unsigned long pfn = -1;
>>  	unsigned long addr;
>>  	unsigned long enabled_orders;
>>  	spinlock_t *ptl;
>> @@ -1778,6 +1779,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
>>  		result = SCAN_SUCCEED;
>>  	}
>>  out_unmap:
>> +	pfn = folio ? folio_pfn(folio) : -1;
>>  	pte_unmap_unlock(pte, ptl);
>>  	if (result == SCAN_SUCCEED) {
>>  		/* collapse_huge_page expects the lock to be dropped before calling */
>> @@ -1788,7 +1790,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
>>  		*lock_dropped = true;
>>  	}
>>  out:
>> -	trace_mm_khugepaged_scan_pmd(mm, folio, referenced,
>> +	trace_mm_khugepaged_scan_pmd(mm, pfn, referenced,
> 
> Same comment as 1/3 I don't see why we should be storing a pfn value used
> nowhere else just for tracing.

I prefer it that way. Even if just for tracing. :)

-- 
Cheers,

David