mm: khugepaged: fix tracepoint UAF

From: Vernon Yang <[email protected]>

When the swap entries found exceed max_ptes_swap, the loop is left via break with folio still holding the xarray value that encodes the swap entry, not valid folio pointer.

That value is passed to trace_mm_khugepaged_scan_file(), which feeds it to folio_pfn(). On FLATMEM and SPARSEMEM_VMEMMAP, the page_to_pfn() is plain pointer arithmetic, so the trace event merely prints bogus scan_pfn. On classic SPARSEMEM, the page_to_pfn() reads page->flags, dereferencing the tiny encoded integer and oopsing khugepaged whenever the trace event is enabled.

So when folio is the swap entry value, simply set pfn to -1, just like exhausted scan naturally.

And the folio_put() has maybe dropped the last reference of folio. The trace_mm_khugepaged_scan_file() is left with a dangling folio pointer. so using the folio_pfn() before dropping the reference, closing use-after-free window.

Fixes d41fd2016ed0 ("mm/khugepaged: add tracepoint to hpage_collapse_scan_file()")

Cc [email protected]

Signed-off-by Vernon Yang <[email protected]>

---
 include/trace/events/huge_memory.h |  6 +++---
 mm/khugepaged.c                    | 14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

include/trace/events/huge_memory.h +2 -2

index 291fae364c62..d3572d4ef453 100644
@@ -178,10 +178,10 @@ TRACE_EVENT(mm_collapse_huge_page_swapin,
 
 TRACE_EVENT(mm_khugepaged_scan_file,
 
-	TP_PROTO(struct mm_struct *mm, struct folio *folio, struct file *file,
+	TP_PROTO(struct mm_struct *mm, unsigned long pfn, struct file *file,
 		 int present, int swap, int result),
 
-	TP_ARGS(mm, folio, file, present, swap, result),
+	TP_ARGS(mm, pfn, file, present, swap, result),
 
 	TP_STRUCT__entry(
 		__field(struct mm_struct *, mm)

mm/khugepaged.c +0 -4

 	present = 0;
 	swap = 0;

@@ -2720,27 +2721,23 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			 * PMD-sized THP implies that we can only try
 			 * retracting the PTE table.
 			 */
-			folio_put(folio);
 			break;
 		}
 
 		node = folio_nid(folio);
 		if (collapse_scan_abort(node, cc)) {
 			result = SCAN_SCAN_ABORT;
-			folio_put(folio);
 			break;
 		}
 		cc->node_load[node]++;
 
 		if (!folio_test_lru(folio)) {
 			result = SCAN_PAGE_LRU;
-			folio_put(folio);
 			break;
 		}
 
 		if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio)) {
 			result = SCAN_PAGE_COUNT;
-			folio_put(folio);
 			break;
 		}
 

mm/khugepaged.c +0 -1

 	return result;
 }
 
--

2.53.0

I mean maybe better to just reduce this to something like:

	/* folio is NULL unless exited early. */
	trace_mm_khugepaged_scan_file(mm,
			(!folio || xa_is_value(folio)) ? -1 : folio_pfn(folio),
			file, present, swap, result);

As the least invasive way of fixing the trace + limiting the contageon?

Quoted 9 lines

I mean maybe better to just reduce this to something like:

	/* folio is NULL unless exited early. */
	trace_mm_khugepaged_scan_file(mm,
			(!folio || xa_is_value(folio)) ? -1 : folio_pfn(folio),
			file, present, swap, result);

As the least invasive way of fixing the trace + limiting the contageon?

I raised that using a folio after dropping relevant refs+locks is in general an anti-pattern (IOW wrong if folios can just get split afterwards and are suddenly no longer folios).

I'd like us to avoid that where possible (and suspect Willy will thank us later ;) )

Isn't there some minimal way to just calculate the pfn once we obtain a folio? I was assuming that there should be an easier way to do that.

--
Cheers,

David

Quoted 6 lines · Vernon Yang

From: Vernon Yang <[email protected]>

When the swap entries found exceed max_ptes_swap, the loop is left via break with folio still holding the xarray value that encodes the swap entry, not valid folio pointer.

Quoted 6 lines

That value is passed to trace_mm_khugepaged_scan_file(), which feeds it to folio_pfn(). On FLATMEM and SPARSEMEM_VMEMMAP, the page_to_pfn() is plain pointer arithmetic, so the trace event merely prints bogus scan_pfn. On classic SPARSEMEM, the page_to_pfn() reads page->flags, dereferencing the tiny encoded integer and oopsing khugepaged whenever the trace event is enabled.

Hmm I seem to recall that we were no longer spporting !VMEMMAP SPARSEMEM, but maybe I was imagining that :)

But this needs fixing in general anyway.

Quoted 35 lines

So when folio is the swap entry value, simply set pfn to -1, just like exhausted scan naturally.

And the folio_put() has maybe dropped the last reference of folio. The trace_mm_khugepaged_scan_file() is left with a dangling folio pointer. so using the folio_pfn() before dropping the reference, closing use-after-free window.

Fixes d41fd2016ed0 ("mm/khugepaged: add tracepoint to hpage_collapse_scan_file()")

Cc [email protected]

Signed-off-by Vernon Yang <[email protected]>

---
 include/trace/events/huge_memory.h |  6 +++---
 mm/khugepaged.c                    | 14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

include/trace/events/huge_memory.h +2 -2

index 291fae364c62..d3572d4ef453 100644
@@ -178,10 +178,10 @@ TRACE_EVENT(mm_collapse_huge_page_swapin,
 
 TRACE_EVENT(mm_khugepaged_scan_file,
 
-	TP_PROTO(struct mm_struct *mm, struct folio *folio, struct file *file,
+	TP_PROTO(struct mm_struct *mm, unsigned long pfn, struct file *file,
 		 int present, int swap, int result),
 
-	TP_ARGS(mm, folio, file, present, swap, result),
+	TP_ARGS(mm, pfn, file, present, swap, result),
 
 	TP_STRUCT__entry(
 		__field(struct mm_struct *, mm)

include/trace/events/huge_memory.h TRACE_EVENT(mm_khugepaged_scan_file, · line 197

Quoted 5 lines

include/trace/events/huge_memory.h +1 -1

@@ -194,7 +194,7 @@ TRACE_EVENT(mm_khugepaged_scan_file,
 
 	TP_fast_assign(
 		__entry->mm = mm;
-		__entry->pfn = folio ? folio_pfn(folio) : -1;
+		__entry->pfn = pfn;

I wonder how easy it is for people to interpret that this is a PFN of something only in the case of an early exit.

It's a bit of a mess that we're exposing internal implementation details like this, quite honestly.

I hope there is no expectation of this being there indefinitely.

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2686

Quoted 12 lines

include/trace/events/huge_memory.h +0 -0

 		__assign_str(filename);
 		__entry->present = present;
 		__entry->swap = swap;

mm/khugepaged.c +1 -0

index 617bca76db49..e7830761d3a2 100644
@@ -2683,6 +2683,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 	int present, swap;
 	int node = NUMA_NO_NODE;
 	enum scan_result result = SCAN_SUCCEED;
+	unsigned long pfn;

See below I think we should drop it.

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2723

Quoted 9 lines

mm/khugepaged.c +0 -1

 	present = 0;
 	swap = 0;

@@ -2720,27 +2721,23 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			 * PMD-sized THP implies that we can only try
 			 * retracting the PTE table.
 			 */
-			folio_put(folio);

Quoted 24 lines

mm/khugepaged.c +0 -3

 			break;
 		}
 
 		node = folio_nid(folio);
 		if (collapse_scan_abort(node, cc)) {
 			result = SCAN_SCAN_ABORT;
-			folio_put(folio);
 			break;
 		}
 		cc->node_load[node]++;
 
 		if (!folio_test_lru(folio)) {
 			result = SCAN_PAGE_LRU;
-			folio_put(folio);
 			break;
 		}
 
 		if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio)) {
 			result = SCAN_PAGE_COUNT;
-			folio_put(folio);
 			break;
 		}
 

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2764

Quoted 9 lines

mm/khugepaged.c +6 -0

@@ -2759,7 +2756,14 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			cond_resched_rcu();
 		}
 	}
+	if (!folio || xa_is_value(folio)) {
+		pfn = -1;
+	} else {
+		pfn = folio_pfn(folio);
+		folio_put(folio);
+	}

Hmm kinda hate this you're now mixing clean up logic with some trace logic.

You have to go check out xas_for_each() and see that xas_next_entry() sets folio to NULL at the end for the normal case to know this is OK and then figure out that this is just for the break cases.

That's not intuitive in already not intuitive code.

And this is a patch you're wanting to backport to fix a bug too... :)

I think you should keep the folio_put()'s where they are for now and limit this fix to the trace code please.

I think you should drop the pfn var altogether and do this far more simply, see below.

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2766

Quoted 2 lines

mm/khugepaged.c +1 -0

 	rcu_read_unlock();
+

Stray newline added, let's not do that for a backported fix please :)

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2781

Quoted 9 lines

mm/khugepaged.c +1 -1

 	if (result == SCAN_PTE_MAPPED_HUGEPAGE)
 		cc->progress++;
 	else

@@ -2774,7 +2778,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 		}
 	}
 
-	trace_mm_khugepaged_scan_file(mm, folio, file, present, swap, result);
+	trace_mm_khugepaged_scan_file(mm, pfn, file, present, swap, result);

I mean maybe better to just reduce this to something like:

	/* folio is NULL unless exited early. */
	trace_mm_khugepaged_scan_file(mm,
			(!folio || xa_is_value(folio)) ? -1 : folio_pfn(folio),
			file, present, swap, result);

As the least invasive way of fixing the trace + limiting the contageon?

Quoted 7 lines

mm/khugepaged.c +0 -1

 	return result;
 }
 
--

2.53.0

--
Cheers, Lorenzo

Quoted 14 lines · Lorenzo Stoakes (ARM)

Quoted 6 lines · Vernon Yang

From: Vernon Yang <[email protected]>

When the swap entries found exceed max_ptes_swap, the loop is left via break with folio still holding the xarray value that encodes the swap entry, not valid folio pointer.

Quoted 6 lines

That value is passed to trace_mm_khugepaged_scan_file(), which feeds it to folio_pfn(). On FLATMEM and SPARSEMEM_VMEMMAP, the page_to_pfn() is plain pointer arithmetic, so the trace event merely prints bogus scan_pfn. On classic SPARSEMEM, the page_to_pfn() reads page->flags, dereferencing the tiny encoded integer and oopsing khugepaged whenever the trace event is enabled.

Quoted 2 lines

Hmm I seem to recall that we were no longer spporting !VMEMMAP SPARSEMEM, but maybe I was imagining that :)

OK seems some museum piece architectures are causing us issues again, fun times (parisc and some mips).

Quoted 52 lines

But this needs fixing in general anyway.

Quoted 35 lines

So when folio is the swap entry value, simply set pfn to -1, just like exhausted scan naturally.

And the folio_put() has maybe dropped the last reference of folio. The trace_mm_khugepaged_scan_file() is left with a dangling folio pointer. so using the folio_pfn() before dropping the reference, closing use-after-free window.

Fixes d41fd2016ed0 ("mm/khugepaged: add tracepoint to hpage_collapse_scan_file()")

Cc [email protected]

Signed-off-by Vernon Yang <[email protected]>

---
 include/trace/events/huge_memory.h |  6 +++---
 mm/khugepaged.c                    | 14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

include/trace/events/huge_memory.h +2 -2

index 291fae364c62..d3572d4ef453 100644
@@ -178,10 +178,10 @@ TRACE_EVENT(mm_collapse_huge_page_swapin,
 
 TRACE_EVENT(mm_khugepaged_scan_file,
 
-	TP_PROTO(struct mm_struct *mm, struct folio *folio, struct file *file,
+	TP_PROTO(struct mm_struct *mm, unsigned long pfn, struct file *file,
 		 int present, int swap, int result),
 
-	TP_ARGS(mm, folio, file, present, swap, result),
+	TP_ARGS(mm, pfn, file, present, swap, result),
 
 	TP_STRUCT__entry(
 		__field(struct mm_struct *, mm)

include/trace/events/huge_memory.h TRACE_EVENT(mm_khugepaged_scan_file, · line 197

Quoted 5 lines

include/trace/events/huge_memory.h +1 -1

@@ -194,7 +194,7 @@ TRACE_EVENT(mm_khugepaged_scan_file,
 
 	TP_fast_assign(
 		__entry->mm = mm;
-		__entry->pfn = folio ? folio_pfn(folio) : -1;
+		__entry->pfn = pfn;

I wonder how easy it is for people to interpret that this is a PFN of something only in the case of an early exit.

It's a bit of a mess that we're exposing internal implementation details like this, quite honestly.

I hope there is no expectation of this being there indefinitely.

Quoted 14 lines

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2686

Quoted 12 lines

include/trace/events/huge_memory.h +0 -0

 		__assign_str(filename);
 		__entry->present = present;
 		__entry->swap = swap;

mm/khugepaged.c +1 -0

index 617bca76db49..e7830761d3a2 100644
@@ -2683,6 +2683,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 	int present, swap;
 	int node = NUMA_NO_NODE;
 	enum scan_result result = SCAN_SUCCEED;
+	unsigned long pfn;

See below I think we should drop it.

OK we can't because of museum piece architectures that will actually deref the folio to get the PFN...

I'd startr by initialising this to -1 then since I guess in theory it's possible xas_for_each() could just not do anything is it? (folio is being set to NULL so that suggests so).

Quoted 9 lines

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2723

Quoted 9 lines

mm/khugepaged.c +0 -1

 	present = 0;
 	swap = 0;

@@ -2720,27 +2721,23 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			 * PMD-sized THP implies that we can only try
 			 * retracting the PTE table.
 			 */
-			folio_put(folio);

I still hate the idea of moving these outside of the loop, it's just asking for trouble.

So I guess instead:

	xas_for_each(&xas, folio, start + HPAGE_PMD_NR - 1) {
		pfn = -1;
		...
		if (xa_is_value(folio)) {
			...
		}
		pfn = folio_pfn(folio);
		...
	}
	...
	trace_mm_khugepaged_scan_file(mm, pfn, file, present, swap, result);

That way you avoid the horror or moving the folio put somewhere deeply unintuitive and inconsistent, and maintain pfn lifetime for some arches nobody uses.

Quoted 68 lines

Quoted 25 lines

mm/khugepaged.c +0 -3

 			break;
 		}
 
 		node = folio_nid(folio);
 		if (collapse_scan_abort(node, cc)) {
 			result = SCAN_SCAN_ABORT;
-			folio_put(folio);
 			break;
 		}
 		cc->node_load[node]++;
 
 		if (!folio_test_lru(folio)) {
 			result = SCAN_PAGE_LRU;
-			folio_put(folio);
 			break;
 		}
 
 		if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio)) {
 			result = SCAN_PAGE_COUNT;
-			folio_put(folio);
 			break;
 		}
 

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2764

Quoted 9 lines

mm/khugepaged.c +6 -0

@@ -2759,7 +2756,14 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			cond_resched_rcu();
 		}
 	}
+	if (!folio || xa_is_value(folio)) {
+		pfn = -1;
+	} else {
+		pfn = folio_pfn(folio);
+		folio_put(folio);
+	}

Hmm kinda hate this you're now mixing clean up logic with some trace logic.

You have to go check out xas_for_each() and see that xas_next_entry() sets folio to NULL at the end for the normal case to know this is OK and then figure out that this is just for the break cases.

That's not intuitive in already not intuitive code.

And this is a patch you're wanting to backport to fix a bug too... :)

I think you should keep the folio_put()'s where they are for now and limit this fix to the trace code please.

I think you should drop the pfn var altogether and do this far more simply, see below.

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2766

Quoted 2 lines

mm/khugepaged.c +1 -0

 	rcu_read_unlock();
+

Stray newline added, let's not do that for a backported fix please :)

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2781

Quoted 9 lines

mm/khugepaged.c +1 -1

 	if (result == SCAN_PTE_MAPPED_HUGEPAGE)
 		cc->progress++;
 	else

@@ -2774,7 +2778,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 		}
 	}
 
-	trace_mm_khugepaged_scan_file(mm, folio, file, present, swap, result);
+	trace_mm_khugepaged_scan_file(mm, pfn, file, present, swap, result);

I mean maybe better to just reduce this to something like:

Quoted 6 lines

	/* folio is NULL unless exited early. */
	trace_mm_khugepaged_scan_file(mm,
			(!folio || xa_is_value(folio)) ? -1 : folio_pfn(folio),
			file, present, swap, result);

As the least invasive way of fixing the trace + limiting the contageon?

Yeah as above this isn't right.

Quoted 10 lines

Quoted 7 lines

mm/khugepaged.c +0 -1

 	return result;
 }
 
--

2.53.0

--
Cheers, Lorenzo

--
Cheers, Lorenzo

Quoted 121 lines · Lorenzo Stoakes (ARM)

Quoted 14 lines · Lorenzo Stoakes (ARM)

Quoted 6 lines · Vernon Yang

From: Vernon Yang <[email protected]>

When the swap entries found exceed max_ptes_swap, the loop is left via break with folio still holding the xarray value that encodes the swap entry, not valid folio pointer.

Quoted 6 lines

That value is passed to trace_mm_khugepaged_scan_file(), which feeds it to folio_pfn(). On FLATMEM and SPARSEMEM_VMEMMAP, the page_to_pfn() is plain pointer arithmetic, so the trace event merely prints bogus scan_pfn. On classic SPARSEMEM, the page_to_pfn() reads page->flags, dereferencing the tiny encoded integer and oopsing khugepaged whenever the trace event is enabled.

Quoted 2 lines

Hmm I seem to recall that we were no longer spporting !VMEMMAP SPARSEMEM, but maybe I was imagining that :)

OK seems some museum piece architectures are causing us issues again, fun times (parisc and some mips).

Quoted 52 lines

But this needs fixing in general anyway.

Quoted 35 lines

So when folio is the swap entry value, simply set pfn to -1, just like exhausted scan naturally.

And the folio_put() has maybe dropped the last reference of folio. The trace_mm_khugepaged_scan_file() is left with a dangling folio pointer. so using the folio_pfn() before dropping the reference, closing use-after-free window.

Fixes d41fd2016ed0 ("mm/khugepaged: add tracepoint to hpage_collapse_scan_file()")

Cc [email protected]

Signed-off-by Vernon Yang <[email protected]>

---
 include/trace/events/huge_memory.h |  6 +++---
 mm/khugepaged.c                    | 14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

include/trace/events/huge_memory.h +2 -2

index 291fae364c62..d3572d4ef453 100644
@@ -178,10 +178,10 @@ TRACE_EVENT(mm_collapse_huge_page_swapin,
 
 TRACE_EVENT(mm_khugepaged_scan_file,
 
-	TP_PROTO(struct mm_struct *mm, struct folio *folio, struct file *file,
+	TP_PROTO(struct mm_struct *mm, unsigned long pfn, struct file *file,
 		 int present, int swap, int result),
 
-	TP_ARGS(mm, folio, file, present, swap, result),
+	TP_ARGS(mm, pfn, file, present, swap, result),
 
 	TP_STRUCT__entry(
 		__field(struct mm_struct *, mm)

include/trace/events/huge_memory.h TRACE_EVENT(mm_khugepaged_scan_file, · line 197

Quoted 5 lines

include/trace/events/huge_memory.h +1 -1

@@ -194,7 +194,7 @@ TRACE_EVENT(mm_khugepaged_scan_file,
 
 	TP_fast_assign(
 		__entry->mm = mm;
-		__entry->pfn = folio ? folio_pfn(folio) : -1;
+		__entry->pfn = pfn;

I wonder how easy it is for people to interpret that this is a PFN of something only in the case of an early exit.

It's a bit of a mess that we're exposing internal implementation details like this, quite honestly.

I hope there is no expectation of this being there indefinitely.

Quoted 14 lines

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2686

Quoted 12 lines

include/trace/events/huge_memory.h +0 -0

 		__assign_str(filename);
 		__entry->present = present;
 		__entry->swap = swap;

mm/khugepaged.c +1 -0

index 617bca76db49..e7830761d3a2 100644
@@ -2683,6 +2683,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 	int present, swap;
 	int node = NUMA_NO_NODE;
 	enum scan_result result = SCAN_SUCCEED;
+	unsigned long pfn;

See below I think we should drop it.

OK we can't because of museum piece architectures that will actually deref the folio to get the PFN...

I'd startr by initialising this to -1 then since I guess in theory it's possible xas_for_each() could just not do anything is it? (folio is being set to NULL so that suggests so).

Quoted 9 lines

mm/khugepaged.c static enum scan_result collapse_scan_file(struct mm_struct *mm, · line 2723

Quoted 9 lines

mm/khugepaged.c +0 -1

 	present = 0;
 	swap = 0;

@@ -2720,27 +2721,23 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			 * PMD-sized THP implies that we can only try
 			 * retracting the PTE table.
 			 */
-			folio_put(folio);

I still hate the idea of moving these outside of the loop, it's just asking for trouble.

So I guess instead:

	xas_for_each(&xas, folio, start + HPAGE_PMD_NR - 1) {
		pfn = -1;
		...
		if (xa_is_value(folio)) {
			...
		}
		pfn = folio_pfn(folio);
		...
	}
	...
	trace_mm_khugepaged_scan_file(mm, pfn, file, present, swap, result);

Quoted 3 lines

That way you avoid the horror or moving the folio put somewhere deeply unintuitive and inconsistent, and maintain pfn lifetime for some arches nobody uses.

Thank you for the reference solution. After studying the overall process, I found that the above solution still has two issues:

- When !folio_try_get(folio) or folio != xas_reload(&xas), if the next xas_for_each() iteration happens to terminate, pfn will be incorrect, which is inconsistent with previous versions.

- when the xas_for_each() iteration to terminate and the folio operation preceding is normal. but pfn will be incorrect, also same issue.

To keep it simple, I modified it as follows. Is this OK?

mm/khugepaged.c +2 -0

index 79effd3f3da4..00337405c0e0 100644
@@ -2689,6 +2689,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 	int present, swap;
 	int node = NUMA_NO_NODE;
 	enum scan_result result = SCAN_SUCCEED;
+	unsigned long pfn;
 
 	present = 0;
 	swap = 0;

@@ -2719,6 +2720,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm,
 			continue;
 		}
 
+		pfn = folio_pfn(folio);
 		if (is_pmd_order(folio_order(folio))) {
 			result = SCAN_PTE_MAPPED_HUGEPAGE;
 			/*

mm/khugepaged.c +0 -1

--

Cheers,
Vernon

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(-)

include/trace/events/huge_memory.h +3 -3

index d3572d4ef453..5dc71d292f47 100644
@@ -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;

mm/khugepaged.c +2 -0

index e7830761d3a2..7c8c48577408 100644
@@ -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 */

mm/khugepaged.c +0 -1

 				     none_or_zero, result, unmapped);
 	return result;
 }
--

2.53.0

Quoted 64 lines · Vernon Yang

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(-)

include/trace/events/huge_memory.h +3 -3

index d3572d4ef453..5dc71d292f47 100644
@@ -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;

mm/khugepaged.c +2 -0

index e7830761d3a2..7c8c48577408 100644
@@ -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,

mm/khugepaged.c static enum scan_result collapse_scan_pmd(struct mm_struct *mm, · line 1793

Quoted 5 lines

mm/khugepaged.c +1 -1

 		*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.

Quoted 2 lines · Lorenzo Stoakes (ARM)

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

Quoted 52 lines · Vernon Yang

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(-)

include/trace/events/huge_memory.h +3 -3

index d3572d4ef453..5dc71d292f47 100644
@@ -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;

mm/khugepaged.c +1 -0

index e7830761d3a2..7c8c48577408 100644
@@ -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;

mm/khugepaged.c static enum scan_result collapse_scan_pmd(struct mm_struct *mm, · line 1782

Quoted 8 lines

mm/khugepaged.c +1 -0

 	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;

Quoted 4 lines

mm/khugepaged.c +0 -0

 	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,

mm/khugepaged.c static enum scan_result collapse_scan_pmd(struct mm_struct *mm, · line 1793

Quoted 5 lines

mm/khugepaged.c +1 -1

 		*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.

Quoted 7 lines

mm/khugepaged.c +0 -1

 				     none_or_zero, result, unmapped);
 	return result;
 }
--

2.53.0

--
Cheers, Lorenzo

Quoted 53 lines · Lorenzo Stoakes (ARM)

Quoted 52 lines · Vernon Yang

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(-)

include/trace/events/huge_memory.h +3 -3

index d3572d4ef453..5dc71d292f47 100644
@@ -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;

mm/khugepaged.c +1 -0

index e7830761d3a2..7c8c48577408 100644
@@ -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;

Quoted 8 lines

mm/khugepaged.c static enum scan_result collapse_scan_pmd(struct mm_struct *mm, · line 1782

Quoted 8 lines

mm/khugepaged.c +1 -0

 	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;

You already defaulted the value to -1, better as:

	if (folio)
		pfn = folio_pfn(folio);

From: Vernon Yang <[email protected]>

The khugepaged tracepoints take a folio pointer and call folio_pfn(), but by then the folio may no longer be valid: freed after folio_put(), folio_unlock() or pte_unmap_unlock(), or not a folio at all but an xarray-encoded swap entry. On classic SPARSEMEM, dereferencing it oopses khugepaged as soon as the trace event is enabled; on other memory models it merely prints a bogus pfn.

Pass the pfn to the tracepoints directly, captured while the folio is still pinned, closing the use-after-free windows in mm_khugepaged_scan_file(), mm_khugepaged_scan_pmd() and mm_khugepaged_collapse_file().

Quoted 9 lines · Vernon Yang

From: Vernon Yang <[email protected]>

The khugepaged tracepoints take a folio pointer and call folio_pfn(), but by then the folio may no longer be valid: freed after folio_put(), folio_unlock() or pte_unmap_unlock(), or not a folio at all but an xarray-encoded swap entry. On classic SPARSEMEM, dereferencing it oopses khugepaged as soon as the trace event is enabled; on other memory models it merely prints a bogus pfn.

Quoted 4 lines

Pass the pfn to the tracepoints directly, captured while the folio is still pinned, closing the use-after-free windows in mm_khugepaged_scan_file(), mm_khugepaged_scan_pmd() and mm_khugepaged_collapse_file().

Well spotted!

Gave the series a run on x86_64 (KVM), all good (only classic SPARSEMEM untested) :)

+Cc Baolin

Quoted 16 lines · Lance Yang

Quoted 9 lines · Vernon Yang

From: Vernon Yang <[email protected]>

The khugepaged tracepoints take a folio pointer and call folio_pfn(), but by then the folio may no longer be valid: freed after folio_put(), folio_unlock() or pte_unmap_unlock(), or not a folio at all but an xarray-encoded swap entry. On classic SPARSEMEM, dereferencing it oopses khugepaged as soon as the trace event is enabled; on other memory models it merely prints a bogus pfn.

Quoted 4 lines

Pass the pfn to the tracepoints directly, captured while the folio is still pinned, closing the use-after-free windows in mm_khugepaged_scan_file(), mm_khugepaged_scan_pmd() and mm_khugepaged_collapse_file().

Quoted 4 lines

Well spotted!

Gave the series a run on x86_64 (KVM), all good (only classic SPARSEMEM untested) :)

Hmm ... stumbled over something else while testing this ...

With tmpfs mounted huge=advise, one MADV_HUGEPAGE isn't enough to get an unregistered mm onto khugepaged's list. Do it twice, and khugepaged starts scanning right away.

The pending flags make it into khugepaged just fine:

int hugepage_madvise(struct vm_area_struct *vma,
vm_flags_t *vm_flags, int advice)

{
	switch (advice) {
	case MADV_HUGEPAGE:
		*vm_flags &= ~VM_NOHUGEPAGE;
		*vm_flags |= VM_HUGEPAGE;
...
		khugepaged_enter_vma(vma, *vm_flags);
		break;
...
	}

	return 0;
}

and survive the common eligibility check:

unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
vm_flags_t vm_flags, enum tva_type type, unsigned long orders)
{
...

	/*
	 * Enabled via shmem mount options or sysfs settings.
	 * Must be done before hugepage flags check since shmem has its
	 * own flags.
	 */
	if (!in_pf && shmem_file(vma->vm_file))
		return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file),
						   vma, vma_start_pgoff(vma), 0,
						   forced_collapse);

...
}

But then the shmem helper reads them back from the VMA:

unsigned long shmem_allowable_huge_orders(struct inode *inode,
struct vm_area_struct *vma, pgoff_t index, loff_t write_end, bool shmem_huge_force)
{
...

	vm_flags_t vm_flags = vma ? vma->vm_flags : 0;

...
}

At that point vma->vm_flags still has the old value, so huge=advise quietly gives us no allowable order.

First madvise still succeeds, of course. The second one works because it finds VM_HUGEPAGE already installed by the first call.

Looked at history too ... we've been here before. 2cf1338454a8 ("mm: fix khugepaged with shmem_enabled=advise") fixed this exact ordering bug and tagged cd89fb065099 as the culprit. Then 6beeab870e70 was meant to be "No functional changes", but moving shmem_huge_global_enabled() into shmem_allowable_huge_orders() seems to have wired the stale vma->vm_flags read back in.

So AFAICT, this regressed in v6.12 with 6beeab870e70.

@Baolin, does that ring a bell? Any reason shmem_allowable_huge_orders() can't just take the pending vm_flags as well?

Hi Lance,

Quoted 5 lines · Lance Yang

+Cc Baolin

Quoted 2 lines · Lance Yang

On Sat, Aug 15, 2026 at 01:19:21PM +0800, Vernon Yang wrote:

Otherwise, I can fix it. Thanks for your report and analysis.

Quoted 5 lines · Baolin Wang

Hi Lance,

Quoted 2 lines · Lance Yang

+Cc Baolin

Quoted 3 lines

Quoted 3 lines · Lance Yang

Quoted 2 lines · Vernon Yang

[...]

Quoted 1 line

Otherwise, I can fix it. Thanks for your report and analysis.

Yeah, please go ahead :) Thanks, Baolin!

Quoted 9 lines

Quoted 4 lines

Pass the pfn to the tracepoints directly, captured while the folio is still pinned, closing the use-after-free windows in mm_khugepaged_scan_file(), mm_khugepaged_scan_pmd() and mm_khugepaged_collapse_file().

Well spotted!

Gave the series a run on x86_64 (KVM), all good (only classic SPARSEMEM untested) :)

Hmm ... stumbled over something else while testing this ...

Quoted 10 lines · On Sun, 16 Aug 2026 02:16:32 +0800 Lance Yang <[email protected]> wrote:

Quoted 9 lines

Quoted 4 lines

Pass the pfn to the tracepoints directly, captured while the folio is still pinned, closing the use-after-free windows in mm_khugepaged_scan_file(), mm_khugepaged_scan_pmd() and mm_khugepaged_collapse_file().

Well spotted!

Gave the series a run on x86_64 (KVM), all good (only classic SPARSEMEM untested) :)

Quoted 1 line

Hmm ... stumbled over something else while testing this ...

Sashiko might have found a couple of pre-existing things.

	https://sashiko.dev/#/patchset/[email protected]

Vernon, you may decide that the second one is on-topic for this patchset?