Re: [PATCH v9 3/8] crash_core: serialize crash header preparation against hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] crash_core: cmem->ranges overflow via unlocked iomem_resource hotplug
--

commit 24cd8bf08b74ae3e07f992972d3cd97b77d9fcf6
Author: Wandun Chen <chenwandun@lixiang.com>

crash_core: serialize crash header preparation against hotplug

This commit serializes crash_prepare_headers() with device_hotplug_lock
to prevent concurrent memory hotplug from changing the number of memory
ranges between counting and populating cmem->ranges. It also introduces
__crash_prepare_headers() for hotplug paths that already hold the lock.

Link: https://sashiko.dev/#/message/20260806101002.1F84E1F000E9@smtp.kernel.org

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index faa0f22dbb41d..8a861a946e261 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
[ ... ]
> @@ -344,8 +345,8 @@ int crash_exclude_core_ranges(struct crash_mem **cmem)
>  	return 0;
>  }
>  
> -int crash_prepare_headers(int need_kernel_map, void **addr, unsigned long *sz,
> -			  unsigned long *nr_mem_ranges)
> +int __crash_prepare_headers(int need_kernel_map, void **addr, unsigned long *sz,
> +			    unsigned long *nr_mem_ranges)
>  {
>  	unsigned int max_nr_ranges;
>  	struct crash_mem *cmem;
> @@ -382,6 +383,18 @@ int crash_prepare_headers(int need_kernel_map, void **addr, unsigned long *sz,
>  	return ret;
>  }
>  
> +int crash_prepare_headers(int need_kernel_map, void **addr, unsigned long *sz,
> +			  unsigned long *nr_mem_ranges)
> +{
> +	int ret;
> +
> +	lock_device_hotplug();
> +	ret = __crash_prepare_headers(need_kernel_map, addr, sz, nr_mem_ranges);
> +	unlock_device_hotplug();
> +
> +	return ret;
> +}

[Severity: Medium]
Can cmem->ranges[] still overflow during __crash_prepare_headers() even when
device_hotplug_lock is held?

In __crash_prepare_headers(), arch_get_system_nr_ranges() counts the current
system ram ranges (plus a fixed number of split slots for exclusions),
allocates cmem via alloc_cmem(max_nr_ranges), and then populates
cmem->ranges[] in arch_crash_populate_cmem().

Neither the x86 callback in arch/x86/kernel/crash.c:

static int prepare_elf64_ram_headers_callback(struct resource *res, void *arg)
{
	struct crash_mem *cmem = arg;

	cmem->ranges[cmem->nr_ranges].start = res->start;
	cmem->ranges[cmem->nr_ranges].end = res->end;
	cmem->nr_ranges++;

	return 0;
}

nor the weak implementation in kernel/crash_core.c:

int __weak arch_crash_populate_cmem(struct crash_mem *cmem)
{
	phys_addr_t start, end;
	u64 i;

	for_each_mem_range(i, &start, &end) {
		cmem->ranges[cmem->nr_ranges].start = start;
		cmem->ranges[cmem->nr_ranges].end = end - 1;
		cmem->nr_ranges++;
	}
	return 0;
}

checks whether cmem->nr_ranges has reached cmem->max_nr_ranges before writing
to cmem->ranges[cmem->nr_ranges++].

However, Xen balloon memory hotplug inserts new system ram resources into
iomem_resource before acquiring device_hotplug_lock via the call chain:

balloon_thread() / xen_alloc_ballooned_pages()
  -> add_ballooned_pages()
    -> reserve_additional_memory()
      -> additional_memory_resource()

Specifically, in drivers/xen/balloon.c:

static struct resource *additional_memory_resource(phys_addr_t size)
{
	...
	res->name = "System RAM";
	res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;

	ret = allocate_resource(&iomem_resource, res,
				size, 0, -1,
				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
	...
}

static enum bp_state reserve_additional_memory(void)
{
	...
	resource = additional_memory_resource(balloon_hotplug * PAGE_SIZE);
	if (!resource)
		goto err;
	...
	mutex_unlock(&balloon_mutex);
	/* add_memory_resource() requires the device_hotplug lock */
	lock_device_hotplug();
	rc = add_memory_resource(nid, resource, MHP_MERGE_RESOURCE);
	unlock_device_hotplug();
	mutex_lock(&balloon_mutex);
	...
	balloon_stats.total_pages += balloon_hotplug;
	...
}

Because reserve_additional_memory() inserts the resource into iomem_resource
and drops balloon_mutex before waiting on lock_device_hotplug(), and only
updates balloon_stats.total_pages after add_memory_resource() completes,
concurrent callers (such as balloon_thread() and xen_alloc_ballooned_pages())
can insert new system ram entries into iomem_resource while
crash_prepare_headers() or arch_crash_handle_hotplug_event() holds
device_hotplug_lock.

If those entries are inserted between arch_get_system_nr_ranges() and
arch_crash_populate_cmem(), wouldn't the extra ranges consume the reserved
split slots (causing crash_exclude_mem_range() to fail) or overflow
cmem->ranges[]? Should arch_crash_populate_cmem() and
prepare_elf64_ram_headers_callback() also bounds-check cmem->nr_ranges
against cmem->max_nr_ranges?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021083011.2170525-1-chenwandun1@gmail.com?part=3