[PATCH v7 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[PATCH v7 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[Brian Bunker]

This series teaches the SCSI rescan path to refetch standard INQUIRY data
and reprobe the device when the peripheral qualifier or device type has
changed.  The motivating case is an ALUA target that transitions through
the "unavailable" state and afterwards reports a different peripheral
qualifier; today the kernel keeps the stale INQUIRY data and the device's
sysfs attributes diverge from what the target reports.

  1/5 serializes the INQUIRY-derived sysfs attributes under
      sdev->inquiry_mutex so a concurrent reprobe cannot tear the read.
  2/5 adds scsi_update_inquiry_data() as the single point that stores
      INQUIRY data in sdev and updates the fields derived from it.
      Returns SCSI_INQ_REPROBE_NEEDED only when peripheral qualifier or
      device type changes, as these are the only fields that determine
      driver binding.
  3/5 refactors scsi_add_lun() to use scsi_update_inquiry_data(), sharing
      one implementation between initial-probe and reprobe paths.
  4/5 teaches scsi_rescan_device() to refetch INQUIRY and reprobe the
      device when scsi_update_inquiry_data() reports a change.
  5/5 calls scsi_rescan_device() from the scan path for existing devices
      so that a SCAN_LUNS pass refreshes them as well.

Changes since v6 (all prompted by the Sashiko review of v6):
  - 2/5: sdev->inquiry is now allocated once at its maximum size
    (256 bytes) and later updates are copied into it in place. v6 freed
    and replaced the buffer, which is a use-after-free for the helpers
    in scsi_device.h that read sdev->inquiry without a lock
    (scsi_device_tpgs(), scsi_device_protection(), ...). The fixed size
    also means a growing inquiry_len can never run past the buffer, and
    inq_len is clamped to 255 to fit the unsigned char inquiry_len.
  - 2/5: Capability flags (removable, lockable, ppr, wdtr, sdtr,
    tagged_supported, simple_tags, soft_reset, is_ata, allow_restart)
    are now set only on the first call. Rewriting them on a live device
    undid adjustments made after probe (BLIST_NOT_LOCKABLE, usb-storage
    and scsi_ioctl clearing lockable, aacraid forcing tagged_supported,
    ...), and raced with runtime state bits in the same word. A rescan
    now updates the INQUIRY buffer, vendor/model/rev, PQ, type and
    scsi_level only. Dropped Hannes' Reviewed-by.
  - 3/5: No code changes.
  - 4/5: Rewritten. If the INQUIRY refresh fails, the rescan carries on
    with the existing data instead of skipping the VPD update and the
    driver rescan and returning success. The device handler rescan
    method is called in all cases again, as before this series. The
    get_device()/put_device() pair around device_reprobe() is removed:
    callers hold a reference, and dropping the last one from
    scsi_evt_thread() would have run the release function from the
    work it cancels. The reprobe failure path now checks whether the
    device could actually be set offline before saying so.
    scsi_probe_lun() is now called for live devices, so it returns the
    INQUIRY length instead of storing it in sdev (the length is updated
    with the data under inquiry_mutex), and only writes lun_in_cdb and
    the target's scsi_level when they change. Dropped Hannes'
    Reviewed-by.
  - 5/5: Rewritten. v6 allocated a second scsi_device when an initial
    scan found a device still in SDEV_CREATED, passed a length of 256
    that truncated inquiry_len to 0, leaked a reference for pseudo
    devices, and called the driver rescan method without the device
    lock. The scan path is now unchanged apart from calling
    scsi_rescan_device() for an existing, fully set up device. Dropped
    Hannes' Reviewed-by.
  - Build-tested every patch with gcc W=1, and the series with clang 23
    and CONFIG_WARN_CONTEXT_ANALYSIS=y; no new warnings.

v6 cover-letter Message-ID: <20261012210511.16061-1-brian@purestorage.com>
v5 cover-letter Message-ID: <20260618233508.97960-1-brian@purestorage.com>
v4 cover-letter Message-ID: <20260530002019.47109-1-brian@purestorage.com>
v3 cover-letter Message-ID: <20260429224939.77082-1-brian@purestorage.com>
v2 cover-letter Message-ID: <20260429012733.40855-1-brian@purestorage.com>
v1 cover-letter Message-ID: <20260424215324.99045-1-brian@purestorage.com>

Brian Bunker (5):
  scsi: core: Protect INQUIRY sysfs attributes with mutex
  scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
  scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
  scsi: core: Add device reprobe support to scsi_rescan_device()
  scsi: core: Handle reprobe for existing devices during SCSI scan

 drivers/scsi/scsi.c        | 205 ++++++++++++++++++++++++++++++
 drivers/scsi/scsi_scan.c   | 250 +++++++++++++++++++++----------------
 drivers/scsi/scsi_sysfs.c  |  28 +++--
 include/scsi/scsi_device.h |  13 ++
 4 files changed, 377 insertions(+), 119 deletions(-)


base-commit: 2f4880c22e838d54eae9ab153b0f8dce60531bf1
-- 
2.56.0

[PATCH v7 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex

[Brian Bunker]

All INQUIRY-derived sysfs attributes (type, scsi_level, vendor, model,
rev, cdl_supported, and the binary inquiry attribute) read data that
can be updated during device rescan. These reads must be protected
against concurrent updates.

Use the existing inquiry_mutex to protect access to these sysfs
attributes. This ensures that userspace always sees consistent INQUIRY
data, even if a rescan is updating the buffer concurrently.

Update the sdev_rd_attr macro to take inquiry_mutex around the field
access and switch to sysfs_emit. Since vendor, model, and rev are
NUL-terminated fixed-size arrays, %s format handles all field types
correctly.

This is preparatory work for adding INQUIRY data update support during
device rescan operations.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_sysfs.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fe54792ed69e..a20707f6f1ac 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -638,21 +638,30 @@ sdev_show_##field (struct device *dev, struct device_attribute *attr,	\
 
 /*
  * sdev_rd_attr: macro to create a function and attribute variable for a
- * read only field.
+ * read-only field. inquiry_mutex protects INQUIRY-derived fields against
+ * concurrent updates during device rescan.
  */
 #define sdev_rd_attr(field, format_string)				\
-	sdev_show_function(field, format_string)			\
-static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
+static ssize_t								\
+sdev_show_##field(struct device *dev, struct device_attribute *attr,	\
+		  char *buf)						\
+{									\
+	struct scsi_device *sdev = to_scsi_device(dev);			\
+									\
+	guard(mutex)(&sdev->inquiry_mutex);				\
+	return sysfs_emit(buf, format_string, sdev->field);		\
+}									\
+static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL)
 
 /*
  * Create the actual show/store functions and data structures.
  */
-sdev_rd_attr (type, "%d\n");
-sdev_rd_attr (scsi_level, "%d\n");
-sdev_rd_attr (vendor, "%.8s\n");
-sdev_rd_attr (model, "%.16s\n");
-sdev_rd_attr (rev, "%.4s\n");
-sdev_rd_attr (cdl_supported, "%d\n");
+sdev_rd_attr(type, "%d\n");
+sdev_rd_attr(scsi_level, "%d\n");
+sdev_rd_attr(cdl_supported, "%d\n");
+sdev_rd_attr(vendor, "%s\n");
+sdev_rd_attr(model, "%s\n");
+sdev_rd_attr(rev, "%s\n");
 
 static ssize_t
 sdev_show_device_busy(struct device *dev, struct device_attribute *attr,
@@ -917,6 +926,7 @@ static ssize_t show_inquiry(struct file *filep, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
 
+	guard(mutex)(&sdev->inquiry_mutex);
 	if (!sdev->inquiry)
 		return -EINVAL;
 
-- 
2.56.0

[PATCH v7 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data

[Brian Bunker]

Add a new function scsi_update_inquiry_data() that stores standard
INQUIRY data for a SCSI device and updates the fields derived from it:

- Vendor, model, revision strings
- Peripheral qualifier, device type and SCSI level
- On the first call only: capability flags (removable, lockable,
  tagged queuing support, transfer modes, ATA detection, ...)

The function:
- Takes the inquiry_mutex to protect against concurrent sysfs reads
- Allocates sdev->inquiry once at its maximum size and copies later
  updates into it in place. Several helpers in <scsi/scsi_device.h>
  (scsi_device_tpgs(), scsi_device_protection(), ...) read
  sdev->inquiry without any lock, so the buffer must never be freed
  while the device is live.
- Leaves the capability flags alone when refreshing an existing device.
  LLDs and upper-level drivers adjust several of them after the initial
  probe (usb-storage and scsi_ioctl clear lockable, aacraid forces
  tagged_supported, ...), and they share a word with runtime state bits
  that are written without inquiry_mutex.
- Applies the BLIST_ISROM type override on every call. The BLIST_ISROM
  removable override and BLIST_NOTQ only affect the capability flags,
  so they are applied on the first call only
- Returns 1 if device type or peripheral qualifier changed, indicating
  the caller should call device_reprobe() to re-match drivers
- Returns 0 on success with no changes requiring reprobe
- Returns negative errno on failure

This is the core infrastructure needed for updating INQUIRY data during
device rescan operations, which is required for proper ALUA unavailable
state handling.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi.c        | 205 +++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  13 +++
 2 files changed, 218 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3af9e77847f7..cfb0f53b2b1a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -61,6 +61,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_host.h>
@@ -549,6 +550,210 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	kfree(vpd_buf);
 }
 
+/*
+ * Size of the buffer that holds sdev->inquiry. sdev->inquiry_len is an
+ * unsigned char, so at most 255 bytes of INQUIRY data are ever stored.
+ */
+#define SCSI_INQUIRY_BUF_LEN	256
+
+/*
+ * Set the capability flags derived from INQUIRY data. These are only set
+ * when the device is first probed: LLDs and upper-level drivers adjust
+ * several of them afterwards (lockable, tagged_supported, ppr, ...), and
+ * they share a word with runtime state bits that are updated without
+ * inquiry_mutex, so they must not be rewritten on a live device.
+ */
+static void scsi_set_inquiry_flags(struct scsi_device *sdev,
+				   unsigned char *inq_result, size_t inq_len)
+	__must_hold(&sdev->inquiry_mutex)
+{
+	/*
+	 * Check if this is an ATA device (SATA emulation layer).
+	 * ATA devices need allow_restart set to work around SATL power
+	 * management specifications.
+	 */
+	if (strncmp(sdev->vendor, "ATA     ", 8) == 0) {
+		sdev->is_ata = 1;
+		sdev->allow_restart = 1;
+	}
+
+	/*
+	 * BLIST_ISROM is a quirk for devices that report the wrong type but
+	 * should be treated as (removable) CD-ROM.
+	 */
+	if (sdev->sdev_bflags & BLIST_ISROM)
+		sdev->removable = 1;
+	else
+		sdev->removable = (inq_result[1] & 0x80) >> 7;
+
+	/*
+	 * Set lockable to match removable. Devices with removable media
+	 * can typically have their media locked/unlocked via the
+	 * ALLOW_MEDIUM_REMOVAL command.
+	 */
+	sdev->lockable = sdev->removable;
+
+	/* Capability flags from INQUIRY bytes 3, 7 and 56 */
+	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
+	if (sdev->scsi_level >= SCSI_3 ||
+	    (inq_len > 56 && inq_result[56] & 0x04))
+		sdev->ppr = 1;
+	if (inq_result[7] & 0x60)
+		sdev->wdtr = 1;
+	if (inq_result[7] & 0x10)
+		sdev->sdtr = 1;
+
+	/* BLIST_NOTQ forces tagged queuing off */
+	if (sdev->scsi_level >= SCSI_2 && (inq_result[7] & 2) &&
+	    !(sdev->sdev_bflags & BLIST_NOTQ)) {
+		sdev->tagged_supported = 1;
+		sdev->simple_tags = 1;
+	}
+}
+
+/**
+ * scsi_update_inquiry_data - Update standard INQUIRY data for a SCSI device
+ * @sdev: The device to update
+ * @inq_result: Buffer containing new INQUIRY data
+ * @inq_len: Length of inquiry data
+ *
+ * Stores the standard INQUIRY data for @sdev and updates the fields derived
+ * from it: vendor, model, revision, peripheral qualifier, device type and
+ * SCSI level. This is used both during initial device setup and when an
+ * existing device is rescanned. On the first call the capability flags
+ * derived from INQUIRY data are set as well; see scsi_set_inquiry_flags().
+ *
+ * The INQUIRY buffer is allocated once, at its maximum size, and later
+ * updates copy the new data into it in place. Several helpers in
+ * <scsi/scsi_device.h> read sdev->inquiry without holding inquiry_mutex,
+ * so the buffer must not be freed or reallocated while the device is live.
+ *
+ * The BLIST_ISROM type override is applied on every call. Its removable
+ * override, like BLIST_NOTQ, only affects the capability flags and is
+ * therefore only applied on the first call.
+ *
+ * Returns:
+ *   SCSI_INQ_UNCHANGED on success
+ *   SCSI_INQ_REPROBE_NEEDED if peripheral qualifier or device type changed
+ *  -ENOMEM on allocation failure
+ *  -EINVAL if inquiry data is too short
+ */
+int scsi_update_inquiry_data(struct scsi_device *sdev,
+			     unsigned char *inq_result, size_t inq_len)
+{
+	unsigned char *new_inquiry = NULL;
+	unsigned char old_type;
+	unsigned char old_periph_qual;
+	bool refresh;
+
+	/*
+	 * Ensure we have at least the minimum standard INQUIRY data (36 bytes)
+	 * to safely access device type, vendor, model, rev, and capability flags.
+	 */
+	if (inq_len < 36) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "INQUIRY data too short (%zu bytes), need at least 36\n",
+			    inq_len);
+		return -EINVAL;
+	}
+	inq_len = min_t(size_t, inq_len, SCSI_INQUIRY_BUF_LEN - 1);
+
+	/*
+	 * The buffer is only allocated on the first call, from scsi_add_lun(),
+	 * before the device is visible to anyone else.
+	 */
+	if (!sdev->inquiry) {
+		new_inquiry = kzalloc(SCSI_INQUIRY_BUF_LEN, GFP_KERNEL);
+		if (!new_inquiry)
+			return -ENOMEM;
+	}
+
+	mutex_lock(&sdev->inquiry_mutex);
+
+	/*
+	 * Save peripheral qualifier and device type before updating. These
+	 * are the only two values that determine whether device_reprobe() is
+	 * needed: type controls which upper-layer driver (sd, st, sr, ...) is
+	 * bound, and PQ controls whether the LUN is accessible at all
+	 * (scsi_bus_match() only matches PQ == 0).
+	 */
+	refresh = !new_inquiry;
+	old_type = sdev->type;
+	old_periph_qual = sdev->inq_periph_qual;
+
+	if (new_inquiry)
+		sdev->inquiry = new_inquiry;
+	memcpy(sdev->inquiry, inq_result, inq_len);
+	memset(sdev->inquiry + inq_len, 0, SCSI_INQUIRY_BUF_LEN - inq_len);
+	sdev->inquiry_len = inq_len;
+
+	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
+	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
+	/*
+	 * memcpy() instead of strscpy() because strscpy() would read past
+	 * the end of the INQUIRY data if its length is exactly 36 bytes.
+	 */
+	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
+	       INQUIRY_REVISION_LEN);
+	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
+	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
+
+	/*
+	 * Compute scsi_level from INQUIRY bytes 2 and 3. This must be
+	 * updated under inquiry_mutex alongside the other INQUIRY-derived
+	 * fields so sysfs readers always see a consistent snapshot.
+	 */
+	sdev->scsi_level = inq_result[2] & 0x0f;
+	if (sdev->scsi_level >= 2 ||
+	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
+		sdev->scsi_level++;
+
+	/*
+	 * Update device type from INQUIRY byte 0. BLIST_ISROM overrides the
+	 * reported type with TYPE_ROM.
+	 */
+	if (sdev->sdev_bflags & BLIST_ISROM) {
+		sdev->type = TYPE_ROM;
+	} else {
+		sdev->type = inq_result[0] & 0x1f;
+
+		/*
+		 * Some devices may respond with wrong type for well-known
+		 * logical units. Force well-known type to enumerate them
+		 * correctly.
+		 */
+		if (scsi_is_wlun(sdev->lun) && sdev->type != TYPE_WLUN) {
+			sdev_printk(KERN_WARNING, sdev,
+				"%s: correcting incorrect peripheral device type 0x%x for W-LUN 0x%16xhN\n",
+				__func__, sdev->type,
+				(unsigned int)sdev->lun);
+			sdev->type = TYPE_WLUN;
+		}
+	}
+
+	if (!refresh)
+		scsi_set_inquiry_flags(sdev, inq_result, inq_len);
+
+	mutex_unlock(&sdev->inquiry_mutex);
+
+	if (refresh) {
+		if (old_type != sdev->type) {
+			sdev_printk(KERN_NOTICE, sdev,
+				    "device type changed from %d to %d\n",
+				    old_type, sdev->type);
+			return SCSI_INQ_REPROBE_NEEDED;
+		}
+		if (old_periph_qual != sdev->inq_periph_qual) {
+			sdev_printk(KERN_NOTICE, sdev,
+				    "peripheral qualifier changed from %d to %d\n",
+				    old_periph_qual, sdev->inq_periph_qual);
+			return SCSI_INQ_REPROBE_NEEDED;
+		}
+	}
+
+	return SCSI_INQ_UNCHANGED;
+}
+
 /**
  * scsi_report_opcode - Find out if a given command is supported
  * @sdev:	scsi device to query
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e6fd6bc30644..743babf1602c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -408,6 +408,19 @@ void scsi_attach_vpd(struct scsi_device *sdev);
 void scsi_cdl_check(struct scsi_device *sdev);
 int scsi_cdl_enable(struct scsi_device *sdev, bool enable);
 
+/**
+ * enum scsi_inq_update_result - Return values for scsi_update_inquiry_data()
+ * @SCSI_INQ_UNCHANGED: INQUIRY data updated, no reprobe needed
+ * @SCSI_INQ_REPROBE_NEEDED: INQUIRY data updated, standard INQUIRY data changed
+ */
+enum scsi_inq_update_result {
+	SCSI_INQ_UNCHANGED = 0,
+	SCSI_INQ_REPROBE_NEEDED = 1,
+};
+
+int scsi_update_inquiry_data(struct scsi_device *sdev,
+			     unsigned char *inq_result, size_t inq_len);
+
 extern int __must_check scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-- 
2.56.0

[PATCH v7 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()

[Brian Bunker]

Refactor scsi_add_lun() to use the new scsi_update_inquiry_data()
function instead of inline INQUIRY parsing code. This consolidates
INQUIRY data handling in one place and ensures consistent behavior
between initial device setup and device rescan operations.

The following fields are now set by scsi_update_inquiry_data():
- inquiry buffer, vendor, model, rev pointers
- type, removable, lockable
- inq_periph_qual
- soft_reset, ppr, wdtr, sdtr
- tagged_supported, simple_tags
- is_ata, allow_restart

Also update scsi_probe_lun() to compute scsi_level into a local
variable rather than writing directly to sdev->scsi_level.
scsi_update_inquiry_data() is now the authoritative setter of
sdev->scsi_level under inquiry_mutex; scsi_probe_lun() needs the
level early for lun_in_cdb and sdev_target->scsi_level before
scsi_update_inquiry_data() is called.

scsi_add_lun() is only ever called for freshly allocated sdev instances
where sdev->inquiry is NULL, so the redundant !sdev->inquiry guard is
dropped along with the now-unreachable sanity check that followed it.

This patch maintains identical behavior to the previous code.
scsi_add_lun() continues to handle the remaining BLIST flags and
device-specific setup that doesn't come directly from INQUIRY data.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 127 +++++++++------------------------------
 1 file changed, 29 insertions(+), 98 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8aae12fd38f3..93a8719fcf26 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -650,6 +650,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result, resid;
+	char scsi_level;
 	struct scsi_failure failure_defs[] = {
 		/*
 		 * not-ready to ready transition or power-on, reset, continue.
@@ -838,23 +839,26 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 */
 
 	/*
-	 * The scanning code needs to know the scsi_level, even if no
-	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
-	 * non-zero LUNs can be scanned.
+	 * The scanning code needs to know the scsi_level before
+	 * scsi_update_inquiry_data() is called, both to set the target
+	 * scsi_level and to determine lun_in_cdb. Use a local variable
+	 * here; sdev->scsi_level is set later under inquiry_mutex in
+	 * scsi_update_inquiry_data() to avoid races with concurrent sysfs
+	 * readers.
 	 */
-	sdev->scsi_level = inq_result[2] & 0x0f;
-	if (sdev->scsi_level >= 2 ||
-	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
-		sdev->scsi_level++;
-	sdev->sdev_target->scsi_level = sdev->scsi_level;
+	scsi_level = inq_result[2] & 0x0f;
+	if (scsi_level >= 2 ||
+	    (scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
+		scsi_level++;
+	sdev->sdev_target->scsi_level = scsi_level;
 
 	/*
 	 * If SCSI-2 or lower, and if the transport requires it,
 	 * store the LUN value in CDB[1].
 	 */
 	sdev->lun_in_cdb = 0;
-	if (sdev->scsi_level <= SCSI_2 &&
-	    sdev->scsi_level != SCSI_UNKNOWN &&
+	if (scsi_level <= SCSI_2 &&
+	    scsi_level != SCSI_UNKNOWN &&
 	    !sdev->host->no_scsi2_lun_in_cdb)
 		sdev->lun_in_cdb = 1;
 
@@ -883,17 +887,6 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	struct queue_limits lim;
 	int ret;
 
-	/*
-	 * XXX do not save the inquiry, since it can change underneath us,
-	 * save just vendor/model/rev.
-	 *
-	 * Rather than save it and have an ioctl that retrieves the saved
-	 * value, have an ioctl that executes the same INQUIRY code used
-	 * in scsi_probe_lun, let user level programs doing INQUIRY
-	 * scanning run at their own risk, or supply a user level program
-	 * that can correctly scan.
-	 */
-
 	/*
 	 * Copy at least 36 bytes of INQUIRY data, so that we don't
 	 * dereference unallocated memory when accessing the Vendor,
@@ -902,54 +895,26 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 * these strings are invalid, but often they contain plausible data
 	 * nonetheless.  It doesn't matter if the device sent < 36 bytes
 	 * total, since scsi_probe_lun() initializes inq_result with 0s.
+	 *
+	 * Set sdev_bflags before calling scsi_update_inquiry_data() so it
+	 * can use the correct blacklist flags (especially BLIST_ISROM).
 	 */
-	sdev->inquiry = kmemdup(inq_result,
-				max_t(size_t, sdev->inquiry_len, 36),
-				GFP_KERNEL);
-	if (sdev->inquiry == NULL)
-		return SCSI_SCAN_NO_RESPONSE;
+	sdev->sdev_bflags = *bflags;
 
-	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
-	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
 	/*
-	 * memcpy() instead of strscpy() because strscpy() would read past
-	 * the end of sdev->inquiry if its length is exactly 36 bytes.
+	 * scsi_probe_lun() already enforces a minimum of 36 bytes, so
+	 * sdev->inquiry_len is guaranteed >= 36 here.
 	 */
-	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
-	       INQUIRY_REVISION_LEN);
-	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
-
-	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
-	if (sdev->is_ata) {
-		/*
-		 * sata emulation layer device.  This is a hack to work around
-		 * the SATL power management specifications which state that
-		 * when the SATL detects the device has gone into standby
-		 * mode, it shall respond with NOT READY.
-		 */
-		sdev->allow_restart = 1;
-	}
-
-	if (*bflags & BLIST_ISROM) {
-		sdev->type = TYPE_ROM;
-		sdev->removable = 1;
-	} else {
-		sdev->type = (inq_result[0] & 0x1f);
-		sdev->removable = (inq_result[1] & 0x80) >> 7;
-
-		/*
-		 * some devices may respond with wrong type for
-		 * well-known logical units. Force well-known type
-		 * to enumerate them correctly.
-		 */
-		if (scsi_is_wlun(sdev->lun) && sdev->type != TYPE_WLUN) {
-			sdev_printk(KERN_WARNING, sdev,
-				"%s: correcting incorrect peripheral device type 0x%x for W-LUN 0x%16xhN\n",
-				__func__, sdev->type, (unsigned int)sdev->lun);
-			sdev->type = TYPE_WLUN;
-		}
+	if (scsi_update_inquiry_data(sdev, inq_result, sdev->inquiry_len) < 0)
+		return SCSI_SCAN_NO_RESPONSE;
 
-	}
+	/*
+	 * scsi_update_inquiry_data() has already set type, removable, lockable,
+	 * inq_periph_qual, scsi_level, inquiry_len, soft_reset, ppr, wdtr, sdtr,
+	 * tagged_supported, simple_tags, is_ata, and allow_restart from INQUIRY
+	 * data. Handle special cases that need the raw inq_result or additional
+	 * logic.
+	 */
 
 	if (sdev->type == TYPE_RBC || sdev->type == TYPE_ROM) {
 		/* RBC and MMC devices can return SCSI-3 compliance and yet
@@ -960,46 +925,12 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			*bflags |= BLIST_NOREPORTLUN;
 	}
 
-	/*
-	 * For a peripheral qualifier (PQ) value of 1 (001b), the SCSI
-	 * spec says: The device server is capable of supporting the
-	 * specified peripheral device type on this logical unit. However,
-	 * the physical device is not currently connected to this logical
-	 * unit.
-	 *
-	 * The above is vague, as it implies that we could treat 001 and
-	 * 011 the same. Stay compatible with previous code, and create a
-	 * scsi_device for a PQ of 1
-	 *
-	 * Don't set the device offline here; rather let the upper
-	 * level drivers eval the PQ to decide whether they should
-	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
-	 */ 
-
-	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
-	sdev->lockable = sdev->removable;
-	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
-
-	if (sdev->scsi_level >= SCSI_3 ||
-			(sdev->inquiry_len > 56 && inq_result[56] & 0x04))
-		sdev->ppr = 1;
-	if (inq_result[7] & 0x60)
-		sdev->wdtr = 1;
-	if (inq_result[7] & 0x10)
-		sdev->sdtr = 1;
-
 	sdev_printk(KERN_NOTICE, sdev, "%s %.8s %.16s %.4s PQ: %d "
 			"ANSI: %d%s\n", scsi_device_type(sdev->type),
 			sdev->vendor, sdev->model, sdev->rev,
 			sdev->inq_periph_qual, inq_result[2] & 0x07,
 			(inq_result[3] & 0x0f) == 1 ? " CCS" : "");
 
-	if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
-	    !(*bflags & BLIST_NOTQ)) {
-		sdev->tagged_supported = 1;
-		sdev->simple_tags = 1;
-	}
-
 	/*
 	 * Some devices (Texel CD ROM drives) have handshaking problems
 	 * when used with the Seagate controllers. borken is initialized
-- 
2.56.0

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

[Brian Bunker]

Update INQUIRY data on rescan and call device_reprobe() if PQ or type
changed. Critical for ALUA unavailable state handling (SPC-4 5.15.2.4.4).

If the INQUIRY data cannot be refreshed (allocation failure, or the
device does not answer INQUIRY), the failure is logged and the rescan
continues as before with the existing data, so the VPD pages and the
device handler and upper-level driver rescan methods are still updated.

The device lock is dropped around device_reprobe(), which takes it
itself. sdev remains valid in that window because callers hold a
reference; scsi_evt_thread() is covered by the cancel_work_sync() in
scsi_device_dev_release(). Taking an extra reference here instead would
be wrong: dropping the last reference from the event work would run the
release function, and its cancel_work_sync(), from that same work.

scsi_probe_lun() is now called for live devices, so:
- Return the INQUIRY length instead of storing it in sdev->inquiry_len.
  scsi_update_inquiry_data() stores it under inquiry_mutex together
  with the data, so sysfs never pairs a new length with old data.
- Only write lun_in_cdb and the target's scsi_level when they change,
  instead of clearing lun_in_cdb and then setting it again.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 119 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 104 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 93a8719fcf26..2c848265c9b9 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -639,9 +639,12 @@ EXPORT_SYMBOL(scsi_sanitize_inquiry_string);
  * Description:
  *     Probe the lun associated with @req using a standard SCSI INQUIRY;
  *
- *     If the INQUIRY is successful, zero is returned and the
- *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
- *     are copied to the scsi_device any flags value is stored in *@bflags.
+ *     If the INQUIRY is successful, the number of valid bytes of INQUIRY
+ *     data (at least 36) is returned, the INQUIRY data is in @inq_result
+ *     and any flags value is stored in *@bflags. The INQUIRY length is not
+ *     stored in @sdev: this function is also called for live devices, and
+ *     scsi_update_inquiry_data() updates it together with the data.
+ *     A negative errno is returned on failure.
  **/
 static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			  int result_len, blist_flags_t *bflags)
@@ -650,7 +653,9 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result, resid;
+	int inquiry_len;
 	char scsi_level;
+	bool lun_in_cdb;
 	struct scsi_failure failure_defs[] = {
 		/*
 		 * not-ready to ready transition or power-on, reset, continue.
@@ -798,7 +803,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		return -EIO;
 
 	/* Don't report any more data than the device says is valid */
-	sdev->inquiry_len = min(try_inquiry_len, response_len);
+	inquiry_len = min(try_inquiry_len, response_len);
 
 	/*
 	 * XXX Abort if the response length is less than 36? If less than
@@ -815,14 +820,14 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 * and displaying garbage for the Vendor, Product, or Revision
 	 * strings.
 	 */
-	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;
 		}
-		sdev->inquiry_len = 36;
+		inquiry_len = 36;
 	}
 
 	/*
@@ -850,19 +855,25 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (scsi_level >= 2 ||
 	    (scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		scsi_level++;
-	sdev->sdev_target->scsi_level = scsi_level;
+	if (sdev->sdev_target->scsi_level != scsi_level)
+		sdev->sdev_target->scsi_level = scsi_level;
 
 	/*
 	 * If SCSI-2 or lower, and if the transport requires it,
 	 * store the LUN value in CDB[1].
 	 */
-	sdev->lun_in_cdb = 0;
-	if (scsi_level <= SCSI_2 &&
-	    scsi_level != SCSI_UNKNOWN &&
-	    !sdev->host->no_scsi2_lun_in_cdb)
-		sdev->lun_in_cdb = 1;
+	lun_in_cdb = scsi_level <= SCSI_2 &&
+		     scsi_level != SCSI_UNKNOWN &&
+		     !sdev->host->no_scsi2_lun_in_cdb;
+	/*
+	 * 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;
 
-	return 0;
+	return inquiry_len;
 }
 
 /**
@@ -1131,6 +1142,7 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 	unsigned char *result;
 	blist_flags_t bflags;
 	int res = SCSI_SCAN_NO_RESPONSE, result_len = 256;
+	int ret;
 
 	/*
 	 * The rescan flag is used as an optimization, the first scan of a
@@ -1169,8 +1181,10 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 	if (!result)
 		goto out_free_sdev;
 
-	if (scsi_probe_lun(sdev, result, result_len, &bflags))
+	ret = scsi_probe_lun(sdev, result, result_len, &bflags);
+	if (ret < 0)
 		goto out_free_result;
+	sdev->inquiry_len = ret;
 
 	if (bflagsp)
 		*bflagsp = bflags;
@@ -1658,9 +1672,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);
+		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 +1736,42 @@ 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);
+
+		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)
+				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);
+		}
+		goto unlock;
+	}
+
 	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);
 
-- 
2.56.0

[PATCH v7 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan

[Brian Bunker]

Complement scsi_rescan_device() reprobe by handling the scan path.
When a manual or transport-initiated scan finds a LUN that already has
a fully set up scsi_device, call scsi_rescan_device() for it so that
changes to the standard INQUIRY data are picked up and the device is
reprobed if its peripheral qualifier or device type changed.

The device lock is taken inside scsi_rescan_device() while the host's
scan_mutex is held; that lock order is already used by
scsi_remove_device(). Pseudo devices and devices that are still being
set up by another scan are left alone.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2c848265c9b9..ea54c9cf9f0b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1154,6 +1154,16 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
 				"scsi scan: device exists on %s\n",
 				dev_name(&sdev->sdev_gendev)));
+			/*
+			 * Rescanning a device that has been fully set up:
+			 * refresh its INQUIRY data so that peripheral
+			 * qualifier or device type changes (e.g. ALUA state
+			 * transitions) are picked up.
+			 */
+			if (rescan != SCSI_SCAN_INITIAL &&
+			    !scsi_device_created(sdev) &&
+			    !scsi_device_is_pseudo_dev(sdev))
+				scsi_rescan_device(sdev);
 			if (sdevp)
 				*sdevp = sdev;
 			else
-- 
2.56.0

Re: [PATCH v7 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: sysfs modalias and uevent missing inquiry_mutex protection
--

commit 81f6a8643e20a98783acfb264a3488a0b0ad95ac
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Protect INQUIRY sysfs attributes with mutex
    
All INQUIRY-derived sysfs attributes (type, scsi_level, vendor, model,
rev, cdl_supported, and the binary inquiry attribute) read data that
can be updated during device rescan. These reads must be protected
against concurrent updates.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fe54792ed69e4..a20707f6f1ac7 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -638,21 +638,30 @@ sdev_show_##field (struct device *dev, struct device_attribute *attr,	\
>  
>  /*
>   * sdev_rd_attr: macro to create a function and attribute variable for a
> - * read only field.
> + * read-only field. inquiry_mutex protects INQUIRY-derived fields against
> + * concurrent updates during device rescan.
>   */
>  #define sdev_rd_attr(field, format_string)				\
> -	sdev_show_function(field, format_string)			\
> -static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
> +static ssize_t								\
> +sdev_show_##field(struct device *dev, struct device_attribute *attr,	\
> +		  char *buf)						\
> +{									\
> +	struct scsi_device *sdev = to_scsi_device(dev);			\
> +									\
> +	guard(mutex)(&sdev->inquiry_mutex);				\
> +	return sysfs_emit(buf, format_string, sdev->field);		\
> +}									\
> +static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL)
>  
>  /*
>   * Create the actual show/store functions and data structures.
>   */
> -sdev_rd_attr (type, "%d\n");
> -sdev_rd_attr (scsi_level, "%d\n");
> -sdev_rd_attr (vendor, "%.8s\n");
> -sdev_rd_attr (model, "%.16s\n");
> -sdev_rd_attr (rev, "%.4s\n");
> -sdev_rd_attr (cdl_supported, "%d\n");
> +sdev_rd_attr(type, "%d\n");

[Severity: Medium]
The commit states all INQUIRY-derived sysfs attributes, including type, must
be protected by inquiry_mutex against concurrent updates.

However, shouldn't sdev_show_modalias() and scsi_bus_uevent() also be updated?
They both read sdev->type locklessly to construct the modalias string:

drivers/scsi/scsi_sysfs.c:sdev_show_modalias() {
    ...
    sdev = to_scsi_device(dev);
    return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
}

drivers/scsi/scsi_sysfs.c:scsi_bus_uevent() {
    ...
    sdev = to_scsi_device(dev);
    add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT, sdev->type);
    ...
}

Can concurrent device rescans cause these functions to read an inconsistent
or transient sdev->type?

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

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

[sashiko-bot]

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