Re: [PATCH 2/2] xen/mm: Fix offlining pages only make aligned buddies, fixes Xen crash

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.05.2026 16:47, Bernhard Kaindl wrote:
> reserve_offline_pages() is missing an alignment check and thus
> has a relatively high probability of growing unaligned buddies.
> 
> Fix this by checking alignment before growing spans to the next order.
> Update the test case to verify the fix and prevent future regressions.
> 
> Fixes: e4865c2315 ('Page offline support in Xen side')
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>
> ---
>  tools/tests/native/offline-unaligned.c | 92 --------------------------
>  xen/common/page_alloc.c                |  5 ++
>  2 files changed, 5 insertions(+), 92 deletions(-)

As we will want to backport the bugfix (without the test), and as it makes
little sense ...

> --- a/tools/tests/native/offline-unaligned.c
> +++ b/tools/tests/native/offline-unaligned.c
> @@ -17,38 +17,6 @@
>   *
>   * Copyright (C) 2026 Cloud Software Group
>   */
> -#include "harness/common.h"
> -
> -/* test_bss_start must be first in the BSS segment */
> -void __aligned(PAGE_SIZE) *test_bss_start;
> -
> -/* Include xen/mm.h so we can wrap page_list_del() to assert the corruption. 
> */
> -#define TEST_WRAP_XEN_INCLUDE_XEN_MM_H
> -#include "harness/mm-wrapper.h"
> -
> -static bool expect_free_list_corruption;
> -
> -/*
> - * Wrap page_list_del() to not fail the test by virtue of the prepared
> - * free list state but continue the test like a running Xen instance
> - * would in many cases. Assert and expect the corruption, and continue.
> - */
> -static inline void wrap_page_list_del(struct page_info *page,
> -                                      struct page_list_head *head)
> -{
> -    printf("page_list_del: page MFN %lu, order %u\n",
> -           mfn_x(page_to_mfn(page)), PFN_ORDER(page));
> -
> -    if ( expect_free_list_corruption )
> -        EXPECT_FAIL_BEGIN();
> -    CHECK(page->list.next && page->list.prev, "The free list is corrupt 
> now!");
> -    if ( expect_free_list_corruption )
> -        EXPECT_FAIL_END(1);
> -
> -    if ( page->list.next && page->list.prev )
> -        page_list_del(page, head);
> -}
> -#define page_list_del(page, head) wrap_page_list_del(page, head)
>  
>  /*
>   * Include the main test library that sets up scenarios, asserts
> @@ -84,78 +52,18 @@ static void test_unaligned_buddy_merge(int start_mfn)
>       * | offlined page | single page     |    head page with a tail page    |
>       * +---------------+-----------------+-----------------+----------------+
>       */
> -    EXPECT_FAIL_BEGIN();
> -    /*
> -     * Due to a bug in reserve_offlined_page(), we get an unaligned buddy:
> -     * +---------------+-----------------+-----------------+----------------+
> -     * | offlined page |     head page with a tail page    | single page    |
> -     * +---------------+-----------------+-----------------+----------------+
> -     */
>      CHECK(page_aligned(pg + 1), "The buddy #%lu is not aligned to order-%d",
>            mfn_x(page_to_mfn(pg + 1)), PFN_ORDER(pg + 1));
> -    EXPECT_FAIL_END(1);
>  
>      /* Allocate and free a page to trigger buddy merging on free. */
> -
> -    /*
> -     * After allocating and freeing MFN 7, we get a double-freed MFN 6 due
> -     * to aligned predecessor merging in free_heap_pages():
> -     *
> -     *         MFN 4             MFN 5             MFN 6            MFN 7
> -     *   +---------------+-----------------+-----------------+
> -     *   | offlined page |    head page         tail page    |
> -     *   |               |       Unaligned buddies are       |
> -     *   |               |      an invariant violation!      |
> -     *   
> +---------------+-----------------+-----------------+----------------+
> -     *                                     |    head page        tail page   
>  |
> -     *                                     
> +-----------------+----------------+
> -     */
> -    expect_free_list_corruption = true;
>      free_domheap_pages(alloc_domheap_pages(dom1, order0, 0), order0);
> -
> -    /*
> -     * At this point, the free list is already corrupt. In free_heap_pages(),
> -     * the tail of the unaligned buddy was added to the free list a 2nd time
> -     * as the page of an overlapping aligned buddy. This is per design of the
> -     * algorithm: These pages are free and thus the merging occurs as 
> expected.
> -     *
> -     * The next allocation allocates the tail of the unaligned buddy, which
> -     * is now, due to the merge, also the head of the new aligned buddy.
> -     */
>      CHECK((pg = alloc_domheap_pages(dom1, order1, 0)), "Alloc the order-1 
> pg");
>  
>      /* Inspect the predecessor (pg is the tail of the unaligned buddy) */
> -    EXPECT_FAIL_BEGIN();
> -    /*
> -     * After allocating two more pages, MFN 6 is free AND in-use:
> -     *
> -     *         MFN 4             MFN 5             MFN 6            MFN 7
> -     *   +---------------+-----------------+-----------------+
> -     *   | offlined page |    head page         tail page    |
> -     *   
> +---------------+-----------------+-----------------+----------------+
> -     *                                     |    in-use page      in-use page 
>  |
> -     *                                     
> +-----------------+----------------+
> -     */
>      CHECK(page_aligned(pg - 1), "The buddy #%lu is not aligned to order-%d!",
>            mfn_x(page_to_mfn(pg - 1)), PFN_ORDER(pg - 1));
> -    EXPECT_FAIL_END(1);
>  
>      /* Allocate the remaining page; a clean heap should not hit BUG(). */
> -    testcase_assert_expect_to_hit_bug = true;
> -    /*
> -     * As described above, if pg is the tail of an unaligned order-1 buddy,
> -     * the unaligned buddy is still on the free list and this allocation will
> -     * remove it from the free list and check alloc_heap_pages() checks the
> -     * buddies to have a reference count of zero, and the already allocated
> -     * page is returned as the tail of the unaligned buddy, causing the 
> BUG().
> -     *
> -     *         MFN 4             MFN 5             MFN 6            MFN 7
> -     *   +---------------+-----------------+-----------------+
> -     *   | offlined page |    head page         tail page    | <- panic's Xen
> -     *   
> +---------------+-----------------+-----------------+----------------+
> -     *                                     |    in-use page      in-use page 
>  |
> -     *                                     
> +-----------------+----------------+
> -     */
>      alloc_domheap_pages(dom1, order0, 0); /* Triggers BUG() */
>  }

... to first add a test covering the bad behavior (reporting it as good, by way
of the test succeeding), I think the actual bugfix (below wants to come first,
with the new test then being added to check for correct behavior right away.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1434,6 +1434,11 @@ static int reserve_offlined_page(struct page_info 
> *head)
>              if ( (cur_head + (1 << next_order)) >= (head + ( 1 << 
> head_order)) )
>                  goto merge;
>  
> +            /* Do not grow to next_order if cur_head is not aligned to it. */
> +            if ( (mfn_x(page_to_mfn(cur_head)) & ((1UL << next_order) - 1)) )
> +                goto merge;

With this isolated, with the title changed to something which can be parsed
and doesn't duplicate "fix" as a word, and with the excess parentheses removed
from the if()'s expression:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, I'd like to suggest a possible simplification: Inductively we know
that cur_head is aligned to cur_order. Since next_order == cur_order + 1

            if ( mfn_x(page_to_mfn(cur_head)) & (1UL << cur_order) )
                goto merge;

ought to suffice? Of course if desired this could be written more explicitly
as

            if ( mfn_x(page_to_mfn(cur_head)) & (1UL << (next_order - 1)) )
                goto merge;

Yet overall I'd be tempted to drop the next_order variable altogether anyway
(not in this patch of course).

> +            /* Check if any page in the next_order range is offlined. */

This isn't quite accurate, as ...

>              for ( i = (1 << cur_order), pg = cur_head + (1 << cur_order );

... we start at 1 << cur_order. I.e. it's only the upper half of the range
covered by next_order which is being checked.

>                    i < (1 << next_order);
>                    i++, pg++ )

Jan