Re: [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Speculative format label cache update is not rolled back on format I/O failure.
- [Medium] Incoherent ESE fulltrack heuristic state due to premature application using pre-format `is_ese()` state.
- [Medium] Unlocked data race on `private->ese_label` copy exposes read tearing to sysfs readers.

Pre-existing issues:
- [High] Unprivileged userspace can trigger a kernel panic by reading sysfs attributes during device bring-up (NULL pointer dereference).
- [High] Pre-existing logic error in `dasd_release_space()` causes a kernel panic on FBA/DIAG devices and bypasses ESE validation on ECKD.
--

--- Patch [10]: [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit 6e2aa2c452098583cebc915d1bb76216c96eecf8
Author: Stefan Haberland <sth@linux.ibm.com>
Subject: s390/dasd: Detect ESE volumes from the on-disk format label

This commit reads the format label from track 0 record 4 at device
bring-up and caches it. When a valid label is present, is_ese() is
derived from it instead of the hardware volume field. The cache is
refreshed after a format so the state stays coherent.

> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
> @@ -2473,9 +2473,10 @@ static ssize_t dasd_##_name##_show(struct device *dev,			\
>  									\
>  	return sysfs_emit(buf, "%d\n", val);			\
>  }									\
> -static DEVICE_ATTR(_name, 0444, dasd_##_name##_show, NULL);		\
> +static DEVICE_ATTR(_name, 0444, dasd_##_name##_show, NULL);
>  
> -DASD_DEFINE_ATTR(ese, device->discipline->is_ese);
> +DASD_DEFINE_ATTR(ese, device->discipline->ese_capable);
> +DASD_DEFINE_ATTR(on_demand_formatting, device->discipline->on_demand_format);

[Severity: High]
This is a pre-existing issue, but I noticed that unprivileged userspace
can trigger a kernel panic by reading these sysfs attributes during device
bring-up. 

In the dasd_##_name##_show macro, it checks device->discipline but fails to
verify that device->private is allocated. During dasd_generic_set_online(),
device->discipline is assigned before discipline->check_device() allocates
device->private. This creates a race window where dasd_eckd_ese_capable()
or dasd_eckd_on_demand_format() will unconditionally dereference a NULL
private pointer.

Is there a way to add a check for device->private in the sysfs macro or
defer attribute registration until initialization completes?

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -1673,13 +1673,53 @@ static int dasd_eckd_read_vol_info(struct dasd_device *device)
>  	return rc;
>  }
>  
> -static int dasd_eckd_is_ese(struct dasd_device *device)
> +/* Hardware/volume ESE capability, from the Volume Storage Query. */
> +static int dasd_eckd_ese_capable(struct dasd_device *device)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +
> +	return private->vsq.vol_info.ese;
> +}
> +
> +/*
> + * Whether the volume is to be handled as ESE (thin).
> + */
> +static int dasd_eckd_is_ese(struct dasd_device *device)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +
> +	if (private->ese_label_valid)
> +		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);

[Severity: Medium]
Is this state exposed to read tearing when sysfs attributes are accessed
concurrently with a format operation?

In dasd_eckd_fill_format_label(), the 512-byte dasd_format_label structure
is copied via struct assignment without holding any locks. Concurrently,
sysfs handlers call dasd_eckd_is_ese() which reads private->ese_label.flags
without synchronization. Could this unprotected copy allow a concurrent
sysfs read to fetch an intermediate or torn state?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at is_ese,
I noticed a pre-existing logic error in dasd_release_space() in
drivers/s390/block/dasd_ioctl.c that causes a kernel panic on FBA/DIAG
devices.

The check there evaluates:
if (!device->discipline->is_ese && !device->discipline->is_ese(device))

Since it uses a logical AND, if is_ese is NULL (true for !NULL), it will
evaluate the right side and attempt to call a NULL function pointer.
Conversely, for ECKD, !is_ese is false, short-circuiting the check and
bypassing the intended validation altogether.

Could this check be changed to use a logical OR instead?

[ ... ]

> @@ -2709,6 +2823,13 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
>  	label->format_tod = get_tod_clock();
>  	strscpy(label->driver_ver, DASD_ESE_LABEL_DRIVER,
>  		sizeof(label->driver_ver));
> +
> +	/*
> +	 * Populate the cache directly from the bytes just computed instead of
> +	 * synchronously reading them back from disk after the write lands.
> +	 */
> +	private->ese_label = *label;
> +	private->ese_label_valid = true;
>  }

[Severity: Medium]
Does this speculative cache update get rolled back if the format I/O fails?

The format process updates the in-memory cache private->ese_label and sets
private->ese_label_valid = true here during CCW request construction.
However, if the subsequent I/O submission fails in
dasd_eckd_format_process_data(), it propagates -EIO but does not appear
to revert this cache update.

Could this leave the sysfs attributes and the driver's internal state
permanently desynchronized with the actual on-disk label after a failure?

[ ... ]

> @@ -3208,7 +3329,7 @@ static int dasd_eckd_format_device(struct dasd_device *base,
>  	 * track 0; clear it so a later format that is not preceded by a full
>  	 * space release is recorded as a full format.
>  	 */
> -	if (fdata->start_unit == 0)
> +	if (fdata->start_unit == 0 && !rc)
>  		private->ese_format_quick = 0;
>  
>  	return rc;

[Severity: Medium]
In dasd_eckd_format_device(), dasd_ft_bias_apply() is called prior to the
format process. Since it evaluates the heuristic based on the old is_ese()
state, will this lead to an incoherent ESE fulltrack heuristic state?

For example, if a volume is copied from ESE to non-ESE hardware and is
fully reformatted (clearing the F_ESE label), dasd_ft_bias_apply() might
configure the heuristic based on the pre-existing label. After the format
completes and updates the cache, is_ese() may become false, but the
heuristic state machine would remain inappropriately active.

Should dasd_ft_bias_apply() be evaluated after the format completes and the
new label is committed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=10