[PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Vernon Yang]

From: Vernon Yang <yanglincheng@kylinos.cn>

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: stable@vger.kernel.org
Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
---
 include/trace/events/huge_memory.h |  6 +++---
 mm/khugepaged.c                    | 14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
index 291fae364c62..d3572d4ef453 100644
--- a/include/trace/events/huge_memory.h
+++ b/include/trace/events/huge_memory.h
@@ -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)
@@ -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;
 		__assign_str(filename);
 		__entry->present = present;
 		__entry->swap = swap;
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 617bca76db49..e7830761d3a2 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -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;
 
 	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;
 		}
 
@@ -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);
+	}
 	rcu_read_unlock();
+
 	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);
 	return result;
 }
 
-- 
2.53.0

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Lorenzo Stoakes (ARM)]

On Sat, Aug 15, 2026 at 01:19:22PM +0800, Vernon Yang wrote:
> From: Vernon Yang <yanglincheng@kylinos.cn>
>
> 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.

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.

>
> 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: stable@vger.kernel.org
> Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> ---
>  include/trace/events/huge_memory.h |  6 +++---
>  mm/khugepaged.c                    | 14 +++++++++-----
>  2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> index 291fae364c62..d3572d4ef453 100644
> --- a/include/trace/events/huge_memory.h
> +++ b/include/trace/events/huge_memory.h
> @@ -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)
> @@ -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.

>  		__assign_str(filename);
>  		__entry->present = present;
>  		__entry->swap = swap;
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 617bca76db49..e7830761d3a2 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -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.

>
>  	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;
>  		}
>
> @@ -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.

>  	rcu_read_unlock();
> +

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

>  	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?

>  	return result;
>  }
>
> --
> 2.53.0
>

--
Cheers, Lorenzo

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[David Hildenbrand (Arm)]

> 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

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Lorenzo Stoakes (ARM)]

On Mon, Aug 17, 2026 at 06:19:16PM +0200, David Hildenbrand (Arm) wrote:
>
> > 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.

See my reply-to-self. My initial reply was wrong because parisc + mips do
actually do classic sparsemem (sigh).

I suggest what you're saying here basically.

(Moving folio_put()'s for the break cases out of the loop is nasty)

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Lorenzo Stoakes (ARM)]

On Mon, Aug 17, 2026 at 05:10:39PM +0100, Lorenzo Stoakes (ARM) wrote:
> On Sat, Aug 15, 2026 at 01:19:22PM +0800, Vernon Yang wrote:
> > From: Vernon Yang <yanglincheng@kylinos.cn>
> >
> > 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.
>
> 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).

>
> But this needs fixing in general anyway.
>
> >
> > 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: stable@vger.kernel.org
> > Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> > ---
> >  include/trace/events/huge_memory.h |  6 +++---
> >  mm/khugepaged.c                    | 14 +++++++++-----
> >  2 files changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> > index 291fae364c62..d3572d4ef453 100644
> > --- a/include/trace/events/huge_memory.h
> > +++ b/include/trace/events/huge_memory.h
> > @@ -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)
> > @@ -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.
>
> >  		__assign_str(filename);
> >  		__entry->present = present;
> >  		__entry->swap = swap;
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 617bca76db49..e7830761d3a2 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -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).

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


> >  			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;
> >  		}
> >
> > @@ -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.
>
> >  	rcu_read_unlock();
> > +
>
> Stray newline added, let's not do that for a backported fix please :)
>
> >  	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?

Yeah as above this isn't right.

>
> >  	return result;
> >  }
> >
> > --
> > 2.53.0
> >
>
> --
> Cheers, Lorenzo

--
Cheers, Lorenzo

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Vernon Yang]

On Mon, Aug 17, 2026 at 05:23:30PM +0100, Lorenzo Stoakes (ARM) wrote:
> On Mon, Aug 17, 2026 at 05:10:39PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Sat, Aug 15, 2026 at 01:19:22PM +0800, Vernon Yang wrote:
> > > From: Vernon Yang <yanglincheng@kylinos.cn>
> > >
> > > 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.
> >
> > 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).
>
> >
> > But this needs fixing in general anyway.
> >
> > >
> > > 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: stable@vger.kernel.org
> > > Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> > > ---
> > >  include/trace/events/huge_memory.h |  6 +++---
> > >  mm/khugepaged.c                    | 14 +++++++++-----
> > >  2 files changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> > > index 291fae364c62..d3572d4ef453 100644
> > > --- a/include/trace/events/huge_memory.h
> > > +++ b/include/trace/events/huge_memory.h
> > > @@ -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)
> > > @@ -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.
> >
> > >  		__assign_str(filename);
> > >  		__entry->present = present;
> > >  		__entry->swap = swap;
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index 617bca76db49..e7830761d3a2 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -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).
>
> >
> > >
> > >  	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.

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?

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 79effd3f3da4..00337405c0e0 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -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;
 			/*
@@ -2779,7 +2781,8 @@ 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, (!folio || xa_is_value(folio)) ? -1 : pfn,
+				      file, present, swap, result);
 	return result;
 }

--
Cheers,
Vernon

>
> > >  			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;
> > >  		}
> > >
> > > @@ -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.
> >
> > >  	rcu_read_unlock();
> > > +
> >
> > Stray newline added, let's not do that for a backported fix please :)
> >
> > >  	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?
>
> Yeah as above this isn't right.
>
> >
> > >  	return result;
> > >  }
> > >
> > > --
> > > 2.53.0
> > >
> >
> > --
> > Cheers, Lorenzo
>
> --
> Cheers, Lorenzo
>

Re: [PATCH v2 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Lorenzo Stoakes (ARM)]

On Fri, Aug 21, 2026 at 04:17:55PM +0800, Vernon Yang wrote:
> On Mon, Aug 17, 2026 at 05:23:30PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Mon, Aug 17, 2026 at 05:10:39PM +0100, Lorenzo Stoakes (ARM) wrote:
> > > On Sat, Aug 15, 2026 at 01:19:22PM +0800, Vernon Yang wrote:
> > > > From: Vernon Yang <yanglincheng@kylinos.cn>
> > > >
> > > > 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.
> > >
> > > 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).
> >
> > >
> > > But this needs fixing in general anyway.
> > >
> > > >
> > > > 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: stable@vger.kernel.org
> > > > Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> > > > ---
> > > >  include/trace/events/huge_memory.h |  6 +++---
> > > >  mm/khugepaged.c                    | 14 +++++++++-----
> > > >  2 files changed, 12 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> > > > index 291fae364c62..d3572d4ef453 100644
> > > > --- a/include/trace/events/huge_memory.h
> > > > +++ b/include/trace/events/huge_memory.h
> > > > @@ -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)
> > > > @@ -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.
> > >
> > > >  		__assign_str(filename);
> > > >  		__entry->present = present;
> > > >  		__entry->swap = swap;
> > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > > index 617bca76db49..e7830761d3a2 100644
> > > > --- a/mm/khugepaged.c
> > > > +++ b/mm/khugepaged.c
> > > > @@ -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).
> >
> > >
> > > >
> > > >  	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.
>
> 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.

Ah you're right of course!

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

Yeah... :)

>
> To keep it simple, I modified it as follows. Is this OK?
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 79effd3f3da4..00337405c0e0 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -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;
>  			/*
> @@ -2779,7 +2781,8 @@ 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, (!folio || xa_is_value(folio)) ? -1 : pfn,
> +				      file, present, swap, result);
>  	return result;
>  }

OK looks reasonable, thanks! :)

>
> --
> Cheers,
> Vernon
>

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

[Vernon Yang]

From: Vernon Yang <yanglincheng@kylinos.cn>

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@vger.kernel.org
Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
---
 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,
 				     none_or_zero, result, unmapped);
 	return result;
 }
-- 
2.53.0

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

[Lorenzo Stoakes (ARM)]

On Sat, Aug 15, 2026 at 01:19:23PM +0800, Vernon Yang wrote:
> From: Vernon Yang <yanglincheng@kylinos.cn>
>
> 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@vger.kernel.org
> Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> ---
>  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.

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

--
Cheers, Lorenzo

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@kylinos.cn>
>>
>> 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@vger.kernel.org
>> Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
>> ---
>>  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

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

[Lorenzo Stoakes (ARM)]

On Mon, Aug 17, 2026 at 06:20:02PM +0200, David Hildenbrand (Arm) wrote:
> On 8/17/26 18:11, Lorenzo Stoakes (ARM) wrote:
> > 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. :)

This is moot because you have to store the PFN for lifetime for museum-piece
architectures.

But anyway FWIW:

Yes I agree accessing a now-invalid folio isn't great, but the reason I dislike
this pattern is that you're tracking this state in a different way that now has
maintenance overhead (if anybody reassigns folio or does weird control flow
etc. they must now update 2 things and etc.)

For the sake of CONFIG_SPARSEMEM and tracing for a value that is not at all
intuitive as to what it means, that's not great.

Let me go back and review this again accounting for the lifetime issue that I
missed first time.

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo

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

[Lorenzo Stoakes]

On Mon, Aug 17, 2026 at 05:12:01PM +0100, Lorenzo Stoakes (ARM) wrote:
> On Sat, Aug 15, 2026 at 01:19:23PM +0800, Vernon Yang wrote:
> > From: Vernon Yang <yanglincheng@kylinos.cn>
> >
> > 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@vger.kernel.org
> > Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> > ---
> >  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;

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

	if (folio)
		pfn = folio_pfn(folio);

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

Disregard, I didn't account for lifetime.

>
> >  				     none_or_zero, result, unmapped);
> >  	return result;
> >  }
> > --
> > 2.53.0
> >
>
> --
> Cheers, Lorenzo

--
Cheers, Lorenzo

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

[Vernon Yang]

On Mon, Aug 17, 2026 at 05:33:40PM +0100, Lorenzo Stoakes wrote:
> On Mon, Aug 17, 2026 at 05:12:01PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Sat, Aug 15, 2026 at 01:19:23PM +0800, Vernon Yang wrote:
> > > From: Vernon Yang <yanglincheng@kylinos.cn>
> > >
> > > 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@vger.kernel.org
> > > Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> > > ---
> > >  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;
>
> You already defaulted the value to -1, better as:
>
> 	if (folio)
> 		pfn = folio_pfn(folio);
>

LGTM, Thanks!

--
Cheers,
Vernon

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

[Vernon Yang]

From: Vernon Yang <yanglincheng@kylinos.cn>

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@vger.kernel.org
Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
---
 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 5dc71d292f47..1b301d4e2dba 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 7c8c48577408..7df0fd2d2fdc 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2247,6 +2247,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);
@@ -2626,6 +2627,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);
 
 	/*
@@ -2664,12 +2666,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