Re: [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label

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

New issues:
- [High] Unconditional dereference of `device->private` in `dasd_eckd_is_ese()` causes a kernel panic when accessed via the `full_track_bias` sysfs attribute on an initializing or offline device.
- [High] Valid on-disk ESE label cache is incorrectly invalidated on early format failures, breaking thin-provisioning state.
- [Medium] Concurrent execution of dasdfmt and sysfs reads leads to data races and torn reads on the 512-byte `ese_label` structure.
- [Medium] The `ese_label_valid` cache is not invalidated when track 0 is formatted with LDL (intensity 0x00) or invalidate-track, leaving stale in-memory state.

Pre-existing issues:
- [High] The `dasd_release_space()` ioctl helper triggers a NULL pointer dereference when called on a device whose discipline does not implement the `is_ese()` callback (e.g., FBA devices).
--

--- Patch [14]: [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit 6689b09c2730c7fa8fae2e18e3bbf4f3671425e2
Author: Stefan Haberland <[email protected]>

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 to determine if a volume is ESE. It also updates sysfs
attributes and format handling to reflect the cached label state, ensuring
volumes copied off ESE storage are still handled as thin.

> 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
> @@ -1686,6 +1687,49 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
>  	return private->vsq.vol_info.ese;
>  }
>  
> +/*
> + * Whether the volume is to be handled as ESE (thin). This reflects the state
> + * of the data, not the hardware: a volume copied off ESE storage onto other
> + * hardware still needs ESE handling. The on-disk format label is authoritative
> + * when present; without it (e.g. a volume formatted by an older driver) fall
> + * back to the hardware ESE field.
> + *
> + * Only the F_ESE flag gates this. An ESE volume is thin regardless of whether
> + * it was quick- or full-formatted (tracks are allocated on write, and discard
> + * re-thins a full one).
> + */
> +static int dasd_eckd_is_ese(struct dasd_device *device)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +
> +	if (private->ese_label_valid)

[Severity: High]
Can this cause a NULL pointer dereference if device->private is not yet
allocated?

The full_track_bias_store() sysfs function can indirectly call
dasd_eckd_is_ese() during the initialization window before private is
allocated, or if allocation fails:

full_track_bias_store()
  dasd_ft_bias_apply()
    dasd_eckd_is_ese()
      private->ese_label_valid

> +		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);
> +
> +	return dasd_eckd_ese_capable(device);
> +}

[ ... ]

> @@ -2730,7 +2843,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 introduce a data race that could lead to torn reads?

Sysfs readers accessing on_demand_formatting or full_track_bias can call
dasd_eckd_is_ese(), which reads private->ese_label.flags locklessly. If a
format is concurrently updating this 512-byte structure here, could a
reader observe ese_label_valid == true but see stale or partially written
flags?

[ ... ]

> @@ -3224,23 +3349,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: High]
Does this incorrectly drop the valid in-memory label state on early format
failures?

If dasd_eckd_format_process_data() fails early due to sanity checks (like
invalid parameters where stop_unit is out of bounds) before any I/O is
issued, rc will be non-zero and the cache is cleared. Would this cause
subsequent writes to unallocated tracks to fail with 'No Record Found'
instead of triggering on-demand formatting?

[Severity: Medium]
Does this leave a stale cache in memory if the format succeeds but destroys
the on-disk label?

If track 0 is formatted using the LDL format mode (intensity == 0x00), it
skips writing the label but rc will be 0. Would this leave the old
ese_label_valid state incorrectly set to true?

[ ... ]

While reviewing these changes, I also noticed a pre-existing issue in the
dasd ioctl handling:

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

If a device lacks the is_ese callback (such as dasd_fba), this logic:

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

evaluates to true for the first condition when the pointer is NULL, and
then attempts to call the NULL pointer in the second condition.

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