Re: [PATCH v2 2/3] mm: khugepaged: fix folio is used after pte_unmap_unlock()
"David Hildenbrand (Arm)" <[email protected]>
| Newsgroups | org.kernel.vger.stable,org.kernel.vger.linux-kernel,org.kvack.linux-mm |
|---|---|
| Message-ID | <[email protected]> |
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 <[email protected]> >> >> 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: [email protected] >> Signed-off-by: Vernon Yang <[email protected]> >> --- >> 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