Re: [PATCH v7 16/19] s390/dasd: Detect ESE volumes from the on-disk format label

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

New issues:
- [Medium] The in-memory ESE format label cache (`ese_label_valid`) is not invalidated when the on-disk label is overwritten or erased without a new label being explicitly written.
- [Medium] Lockless read and non-atomic update of the `ese_label` cache causes data races and potential torn reads.

Pre-existing issues:
- [Critical] Heap buffer overflow in `dasd_eckd_build_format()` when formatting multiple tracks with intensity 0x01.
- [High] Pre-existing NULL pointer dereference in `dasd_release_space()` for devices without an `is_ese` discipline function.
--

--- Patch [16]: [PATCH v7 16/19] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit ac9e4583836d81406ee7f0dc251d245f5892a7de
Author: Stefan Haberland <[email protected]>
Subject: s390/dasd: Detect ESE volumes from the on-disk format label

Read the format label from track 0 record 4 at device bring-up and cache
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 is_ese()
stays coherent without an offline/online cycle.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 7a27dc08fb961..68736f9efcaf2 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -1676,7 +1676,8 @@ static int dasd_eckd_read_vol_info(struct dasd_device *device)
> +static int dasd_eckd_is_ese(struct dasd_device *device)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +
> +	/* sysfs may read this during set_online before private is allocated */
> +	if (!private)
> +		return 0;
> +
> +	if (private->ese_label_valid)
> +		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);

[Severity: Medium]
Does this code allow a data race when reading the label flags?

Looking at dasd_eckd_is_ese() and dasd_eckd_on_demand_format(), they read
private->ese_label_valid and private->ese_label.flags without locks or
memory barriers.

If a concurrent update happens in dasd_eckd_fill_format_label() during a
format operation, could this lead to a torn read and transiently incorrect
sysfs output?

> +
> +	return dasd_eckd_ese_capable(device);
> +}

[ ... ]

> @@ -2733,7 +2847,12 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
>  	label->format_tod = get_tod_clock();
>  	strscpy(label->kernel_version, init_utsname()->release,
>  		sizeof(label->kernel_version));
> +
> +	/*
> +	 * 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 code update the 512-byte label cache safely?

Since this struct copy is not atomic, could concurrent readers in
dasd_eckd_is_ese() observe an inconsistent state?

>  }

[ ... ]

> @@ -3227,23 +3353,35 @@ static int dasd_eckd_format_device(struct dasd_device *base,
> -	if (fdata->start_unit == 0)
> -		private->ese_format_quick = 0;
> +	private->ese_format_quick = 0;
> +
> +	/*
> +	 * A full format returns the device to a fully sparse state and has just
> +	 * committed a fresh label; restart the heuristic from ft1 on the now
> +	 * current is_ese state, without an offline cycle.
> +	 */
> +	dasd_ft_bias_apply(base);
> +
>  	return rc;
>  }

[Severity: Medium]
Does this code properly invalidate the label cache when the label is erased?

In dasd_eckd_format_device(), the success path clears ese_format_quick but
does not clear ese_label_valid when track 0 is formatted with a non-CDL
layout (intensity 0) where no label is explicitly written.

Similarly, space release successfully unallocates all tracks (erasing
track 0) in dasd_eckd_release_space_full() but fails to invalidate the
label cache.

Could this cause the driver to retain a stale label cache and incorrectly
apply ESE-specific performance heuristics to non-ESE devices?


Additional observations about related pre-existing code:

[Severity: High]
This is a pre-existing issue, but does dasd_release_space() in
drivers/s390/block/dasd_ioctl.c have a NULL pointer dereference?

If a device uses a discipline that does not implement the is_ese callback,
such as FBA:

    if (!device->discipline->is_ese && !device->discipline->is_ese(device))

Could this logic flaw trigger a panic when a user issues the release space
ioctl, since it uses a logical AND instead of OR?


[Severity: Critical]
This is a pre-existing issue, but does dasd_eckd_build_format() in
drivers/s390/block/dasd_eckd.c have a heap buffer overflow?

When formatting multiple tracks with intensity 0x01, the memory is allocated
for the CCW chain using:

    cplength = 2 + rpt * nr_tracks;

However, the generation loop consumes an extra CCW per track for record zero,
meaning it consumes 1 + rpt CCWs per track.

If a user requests a format spanning multiple tracks, could this deficit
cause an out-of-bounds write into the dasd_alloc_chunk heap memory pool?

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