Re: [PATCH v7 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: data race in scsi_rescan_device() due to missing state_mutex
- [High] scsi: data race on Scsi_Host bitfield in scsi_probe_lun() clobbers tmf_in_progress
- [Medium] scsi: bitfield data race in lun_in_cdb on live device rescan
--

commit 0c2ca40c3d438ceb73cba9f08912149462c2d8ba
Author: Brian Bunker <brian@purestorage.com>
scsi: core: Add device reprobe support to scsi_rescan_device()

Updates INQUIRY data on rescan and calls device_reprobe() if the peripheral
qualifier or device type changes. Handles ALUA unavailable state.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 93a8719fcf265..2c848265c9b9c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
[ ... ]
> @@ -815,14 +820,14 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
[ ... ]
> -	if (sdev->inquiry_len < 36) {
> +	if (inquiry_len < 36) {
>  		if (!sdev->host->short_inquiry) {
>  			shost_printk(KERN_INFO, sdev->host,
>  				    "scsi scan: INQUIRY result too short (%d),"
> -				    " using 36\n", sdev->inquiry_len);
> +				    " using 36\n", inquiry_len);
>  			sdev->host->short_inquiry = 1;

[Severity: High]
Because this function is now called for a live device by scsi_rescan_device()
(via scsi_refresh_inquiry()), could this assignment to the short_inquiry
bitfield race with updates to adjacent bitfields in struct Scsi_Host?

The short_inquiry bitfield is in the same machine word as tmf_in_progress
and other dynamic host states.

Since we only hold the device_lock() here and not the shost->host_lock, a
concurrent update to tmf_in_progress, such as:

scsi_error.c:scsi_ioctl_reset() {
    ...
    shost->tmf_in_progress = 1;
    ...
}

might be clobbered by the unlocked read-modify-write of the bitfield word,
potentially breaking host error recovery.

[ ... ]
> @@ -850,19 +855,25 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
[ ... ]
> +	/*
> +	 * scsi_rescan_device() calls this function for a live device, so
> +	 * only write lun_in_cdb if it changes. Clearing it first and then
> +	 * setting it again could expose a wrong value to the I/O path.
> +	 */
> +	if (sdev->lun_in_cdb != lun_in_cdb)
> +		sdev->lun_in_cdb = lun_in_cdb;

[Severity: Medium]
Could this conditional bitfield assignment cause a data race during a live
rescan?

Although the check avoids writing when the value hasn't changed, if it does
evaluate to true, writing to sdev->lun_in_cdb will perform a non-atomic
read-modify-write of the 32-bit word containing it. 

This word also holds sdev->cdl_enable, which can be modified concurrently by
sysfs writes without holding the device_lock:

scsi_sysfs.c:sdev_store_cdl_enable() {
    ...
    sdev->cdl_enable = enable;
    ...
}

If a sysfs write overlaps with this update, the sysfs state might be
silently overwritten.

[ ... ]
> @@ -1677,12 +1736,42 @@ int scsi_rescan_device(struct scsi_device *sdev)
[ ... ]
> +	if (need_reprobe) {
> +		/*
> +		 * The peripheral qualifier or device type changed, so the
> +		 * driver binding has to be re-evaluated. device_reprobe()
> +		 * takes the device lock itself. sdev stays valid while the
> +		 * lock is dropped because the caller holds a reference;
> +		 * scsi_evt_thread() is covered by the cancel_work_sync() in
> +		 * scsi_device_dev_release().
> +		 */
> +		device_unlock(dev);
> +		ret = device_reprobe(dev);
> +		device_lock(dev);
> +
> +		if (sdev->sdev_state == SDEV_CANCEL ||
> +		    sdev->sdev_state == SDEV_DEL) {
> +			ret = -ENODEV;
> +		} else if (ret < 0) {
> +			if (scsi_device_set_state(sdev, SDEV_OFFLINE) == 0)

[Severity: High]
Is it safe to call scsi_device_set_state() here without holding the
state_mutex?

scsi_device_set_state() performs a non-atomic read-modify-write of
sdev->sdev_state. If this races with a concurrent device removal
(__scsi_remove_device), the removal path acquires state_mutex and sets
SDEV_CANCEL or SDEV_DEL.

If scsi_rescan_device() concurrently calls scsi_device_set_state() without
state_mutex, it could read SDEV_RUNNING, allow the removal path to transition
the device to SDEV_CANCEL, and then overwrite that state with SDEV_OFFLINE.

Since SDEV_OFFLINE still permits scsi_device_get(), subsequent callers could
successfully grab a reference to a deleted device and use a destroyed request
queue, potentially leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014220014.94330-1-brian@purestorage.com?part=4