Re: [PATCH v6 15/18] s390/dasd: Detect ESE volumes from the on-disk format label

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The in-memory ESE label cache (`private->ese_label_valid`) is not invalidated when track 0 is formatted without an ESE label, leaving the driver with a stale ESE state that no longer matches the physical disk.

Pre-existing issues:
- [High] Fatal NULL pointer dereference in `dasd_release_space()` for devices whose discipline lacks the `is_ese` method (e.g., FBA or DIAG devices).
--

--- Patch [15]: [PATCH v6 15/18] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit d17ebb7bd94d36c62afdabb020d82a8a18a64bba
Author: Stefan Haberland <[email protected]>
Subject: s390/dasd: Detect ESE volumes from the on-disk format label

This commit updates the DASD driver to detect Extent Space Efficient (ESE)
volumes by reading the format label from track 0 during device bring-up. It
caches this label and derives the ESE status from it, allowing volumes copied
from ESE storage to maintain their thin-provisioned handling on different
hardware.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index ebffb3364c33c..35a970e006fd0 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3224,23 +3353,35 @@ static int dasd_eckd_format_device(struct dasd_device *base,
>  	struct dasd_eckd_private *private = base->private;
>  	int rc;
>  
>  	rc = dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
>  					   0, NULL);
> +	if (fdata->start_unit != 0)
> +		return rc;
> +
> +	if (rc) {
> +		/*
> +		 * The format failed, so the label cached speculatively during
> +		 * CCW build may not match the disk; drop it so is_ese() falls
> +		 * back to the hardware field until the next successful format
> +		 * or bring-up.
> +		 */
> +		private->ese_label_valid = false;
> +		return rc;
> +	}

[Severity: Medium]
When track 0 is formatted, what happens if no ESE label is written?

If a user issues the BIODASDFMT ioctl with start_unit == 0 and a non-CDL
layout (e.g., intensity & 0x08 == 0), dasd_eckd_build_format() might not
write a new label. The physical label on track 0 is then zeroed or destroyed
by the format.

However, dasd_eckd_format_device() only clears private->ese_label_valid on
failure. Could this regression leave the driver using a stale ESE state that
no longer matches the physical disk after a successful format?

[ ... ]
> @@ -4241,6 +4382,14 @@ static int dasd_eckd_release_space_trks(struct dasd_device *device,
>  static int dasd_eckd_release_space(struct dasd_device *device,
>  				   struct format_data_t *rdata)
>  {
> +	/*
> +	 * Space release (and thus a quick format) requires real ESE hardware.
> +	 * is_ese() may be true from a copied label on non-ESE hardware, so gate
> +	 * on the hardware capability, not on is_ese().
> +	 */
> +	if (!dasd_eckd_ese_capable(device))
> +		return -EOPNOTSUPP;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the ioctl path for space
release have a NULL pointer dereference for devices without an is_ese method?

In drivers/s390/block/dasd_ioctl.c, dasd_release_space() contains this check:

    if (!device->discipline->is_ese && !device->discipline->is_ese(device))
        return -ENOTSUPP;

Since it uses a logical AND (&&) instead of OR (||), if the discipline (e.g.,
FBA or DIAG) lacks the is_ese method, the first operand evaluates to true.

Will this force the evaluation of the second operand, immediately
dereferencing the NULL function pointer when a user issues the BIODASDRAS
ioctl?

>  	if (rdata->intensity & DASD_FMT_INT_ESE_FULL)
>  		return dasd_eckd_release_space_full(device);

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