Re: [PATCH v3] 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:
- [High] The logic to conditionally register the initrd resource is entirely dead code because it is gated by a condition that unconditionally evaluates to false.
- [Medium] Using `strstr()` to parse `retain_initrd` from the command line incorrectly matches substrings like `retain_initrd=0`, creating a state mismatch with generic kernel memory management.
--

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

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

This commit adds the initrd memory range to the standard resources in
/proc/iomem on s390. It conditionally registers it as a child of
iomem_resources if retain_initrd is specified on the boot command line.

> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index b60284328fe3d..45b0e19bdab2c 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
[ ... ]
>  static void __init setup_resources(void)
>  {
>  	struct resource *res, *std_res, *sub_res;
> +	bool retain_initrd = false;
>  	phys_addr_t start, end;
>  	int j;
>  	u64 i;

[ ... ]

> @@ -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 (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && retain_initrd) {

[Severity: High]
Since retain_initrd is initialized to false at the start of setup_resources()
and hasn't been modified before this check, will this condition always
evaluate to false?

It looks like this block is completely unreachable, which would prevent the
initrd resource from being populated or registered.

> +		unsigned long initrd_addr, initrd_size;
> +	
> +		if (strstr(boot_command_line, "retain_initrd"))

[Severity: Medium]
If the unreachable code issue above is addressed, is there a risk that using
strstr() will incorrectly match strings like "retain_initrd=0"?

The generic kernel code in init/initramfs.c interprets "retain_initrd=0" as
disabled and frees the initrd memory back to the buddy allocator. If this
string match catches "retain_initrd=0", would we end up registering freed
memory as an IORESOURCE_BUSY initrd resource? 

This mismatch could mislead userspace tools about system RAM availability.

> +			retain_initrd = true;
> +		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;
> +		}
> +	}

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