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

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 15.10.2026 10:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] of: memory corruption due to initializing unreserved memory regions
> - [Medium] of: memory leak on dynamically-placed nomap region init failure
> --
>
> commit cc45d1ed473ae1f1390908145c8217f0d9614ac0
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
>
>     of: reserved_mem: allocate and map the reserved_mem array early
>     
>     Get rid of the static, limited-size reserved_mem array and replace it
>     with array allocated by memblock_alloc_raw() and accessed directly or
>     through a temporary early_memremap() mapping.
>
>     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..0fd1683cba011 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -381,143 +384,109 @@ static void __init __rmem_check_for_overlap(void)
> [ ... ]
>> +	/* Second pass: initialize the static regions */
>> +	fdt_for_each_subnode(child, fdt, node) {
>>  		if (!of_fdt_device_is_available(fdt, child))
>>  			continue;
>>  
>> -		uname = fdt_get_name(fdt, child, NULL);
>> -
>> -		err = __reserved_mem_reserve_reg(child, uname);
>> -		if (!err)
>> -			count++;
>> +		__reserved_mem_init_reg(child, fdt_get_name(fdt, child, NULL));
>> +	}
> [Severity: High]
> Does this unconditionally initialize memory regions even if they failed
> reservation in the first pass?
>
> If __reserved_mem_reserve_reg() fails for a region, it sets
> reserved_mem_reserve_failed = true but does not exclude the node. When
> __reserved_mem_init_reg() is called here, it will invoke
> fdt_init_reserved_mem_node() for that node anyway.
>
> Could this lead to blindly adding unreserved physical memory to the
> reserved_mem array, allowing drivers to silently overwrite standard memory
> that the page allocator considers free?
>
> Additionally, if the node initialization subsequently fails (e.g. due to a
> missing compatible match), could it execute memblock_phys_free() or
> memblock_clear_nomap() on memory it never successfully owned?


This is preexisting issue, this patch only moves the node initialization calls.
The better error handling has been proposed in the following patchset:

https://lore.kernel.org/all/20260920092852.614973-1-chenwandun1@gmail.com/


If the new memblock flag is not an option, the error handling should use
the simple, best effort approach proposed in:

https://lore.kernel.org/all/72074a49-2963-4f96-b939-79bf95a829cb@samsung.com/
https://lore.kernel.org/all/15f70a45-a754-488c-8ba3-9e63bb956033@samsung.com/

what is still out of scope of this patch.


> drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
>     ...
> 	err = __reserved_mem_init_node(rmem, node);
> 	if (err != 0 && err != -ENODEV) {
> 		pr_info("node %s compatible matching fail\n", rmem->name);
> 		rmem->name = NULL;
>
> 		if (nomap)
> 			memblock_clear_nomap(rmem->base, rmem->size);
> 		else
> 			memblock_phys_free(rmem->base, rmem->size);
> 		return;
>     ...
> }
>
>>  
>> -		/*
>> -		 * 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))
>> -			continue;
>> +	/*
>> +	 * Allocate the dynamically-placed regions only after all the
>> +	 * statically-placed ones are reserved, to avoid overlapping with them.
>> +	 */
>> +	fdt_for_each_subnode(child, fdt, node) {
>> +		int len;
>>  
>> -		if (dynamic_nodes_cnt == MAX_RESERVED_REGIONS) {
>> -			pr_err("too many defined dynamic regions, skip '%s'\n",
>> -			       uname);
>> +		if (!of_fdt_device_is_available(fdt, child) ||
>> +		    (of_flat_dt_get_addr_size_prop(child, "reg", &len) && len))
>>  			continue;
>> -		}
>>  
>> -		dynamic_nodes[dynamic_nodes_cnt] = child;
>> -		dynamic_nodes_cnt++;
>> -	}
>> -	for (int i = 0; i < dynamic_nodes_cnt; i++) {
>> -		const char *uname;
>> -		int err;
>> -
>> -		child = dynamic_nodes[i];
>> -		uname = fdt_get_name(fdt, child, NULL);
>> -		err = __reserved_mem_alloc_size(child, uname);
>> -		if (!err)
>> -			count++;
>> +		__reserved_mem_alloc_size(child,
>> +					  fdt_get_name(fdt, child, NULL));
>>  	}
> [Severity: Medium]
> If a dynamically-placed 'no-map' reserved memory region fails initialization
> here, does it permanently leak memory from memblock.reserved?
>
> When __reserved_mem_alloc_size() allocates memory via
> memblock_phys_alloc_range(), the memory is added to memblock.reserved. It
> then calls fdt_init_reserved_mem_node(), which has the following error
> handling path:
>
> drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
>     ...
> 	err = __reserved_mem_init_node(rmem, node);
> 	if (err != 0 && err != -ENODEV) {
> 		pr_info("node %s compatible matching fail\n", rmem->name);
> 		rmem->name = NULL;
>
> 		if (nomap)
> 			memblock_clear_nomap(rmem->base, rmem->size);
> 		else
> 			memblock_phys_free(rmem->base, rmem->size);
> 		return;
>     ...
> }
>
> For 'no-map' regions, this clears the nomap flag but appears to omit
> memblock_phys_free(rmem->base, rmem->size), which would leave the dynamically
> allocated region permanently trapped in memblock.reserved.

Again, this is preexisting issue targeted in the mentioned Wandun's patchset.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland