Re: [PATCH v2 2/2] of: reserved_mem: allocate and map the reserved_mem array early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] of: reserved_mem: dynamic array allocation overlaps with static regions
--

commit 363ba30305c792dd8e994b354929ef72e89a362e
Author: Marek Szyprowski <m.szyprowski@samsung.com>

of: reserved_mem: allocate and map the reserved_mem array early

This commit replaces the statically sized reserved_mem array with a
dynamically allocated array using memblock_alloc_raw(). It maps the new array
early to handle architectures where the linear map is not yet available during
the early boot scan. The commit aims to eliminate the two-step initialization,
processing all regions during the early scan.

Link: https://lore.kernel.org/all/asc6brs_g0ky0wOM@kernel.org/

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 300fb236ab59f..51eea497a3192 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c

[ ... ]

> @@ -381,143 +372,89 @@ static void __init __rmem_check_for_overlap(void)
>  	}
>  }
>  

[ ... ]

>  /*
>   * fdt_scan_reserved_mem() - reserve and allocate memory occupied by
>   * reserved memory regions.
>   *
> - * This function is used to scan through the FDT and mark memory occupied
> - * by all static (defined by the "reg" property) reserved memory regions.
> - * Then memory for all dynamic regions (defined by size & alignment) is
> - * allocated, a region specific init function is called and region information
> - * is stored in the reserved_mem array.
> + * This function is used to scan through the FDT and count the number of
> + * reserved memory regions, so the reserved_mem array can be allocated with
> + * the exact size. Then all static (defined by the "reg" property) reserved
> + * memory regions are marked as reserved, memory for all dynamic regions
> + * (defined by size & alignment) is allocated, a region specific init function
> + * is called and region information is stored in the reserved_mem array.
>   */
>  int __init fdt_scan_reserved_mem(void)
>  {
> -	int node, child;
> -	int dynamic_nodes_cnt = 0, count = 0;
> -	int dynamic_nodes[MAX_RESERVED_REGIONS];
> +	struct reserved_mem *array, *map;
> +	size_t array_bytes;
> +	int node, child, cnt;
>  	const void *fdt = initial_boot_params;
>  
>  	node = fdt_path_offset(fdt, "/reserved-memory");
> -	if (node < 0) {
> -		total_reserved_mem_cnt = 0;
> +	if (node < 0)
>  		return -ENODEV;
> -	}
>  
>  	if (__reserved_mem_check_root(node) != 0) {
>  		pr_err("Reserved memory: unsupported node format, ignoring\n");
> -		total_reserved_mem_cnt = 0;
>  		return -EINVAL;
>  	}
>  
> -	fdt_for_each_subnode(child, fdt, node) {
> -		const char *uname;
> -		int err;
> +	/* First pass: count the entries needed for the reserved_mem array */
> +	cnt = count_reserved_mem_nodes(fdt, node);
> +	if (!cnt)
> +		return 0;
>  
> -		if (!of_fdt_device_is_available(fdt, child))
> -			continue;
> +	array_bytes = array_size(cnt, sizeof(*array));
> +	array = memblock_alloc_raw(array_bytes, SMP_CACHE_BYTES);

[Severity: High]
Does allocating the array here introduce a regression by creating a race
with static reserved memory regions?

Because the static regions have not yet been marked as reserved in memblock,
memblock_alloc_raw() considers their physical addresses available and might
allocate the array overlapping a device's static reserved region.

> +	if (!array) {
> +		pr_err("Failed to allocate memory for reserved_mem array\n");
> +		return -ENOMEM;
> +	}
>  
> -		uname = fdt_get_name(fdt, child, NULL);
> +	map = reserved_mem_array_map(array, array_bytes);
> +	if (!map) {
> +		pr_err("Failed to map memory for reserved_mem array\n");
> +		memblock_free(array, array_bytes);
> +		return -ENOMEM;
> +	}
> +	memset(map, 0, array_bytes);
>  
> -		err = __reserved_mem_reserve_reg(child, uname);
> -		if (!err)
> -			count++;
> +	reserved_mem = map;
> +	total_reserved_mem_cnt = cnt;
>  
> -		/*
> -		 * Save the nodes for the dynamically-placed regions
> -		 * into an array which will be used for allocation right
> -		 * after all the statically-placed regions are reserved
> -		 * or marked as no-map. This is done to avoid dynamically
> -		 * allocating from one of the statically-placed regions.
> -		 */
> -		if (err != -ENOENT || !of_get_flat_dt_prop(child, "size", NULL))
> +	/* Second pass: reserve and initialize the static regions */
> +	fdt_for_each_subnode(child, fdt, node) {
> +		if (!of_fdt_device_is_available(fdt, child))
>  			continue;
>  
> -		if (dynamic_nodes_cnt == MAX_RESERVED_REGIONS) {
> -			pr_err("too many defined dynamic regions, skip '%s'\n",
> -			       uname);
> +		__reserved_mem_reserve_reg(child,
> +					   fdt_get_name(fdt, child, NULL));
> +	}

If the memblock array allocation overlaps a no-map static region,
early_init_dt_reserve_memory() inside __reserved_mem_reserve_reg() will
detect the overlap, return -EBUSY, and fail the device's reservation.

If the region lacks no-map, memblock_reserve() will silently merge the
reservations. The kernel's reserved_mem array will then reside inside the
device's reserved memory, which could lead to memory corruption when the
device driver later uses this memory.

Can the memblock allocation be delayed until after the static regions are
reserved?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008152403.766439-1-m.szyprowski@samsung.com?part=2