[PATCH] mm/compaction: guard move_freelist_head() against invalid freepage

[PATCH] mm/compaction: guard move_freelist_head() against invalid freepage

[Giorgi Tchankvetadze]

In fast_isolate_freepages(), freepage is declared uninitialized and
is only assigned a valid page pointer if list_for_each_entry_reverse
exits via break. If the loop runs to completion (all pages in the
freelist have pfn < min_pfn), freepage holds the list head sentinel
and high_pfn remains zero, so the high_pfn fallback does not update
it either.

The subsequent unconditional call to move_freelist_head(freelist,
freepage) then passes the sentinel as a page pointer, which is
invalid.

Guard move_freelist_head() inside the existing 'if (page)' block
where freepage is guaranteed to refer to a real page.

This issue was identified via Coccinelle (use_after_iter.cocci).

Fixes: 5a811889de10 ("mm, compaction: use free lists to quickly locate a migration target")
Cc: stable@vger.kernel.org
Signed-off-by: Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>
---
 mm/compaction.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 8f664fb09f24..320c082420fd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1611,11 +1611,10 @@ static void fast_isolate_freepages(struct compact_control *cc)
 			freepage = page;
 		}
 
-		/* Reorder to so a future search skips recent pages */
-		move_freelist_head(freelist, freepage);
-
-		/* Isolate the page if available */
 		if (page) {
+			/* Reorder so a future search skips recent pages */
+			move_freelist_head(freelist, freepage);
+			/* Isolate the page if available */
 			if (__isolate_free_page(page, order)) {
 				set_page_private(page, order);
 				nr_isolated = 1 << order;
-- 
2.52.0

Re: [PATCH] mm/compaction: guard move_freelist_head() against invalid freepage

[Andrew Morton]

On Mon,  1 Jun 2026 17:39:42 +0400 Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com> wrote:

> In fast_isolate_freepages(), freepage is declared uninitialized and
> is only assigned a valid page pointer if list_for_each_entry_reverse
> exits via break. If the loop runs to completion (all pages in the
> freelist have pfn < min_pfn), freepage holds the list head sentinel
> and high_pfn remains zero, so the high_pfn fallback does not update
> it either.
> 
> The subsequent unconditional call to move_freelist_head(freelist,
> freepage) then passes the sentinel as a page pointer, which is
> invalid.
> 
> Guard move_freelist_head() inside the existing 'if (page)' block
> where freepage is guaranteed to refer to a real page.

Seems correct from my reading.  That code is rather twisty.

> This issue was identified via Coccinelle (use_after_iter.cocci).

But AI review is worried:
	https://sashiko.dev/#/patchset/20260601133941.111989-2-giorgitchankvetadze1997@gmail.com

Re: [PATCH] mm/compaction: guard move_freelist_head() against invalid freepage

[Giorgi Tchankvetadze]

On Tue, Jun 2, 2026 at 12:44 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> Seems correct from my reading.  That code is rather twisty.
>
> > This issue was identified via Coccinelle (use_after_iter.cocci).
>
> But AI review is worried:
>         https://sashiko.dev/#/patchset/20260601133941.111989-2-giorgitchankvetadze1997@gmail.com
>
>

Hey Andrew. Thanks for the review. Yes, Sashiko is right. This seems
to be deeper than I initially thought.

My patch is too broad and skips the freelist rotation mechanism when
the scan reached the limit without finding a suitable page. In that
case page is NULL, but freepage still points to the last scanned real
entry, so the rotation should be preserved to avoid retrying the same
unsuitable tail entries.

I'm thinking of assigning freepage to NULL like this :
struct page *freepage = NULL;

and later checking:
if (freepage)
move_freelist_head(freelist, freepage);

Re: [PATCH] mm/compaction: guard move_freelist_head() against invalid freepage

[Vlastimil Babka (SUSE)]

On 6/1/26 15:39, Giorgi Tchankvetadze wrote:
> In fast_isolate_freepages(), freepage is declared uninitialized and
> is only assigned a valid page pointer if list_for_each_entry_reverse
> exits via break. If the loop runs to completion (all pages in the
> freelist have pfn < min_pfn), freepage holds the list head sentinel
> and high_pfn remains zero, so the high_pfn fallback does not update
> it either.
> 
> The subsequent unconditional call to move_freelist_head(freelist,
> freepage) then passes the sentinel as a page pointer, which is
> invalid.

I was wondering why we're not crashing horribly ever since the code was
written, as those condition have to happen a lot.

Looking at move_freelist_head() "if (!list_is_first(...))" will be true,
unless the freelist is empty.
Then luckily list_cut_before() has this very helpful comment:

 * If @entry == @head, all entries on @head are moved to
 * @list.

AFAICS that's what should be true here when freelist points to the sentinel.
I'll trust the comment and not try to reconstruct what the code is doing.

Back in move_freelist_head(), so we isolate all of freelist on the sublist,
then splice it back on the (temporarily empty) freelist.

I.e. I think it's harmless, but just accidentally, and it's useless pointer
churn. So very much worth a cleanup, but we don't need to rush a stable fix
here.

> Guard move_freelist_head() inside the existing 'if (page)' block
> where freepage is guaranteed to refer to a real page.
> 
> This issue was identified via Coccinelle (use_after_iter.cocci).
> 
> Fixes: 5a811889de10 ("mm, compaction: use free lists to quickly locate a migration target")
> Cc: stable@vger.kernel.org
> Signed-off-by: Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>
> ---
>  mm/compaction.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 8f664fb09f24..320c082420fd 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1611,11 +1611,10 @@ static void fast_isolate_freepages(struct compact_control *cc)
>  			freepage = page;
>  		}
>  
> -		/* Reorder to so a future search skips recent pages */
> -		move_freelist_head(freelist, freepage);
> -
> -		/* Isolate the page if available */
>  		if (page) {
> +			/* Reorder so a future search skips recent pages */
> +			move_freelist_head(freelist, freepage);
> +			/* Isolate the page if available */
>  			if (__isolate_free_page(page, order)) {
>  				set_page_private(page, order);
>  				nr_isolated = 1 << order;