Re: [PATCH v2 1/3] kho: walk bootmem areas in address order

[Mukesh Pilaniya]

Hi Mike,

On 04/11/26 10:20 pm, Mike Rapoport (Microsoft) wrote:
> kexec_add_buffer() users may depend on the order in which memory holes are
> searched. For instance, arm64 places the kernel bottom-up and then
> requires the DTB to be above the kernel.
> 
> kho_walk_bootmem() walks the bootmem areas in the order they were
> reserved, so if the kernel lands in a small area at the top of the memory
> there is no room left for the DTB and kexec fails. For example, on an
> arm64 VM with 2G of RAM and kho_bootmem=8M,64M,64M, the lowmem area is
> reserved first at the top of the memory, the kernel image takes most of
> it and kexec fails with
> 
>   kexec_file(Image): Could not find any suitable kernel location!
> 
> Sort the bootmem areas by address and walk them in ascending order, or in
> descending order for top-down searches, like kexec_walk_resources() does.
> 
> Fixes: 3bdecc3c93f9 ("kexec: add KHO support to kexec file loads")
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> ---
>  kernel/liveupdate/kexec_handover.c | 57 ++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> index 6f1f55fe31819..2ac5d94f33fd4 100644
> --- a/kernel/liveupdate/kexec_handover.c
> +++ b/kernel/liveupdate/kexec_handover.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/memblock.h>
>  #include <linux/page-isolation.h>
> +#include <linux/sort.h>
>  #include <linux/unaligned.h>
>  #include <linux/vmalloc.h>
>  
> @@ -820,6 +821,14 @@ bool kho_bootmem_overlap(phys_addr_t phys, size_t size)
>  	return false;
>  }
>  
> +static int __init kho_bootmem_cmp(const void *_a, const void *_b)
> +{
> +	const struct kho_bootmem *a = _a;
> +	const struct kho_bootmem *b = _b;
> +
> +	return cmp_int(a->addr, b->addr);
> +}
> +
>  /**
>   * kho_reserve_bootmem - Reserve a contiguous chunk of memory for kexec
>   *
> @@ -904,6 +913,9 @@ static void __init kho_reserve_bootmem(void)
>  	 */
>  	kho_bootmem_cnt = i;
>  
> +	sort(kho_bootmem, kho_bootmem_cnt, sizeof(*kho_bootmem),
> +	     kho_bootmem_cmp, NULL);
This only covers the case where this kernel allocated the areas itself.
On a KHO boot the array comes from the previous kernel. A kernel with
this series that is handed over from one without it therefore keeps
walking the areas in reservation order for every following
kexec_file_load, and the arm64 failure you describe comes back until the
next cold boot and vise versa because the ABI string is still
"kho-v4".

So, sorting once in kho_memory_init_early after kho_bootmem is
mapped would make the invariant independent of the producer.  When the
previous kernel does have the sort this is a no-op, but the consumer
cannot tell which kernel recorded the array, because the ABI string is
the same.

> +
>  	return;
>  
>  err_free_bootmem_areas:
> @@ -2081,20 +2093,40 @@ int kho_fill_kimage(struct kimage *image)
>  	return 0;
>  }
>  
> -static int kho_walk_bootmem(struct kexec_buf *kbuf,
> -			    int (*func)(struct resource *, void *))
> +static int kho_bootmem_walk_one(struct kexec_buf *kbuf,
> +				const struct kho_bootmem *area,
> +				int (*func)(struct resource *, void *))
> +{
> +	struct resource res = {
> +		.start = area->addr,
> +		.end = area->addr + area->size - 1,
> +	};
> +
> +	/* Try to fit the kbuf into our KHO bootmem region */
> +	return func(&res, kbuf);
> +}
> +
> +static int kho_bootmem_walk_top_down(struct kexec_buf *kbuf,
> +				     int (*func)(struct resource *, void *))
>  {
>  	int ret = 0;
> -	int i;
>  
> -	for (i = 0; i < kho_bootmem_cnt; i++) {
> -		struct resource res = {
> -			.start = kho_bootmem[i].addr,
> -			.end = kho_bootmem[i].addr + kho_bootmem[i].size - 1,
> -		};
> +	for (int i = kho_bootmem_cnt - 1; i >= 0; i--) {
kho_bootmem_cnt is unsigned, but the reverse loops in
kho_reserve_bootmem() and kho_bootmem_walk_top_down() start an int
counter at kho_bootmem_cnt - 1, which for a zero count is UINT_MAX
converted to int.  That conversion is implementation-defined; with the
current compilers it yields -1 and the loop body is skipped.

How about using this one.
	for (unsigned int i = kho_bootmem_cnt; i-- > 0;)

Reviewed-by: Mukesh Pilaniya <mpilaniy@xxxxxxxxxx>

> +		ret = kho_bootmem_walk_one(kbuf, &kho_bootmem[i], func);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
>  
> -		/* Try to fit the kimage into our KHO bootmem region */
> -		ret = func(&res, kbuf);
> +static int kho_bootmem_walk_bottom_up(struct kexec_buf *kbuf,
> +				      int (*func)(struct resource *, void *))
> +{
> +	int ret = 0;
> +
> +	for (int i = 0; i < kho_bootmem_cnt; i++) {
> +		ret = kho_bootmem_walk_one(kbuf, &kho_bootmem[i], func);
>  		if (ret)
>  			break;
>  	}
> @@ -2110,7 +2142,10 @@ int kho_locate_mem_hole(struct kexec_buf *kbuf,
>  	if (!kho_enable || kbuf->image->type == KEXEC_TYPE_CRASH)
>  		return 1;
>  
> -	ret = kho_walk_bootmem(kbuf, func);
> +	if (kbuf->top_down)
> +		ret = kho_bootmem_walk_top_down(kbuf, func);
> +	else
> +		ret = kho_bootmem_walk_bottom_up(kbuf, func);
>  
>  	return ret == 1 ? 0 : -EADDRNOTAVAIL;
>  }
> 

-- 
Regards,
Mukesh Pilaniya