Re: [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lockless reads of `uid.real_unit_addr` in I/O submission paths can observe transient zeros due to `memset()` in `create_uid()`, causing CCWs to misdirect I/O to unit 0.

Pre-existing issues:
- [High] Concurrent device reconfiguration can set `conf.ned` to NULL after it is validated, leading to a NULL pointer dereference in `create_uid()`.
--

--- Patch [13]: [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit 9a16f7344a5a15020b53dfe98bcf28e857e3119a
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Read cached unit address and LSS in the CCW build path

This commit updates the CCW build path to read the cached unit address
and LSS instead of the configuration data directly, to avoid a
use-after-free when conf.ned is reallocated concurrently.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 571a693a33a3c..1a7d40e45d963 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -587,8 +587,9 @@ static int prefix_LRE(struct ccw1 *ccw, struct PFX_eckd_data *pfxdata,
>  		return -EINVAL;
>  	}
>  	pfxdata->format = format;
> -	pfxdata->base_address = basepriv->conf.ned->unit_addr;
> -	pfxdata->base_lss = basepriv->conf.ned->ID;
> +	/* cached copies - conf.ned may be freed under us by the reload worker */
> +	pfxdata->base_address = basepriv->uid.real_unit_addr;

[Severity: High]
Can this lockless read observe transient zeros and misdirect I/O to unit 0?

When building a CCW for a PAV alias device, this executes under the alias
device's lock, leaving the base device lock unacquired. Concurrently, a
configuration reload on the base device triggers dasd_eckd_generate_uid(),
which takes the base device's lock and calls create_uid().

Looking at create_uid(), it zeroes the entire structure in-place:

drivers/s390/block/dasd_eckd.c:create_uid() {
	memset(uid, 0, sizeof(struct dasd_uid));
	...
}

Since this CCW build path operates locklessly with respect to the base
device, won't it read a transient zero for real_unit_addr, leading to
silent data corruption on volume 0 and missing data on the intended volume?

> +	pfxdata->base_lss = basepriv->ned_lss;
>  	pfxdata->validity.define_extent = 1;
>  
>  	/* private uid is kept up to date, conf_data may be outdated */

[ ... ]

> @@ -805,6 +806,8 @@ static int dasd_eckd_generate_uid(struct dasd_device *device)
>  		return -ENODEV;
>  	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
>  	create_uid(&private->conf, &private->uid);

[Severity: High]
This is a pre-existing issue, but does this sequence introduce a TOCTOU race
leading to a NULL pointer dereference?

dasd_eckd_generate_uid() locklessly checks private->conf.ned right before
acquiring the ccwdev_lock to call create_uid() here.

Concurrently, path verification without locks can clear conf->ned:

do_pe_handler_work()
  -> rebuild_device_uid()
    -> dasd_eckd_identify_conf_parts()
         conf->ned = NULL;

If a thread context switches between the lockless check and lock acquisition
here, won't create_uid() dereference the NULL conf->ned pointer?

drivers/s390/block/dasd_eckd.c:create_uid() {
	...
	memcpy(uid->vendor, conf->ned->HDA_manufacturer, ...);
	...
}

> +	/* cache the LSS for the lockless CCW-build path (see ned_lss) */
> +	private->ned_lss = private->conf.ned->ID;
>  	spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
>  	return 0;
>  }

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