Re: [PATCH v8 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: core: broken device_reprobe() error check in scsi_rescan_device()
- [High] scsi: core: NULL deref in scsi_rescan_device() before device_add()
- [Medium] scsi: core: scsi_probe_lun() misuses sdev->inquiry_len on rescan
--

commit 09090dd4acc8a64408156c003c7bd95cb8781601
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Add device reprobe support to scsi_rescan_device()

This commit adds INQUIRY data refresh and device reprobe support to
scsi_rescan_device() when the peripheral qualifier or device type changes.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 93a8719fcf265..87fd3ba5c3cc0 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c

[ ... ]

> @@ -1658,9 +1674,54 @@ int scsi_resume_device(struct scsi_device *sdev)
>  }
>  EXPORT_SYMBOL(scsi_resume_device);
>  
> +/**
> + * scsi_refresh_inquiry - Fetch fresh standard INQUIRY data for a device
> + * @sdev: The SCSI device to refresh
> + *
> + * Failures are logged and otherwise ignored, so that the caller carries on
> + * with the rest of the rescan using the INQUIRY data it already has.
> + *
> + * Returns true if the peripheral qualifier or device type changed and the
> + * device needs to be reprobed.
> + */
> +static bool scsi_refresh_inquiry(struct scsi_device *sdev)
> +{
> +	unsigned char *inq_result;
> +	blist_flags_t bflags;
> +	int ret = -ENOMEM;
> +
> +	inq_result = kmalloc(256, GFP_KERNEL);
> +	if (inq_result) {
> +		ret = scsi_probe_lun(sdev, inq_result, 256, &bflags, true);

[Severity: Medium]
Does calling scsi_probe_lun() on a live sdev cause it to misinterpret
sdev->inquiry_len?

During initial probing in scsi_probe_and_add_lun(), sdev->inquiry_len is 0
unless shost->hostt->sdev_init() configured a maximum INQUIRY transfer
length. Once probed, scsi_probe_and_add_lun() and scsi_update_inquiry_data()
overwrite sdev->inquiry_len with the valid INQUIRY response length (at least
36 bytes).

When scsi_refresh_inquiry() calls scsi_probe_lun() on a live device,
sdev->inquiry_len holds the previous response length rather than 0 or a
low-level driver limit. In scsi_probe_lun():

	first_inquiry_len = sdev->inquiry_len ? sdev->inquiry_len : 36;
	try_inquiry_len = first_inquiry_len;
	pass = 1;
	...
		if (pass == 1) {
			if (BLIST_INQUIRY_36 & *bflags)
				next_inquiry_len = 36;
			else if (sdev->inquiry_len &&
				 response_len > sdev->inquiry_len &&
				 (inq_result[2] & 0x7) < 6) /* SPC-4 */
				next_inquiry_len = sdev->inquiry_len;

If a pre-SPC-4 device initially returned a short INQUIRY (for example, 36
bytes while in PQ=1 / ALUA unavailable state) and later reports a larger
response_len on rescan, sdev->inquiry_len is non-zero, so next_inquiry_len is
capped at the old sdev->inquiry_len and pass 2 is skipped.

Conversely, if the device previously returned more than 36 bytes, pass 1
starts at that larger sdev->inquiry_len instead of 36 bytes, and if that
transfer fails after a state change, scsi_probe_lun() returns -EIO without
falling back to a 36-byte inquiry.

> +		if (ret >= 0)
> +			ret = scsi_update_inquiry_data(sdev, inq_result, ret);
> +		kfree(inq_result);
> +	}
> +
> +	if (ret < 0) {
> +		sdev_printk(KERN_WARNING, sdev,
> +			    "failed to refresh INQUIRY data: %d\n", ret);
> +		return false;
> +	}
> +
> +	return ret == SCSI_INQ_REPROBE_NEEDED;
> +}
> +
> +/**
> + * scsi_rescan_device - Rescan a SCSI device
> + * @sdev: The SCSI device to rescan
> + *
> + * Refreshes the INQUIRY data and VPD pages of @sdev and lets the device
> + * handler and the upper-level driver update their view of the device. If
> + * the peripheral qualifier or device type changed, the device is reprobed
> + * so that the right upper-level driver, if any, is bound to it.
> + *
> + * The caller must hold a reference to @sdev.
> + */
>  int scsi_rescan_device(struct scsi_device *sdev)
>  {
>  	struct device *dev = &sdev->sdev_gendev;
> +	bool need_reprobe;
>  	int ret = 0;
>  
>  	device_lock(dev);
> @@ -1677,12 +1738,49 @@ int scsi_rescan_device(struct scsi_device *sdev)
>  		goto unlock;
>  	}
>  
> +	need_reprobe = scsi_refresh_inquiry(sdev);
> +
>  	scsi_attach_vpd(sdev);
>  	scsi_cdl_check(sdev);
>  
>  	if (sdev->handler && sdev->handler->rescan)
>  		sdev->handler->rescan(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);

[Severity: High]
Can device_reprobe(dev) be called here before device_add(dev) has initialized
dev->p, leading to a NULL pointer dereference in __device_attach()?

In scsi_add_lun(), sdev->sdev_state is transitioned to SDEV_RUNNING before
sdev_configure(), scsi_attach_vpd(), scsi_cdl_check(), and
scsi_sysfs_add_sdev() run (and during async scans, scsi_sysfs_add_sdev() is
deferred until scsi_finish_async_scan()):

drivers/scsi/scsi_scan.c:scsi_add_lun() {
	...
	mutex_lock(&sdev->state_mutex);
	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
	if (ret)
		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
	mutex_unlock(&sdev->state_mutex);
	...
	if (!async && scsi_sysfs_add_sdev(sdev) != 0)
		return SCSI_SCAN_NO_RESPONSE;
}

If a command issued during this window receives a UNIT_ATTENTION with
INQUIRY_DATA_HAS_CHANGED (scheduling sdev->event_work -> scsi_evt_thread() ->
scsi_rescan_device()), or a caller invokes scsi_rescan_device() while sdev is
in SDEV_RUNNING before device_add() has been called, scsi_rescan_device()
only checks sdev->sdev_state == SDEV_RUNNING without checking
device_is_registered(dev) or sdev->is_visible.

When need_reprobe is true, device_reprobe() -> bus_rescan_devices_helper() ->
device_attach() -> __device_attach() dereferences dev->p->dead while dev->p
is still NULL.

> +
> +		/*
> +		 * state_mutex serializes against __scsi_remove_device(), so
> +		 * a device that is being removed cannot be moved from
> +		 * SDEV_CANCEL or SDEV_DEL to SDEV_OFFLINE.
> +		 */
> +		mutex_lock(&sdev->state_mutex);
> +		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]
Will checking ret < 0 here ever catch upper-level driver probe failures from
device_reprobe()?

Following the call chain device_reprobe() -> bus_rescan_devices_helper() ->
device_attach() -> __device_attach() -> __device_attach_driver() ->
driver_probe_device() -> __driver_probe_device() -> really_probe():

drivers/base/dd.c:really_probe() {
	...
	ret = call_driver_probe(dev, drv);
	if (ret) {
		if (link_ret == -EAGAIN)
			ret = -EPROBE_DEFER;

		/*
		 * Return probe errors as positive values so that the callers
		 * can distinguish them from other errors.
		 */
		ret = -ret;
		goto probe_failed;
	}
}

And in __device_attach_driver():

drivers/base/dd.c:__device_attach_driver() {
	...
	ret = driver_probe_device(drv, dev);
	if (ret < 0)
		return ret;
	return ret == 0;
}

Because really_probe() negates driver probe errors into positive values and
__device_attach_driver() converts positive returns into 0, device_reprobe()
returns 0 when a SCSI driver's probe callback fails, leaving the device in
SDEV_RUNNING without a driver instead of marking it SDEV_OFFLINE.

Conversely, when probing is deferred (such as when defer_all_probes is set
during system suspend/hibernation), really_probe() returns -EPROBE_DEFER
(negative) and queues dev on deferred_probe_pending_list, which causes the
ret < 0 check to mark the device SDEV_OFFLINE while its probe is deferred.

> +				sdev_printk(KERN_WARNING, sdev,
> +					    "device reprobe failed (%d), marked offline\n",
> +					    ret);
> +			else
> +				sdev_printk(KERN_WARNING, sdev,
> +					    "device reprobe failed (%d)\n", ret);
> +		}
> +		mutex_unlock(&sdev->state_mutex);
> +		goto unlock;
> +	}

[ ... ]

>
> 

完美结束。

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