Re: [PATCH] s390: raw3270: Handle allocation failures

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Freeing `rp` on the `ccw_device_enable_console()` error path leaves a dangling pointer in the global `raw3270_devices` list.
- [Medium] The newly added allocation error paths leak the previously created `cdev` console device.
- [Medium] Adding complex memory allocation failure handling to an `__init` function is unnecessary and violates early boot guidelines.
--

--- Patch [1]: [PATCH] s390: raw3270: Handle allocation failures ---
commit 461fe23d860b7a48d2ce561a29cd0a5780d12446
Author: Triet Hoang <[email protected]>

s390: raw3270: Handle allocation failures

This commit adds return value checks for memory allocations in
raw3270_setup_console() to prevent NULL pointer dereferences. It also
adds error handling to free allocated resources on subsequent failures.

> diff --git a/drivers/s390/char/raw3270.c b/drivers/s390/char/raw3270.c
> index aa9c4d81225cb..69e0f69a3e149 100644
> --- a/drivers/s390/char/raw3270.c
> +++ b/drivers/s390/char/raw3270.c
> @@ -814,15 +814,26 @@ struct raw3270 __init *raw3270_setup_console(void)
>  		return ERR_CAST(cdev);
>  
>  	rp = kzalloc_obj(*rp, GFP_KERNEL | GFP_DMA);
> +	if (!rp)
> +		return ERR_PTR(-ENOMEM);
> +	ascebc = kzalloc(256, GFP_KERNEL);
> +	if (!ascebc) {
> +		kfree(rp);
> +		return ERR_PTR(-ENOMEM);
> +	}

[Severity: Medium]
Since raw3270_setup_console() is an __init function executed early during
boot, is it necessary to handle these memory allocation failures?

Usually, at this stage, if the system is out of memory it cannot boot anyway.
Adding complex error handling to early init paths can introduce dead code or
accidental regressions.

[Severity: Medium]
If this error handling is kept, does returning ERR_PTR(-ENOMEM) directly here
leak the console device (cdev) that was created earlier in the function?

ccw_device_create_console() allocates cdev at the start of
raw3270_setup_console(). It looks like ccw_device_destroy_console(cdev)
should be called before returning here to avoid leaking the device and its
associated resources.

>  	rc = raw3270_setup_device(cdev, rp, ascebc);
> -	if (rc)
> +	if (rc) {
> +		kfree(rp->ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
> +	}
>  	set_bit(RAW3270_FLAGS_CONSOLE, &rp->flags);
>  
>  	rc = ccw_device_enable_console(cdev);
>  	if (rc) {
>  		ccw_device_destroy_console(cdev);
> +		kfree(rp->ascebc);
> +		kfree(rp);
>  		return ERR_PTR(rc);
>  	}

[Severity: High]
Does freeing rp here leave a dangling pointer in the global raw3270_devices
list?

In raw3270_setup_device(), the successfully initialized rp is linked into the
global list:

drivers/s390/char/raw3270.c:raw3270_setup_device() {
    ...
    if (rp->minor == -1 && minor < RAW3270_MAXDEVS + RAW3270_FIRSTMINOR) {
        rp->minor = minor;
        list_add_tail(&rp->list, &raw3270_devices);
    }
    ...
}

If ccw_device_enable_console(cdev) subsequently fails, the added error path
calls kfree(rp) without first acquiring raw3270_mutex and unlinking rp with
list_del(). On the next list iteration, this will cause memory corruption and
a kernel panic.

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