Re: [PATCH v2] s390/iomem: register initrd as an iomem resource when retain_initrd is set

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Imprecise substring matching for 'retain_initrd' creates a split-brain state with the core kernel, causing freed initrd memory to remain falsely marked as BUSY in the iomem resource map.
- [Medium] When CONFIG_BLK_DEV_INITRD is disabled, a bogus 1-byte initrd resource at address 0x0 is incorrectly registered if 'retain_initrd' matches in the boot command line.
--

--- Patch [1]: [PATCH v2] s390/iomem: register initrd as an iomem resource when retain_initrd is set ---
commit 0f4223b27c45aff988c662d5fb36a4161cc6901c
Author: Joseph Cathcart <[email protected]>

s390/iomem: register initrd as an iomem resource when retain_initrd is set

This patch unconditionally adds initrd to the standard_resources array and
conditionally registers it as an iomem resource if the retain_initrd boot
parameter is present.

> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index b60284328fe3d..cd61e286628a0 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
[ ... ]
> @@ -501,6 +508,19 @@ static void __init setup_resources(void)
>  	bss_resource.start = __pa_symbol(__bss_start);
>  	bss_resource.end = __pa_symbol(__bss_stop) - 1;
>  
> +	if (strstr(boot_command_line, "retain_initrd"))
> +		retain_initrd = true;

[Severity: Medium]
Is this substring match too broad? If a user passes "retain_initrd=0" or
"no_retain_initrd" on the command line, this will still evaluate to true.

The core kernel will correctly evaluate the exact parameter and free the
initrd memory, but s390 setup might then register the freed memory as an
IORESOURCE_BUSY initrd resource in /proc/iomem. 

Will this leave a dangling resource entry covering memory that is actively
reused by the buddy allocator?

> +	if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && retain_initrd) {
> +		unsigned long initrd_addr, initrd_size;
> +	
> +		if (get_physmem_reserved(RR_INITRD, &initrd_addr, &initrd_size)) {
> +			initrd_resource.start = initrd_addr;
> +			initrd_resource.end = initrd_resource.start + initrd_size - 1;
> +		} else {
> +			retain_initrd = false;
> +		}
> +	}

[Severity: Medium]
What happens here if CONFIG_BLK_DEV_INITRD is disabled and retain_initrd
matched in the boot command line?

Since this block is skipped entirely, retain_initrd is never reset to
false. Won't the subsequent standard resources loop then process an
uninitialized initrd_resource (which defaults to 0-bounds), resulting in
a bogus 1-byte initrd resource registered at physical address 0x0?

> +
>  	for_each_mem_range(i, &start, &end) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1