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@xxxxxxxxxxxxxxx
> Signed-off-by: Giorgi Tchankvetadze <giorgitchankvetadze1997@xxxxxxxxx>
> ---
>  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;