* [PATCH v6 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex
2026-10-12 21:05 [PATCH v6 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
@ 2026-10-12 21:05 ` Brian Bunker
2026-10-12 21:05 ` [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Brian Bunker @ 2026-10-12 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
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.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
2026-10-12 21:05 [PATCH v6 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
2026-10-12 21:05 ` [PATCH v6 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex Brian Bunker
@ 2026-10-12 21:05 ` Brian Bunker
2026-10-12 21:27 ` sashiko-bot
2026-10-13 10:02 ` Damien Le Moal
2026-10-12 21:05 ` [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data() Brian Bunker
` (2 subsequent siblings)
4 siblings, 2 replies; 11+ messages in thread
From: Brian Bunker @ 2026-10-12 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
Add a new function scsi_update_inquiry_data() that can safely update all
INQUIRY-derived fields for an existing SCSI device:
- Vendor, model, revision strings
- Peripheral qualifier and device type
- Capability flags (removable, lockable, tagged queuing support, etc.)
- ATA device detection and allow_restart setting
The function:
- Takes the inquiry_mutex to protect against concurrent sysfs reads
- Respects BLIST_ISROM and BLIST_NOTQ blacklist flags
- 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>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
drivers/scsi/scsi.c | 178 +++++++++++++++++++++++++++++++++++++
include/scsi/scsi_device.h | 13 +++
2 files changed, 191 insertions(+)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3af9e77847f7..b80378ecf485 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,183 @@ void scsi_attach_vpd(struct scsi_device *sdev)
kfree(vpd_buf);
}
+/**
+ * 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
+ *
+ * Updates the standard INQUIRY data (vendor, model, rev, peripheral qualifier,
+ * device type, removable media flag) and capability flags derived from INQUIRY
+ * data for a SCSI device. This is used during both initial device setup and
+ * when reprobing a device to get fresh INQUIRY information. The old inquiry
+ * buffer is freed and replaced with the new data under the protection of
+ * inquiry_mutex.
+ *
+ * Blacklist flags (BLIST_ISROM, BLIST_NOTQ) are respected when updating
+ * device properties.
+ *
+ * Returns:
+ * SCSI_INQ_UNCHANGED on success
+ * SCSI_INQ_REPROBE_NEEDED if standard INQUIRY data changed (caller should reprobe)
+ * -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;
+ unsigned char old_type;
+ unsigned char old_periph_qual;
+ bool had_prior_inquiry;
+
+ /*
+ * 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;
+ }
+
+ /* Allocate new inquiry buffer */
+ new_inquiry = kmemdup(inq_result, inq_len, GFP_KERNEL);
+ if (!new_inquiry)
+ return -ENOMEM;
+
+ /* Update inquiry data under mutex protection */
+ mutex_lock(&sdev->inquiry_mutex);
+
+ /*
+ * Save peripheral qualifier and device type before updating all
+ * INQUIRY-derived fields. 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). Every other field — vendor, model, revision, capability
+ * flags — is refreshed in place without any driver re-matching.
+ */
+ had_prior_inquiry = (sdev->inquiry != NULL);
+ old_type = sdev->type;
+ old_periph_qual = sdev->inq_periph_qual;
+
+ kfree(sdev->inquiry);
+ sdev->inquiry = new_inquiry;
+ 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 sdev->inquiry 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++;
+
+ /*
+ * 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;
+ } else
+ sdev->is_ata = 0;
+
+ /*
+ * Update device type from INQUIRY byte 0.
+ * BLIST_ISROM is a quirk for devices that report wrong type but should
+ * be treated as (removable) CD-ROM. Override to TYPE_ROM as exception.
+ */
+ if (sdev->sdev_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;
+ }
+ }
+
+ /*
+ * 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;
+
+ /* Update capability flags from INQUIRY byte 7 */
+ sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
+
+ /*
+ * Update protocol support flags.
+ * Only update ppr if we have enough INQUIRY data (>56 bytes) to check
+ * byte 56, or if scsi_level indicates SCSI-3+ support. If we don't have
+ * enough data, leave ppr unchanged to avoid incorrectly clearing it
+ * during rescan with short INQUIRY.
+ */
+ if (sdev->scsi_level >= SCSI_3 || inq_len > 56)
+ sdev->ppr = (sdev->scsi_level >= SCSI_3 ||
+ (inq_len > 56 && inq_result[56] & 0x04)) ? 1 : 0;
+ sdev->wdtr = !!(inq_result[7] & 0x60);
+ sdev->sdtr = !!(inq_result[7] & 0x10);
+
+ /*
+ * Update tagged queuing support from INQUIRY byte 7.
+ * BLIST_NOTQ is an exception to force tagged queuing off.
+ */
+ if (sdev->sdev_bflags & BLIST_NOTQ)
+ sdev->tagged_supported = 0;
+ else
+ sdev->tagged_supported = (sdev->scsi_level >= SCSI_2) &&
+ (inq_result[7] & 2);
+ sdev->simple_tags = sdev->tagged_supported;
+
+ mutex_unlock(&sdev->inquiry_mutex);
+
+ if (had_prior_inquiry) {
+ 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.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
2026-10-12 21:05 ` [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
@ 2026-10-12 21:27 ` sashiko-bot
2026-10-13 10:02 ` Damien Le Moal
1 sibling, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-12 21:27 UTC (permalink / raw)
To: Brian Bunker; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: use-after-free on inquiry buffer for concurrent lockless readers
- [High] scsi: data race on struct scsi_device bitfields corrupts concurrent state updates
- [Medium] scsi: torn reads of INQUIRY strings in procfs and target during rescan
- [Medium] scsi: core: silent truncation of inq_len in scsi_update_inquiry_data()
--
commit 8e9aa2b82f339c1aae88807580c6d0982c8177fa
Author: Brian Bunker <brian@purestorage.com>
scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
Add a new function scsi_update_inquiry_data() that can safely update all
INQUIRY-derived fields for an existing SCSI device:
- Vendor, model, revision strings
- Peripheral qualifier and device type
- Capability flags (removable, lockable, tagged queuing support, etc.)
- ATA device detection and allow_restart setting
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3af9e77847f70..b80378ecf4856 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
[ ... ]
> @@ -549,6 +550,183 @@ void scsi_attach_vpd(struct scsi_device *sdev)
[ ... ]
> +int scsi_update_inquiry_data(struct scsi_device *sdev,
> + unsigned char *inq_result, size_t inq_len)
> +{
[ ... ]
> + had_prior_inquiry = (sdev->inquiry != NULL);
> + old_type = sdev->type;
> + old_periph_qual = sdev->inq_periph_qual;
> +
> + kfree(sdev->inquiry);
> + sdev->inquiry = new_inquiry;
> + sdev->inquiry_len = inq_len;
[Severity: High]
This frees and replaces the existing sdev->inquiry buffer dynamically on a
live device. However, there are multiple existing paths that read the inquiry
buffer locklessly, such as inline subsystem helpers in
include/scsi/scsi_device.h:
scsi_device_enclosure() {
return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
}
Since these lockless readers do not acquire inquiry_mutex and there is no RCU
protection, can a concurrent I/O path or sysfs read dereference the freed
inquiry pointer, resulting in a use-after-free? Furthermore, do these
non-atomic updates to the pointer and its length expose lockless readers to
torn state?
[Severity: Medium]
sdev->inquiry_len is an unsigned char (8 bits), while inq_len is a size_t.
If a manual rescan passes an inq_len of 256, will this assignment silently
truncate the length to 0, causing the device's sysfs inquiry attribute to
return 0 bytes?
> + 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.
> + */
> + memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
> + INQUIRY_REVISION_LEN);
[Severity: Medium]
This patch makes the INQUIRY string arrays mutable at runtime. While the
updates are protected by inquiry_mutex, unprotected readers like procfs and
the PSCSI target read these character arrays without acquiring the newly
introduced inquiry_mutex:
proc_print_scsidevice() {
for (i = 0; i < 8; i++) {
if (sdev->vendor[i] >= 0x20)
...
}
Will unprotected readers race with these non-atomic strscpy() updates, causing
torn reads that expose mixed or garbled strings?
> + sdev->rev[INQUIRY_REVISION_LEN] = '\0';
> + sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
[ ... ]
> + /*
> + * Update device type from INQUIRY byte 0.
> + * BLIST_ISROM is a quirk for devices that report wrong type but should
> + * be treated as (removable) CD-ROM. Override to TYPE_ROM as exception.
> + */
> + if (sdev->sdev_bflags & BLIST_ISROM) {
> + sdev->type = TYPE_ROM;
> + sdev->removable = 1;
> + } else {
> + sdev->type = inq_result[0] & 0x1f;
> + sdev->removable = (inq_result[1] & 0x80) >> 7;
[Severity: High]
These assignments update capability bitfields (e.g., is_ata, removable,
lockable) on a live device while holding only inquiry_mutex. In
struct scsi_device, these capability bitfields are packed into the same memory
word as highly dynamic operational state bitfields:
unsigned is_ata:1;
unsigned removable:1;
unsigned changed:1; /* Data invalid due to media change */
unsigned busy:1; /* Used to prevent races */
Since other subsystems modify the operational bitfields without holding
inquiry_mutex (for example, scsi_test_unit_ready() sets sdev->changed = 1 in
softirq context), does this cause a non-atomic read-modify-write on the shared
word? Could this race overwrite and lose critical runtime state changes like
media change notifications?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012210511.16061-1-brian@purestorage.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
2026-10-12 21:05 ` [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
2026-10-12 21:27 ` sashiko-bot
@ 2026-10-13 10:02 ` Damien Le Moal
1 sibling, 0 replies; 11+ messages in thread
From: Damien Le Moal @ 2026-10-13 10:02 UTC (permalink / raw)
To: Brian Bunker, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Hannes Reinecke
On 2026/10/12 23:05, Brian Bunker wrote:
> + /*
> + * 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;
> + } else
> + sdev->is_ata = 0;
Missing the curly brackets around the else.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
2026-10-12 21:05 [PATCH v6 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
2026-10-12 21:05 ` [PATCH v6 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex Brian Bunker
2026-10-12 21:05 ` [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
@ 2026-10-12 21:05 ` Brian Bunker
2026-10-12 21:27 ` sashiko-bot
2026-10-12 21:05 ` [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
2026-10-12 21:05 ` [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
4 siblings, 1 reply; 11+ messages in thread
From: Brian Bunker @ 2026-10-12 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
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.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
2026-10-12 21:05 ` [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data() Brian Bunker
@ 2026-10-12 21:27 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-12 21:27 UTC (permalink / raw)
To: Brian Bunker; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: core: Use-After-Free in lockless INQUIRY readers during rescan
- [High] scsi: core: Bitfield data race between INQUIRY capability updates and SCSI Error Handler
- [Medium] scsi: core: scsi_update_inquiry_data clobbers BLIST_NOT_LOCKABLE workaround during rescan
--
commit 2b976e6e6ffef889b4ec5235ed09392acc7c67db
Author: Brian Bunker <brian@purestorage.com>
scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
This commit refactors scsi_add_lun() to use a new helper,
scsi_update_inquiry_data(), which centralizes INQUIRY parsing. It moves
several capability and identity assignments into the new helper and ensures
consistent handling between initial probe and runtime rescans.
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3af9e77847f70..b80378ecf4856 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
[ ... ]
> +int scsi_update_inquiry_data(struct scsi_device *sdev,
> + unsigned char *inq_result, size_t inq_len)
> +{
[ ... ]
> + had_prior_inquiry = (sdev->inquiry != NULL);
> + old_type = sdev->type;
> + old_periph_qual = sdev->inq_periph_qual;
> +
> + kfree(sdev->inquiry);
> + sdev->inquiry = new_inquiry;
[Severity: High]
Does this expose concurrent lockless readers to a use-after-free?
During a rescan, the old INQUIRY buffer is freed directly with kfree().
However, there are multiple helpers (like scsi_device_enclosure() used by
SES sysfs traversal, and scsi_device_tpgs() used by ALUA workqueues) that
dereference sdev->inquiry locklessly:
include/scsi/scsi_device.h:scsi_device_enclosure() {
return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
}
If a rescan is triggered concurrently with ALUA background polling or SES
sysfs reads, these lockless readers could dereference the freed memory.
> + sdev->inquiry_len = inq_len;
[ ... ]
> + /*
> + * 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;
> + } else
> + sdev->is_ata = 0;
> +
> + /*
> + * Update device type from INQUIRY byte 0.
> + * BLIST_ISROM is a quirk for devices that report wrong type but should
> + * be treated as (removable) CD-ROM. Override to TYPE_ROM as exception.
> + */
> + if (sdev->sdev_bflags & BLIST_ISROM) {
> + sdev->type = TYPE_ROM;
> + sdev->removable = 1;
> + } else {
> + sdev->type = inq_result[0] & 0x1f;
> + sdev->removable = (inq_result[1] & 0x80) >> 7;
[Severity: High]
Can modifying these bitfields during a rescan corrupt other bitfields via
a data race?
scsi_update_inquiry_data() modifies capability bitfields like is_ata,
removable, and lockable under sdev->inquiry_mutex.
However, the SCSI Error Handler modifies operational state bitfields
asynchronously without this lock:
drivers/scsi/scsi_error.c:__scsi_report_device_reset() {
sdev->was_reset = 1;
sdev->expecting_cc_ua = 1;
}
All of these bitfields (is_ata, removable, was_reset, expecting_cc_ua) are
packed into the same machine word in struct scsi_device.
If a device rescan runs concurrently with a bus reset report, the compiler's
read-modify-write operations will race, potentially clobbering either the
device capabilities or the critical error recovery state.
[ ... ]
> + }
> + }
> +
> + /*
> + * 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;
[Severity: Medium]
Will this unconditionally clobber the BLIST_NOT_LOCKABLE workaround during
a rescan?
During initial device setup in scsi_scan.c:scsi_add_lun(), if a device has
BLIST_NOT_LOCKABLE set, the code forces sdev->lockable = 0. However, that
blacklist guard was not moved into scsi_update_inquiry_data() with the rest
of the logic.
When a blacklisted removable device is rescanned, this assignment will
unconditionally overwrite the blacklist override, setting lockable back to 1.
This could expose broken hardware to ALLOW_MEDIUM_REMOVAL commands,
potentially causing hardware hangs or I/O timeouts.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012210511.16061-1-brian@purestorage.com?part=3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()
2026-10-12 21:05 [PATCH v6 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
` (2 preceding siblings ...)
2026-10-12 21:05 ` [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data() Brian Bunker
@ 2026-10-12 21:05 ` Brian Bunker
2026-10-12 21:27 ` sashiko-bot
2026-10-12 21:05 ` [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
4 siblings, 1 reply; 11+ messages in thread
From: Brian Bunker @ 2026-10-12 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
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).
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 | 135 ++++++++++++++++++++++++++++++++++++---
1 file changed, 125 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 93a8719fcf26..ddb92282d49f 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1097,6 +1097,83 @@ static unsigned char *scsi_inq_str(unsigned char *buf, unsigned char *inq,
}
#endif
+/**
+ * __scsi_reprobe_inquiry - Update INQUIRY data and reprobe device if needed
+ * @sdev: The SCSI device to reprobe
+ * @inq_result: Buffer containing fresh INQUIRY data
+ * @inq_len: Length of INQUIRY data
+ * @need_reprobe: Pointer to store whether device_reprobe() is needed
+ *
+ * Updates the device's INQUIRY data, attaches VPD pages, checks CDL support,
+ * and determines if the device needs to be reprobed due to any change in
+ * the standard INQUIRY data. If no reprobe is needed, calls driver rescan
+ * functions.
+ *
+ * This function does NOT take device_lock - caller must hold it.
+ *
+ * Returns:
+ * SCSI_INQ_UNCHANGED on success (no reprobe needed)
+ * SCSI_INQ_REPROBE_NEEDED if type or PQ changed (reprobe needed)
+ * -ENOMEM on allocation failure
+ * -EINVAL if INQUIRY data is too short
+ */
+static int __scsi_reprobe_inquiry(struct scsi_device *sdev,
+ unsigned char *inq_result,
+ size_t inq_len,
+ bool *need_reprobe)
+{
+ struct device *dev = &sdev->sdev_gendev;
+ int ret;
+
+ /* Update INQUIRY data */
+ ret = scsi_update_inquiry_data(sdev, inq_result, inq_len);
+ if (ret < 0) {
+ sdev_printk(KERN_ERR, sdev,
+ "failed to update inquiry data: %d\n", ret);
+ return ret;
+ }
+
+ SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+ "updated inquiry data (type %d, PQ %d)\n",
+ sdev->type, sdev->inq_periph_qual));
+
+ /* Update VPD pages and CDL support */
+ scsi_attach_vpd(sdev);
+ scsi_cdl_check(sdev);
+
+ /*
+ * If standard INQUIRY data changed, caller should reprobe to update
+ * driver attachment. Any change in the first 36 bytes may affect
+ * driver matching — PQ changes affect scsi_bus_match() which only
+ * matches PQ == 0, and type changes require a different upper-layer
+ * driver (e.g., sd for TYPE_DISK, sr for TYPE_ROM).
+ */
+ if (ret == SCSI_INQ_REPROBE_NEEDED) {
+ SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+ "INQUIRY data changed, reprobe needed\n"));
+ *need_reprobe = true;
+ return ret;
+ }
+
+ /*
+ * PQ and type unchanged, call driver's rescan functions to update
+ * device properties (capacity, etc.)
+ */
+ if (sdev->handler && sdev->handler->rescan)
+ sdev->handler->rescan(sdev);
+
+ if (dev->driver && try_module_get(dev->driver->owner)) {
+ struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+ if (drv->rescan)
+ drv->rescan(dev);
+ module_put(dev->driver->owner);
+ }
+
+ *need_reprobe = false;
+ return ret;
+}
+
/**
* scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
* @shost: SCSI host pointer
@@ -1661,7 +1738,11 @@ EXPORT_SYMBOL(scsi_resume_device);
int scsi_rescan_device(struct scsi_device *sdev)
{
struct device *dev = &sdev->sdev_gendev;
+ unsigned char *inq_result;
+ blist_flags_t bflags;
+ int result_len = 256;
int ret = 0;
+ bool need_reprobe = false;
device_lock(dev);
@@ -1677,18 +1758,52 @@ int scsi_rescan_device(struct scsi_device *sdev)
goto unlock;
}
- scsi_attach_vpd(sdev);
- scsi_cdl_check(sdev);
-
- if (sdev->handler && sdev->handler->rescan)
- sdev->handler->rescan(sdev);
+ /*
+ * Rescan standard INQUIRY data to detect changes in device
+ * properties (vendor, model, rev, peripheral qualifier, device type, etc.)
+ */
+ inq_result = kmalloc(result_len, GFP_KERNEL);
+ if (inq_result) {
+ if (scsi_probe_lun(sdev, inq_result, result_len,
+ &bflags) == 0) {
+ ret = __scsi_reprobe_inquiry(sdev, inq_result,
+ max_t(size_t, sdev->inquiry_len, 36),
+ &need_reprobe);
+ if (ret < 0) {
+ kfree(inq_result);
+ goto unlock;
+ }
+ }
+ kfree(inq_result);
+ }
- if (dev->driver && try_module_get(dev->driver->owner)) {
- struct scsi_driver *drv = to_scsi_driver(dev->driver);
+ /*
+ * If INQUIRY data changed, reprobe to update driver attachment.
+ * Must unlock device before calling device_reprobe() to avoid
+ * deadlock. Hold a device reference across the unlock so sdev
+ * cannot be freed while the lock is dropped. If another thread
+ * changes INQUIRY data in this window, that thread's execution
+ * will trigger the follow-on reprobe.
+ */
+ if (need_reprobe) {
+ get_device(dev);
+ device_unlock(dev);
+ ret = device_reprobe(dev);
+ device_lock(dev);
+
+ if (sdev->sdev_state == SDEV_CANCEL ||
+ sdev->sdev_state == SDEV_DEL) {
+ put_device(dev);
+ ret = -ENODEV;
+ goto unlock;
+ }
- if (drv->rescan)
- drv->rescan(dev);
- module_put(dev->driver->owner);
+ if (ret < 0) {
+ sdev_printk(KERN_WARNING, sdev,
+ "device reprobe failed, marking offline\n");
+ scsi_device_set_state(sdev, SDEV_OFFLINE);
+ }
+ put_device(dev);
}
unlock:
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()
2026-10-12 21:05 ` [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
@ 2026-10-12 21:27 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-12 21:27 UTC (permalink / raw)
To: Brian Bunker; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: error handling skips drv->rescan and returns false success
- [High] scsi: unsynchronized sdev->inquiry_len update causes OOB read in sysfs
- [High] scsi: concurrent lockless access to sdev->inquiry causes UAF during rescan
- [High] scsi: core: Use-After-Free of dev->mutex on reprobe error path
--
commit 6bb20d3d0d04c11c3c6eb45dfb7bb607811d248f
Author: Brian Bunker <brian@purestorage.com>
scsi: core: Add device reprobe support to scsi_rescan_device()
This patch updates INQUIRY data on rescan and calls device_reprobe() if
the peripheral qualifier or type changed. This handles ALUA unavailable
states.
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 93a8719fcf265..ddb92282d49ff 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
[ ... ]
> @@ -1677,18 +1758,52 @@ int scsi_rescan_device(struct scsi_device *sdev)
> goto unlock;
> }
>
> - scsi_attach_vpd(sdev);
> - scsi_cdl_check(sdev);
> -
> - if (sdev->handler && sdev->handler->rescan)
> - sdev->handler->rescan(sdev);
> + /*
> + * Rescan standard INQUIRY data to detect changes in device
> + * properties (vendor, model, rev, peripheral qualifier, device type, etc.)
> + */
> + inq_result = kmalloc(result_len, GFP_KERNEL);
> + if (inq_result) {
[Severity: High]
Could this inadvertently break error handling by skipping the rescan hooks
and returning a false success?
If the kmalloc() fails, or if the device is unresponsive and scsi_probe_lun()
returns an error like -EIO, the function skips the entire block. Because
ret is still initialized to 0, it returns success to the caller.
Crucially, the fallback to drv->rescan(dev) (which was previously executed
unconditionally) is now bypassed. For SCSI disks, sd_rescan() calls
sd_revalidate_disk(). If the device is dead, this revalidation is what
zeroes the disk capacity. Because this patch skips the rescan hook on IO
or memory failure, does this leave dead disks with stale capacity and
geometry data in sysfs and the block layer?
> + if (scsi_probe_lun(sdev, inq_result, result_len,
> + &bflags) == 0) {
[Severity: High]
Can calling scsi_probe_lun() on a live, sysfs-exposed device introduce an
out-of-bounds read in sysfs?
scsi_probe_lun() directly mutates sdev->inquiry_len without acquiring
sdev->inquiry_mutex:
drivers/scsi/scsi_scan.c:scsi_probe_lun() {
...
sdev->inquiry_len = min(try_inquiry_len, response_len);
...
}
If the device responds with more data than previously (e.g., length jumps
from 36 to 255 bytes), sdev->inquiry_len is increased immediately. However,
the actual sdev->inquiry buffer is not reallocated until
scsi_update_inquiry_data() runs later in the flow.
If a concurrent read to the /sys/bus/scsi/devices/.../inquiry sysfs file
occurs, show_inquiry() will acquire the mutex and read the newly increased
sdev->inquiry_len bytes from the old, smaller sdev->inquiry buffer,
potentially leaking adjacent heap memory to userspace.
> + ret = __scsi_reprobe_inquiry(sdev, inq_result,
> + max_t(size_t, sdev->inquiry_len, 36),
> + &need_reprobe);
[Severity: High]
Is it possible that triggering the INQUIRY buffer replacement on an active
device causes a use-after-free due to concurrent lockless readers?
__scsi_reprobe_inquiry() eventually calls scsi_update_inquiry_data(),
which frees the old buffer using a raw kfree() without RCU deferral:
drivers/scsi/scsi.c:scsi_update_inquiry_data() {
...
kfree(sdev->inquiry);
sdev->inquiry = new_inquiry;
...
}
Several kernel paths access sdev->inquiry without acquiring
sdev->inquiry_mutex or using RCU. For example, scsi_device_protection()
(called during block revalidation) reads from sdev->inquiry locklessly:
include/scsi/scsi_device.h:scsi_device_protection() {
return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
}
If one of these lockless readers executes concurrently with
scsi_rescan_device(), it could dereference the sdev->inquiry pointer
exactly as it is being freed.
> + if (ret < 0) {
> + kfree(inq_result);
> + goto unlock;
> + }
> + }
> + kfree(inq_result);
> + }
[ ... ]
> + if (need_reprobe) {
> + get_device(dev);
> + device_unlock(dev);
> + ret = device_reprobe(dev);
> + device_lock(dev);
> +
> + if (sdev->sdev_state == SDEV_CANCEL ||
> + sdev->sdev_state == SDEV_DEL) {
> + put_device(dev);
> + ret = -ENODEV;
> + goto unlock;
> + }
[Severity: High]
Might this error path trigger a use-after-free on dev->mutex?
scsi_evt_thread() (via scsi_evt_emit()) calls scsi_rescan_device() without
holding an explicit get_device() reference, relying solely on
cancel_work_sync(&sdev->event_work) during scsi_device_dev_release() to
prevent device destruction.
If scsi_remove_device() runs concurrently during the unlocked reprobe
window above, it will drop its references. This leaves the temporary
get_device(dev) taken in scsi_rescan_device() as the sole remaining
reference.
When put_device(dev) is called here (or at the end of the block), it
drops that final reference from within the worker thread. This invokes
scsi_device_dev_release() inline, which calls cancel_work_sync() on the
currently executing work. The workqueue core detects the self-flush and
returns immediately, allowing scsi_device_dev_release() to free the sdev.
The code then jumps to the unlock label and executes device_unlock(dev)
on the freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012210511.16061-1-brian@purestorage.com?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan
2026-10-12 21:05 [PATCH v6 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
` (3 preceding siblings ...)
2026-10-12 21:05 ` [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
@ 2026-10-12 21:05 ` Brian Bunker
2026-10-12 21:24 ` sashiko-bot
4 siblings, 1 reply; 11+ messages in thread
From: Brian Bunker @ 2026-10-12 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
Complement scsi_rescan_device() reprobe by handling the scan path.
Update INQUIRY data and reprobe existing devices when standard INQUIRY
data changed.
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 | 91 ++++++++++++++++++++++++++++++++++++----
1 file changed, 83 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ddb92282d49f..6df140d3c748 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1208,6 +1208,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;
+ bool is_reprobe = false;
/*
* The rescan flag is used as an optimization, the first scan of a
@@ -1215,7 +1216,32 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
*/
sdev = scsi_device_lookup_by_target(starget, lun);
if (sdev) {
- if (rescan != SCSI_SCAN_INITIAL || !scsi_device_created(sdev)) {
+ if (rescan == SCSI_SCAN_INITIAL && scsi_device_created(sdev)) {
+ /*
+ * Initial scan found device in CREATED state (being probed
+ * by another thread). Drop reference and allocate new -
+ * the other thread will complete setup of the original.
+ */
+ scsi_device_put(sdev);
+ sdev = scsi_alloc_sdev(starget, lun, hostdata);
+ if (!sdev)
+ goto out;
+ } else if (rescan != SCSI_SCAN_INITIAL && !scsi_device_created(sdev)) {
+ /*
+ * Manual rescan of fully initialized device.
+ * Reprobe to detect peripheral qualifier or device type
+ * changes (e.g., ALUA state transitions).
+ */
+ SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+ "scsi scan: device exists (type %d, PQ %d), reprobing\n",
+ sdev->type, sdev->inq_periph_qual));
+ is_reprobe = true;
+ } else {
+ /*
+ * Either initial scan with fully initialized device,
+ * or manual rescan with device still in CREATED state.
+ * Return that device exists.
+ */
SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
"scsi scan: device exists on %s\n",
dev_name(&sdev->sdev_gendev)));
@@ -1230,11 +1256,11 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
sdev->model);
return SCSI_SCAN_LUN_PRESENT;
}
- scsi_device_put(sdev);
- } else
+ } else {
sdev = scsi_alloc_sdev(starget, lun, hostdata);
- if (!sdev)
- goto out;
+ if (!sdev)
+ goto out;
+ }
if (scsi_device_is_pseudo_dev(sdev)) {
if (bflagsp)
@@ -1249,6 +1275,40 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
if (scsi_probe_lun(sdev, result, result_len, &bflags))
goto out_free_result;
+ /*
+ * For reprobe scenarios, update the inquiry data with fresh
+ * INQUIRY results. The device already exists in sysfs, so we
+ * don't call scsi_add_lun() which would try to add it again.
+ */
+ if (is_reprobe) {
+ bool need_reprobe = false;
+ int update_ret = __scsi_reprobe_inquiry(sdev, result, result_len,
+ &need_reprobe);
+
+ if (update_ret < 0) {
+ res = SCSI_SCAN_NO_RESPONSE;
+ goto out_free_result;
+ }
+
+ if (bflagsp)
+ *bflagsp = bflags;
+
+ /*
+ * If type or PQ changed, reprobe to update driver attachment.
+ * Reprobe failure is not fatal - device exists, just may have
+ * wrong driver attached.
+ */
+ if (need_reprobe) {
+ if (device_reprobe(&sdev->sdev_gendev) < 0)
+ sdev_printk(KERN_WARNING, sdev,
+ "device reprobe failed\n");
+ }
+
+ /* Device already exists, just return success */
+ res = SCSI_SCAN_LUN_PRESENT;
+ goto out_free_result;
+ }
+
if (bflagsp)
*bflagsp = bflags;
/*
@@ -1331,12 +1391,27 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
if (scsi_device_get(sdev) == 0) {
*sdevp = sdev;
} else {
- __scsi_remove_device(sdev);
+ if (!is_reprobe)
+ __scsi_remove_device(sdev);
res = SCSI_SCAN_NO_RESPONSE;
}
}
- } else
- __scsi_remove_device(sdev);
+ /*
+ * For reprobe case, we held a reference from
+ * scsi_device_lookup_by_target(), release it now.
+ */
+ if (is_reprobe)
+ scsi_device_put(sdev);
+ } else {
+ /*
+ * For reprobe, device already exists - don't remove it.
+ * Just release the reference we got from lookup.
+ */
+ if (is_reprobe)
+ scsi_device_put(sdev);
+ else
+ __scsi_remove_device(sdev);
+ }
out:
return res;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan
2026-10-12 21:05 ` [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
@ 2026-10-12 21:24 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-12 21:24 UTC (permalink / raw)
To: Brian Bunker; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: sysfs duplicate filename and list corruption from duplicate scsi_device allocation
- [High] scsi: UAF and NULL pointer dereference in __scsi_reprobe_inquiry due to missing device_lock
- [Medium] scsi: scsi_device reference leak on reprobe of pseudo devices
--
commit ba325f494bfb24c08935735dfa22078230cda945
Author: Brian Bunker <brian@purestorage.com>
scsi: core: Handle reprobe for existing devices during SCSI scan
Complement scsi_rescan_device() reprobe by handling the scan path.
Update INQUIRY data and reprobe existing devices when standard INQUIRY
data changed.
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ddb92282d49ff..6df140d3c7482 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1215,7 +1216,32 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
> */
> sdev = scsi_device_lookup_by_target(starget, lun);
> if (sdev) {
> - if (rescan != SCSI_SCAN_INITIAL || !scsi_device_created(sdev)) {
> + if (rescan == SCSI_SCAN_INITIAL && scsi_device_created(sdev)) {
> + /*
> + * Initial scan found device in CREATED state (being probed
> + * by another thread). Drop reference and allocate new -
> + * the other thread will complete setup of the original.
> + */
> + scsi_device_put(sdev);
> + sdev = scsi_alloc_sdev(starget, lun, hostdata);
> + if (!sdev)
> + goto out;
[Severity: High]
Does allocating a new device here create a duplicate device for the same LUN?
Since scsi_alloc_sdev() blindly adds the new device to the starget->devices
list and the original device is still on the list, both threads will proceed
to call scsi_add_lun() and invoke device_add(). This guarantees a sysfs
duplicate filename collision and failure. Additionally, subsequent lookups
using __scsi_device_lookup_by_target() will permanently intercept and return
the dummy device at the head of the list instead of the successfully probed
one.
[ ... ]
> + } else if (rescan != SCSI_SCAN_INITIAL && !scsi_device_created(sdev)) {
> + /*
> + * Manual rescan of fully initialized device.
> + * Reprobe to detect peripheral qualifier or device type
> + * changes (e.g., ALUA state transitions).
> + */
> + SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
> + "scsi scan: device exists (type %d, PQ %d), reprobing\n",
> + sdev->type, sdev->inq_periph_qual));
> + is_reprobe = true;
> + } else {
[ ... ]
> if (scsi_device_is_pseudo_dev(sdev)) {
> if (bflagsp)
> *bflagsp = BLIST_NOLUN;
> return SCSI_SCAN_LUN_PRESENT;
> }
[Severity: Medium]
Will this early return leak a scsi_device reference when is_reprobe is true?
When a manual rescan encounters an existing pseudo device, the code sets
is_reprobe to true and retains the extra kobject reference acquired by
scsi_device_lookup_by_target(). By returning directly here, it bypasses the
cleanup block at the end of the function where scsi_device_put(sdev) would
drop the lookup reference.
[ ... ]
> + /*
> + * For reprobe scenarios, update the inquiry data with fresh
> + * INQUIRY results. The device already exists in sysfs, so we
> + * don't call scsi_add_lun() which would try to add it again.
> + */
> + if (is_reprobe) {
> + bool need_reprobe = false;
> + int update_ret = __scsi_reprobe_inquiry(sdev, result, result_len,
> + &need_reprobe);
[Severity: High]
Does this __scsi_reprobe_inquiry() call lack the required device_lock()?
This reprobe path calls __scsi_reprobe_inquiry() while holding only
shost->scan_mutex. Inside __scsi_reprobe_inquiry(), dev->driver is checked
and then dev->driver->owner is accessed for try_module_get() before invoking
the driver's rescan callback. Because device_lock(&sdev->sdev_gendev) is
not held, a concurrent sysfs unbind operation or module unload can detach the
driver and clear dev->driver immediately after the check, leading to a NULL
pointer dereference or use-after-free on the freed driver module.
> +
> + if (update_ret < 0) {
> + res = SCSI_SCAN_NO_RESPONSE;
> + goto out_free_result;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012210511.16061-1-brian@purestorage.com?part=5
^ permalink raw reply [flat|nested] 11+ messages in thread