Re: [PATCH v2] kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On 07-17 15:40, Michal Clapinski wrote:
> While booting with KHO, the following crash was observed:
> 
> BUG: unable to handle page fault for address: ff19164fffff8328
> RIP: 0010:__free_one_page+0x1a1/0x6b0
> Call Trace:
>  <TASK>
>  [<ffffffff913208bf>] free_one_page+0xaf/0x240
>  [<ffffffff93973288>] deferred_free_pages+0xa8/0xd0
>  [<ffffffff93971b4f>] deferred_init_memmap_chunk+0x10f/0x1b0
>  [<ffffffff9396e265>] padata_mt_helper+0x65/0xa0
>  [<ffffffff90fac402>] process_scheduled_works+0x202/0x410
>  [<ffffffff90fae739>] worker_thread+0x1f9/0x2d0
>  [<ffffffff90fb62fd>] kthread+0x27d/0x2f0
>  [<ffffffff90fae540>] ? __pfx_worker_thread+0x10/0x10
>  [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10
>  [<ffffffff90efdc55>] ret_from_fork+0x145/0x280
>  [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10
>  [<ffffffff90e2e46a>] ret_from_fork_asm+0x1a/0x30
>  </TASK>
> 
> deferred_init_memmap_chunk() interleaves initialization of struct pages
> with freeing them. This works fine without KHO because free regions
> will never be neighbors. However, with KHO, free memory will be split
> into (free && scratch) and (free && !scratch), which can be neighbors.
> 
> KHO scratch is aligned to CMA_MIN_ALIGNMENT_PAGES pages but buddy looks
> at the neighborhood of MAX_ORDER_NR_PAGES pages. These values are
> configurable but CMA_MIN_ALIGNMENT_PAGES is always less or equal to
> MAX_ORDER_NR_PAGES. In the crashing configuration they were set as
> follows:
> CMA_MIN_ALIGNMENT_PAGES = 1 << 9
> MAX_ORDER_NR_PAGES = 1 << 10
> 
> So while freeing one chunk, buddy accessed uninitialized struct pages
> from another chunk, tried to merge the blocks and crashed.
> 
> To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.
> 
> Fixes: c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
> Signed-off-by: Michal Clapinski <mclapinski@google.com>
> ---
> Unfortunately, this is very hard to catch, so I don't have a good
> reproducer. But I run the code with the fix through extensive testing
> and it seems fine.
> I think I've never caught it before because enabling HUGETLB is what
> causes CMA_MIN_ALIGNMENT_PAGES to be 1 << 9, instead of 1 << 10.
> 
> If this passes review, please cherry-pick it to 7.2 so we don't release
> a broken kernel.

A changelog would be super helful here.

Pasha

> ---
>  kernel/liveupdate/kexec_handover.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> index 4834a809985a..175c08a6e41e 100644
> --- a/kernel/liveupdate/kexec_handover.c
> +++ b/kernel/liveupdate/kexec_handover.c
> @@ -38,6 +38,16 @@
>  #include "../kexec_internal.h"
>  #include "kexec_handover_internal.h"
>  
> +/*
> + * This is the minimal alignment required by deferred struct page init.
> + * deferred_init_memmap_chunk frees memory to the buddy allocator, which looks
> + * at the neighboring pages (up to MAX_PAGE_ORDER) to merge them.
> + * If KHO scratch is not aligned to that value, buddy can access uninitialized
> + * struct pages, which can cause a crash.
> + */
> +#define SCRATCH_ALIGNMENT_BYTES (PAGE_SIZE * MAX_ORDER_NR_PAGES)
> +static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);
> +
>  /* The magic token for preserved pages */
>  #define KHO_PAGE_MAGIC 0x4b484f50U /* ASCII for 'KHOP' */
>  
> @@ -640,8 +650,8 @@ static void __init scratch_size_update(void)
>  	 * Scratch areas are released as MIGRATE_CMA. Round them up to the right
>  	 * size.
>  	 */
> -	scratch_size_lowmem = round_up(scratch_size_lowmem, CMA_MIN_ALIGNMENT_BYTES);
> -	scratch_size_global = round_up(scratch_size_global, CMA_MIN_ALIGNMENT_BYTES);
> +	scratch_size_lowmem = round_up(scratch_size_lowmem, SCRATCH_ALIGNMENT_BYTES);
> +	scratch_size_global = round_up(scratch_size_global, SCRATCH_ALIGNMENT_BYTES);
>  }
>  
>  static phys_addr_t __init scratch_size_node(int nid)
> @@ -656,7 +666,7 @@ static phys_addr_t __init scratch_size_node(int nid)
>  		size = scratch_size_pernode;
>  	}
>  
> -	return round_up(size, CMA_MIN_ALIGNMENT_BYTES);
> +	return round_up(size, SCRATCH_ALIGNMENT_BYTES);
>  }
>  
>  /**
> @@ -692,7 +702,7 @@ static void __init kho_reserve_scratch(void)
>  	 * next kernel
>  	 */
>  	size = scratch_size_lowmem;
> -	addr = memblock_phys_alloc_range(size, CMA_MIN_ALIGNMENT_BYTES, 0,
> +	addr = memblock_phys_alloc_range(size, SCRATCH_ALIGNMENT_BYTES, 0,
>  					 ARCH_LOW_ADDRESS_LIMIT);
>  	if (!addr) {
>  		pr_err("Failed to reserve lowmem scratch buffer\n");
> @@ -705,7 +715,7 @@ static void __init kho_reserve_scratch(void)
>  
>  	/* reserve large contiguous area for allocations without nid */
>  	size = scratch_size_global;
> -	addr = memblock_phys_alloc(size, CMA_MIN_ALIGNMENT_BYTES);
> +	addr = memblock_phys_alloc(size, SCRATCH_ALIGNMENT_BYTES);
>  	if (!addr) {
>  		pr_err("Failed to reserve global scratch buffer\n");
>  		goto err_free_scratch_areas;
> @@ -721,7 +731,7 @@ static void __init kho_reserve_scratch(void)
>  	 */
>  	for_each_node_state(nid, N_MEMORY) {
>  		size = scratch_size_node(nid);
> -		addr = memblock_alloc_range_nid(size, CMA_MIN_ALIGNMENT_BYTES,
> +		addr = memblock_alloc_range_nid(size, SCRATCH_ALIGNMENT_BYTES,
>  						0, MEMBLOCK_ALLOC_ACCESSIBLE,
>  						nid, true);
>  		if (!addr) {
> -- 
> 2.55.0.229.g6434b31f56-goog
>