[PATCH v2 0/2] scsi: mpt3sas: Fix delayed not-responding sdev block handling

[PATCH v2 0/2] scsi: mpt3sas: Fix delayed not-responding sdev block handling

[Jeff Layton]

v2 fixes a couple of issues that Sashiko pointed out.

This fix is for a deadlock that we see regularly in Meta's fleet. The
idea is to break the deadlock by unblocking sdevs after a timeout.

I don't do a lot of SCSI driver work, but this one fell in my lap. I
leaned heavily on agentic help for this series.

If this looks good, it'd be nice to see this in linux-next soon.

Thanks,
Jeff

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- New patch 1: serialize the interrupt-time sdev block with hostdata
  teardown in scsih_sdev_destroy() via new ioc->block_lock (fixes a
  pre-existing UAF)
- Do the timed unblock under ioc->block_lock, so it can't race with an
  interrupt-time block
- Clear block_deadline before the fw event worker polls a returning
  device with TURs, so dnr_unblock_work can't unblock it mid-poll
- Link to v1: https://lore.kernel.org/r/20261030-sas-abba-v1-1-b8cdd714a37c@kernel.org

---
Jeff Layton (2):
      scsi: mpt3sas: Serialize interrupt-time sdev block with hostdata teardown
      scsi: mpt3sas: Time out delayed not-responding device blocks

 drivers/scsi/mpt3sas/mpt3sas_base.c  |  10 +++
 drivers/scsi/mpt3sas/mpt3sas_base.h  |  10 +++
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 142 +++++++++++++++++++++++++++++++++--
 3 files changed, 156 insertions(+), 6 deletions(-)
---
base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
change-id: 20261030-sas-abba-eacfcc45fe36

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 1/2] scsi: mpt3sas: Serialize interrupt-time sdev block with hostdata teardown

[Jeff Layton]

On DELAY_NOT_RESPONDING, the event callback runs _scsih_block_io_device()
at interrupt time. It walks the sdevs with shost_for_each_device() and
uses sdev->hostdata and its sas_target, with no lock held.

The sdev reference only pins the scsi_device. __scsi_remove_device() can
still call ->sdev_destroy() concurrently, e.g. when a scan fails to
probe a new LUN, and scsih_sdev_destroy() frees hostdata, leading to a
use-after-free. The target's hostdata is freed in scsi_target_reap(),
after sdev_destroy.

Add ioc->block_lock. Take it in _scsih_block_io_device() for each sdev,
and in scsih_sdev_destroy() around clearing sdev->hostdata. While
hostdata is non-NULL under the lock, it and its target are live.

Fixes: f92363d12359 ("[SCSI] mpt3sas: add new driver supporting 12GB SAS")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 drivers/scsi/mpt3sas/mpt3sas_base.h  |  2 ++
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 21 +++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 47255bf9cdda..419fdf164fc6 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1249,6 +1249,7 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  * @pcie_device_list: pcie device object list
  * @pcie_device_init_list: pcie device object list (used only at init time)
  * @pcie_device_lock:
+ * @block_lock: serializes sdev blocking against sdev hostdata teardown
  * @io_missing_delay: time for IO completed by fw when PDR enabled
  * @device_missing_delay: time for device missing by fw when PDR enabled
  * @sas_id : used for setting volume target IDs
@@ -1482,6 +1483,7 @@ struct MPT3SAS_ADAPTER {
 	struct list_head pcie_device_list;
 	struct list_head pcie_device_init_list;
 	spinlock_t      pcie_device_lock;
+	spinlock_t	block_lock;
 
 	struct list_head raid_device_list;
 	spinlock_t	raid_device_lock;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 085d4672db85..c7886a12d308 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2176,6 +2176,7 @@ scsih_sdev_init(struct scsi_device *sdev)
 static void
 scsih_sdev_destroy(struct scsi_device *sdev)
 {
+	struct MPT3SAS_DEVICE *sas_device_priv_data;
 	struct MPT3SAS_TARGET *sas_target_priv_data;
 	struct scsi_target *starget;
 	struct Scsi_Host *shost;
@@ -2218,8 +2219,11 @@ scsih_sdev_destroy(struct scsi_device *sdev)
 		spin_unlock_irqrestore(&ioc->sas_device_lock, flags);
 	}
 
-	kfree(sdev->hostdata);
+	spin_lock_irqsave(&ioc->block_lock, flags);
+	sas_device_priv_data = sdev->hostdata;
 	sdev->hostdata = NULL;
+	spin_unlock_irqrestore(&ioc->block_lock, flags);
+	kfree(sas_device_priv_data);
 }
 
 /**
@@ -4234,26 +4238,30 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 handle)
 	struct MPT3SAS_DEVICE *sas_device_priv_data;
 	struct scsi_device *sdev;
 	struct _sas_device *sas_device;
+	unsigned long flags;
 
 	sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
 
 	shost_for_each_device(sdev, ioc->shost) {
+		spin_lock_irqsave(&ioc->block_lock, flags);
 		sas_device_priv_data = sdev->hostdata;
 		if (!sas_device_priv_data)
-			continue;
+			goto next;
 		if (sas_device_priv_data->sas_target->handle != handle)
-			continue;
+			goto next;
 		if (sas_device_priv_data->block)
-			continue;
+			goto next;
 		if (sas_device && sas_device->pend_sas_rphy_add)
-			continue;
+			goto next;
 		if (sas_device_priv_data->ignore_delay_remove) {
 			sdev_printk(KERN_INFO, sdev,
 			"%s skip device_block for SES handle(0x%04x)\n",
 			__func__, sas_device_priv_data->sas_target->handle);
-			continue;
+			goto next;
 		}
 		_scsih_internal_device_block(sdev, sas_device_priv_data);
+next:
+		spin_unlock_irqrestore(&ioc->block_lock, flags);
 	}
 
 	if (sas_device)
@@ -13537,6 +13545,7 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	spin_lock_init(&ioc->raid_device_lock);
 	spin_lock_init(&ioc->pcie_device_lock);
 	spin_lock_init(&ioc->diag_trigger_lock);
+	spin_lock_init(&ioc->block_lock);
 
 	INIT_LIST_HEAD(&ioc->sas_device_list);
 	INIT_LIST_HEAD(&ioc->sas_device_init_list);

-- 
2.56.0

[PATCH v2 2/2] scsi: mpt3sas: Time out delayed not-responding device blocks

[Jeff Layton]

We have an agent that writes "- - -" to the sysfs scan attribute at
regular intervals. Several of these scans ended up deadlocked:

 1. A drive in reset failed its initial scan, so the worker started to
    remove the drive.
 2. A sysfs scan took sas_host->lock, allocated an sdev for the drive
    and sent an INQUIRY.
 3. A DELAY_NOT_RESPONDING event blocked the new sdev.
 4. The worker waited for sas_host->lock in sas_rphy_free():

      __mutex_lock
      sas_port_delete+0x75 [scsi_transport_sas]
      mpt3sas_transport_port_remove
      _scsih_add_device

After that, all further scan attempts got stuck waiting on the
sas_host->lock.

On DELAY_NOT_RESPONDING the event callback blocks every sdev on the
handle. The block is released only:
 - by the callback, if the firmware later reports TARG_NOT_RESPONDING
 - by the firmware event worker, when it processes a later event

Commit e4bc7f5c21a1 ("mpt3sas: Don't block the drive when drive addition
under the control of SML") was a partial fix for a similar issue.  Not
blocking sdevs that a scan is still probing is not enough: the scan also
sends REPORT LUNS to LUN 0 once it is visible, and LUN 0 may be blocked
before or during the scan.

Within the report device missing delay, the firmware either reports the
device as not responding, and the callback unblocks it, or the device
has come back. Past that delay the block has no further use.

Schedule delayed_work to the system_wq for the missing delay plus 30s.
When it runs, unblock any expired sdevs.

Cc: Keith Busch <kbusch@kernel.org>
Fixes: f92363d12359 ("[SCSI] mpt3sas: add new driver supporting 12GB SAS")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c  |  10 +++
 drivers/scsi/mpt3sas/mpt3sas_base.h  |  10 ++-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 121 +++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 1af25a22611a..1c4d10acc7ff 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -5405,6 +5405,7 @@ _base_update_diag_trigger_pages(struct MPT3SAS_ADAPTER *ioc)
 /**
  * _base_assign_fw_reported_qd	- Get FW reported QD for SAS/SATA devices.
  *				- On failure set default QD values.
+ *				- Also get the PCIe device missing delay.
  * @ioc : per adapter object
  *
  * Returns 0 for success, non-zero for failure.
@@ -5416,6 +5417,7 @@ static int _base_assign_fw_reported_qd(struct MPT3SAS_ADAPTER *ioc)
 	Mpi2SasIOUnitPage1_t sas_iounit_pg1;
 	Mpi26PCIeIOUnitPage1_t pcie_iounit_pg1;
 	u16 depth;
+	u8 dmd;
 	int rc = 0;
 
 	ioc->max_wideport_qd = MPT3SAS_SAS_QUEUE_DEPTH;
@@ -5453,6 +5455,14 @@ static int _base_assign_fw_reported_qd(struct MPT3SAS_ADAPTER *ioc)
 	ioc->max_nvme_qd = (le16_to_cpu(pcie_iounit_pg1.NVMeMaxQueueDepth)) ?
 	    (le16_to_cpu(pcie_iounit_pg1.NVMeMaxQueueDepth)) :
 	    MPT3SAS_NVME_QUEUE_DEPTH;
+
+	dmd = pcie_iounit_pg1.DMDReportPCIe;
+	if (dmd & MPI26_PCIEIOUNIT1_DMDRPT_UNIT_16_SEC)
+		ioc->pcie_device_missing_delay =
+		    (dmd & MPI26_PCIEIOUNIT1_DMDRPT_DELAY_TIME_MASK) * 16;
+	else
+		ioc->pcie_device_missing_delay =
+		    dmd & MPI26_PCIEIOUNIT1_DMDRPT_DELAY_TIME_MASK;
 out:
 	dinitprintk(ioc, pr_err(
 	    "MaxWidePortQD: 0x%x MaxNarrowPortQD: 0x%x MaxSataQD: 0x%x MaxNvmeQD: 0x%x\n",
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 419fdf164fc6..e932d1d05ef1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -469,6 +469,8 @@ struct MPT3SAS_TARGET {
  * @flags: MPT_DEVICE_XXX flags
  * @configured_lun: lun is configured
  * @block: device is in SDEV_BLOCK state
+ * @block_deadline: jiffies after which a delayed not-responding block is
+ *	released without waiting for the firmware event worker, 0 if none
  * @tlr_snoop_check: flag used in determining whether to disable TLR
  * @eedp_enable: eedp support enable bit
  * @eedp_type: 0(type_1), 1(type_2), 2(type_3)
@@ -486,6 +488,7 @@ struct MPT3SAS_DEVICE {
 	u8	ignore_delay_remove;
 	/* Iopriority Command Handling */
 	u8	ncq_prio_enable;
+	unsigned long	block_deadline;
 	/*
 	 * Bug workaround for SATL handling: the mpt2/3sas firmware
 	 * doesn't return BUSY or TASK_SET_FULL for subsequent
@@ -1165,6 +1168,7 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  *			pci resource handling
  * @fault_reset_work_q: fw fault workqueue
  * @fault_reset_work: fw fault work
+ * @dnr_unblock_work: releases expired delayed not-responding blocks
  * @firmware_event_thread: fw event work queue
  * @fw_event_lock:
  * @fw_event_list: list of fw events
@@ -1249,9 +1253,11 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  * @pcie_device_list: pcie device object list
  * @pcie_device_init_list: pcie device object list (used only at init time)
  * @pcie_device_lock:
- * @block_lock: serializes sdev blocking against sdev hostdata teardown
+ * @block_lock: serializes sdev blocking against dnr_unblock_work and
+ *	sdev hostdata teardown
  * @io_missing_delay: time for IO completed by fw when PDR enabled
  * @device_missing_delay: time for device missing by fw when PDR enabled
+ * @pcie_device_missing_delay: time for PCIe device missing by fw
  * @sas_id : used for setting volume target IDs
  * @pcie_target_id: used for setting pcie target IDs
  * @blocking_handles: bitmask used to identify which devices need blocking
@@ -1351,6 +1357,7 @@ struct MPT3SAS_ADAPTER {
 	/* fw fault handler */
 	struct workqueue_struct *fault_reset_work_q;
 	struct delayed_work fault_reset_work;
+	struct delayed_work dnr_unblock_work;
 
 	/* fw event handler */
 	struct workqueue_struct	*firmware_event_thread;
@@ -1489,6 +1496,7 @@ struct MPT3SAS_ADAPTER {
 	spinlock_t	raid_device_lock;
 	u8		io_missing_delay;
 	u16		device_missing_delay;
+	u16		pcie_device_missing_delay;
 	int		sas_id;
 	int		pcie_target_id;
 
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index c7886a12d308..af2666f9576e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -3882,6 +3882,7 @@ _scsih_internal_device_block(struct scsi_device *sdev,
 	sdev_printk(KERN_INFO, sdev, "device_block, handle(0x%04x)\n",
 	    sas_device_priv_data->sas_target->handle);
 	sas_device_priv_data->block = 1;
+	sas_device_priv_data->block_deadline = 0;
 
 	r = scsi_internal_device_block_nowait(sdev);
 	if (r == -EINVAL)
@@ -3907,6 +3908,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
 	sdev_printk(KERN_WARNING, sdev, "device_unblock and setting to running, "
 	    "handle(0x%04x)\n", sas_device_priv_data->sas_target->handle);
 	sas_device_priv_data->block = 0;
+	sas_device_priv_data->block_deadline = 0;
 	r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
 	if (r == -EINVAL) {
 		/* The device has been set to SDEV_RUNNING by SD layer during
@@ -3934,6 +3936,26 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
 	}
 }
 
+/**
+ * _scsih_clear_block_deadline - keep dnr_unblock_work off a blocked sdev
+ * @ioc: per adapter object
+ * @sas_device_priv_data: per device driver private data
+ *
+ * Called by the firmware event worker before it polls a blocked device
+ * with TURs.  The worker unblocks the device when polling ends, and
+ * must not have dnr_unblock_work send I/O to it in the meantime.
+ */
+static void
+_scsih_clear_block_deadline(struct MPT3SAS_ADAPTER *ioc,
+			    struct MPT3SAS_DEVICE *sas_device_priv_data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ioc->block_lock, flags);
+	sas_device_priv_data->block_deadline = 0;
+	spin_unlock_irqrestore(&ioc->block_lock, flags);
+}
+
 /**
  * _scsih_ublock_io_all_device - unblock every device
  * @ioc: per adapter object
@@ -3974,6 +3996,7 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
 			continue;
 		}
 
+		_scsih_clear_block_deadline(ioc, sas_device_priv_data);
 		do {
 			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
 			if (pcie_device && (!ioc->tm_custom_handling) &&
@@ -4089,6 +4112,7 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
 		if (!sas_device_priv_data->block)
 			continue;
 
+		_scsih_clear_block_deadline(ioc, sas_device_priv_data);
 		do {
 			host_reset_completion_count = 0;
 			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
@@ -4225,6 +4249,92 @@ _scsih_block_io_all_device(struct MPT3SAS_ADAPTER *ioc)
 	}
 }
 
+/*
+ * Margin on top of the firmware's report device missing delay before
+ * _scsih_dnr_unblock_work() releases a delayed not-responding block.
+ */
+#define MPT3SAS_DNR_BLOCK_SLACK	30
+
+static unsigned long
+_scsih_dnr_block_timeout(struct MPT3SAS_ADAPTER *ioc)
+{
+	return (max(ioc->device_missing_delay, ioc->pcie_device_missing_delay) +
+	    MPT3SAS_DNR_BLOCK_SLACK) * HZ;
+}
+
+/**
+ * _scsih_dnr_unblock_sdev - release an expired delayed not-responding block
+ * @ioc: per adapter object
+ * @sdev: scsi device struct
+ *
+ * Return: the deadline of an unexpired block on @sdev, or 0 if none.
+ */
+static unsigned long
+_scsih_dnr_unblock_sdev(struct MPT3SAS_ADAPTER *ioc, struct scsi_device *sdev)
+{
+	struct MPT3SAS_DEVICE *sas_device_priv_data;
+	unsigned long flags, deadline = 0;
+
+	spin_lock_irqsave(&ioc->block_lock, flags);
+	sas_device_priv_data = sdev->hostdata;
+	if (!sas_device_priv_data || !sas_device_priv_data->block ||
+	    !sas_device_priv_data->block_deadline)
+		goto out;
+	if (time_before(jiffies, sas_device_priv_data->block_deadline)) {
+		deadline = sas_device_priv_data->block_deadline;
+		goto out;
+	}
+	sdev_printk(KERN_WARNING, sdev,
+	    "delayed not-responding block expired, handle(0x%04x)\n",
+	    sas_device_priv_data->sas_target->handle);
+	_scsih_internal_device_unblock(sdev, sas_device_priv_data);
+out:
+	spin_unlock_irqrestore(&ioc->block_lock, flags);
+	return deadline;
+}
+
+/**
+ * _scsih_dnr_unblock_work - release expired delayed not-responding blocks
+ * @work: the dnr_unblock_work of the adapter
+ *
+ * Within the device missing delay, the firmware either reports the device
+ * as not responding, and the event callback unblocks it, or the device
+ * comes back, and only the firmware event worker unblocks it.  The worker
+ * may first need shost->scan_mutex or the SAS transport's host lock, and
+ * a scan holding those can have a command queued on the blocked sdev.
+ * Past the delay the block has no further use, so release it here.
+ */
+static void
+_scsih_dnr_unblock_work(struct work_struct *work)
+{
+	struct MPT3SAS_ADAPTER *ioc = container_of(to_delayed_work(work),
+	    struct MPT3SAS_ADAPTER, dnr_unblock_work);
+	struct scsi_device *sdev;
+	unsigned long deadline, next = 0;
+
+	if (ioc->remove_host)
+		return;
+	if (ioc->shost_recovery || ioc->pci_error_recovery) {
+		queue_delayed_work(system_dfl_wq, &ioc->dnr_unblock_work, HZ);
+		return;
+	}
+
+	shost_for_each_device(sdev, ioc->shost) {
+		deadline = _scsih_dnr_unblock_sdev(ioc, sdev);
+		if (deadline && (!next || time_before(deadline, next)))
+			next = deadline;
+	}
+
+	/*
+	 * A block made while this ran may have queued the work already,
+	 * with its own later deadline.  Pull the timer in to the earliest
+	 * one seen here; the next run reschedules for the rest.
+	 */
+	if (next)
+		mod_delayed_work(system_dfl_wq, &ioc->dnr_unblock_work,
+		    max_t(long, next - jiffies, 1));
+}
+
 /**
  * _scsih_block_io_device - set the device state to SDEV_BLOCK
  * @ioc: per adapter object
@@ -4238,7 +4348,9 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 handle)
 	struct MPT3SAS_DEVICE *sas_device_priv_data;
 	struct scsi_device *sdev;
 	struct _sas_device *sas_device;
+	unsigned long timeout = _scsih_dnr_block_timeout(ioc);
 	unsigned long flags;
+	bool blocked = false;
 
 	sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
 
@@ -4260,12 +4372,18 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 handle)
 			goto next;
 		}
 		_scsih_internal_device_block(sdev, sas_device_priv_data);
+		sas_device_priv_data->block_deadline = (jiffies + timeout) ?: 1;
+		blocked = true;
 next:
 		spin_unlock_irqrestore(&ioc->block_lock, flags);
 	}
 
 	if (sas_device)
 		sas_device_put(sas_device);
+
+	if (blocked)
+		queue_delayed_work(system_dfl_wq, &ioc->dnr_unblock_work,
+		    timeout);
 }
 
 /**
@@ -12549,6 +12667,7 @@ static void scsih_remove(struct pci_dev *pdev)
 		return;
 
 	ioc->remove_host = 1;
+	disable_delayed_work_sync(&ioc->dnr_unblock_work);
 
 	if (!pci_device_is_present(pdev)) {
 		mpt3sas_base_pause_mq_polling(ioc);
@@ -12649,6 +12768,7 @@ scsih_shutdown(struct pci_dev *pdev)
 		return;
 
 	ioc->remove_host = 1;
+	disable_delayed_work_sync(&ioc->dnr_unblock_work);
 
 	if (!pci_device_is_present(pdev)) {
 		mpt3sas_base_pause_mq_polling(ioc);
@@ -13562,6 +13682,7 @@ _scsih_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	INIT_LIST_HEAD(&ioc->delayed_tr_volume_list);
 	INIT_LIST_HEAD(&ioc->reply_queue_list);
 	INIT_LIST_HEAD(&ioc->port_table_list);
+	INIT_DELAYED_WORK(&ioc->dnr_unblock_work, _scsih_dnr_unblock_work);
 
 	sprintf(ioc->name, "%s_cm%d", ioc->driver_name, ioc->id);
 

-- 
2.56.0

Re: [PATCH v2 1/2] scsi: mpt3sas: Serialize interrupt-time sdev block with hostdata teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: unserialized sdev->hostdata publication in scsih_sdev_init()
--

commit 4a6206c3cdf6053f62da0b3cf7f294219f312a94
Author: Jeff Layton <jlayton@kernel.org>

scsi: mpt3sas: Serialize interrupt-time sdev block with hostdata teardown

This commit adds ioc->block_lock to serialize interrupt-time device blocking
in _scsih_block_io_device() against sdev->hostdata teardown in
scsih_sdev_destroy().

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 085d4672db854..c7886a12d3088 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -4234,26 +4238,30 @@ _scsih_block_io_device(struct MPT3SAS_ADAPTER *ioc, u16 handle)
>  	struct MPT3SAS_DEVICE *sas_device_priv_data;
>  	struct scsi_device *sdev;
>  	struct _sas_device *sas_device;
> +	unsigned long flags;
>  
>  	sas_device = mpt3sas_get_sdev_by_handle(ioc, handle);
>  
>  	shost_for_each_device(sdev, ioc->shost) {
> +		spin_lock_irqsave(&ioc->block_lock, flags);
>  		sas_device_priv_data = sdev->hostdata;
>  		if (!sas_device_priv_data)
> -			continue;
> +			goto next;
>  		if (sas_device_priv_data->sas_target->handle != handle)
> -			continue;
> +			goto next;

[Severity: Medium]
During LUN scanning, scsi_alloc_sdev() calls scsi_sysfs_device_initialize(),
which links sdev onto shost->__devices before invoking shost->hostt->sdev_init:

drivers/scsi/scsi_scan.c:scsi_alloc_sdev() {
	...
	scsi_sysfs_device_initialize(sdev);
	...
	if (shost->hostt->sdev_init) {
		ret = shost->hostt->sdev_init(sdev);
	...
}

Because sdev is already in state SDEV_CREATED on shost->__devices,
shost_for_each_device() in _scsih_block_io_device() can visit sdev while
scsih_sdev_init() is executing concurrently on another CPU:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_sdev_init() {
	...
	sas_device_priv_data->lun = sdev->lun;
	sas_device_priv_data->flags = MPT_DEVICE_FLAGS_INIT;

	starget = scsi_target(sdev);
	sas_target_priv_data = starget->hostdata;
	sas_target_priv_data->num_luns++;
	sas_device_priv_data->sas_target = sas_target_priv_data;
	sdev->hostdata = sas_device_priv_data;
	...
}

Since scsih_sdev_init() assigns sas_device_priv_data->sas_target and
sdev->hostdata with plain stores without holding ioc->block_lock or using
smp_store_release(), could _scsih_block_io_device() observe a non-NULL
sdev->hostdata before the store to sas_device_priv_data->sas_target is
visible on weakly-ordered architectures, leading to a NULL pointer
dereference when reading sas_device_priv_data->sas_target->handle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-sas-abba-v2-0-b3b9ba874b5a@kernel.org?part=1

Re: [PATCH v2 2/2] scsi: mpt3sas: Time out delayed not-responding device blocks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mpt3sas: race between _scsih_dnr_unblock_work() and unlocked unblock paths
- [High] scsi: mpt3sas: _scsih_clear_block_deadline() races and misses blocked sdevs
- [Medium] scsi: mpt3sas: _scsih_dnr_unblock_work() unblocks devices during host recovery
--

commit 3086ca833022a9345fb0fd3cc229d91c87b61323
Author: Jeff Layton <jlayton@kernel.org>

scsi: mpt3sas: Time out delayed not-responding device blocks

This commit adds a delayed work item to time out and release device blocks
triggered by DELAY_NOT_RESPONDING events if the device missing delay expires
without a subsequent firmware event unblocking the device. This avoids a
deadlock where a sysfs SCSI scan holds sas_host->lock while waiting on a
blocked sdev and prevents the firmware event worker from acquiring
sas_host->lock.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index c7886a12d3088..af2666f9576e5 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -3934,6 +3936,26 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
>  	}
>  }
>  
> +/**
> + * _scsih_clear_block_deadline - keep dnr_unblock_work off a blocked sdev
> + * @ioc: per adapter object
> + * @sas_device_priv_data: per device driver private data
> + *
> + * Called by the firmware event worker before it polls a blocked device
> + * with TURs.  The worker unblocks the device when polling ends, and
> + * must not have dnr_unblock_work send I/O to it in the meantime.
> + */
> +static void
> +_scsih_clear_block_deadline(struct MPT3SAS_ADAPTER *ioc,
> +			    struct MPT3SAS_DEVICE *sas_device_priv_data)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ioc->block_lock, flags);
> +	sas_device_priv_data->block_deadline = 0;
> +	spin_unlock_irqrestore(&ioc->block_lock, flags);
> +}
> +
>  /**
>   * _scsih_ublock_io_all_device - unblock every device
>   * @ioc: per adapter object
> @@ -3974,6 +3996,7 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
>  			continue;
>  		}
>  
> +		_scsih_clear_block_deadline(ioc, sas_device_priv_data);
>  		do {
>  			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
>  			if (pcie_device && (!ioc->tm_custom_handling) &&
> @@ -4089,6 +4112,7 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
>  		if (!sas_device_priv_data->block)
>  			continue;
>  
> +		_scsih_clear_block_deadline(ioc, sas_device_priv_data);

[Severity: High]
As reported by Sashiko on v1 [1], can _scsih_dnr_unblock_work() still race
with the firmware event worker or unblock remaining sdevs while the worker is
polling devices?

First, in both _scsih_ublock_io_all_device() and
_scsih_ublock_io_device_wait(), sdev->hostdata and
sas_device_priv_data->block are checked without holding ioc->block_lock
before passing sas_device_priv_data to _scsih_clear_block_deadline(). If
__scsi_remove_device() -> scsih_sdev_destroy() runs concurrently, it clears
sdev->hostdata under ioc->block_lock and frees sas_device_priv_data before
_scsih_clear_block_deadline() takes ioc->block_lock and writes to
sas_device_priv_data->block_deadline. Even if the sdev is not being removed,
_scsih_dnr_unblock_sdev() can run between the unlocked
sas_device_priv_data->block check and _scsih_clear_block_deadline(), leaving
the worker to poll an already-unblocked running sdev with TURs.

Second, _scsih_clear_block_deadline() is called inside the
shost_for_each_device() loop for one sdev at a time right before
_scsih_wait_for_device_to_become_ready(), which sleeps across up to
command_retry_count retries. While the worker sleeps polling the first
blocked sdev (such as LUN 0 of a multi-LUN target in
_scsih_ublock_io_device_wait(), or the first blocked device in
_scsih_ublock_io_all_device()), block_deadline remains active on all subseqent
blocked sdevs. If their deadline expires during that sleep,
_scsih_dnr_unblock_work() will unblock those sdevs prematurely and cause the
polling loop to skip them when it later checks sas_device_priv_data->block.

Third, when _scsih_sas_broadcast_primitive_event() calls
_scsih_block_io_all_device() to keep all devices blocked during broadcast
primitive task management, any sdev already blocked by an earlier
DELAY_NOT_RESPONDING event is skipped without clearing block_deadline:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_block_io_all_device()
	shost_for_each_device(sdev, ioc->shost) {
		sas_device_priv_data = sdev->hostdata;
		if (!sas_device_priv_data)
			continue;
		if (sas_device_priv_data->block)
			continue;

If that earlier deadline expires while _scsih_sas_broadcast_primitive_event()
is still running, wouldn't _scsih_dnr_unblock_work() unblock the device in
the middle of task management?

>  		do {
>  			host_reset_completion_count = 0;
>  			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
> @@ -4225,6 +4249,92 @@ _scsih_block_io_all_device(struct MPT3SAS_ADAPTER *ioc)
>  	}
>  }
>  
> +/*
> + * Margin on top of the firmware's report device missing delay before
> + * _scsih_dnr_unblock_work() releases a delayed not-responding block.
> + */
> +#define MPT3SAS_DNR_BLOCK_SLACK	30
> +
> +static unsigned long
> +_scsih_dnr_block_timeout(struct MPT3SAS_ADAPTER *ioc)
> +{
> +	return (max(ioc->device_missing_delay, ioc->pcie_device_missing_delay) +
> +	    MPT3SAS_DNR_BLOCK_SLACK) * HZ;
> +}
> +
> +/**
> + * _scsih_dnr_unblock_sdev - release an expired delayed not-responding block
> + * @ioc: per adapter object
> + * @sdev: scsi device struct
> + *
> + * Return: the deadline of an unexpired block on @sdev, or 0 if none.
> + */
> +static unsigned long
> +_scsih_dnr_unblock_sdev(struct MPT3SAS_ADAPTER *ioc, struct scsi_device *sdev)
> +{
> +	struct MPT3SAS_DEVICE *sas_device_priv_data;
> +	unsigned long flags, deadline = 0;
> +
> +	spin_lock_irqsave(&ioc->block_lock, flags);
> +	sas_device_priv_data = sdev->hostdata;
> +	if (!sas_device_priv_data || !sas_device_priv_data->block ||
> +	    !sas_device_priv_data->block_deadline)
> +		goto out;
> +	if (time_before(jiffies, sas_device_priv_data->block_deadline)) {
> +		deadline = sas_device_priv_data->block_deadline;
> +		goto out;
> +	}
> +	sdev_printk(KERN_WARNING, sdev,
> +	    "delayed not-responding block expired, handle(0x%04x)\n",
> +	    sas_device_priv_data->sas_target->handle);
> +	_scsih_internal_device_unblock(sdev, sas_device_priv_data);
> +out:
> +	spin_unlock_irqrestore(&ioc->block_lock, flags);
> +	return deadline;
> +}

[Severity: High]
Does _scsih_dnr_unblock_sdev() race with the other unblock paths that do not
acquire ioc->block_lock?

While _scsih_dnr_unblock_sdev() runs on system_dfl_wq under ioc->block_lock,
_scsih_ublock_io_device() (called from interrupt context in
mpt3sas_scsih_event_callback() -> _scsih_tm_tr_send() on
MPI2_EVENT_SAS_TOPO_RC_TARG_NOT_RESPONDING and from worker context in
_scsih_check_device() and _scsih_remove_device()),
_scsih_ublock_io_all_device(), and _scsih_ublock_io_device_wait() inspect and
clear sas_device_priv_data->block and call _scsih_internal_device_unblock()
or scsi_internal_device_unblock_nowait() without holding ioc->block_lock:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_ublock_io_device()
	shost_for_each_device(sdev, ioc->shost) {
		...
		if (sas_device_priv_data->block)
			_scsih_internal_device_unblock(sdev,
				sas_device_priv_data);
	}

If _scsih_dnr_unblock_sdev() and _scsih_ublock_io_device() (or
_scsih_ublock_io_all_device()) both see sas_device_priv_data->block == 1 and
call _scsih_internal_device_unblock() concurrently, the first call transitions
sdev->sdev_state from SDEV_BLOCK to SDEV_RUNNING (or from SDEV_CREATED_BLOCK
to SDEV_CREATED). The second caller's scsi_internal_device_unblock_nowait()
then returns -EINVAL and enters the fallback path in
_scsih_internal_device_unblock():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_internal_device_unblock()
	r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
	if (r == -EINVAL) {
		...
		sas_device_priv_data->block = 1;
		r = scsi_internal_device_block_nowait(sdev);
		...
		sas_device_priv_data->block = 0;
		r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
	}

That re-blocks the already-unblocked sdev, and if the sdev was in
SDEV_CREATED during a scan, scsi_internal_device_block_nowait() puts it back
into SDEV_CREATED_BLOCK where the subsequent unblock transitions it only to
SDEV_CREATED, racing with scsi_sysfs_add_sdev().

In addition, when _scsih_ublock_io_device(), _scsih_ublock_io_all_device(), or
_scsih_ublock_io_device_wait() clears sas_device_priv_data->block = 0 without
holding ioc->block_lock before calling scsi_internal_device_unblock_nowait(),
a concurrent DELAY_NOT_RESPONDING interrupt in _scsih_block_io_device() can
take ioc->block_lock, observe block == 0, set block = 1 and block_deadline,
and call scsi_internal_device_block_nowait(). The unblocking thread then runs
scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING), leaving sdev in
SDEV_RUNNING with its request queue started while sas_device_priv_data->block
is still 1 (causing scsih_qcmd() to return SCSI_MLQUEUE_DEVICE_BUSY for all
commands).

Should all unblock paths synchronize using ioc->block_lock?

> +
> +/**
> + * _scsih_dnr_unblock_work - release expired delayed not-responding blocks
> + * @work: the dnr_unblock_work of the adapter
> + *
> + * Within the device missing delay, the firmware either reports the device
> + * as not responding, and the event callback unblocks it, or the device
> + * comes back, and only the firmware event worker unblocks it.  The worker
> + * may first need shost->scan_mutex or the SAS transport's host lock, and
> + * a scan holding those can have a command queued on the blocked sdev.
> + * Past the delay the block has no further use, so release it here.
> + */
> +static void
> +_scsih_dnr_unblock_work(struct work_struct *work)
> +{
> +	struct MPT3SAS_ADAPTER *ioc = container_of(to_delayed_work(work),
> +	    struct MPT3SAS_ADAPTER, dnr_unblock_work);
> +	struct scsi_device *sdev;
> +	unsigned long deadline, next = 0;
> +
> +	if (ioc->remove_host)
> +		return;
> +	if (ioc->shost_recovery || ioc->pci_error_recovery) {
> +		queue_delayed_work(system_dfl_wq, &ioc->dnr_unblock_work, HZ);
> +		return;
> +	}

[Severity: Medium]
Should _scsih_dnr_unblock_work() also check scsi_host_in_recovery(ioc->shost)
and avoid unblocking targets marked sas_target->deleted, and use
mod_delayed_work() here?

When a host reset finishes in mpt3sas_base_hard_reset_handler(),
mpt3sas_scsih_reset_done_handler() marks unresponding targets with
sas_target->deleted = 1 and queues MPT3SAS_REMOVE_UNRESPONDING_DEVICES, and
ioc->shost_recovery is cleared to 0 while scsi_host_in_recovery(ioc->shost)
can remain true until SCSI EH finishes. During SHOST_RECOVERY, scsih_qcmd()
relies on sas_device_priv_data->block remaining set so TEST_UNIT_READY
commands complete immediately with UNIT_ATTENTION / I_T_NEXUS_LOSS_OCCURRED,
which is why _mpt3sas_fw_work() waits for both scsi_host_in_recovery() and
ioc->shost_recovery before running _scsih_remove_unresponding_devices():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_mpt3sas_fw_work()
	case MPT3SAS_REMOVE_UNRESPONDING_DEVICES:
		while (scsi_host_in_recovery(ioc->shost) ||
					 ioc->shost_recovery) {
			if (ioc->remove_host || ioc->fw_events_cleanup)
				goto out;
			ssleep(1);
		}
		_scsih_remove_unresponding_devices(ioc);

Because _scsih_dnr_unblock_work() only checks ioc->shost_recovery and
_scsih_dnr_unblock_sdev() does not check sas_target->deleted (unlike
_scsih_ublock_io_all_device()), a block deadline expiring after
ioc->shost_recovery clears can unblock pre-reset or deleted devices while
SHOST_RECOVERY or post-reset device removal is still pending.

Also, if _scsih_block_io_device() queues dnr_unblock_work with a full timeout
at the same time _scsih_dnr_unblock_work() enters the recovery deferral
branch, queue_delayed_work(..., HZ) will be a no-op because the work item is
already pending, pushing the 1-second retry out to the full timeout unless
mod_delayed_work() is used.

[1] https://lore.kernel.org/r/20261030-sas-abba-v1-1-b8cdd714a37c@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-sas-abba-v2-0-b3b9ba874b5a@kernel.org?part=2

Re: [PATCH v2 2/2] scsi: mpt3sas: Time out delayed not-responding device blocks

[Jeff Layton]

On Mon, 2026-11-02 at 14:06 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] scsi: mpt3sas: race between _scsih_dnr_unblock_work() and unlocked unblock paths
> - [High] scsi: mpt3sas: _scsih_clear_block_deadline() races and misses blocked sdevs
> - [Medium] scsi: mpt3sas: _scsih_dnr_unblock_work() unblocks devices during host recovery
> --
> 
> commit 3086ca833022a9345fb0fd3cc229d91c87b61323
> Author: Jeff Layton <jlayton@kernel.org>
> 
> scsi: mpt3sas: Time out delayed not-responding device blocks
> 
> This commit adds a delayed work item to time out and release device blocks
> triggered by DELAY_NOT_RESPONDING events if the device missing delay expires
> without a subsequent firmware event unblocking the device. This avoids a
> deadlock where a sysfs SCSI scan holds sas_host->lock while waiting on a
> blocked sdev and prevents the firmware event worker from acquiring
> sas_host->lock.
> 
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > index c7886a12d3088..af2666f9576e5 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> 
> [ ... ]
> 
> > @@ -3934,6 +3936,26 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
> >  	}
> >  }
> >  
> > +/**
> > + * _scsih_clear_block_deadline - keep dnr_unblock_work off a blocked sdev
> > + * @ioc: per adapter object
> > + * @sas_device_priv_data: per device driver private data
> > + *
> > + * Called by the firmware event worker before it polls a blocked device
> > + * with TURs.  The worker unblocks the device when polling ends, and
> > + * must not have dnr_unblock_work send I/O to it in the meantime.
> > + */
> > +static void
> > +_scsih_clear_block_deadline(struct MPT3SAS_ADAPTER *ioc,
> > +			    struct MPT3SAS_DEVICE *sas_device_priv_data)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&ioc->block_lock, flags);
> > +	sas_device_priv_data->block_deadline = 0;
> > +	spin_unlock_irqrestore(&ioc->block_lock, flags);
> > +}
> > +
> >  /**
> >   * _scsih_ublock_io_all_device - unblock every device
> >   * @ioc: per adapter object
> > @@ -3974,6 +3996,7 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
> >  			continue;
> >  		}
> >  
> > +		_scsih_clear_block_deadline(ioc, sas_device_priv_data);
> >  		do {
> >  			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
> >  			if (pcie_device && (!ioc->tm_custom_handling) &&
> > @@ -4089,6 +4112,7 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
> >  		if (!sas_device_priv_data->block)
> >  			continue;
> >  
> > +		_scsih_clear_block_deadline(ioc, sas_device_priv_data);
> 
> [Severity: High]
> As reported by Sashiko on v1 [1], can _scsih_dnr_unblock_work() still race
> with the firmware event worker or unblock remaining sdevs while the worker is
> polling devices?
> 

True. We can fix this by taking the block_lock in more places. I'm
working on a v3. I also have a fix for 1/2 for the problem sashiko
spotted there.

> First, in both _scsih_ublock_io_all_device() and
> _scsih_ublock_io_device_wait(), sdev->hostdata and
> sas_device_priv_data->block are checked without holding ioc->block_lock
> before passing sas_device_priv_data to _scsih_clear_block_deadline(). If
> __scsi_remove_device() -> scsih_sdev_destroy() runs concurrently, it clears
> sdev->hostdata under ioc->block_lock and frees sas_device_priv_data before
> _scsih_clear_block_deadline() takes ioc->block_lock and writes to
> sas_device_priv_data->block_deadline. Even if the sdev is not being removed,
> _scsih_dnr_unblock_sdev() can run between the unlocked
> sas_device_priv_data->block check and _scsih_clear_block_deadline(), leaving
> the worker to poll an already-unblocked running sdev with TURs.
> 
> Second, _scsih_clear_block_deadline() is called inside the
> shost_for_each_device() loop for one sdev at a time right before
> _scsih_wait_for_device_to_become_ready(), which sleeps across up to
> command_retry_count retries. While the worker sleeps polling the first
> blocked sdev (such as LUN 0 of a multi-LUN target in
> _scsih_ublock_io_device_wait(), or the first blocked device in
> _scsih_ublock_io_all_device()), block_deadline remains active on all subseqent
> blocked sdevs. If their deadline expires during that sleep,
> _scsih_dnr_unblock_work() will unblock those sdevs prematurely and cause the
> polling loop to skip them when it later checks sas_device_priv_data->block.
> 
> Third, when _scsih_sas_broadcast_primitive_event() calls
> _scsih_block_io_all_device() to keep all devices blocked during broadcast
> primitive task management, any sdev already blocked by an earlier
> DELAY_NOT_RESPONDING event is skipped without clearing block_deadline:
> 
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_block_io_all_device()
> 	shost_for_each_device(sdev, ioc->shost) {
> 		sas_device_priv_data = sdev->hostdata;
> 		if (!sas_device_priv_data)
> 			continue;
> 		if (sas_device_priv_data->block)
> 			continue;
> 
> If that earlier deadline expires while _scsih_sas_broadcast_primitive_event()
> is still running, wouldn't _scsih_dnr_unblock_work() unblock the device in
> the middle of task management?
> 
> >  		do {
> >  			host_reset_completion_count = 0;
> >  			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
> > @@ -4225,6 +4249,92 @@ _scsih_block_io_all_device(struct MPT3SAS_ADAPTER *ioc)
> >  	}
> >  }
> >  
> > +/*
> > + * Margin on top of the firmware's report device missing delay before
> > + * _scsih_dnr_unblock_work() releases a delayed not-responding block.
> > + */
> > +#define MPT3SAS_DNR_BLOCK_SLACK	30
> > +
> > +static unsigned long
> > +_scsih_dnr_block_timeout(struct MPT3SAS_ADAPTER *ioc)
> > +{
> > +	return (max(ioc->device_missing_delay, ioc->pcie_device_missing_delay) +
> > +	    MPT3SAS_DNR_BLOCK_SLACK) * HZ;
> > +}
> > +
> > +/**
> > + * _scsih_dnr_unblock_sdev - release an expired delayed not-responding block
> > + * @ioc: per adapter object
> > + * @sdev: scsi device struct
> > + *
> > + * Return: the deadline of an unexpired block on @sdev, or 0 if none.
> > + */
> > +static unsigned long
> > +_scsih_dnr_unblock_sdev(struct MPT3SAS_ADAPTER *ioc, struct scsi_device *sdev)
> > +{
> > +	struct MPT3SAS_DEVICE *sas_device_priv_data;
> > +	unsigned long flags, deadline = 0;
> > +
> > +	spin_lock_irqsave(&ioc->block_lock, flags);
> > +	sas_device_priv_data = sdev->hostdata;
> > +	if (!sas_device_priv_data || !sas_device_priv_data->block ||
> > +	    !sas_device_priv_data->block_deadline)
> > +		goto out;
> > +	if (time_before(jiffies, sas_device_priv_data->block_deadline)) {
> > +		deadline = sas_device_priv_data->block_deadline;
> > +		goto out;
> > +	}
> > +	sdev_printk(KERN_WARNING, sdev,
> > +	    "delayed not-responding block expired, handle(0x%04x)\n",
> > +	    sas_device_priv_data->sas_target->handle);
> > +	_scsih_internal_device_unblock(sdev, sas_device_priv_data);
> > +out:
> > +	spin_unlock_irqrestore(&ioc->block_lock, flags);
> > +	return deadline;
> > +}
> 
> [Severity: High]
> Does _scsih_dnr_unblock_sdev() race with the other unblock paths that do not
> acquire ioc->block_lock?
> 
> While _scsih_dnr_unblock_sdev() runs on system_dfl_wq under ioc->block_lock,
> _scsih_ublock_io_device() (called from interrupt context in
> mpt3sas_scsih_event_callback() -> _scsih_tm_tr_send() on
> MPI2_EVENT_SAS_TOPO_RC_TARG_NOT_RESPONDING and from worker context in
> _scsih_check_device() and _scsih_remove_device()),
> _scsih_ublock_io_all_device(), and _scsih_ublock_io_device_wait() inspect and
> clear sas_device_priv_data->block and call _scsih_internal_device_unblock()
> or scsi_internal_device_unblock_nowait() without holding ioc->block_lock:
> 
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_ublock_io_device()
> 	shost_for_each_device(sdev, ioc->shost) {
> 		...
> 		if (sas_device_priv_data->block)
> 			_scsih_internal_device_unblock(sdev,
> 				sas_device_priv_data);
> 	}
> 
> If _scsih_dnr_unblock_sdev() and _scsih_ublock_io_device() (or
> _scsih_ublock_io_all_device()) both see sas_device_priv_data->block == 1 and
> call _scsih_internal_device_unblock() concurrently, the first call transitions
> sdev->sdev_state from SDEV_BLOCK to SDEV_RUNNING (or from SDEV_CREATED_BLOCK
> to SDEV_CREATED). The second caller's scsi_internal_device_unblock_nowait()
> then returns -EINVAL and enters the fallback path in
> _scsih_internal_device_unblock():
> 
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_internal_device_unblock()
> 	r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
> 	if (r == -EINVAL) {
> 		...
> 		sas_device_priv_data->block = 1;
> 		r = scsi_internal_device_block_nowait(sdev);
> 		...
> 		sas_device_priv_data->block = 0;
> 		r = scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
> 	}
> 
> That re-blocks the already-unblocked sdev, and if the sdev was in
> SDEV_CREATED during a scan, scsi_internal_device_block_nowait() puts it back
> into SDEV_CREATED_BLOCK where the subsequent unblock transitions it only to
> SDEV_CREATED, racing with scsi_sysfs_add_sdev().
> 
> In addition, when _scsih_ublock_io_device(), _scsih_ublock_io_all_device(), or
> _scsih_ublock_io_device_wait() clears sas_device_priv_data->block = 0 without
> holding ioc->block_lock before calling scsi_internal_device_unblock_nowait(),
> a concurrent DELAY_NOT_RESPONDING interrupt in _scsih_block_io_device() can
> take ioc->block_lock, observe block == 0, set block = 1 and block_deadline,
> and call scsi_internal_device_block_nowait(). The unblocking thread then runs
> scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING), leaving sdev in
> SDEV_RUNNING with its request queue started while sas_device_priv_data->block
> is still 1 (causing scsih_qcmd() to return SCSI_MLQUEUE_DEVICE_BUSY for all
> commands).
> 
> Should all unblock paths synchronize using ioc->block_lock?
> 


These are preexisting bugs, and the timeout should help mitigate them.
Putting everything under the block_lock properly is a larger rework
that I'd rather not take on at the moment.


> > +
> > +/**
> > + * _scsih_dnr_unblock_work - release expired delayed not-responding blocks
> > + * @work: the dnr_unblock_work of the adapter
> > + *
> > + * Within the device missing delay, the firmware either reports the device
> > + * as not responding, and the event callback unblocks it, or the device
> > + * comes back, and only the firmware event worker unblocks it.  The worker
> > + * may first need shost->scan_mutex or the SAS transport's host lock, and
> > + * a scan holding those can have a command queued on the blocked sdev.
> > + * Past the delay the block has no further use, so release it here.
> > + */
> > +static void
> > +_scsih_dnr_unblock_work(struct work_struct *work)
> > +{
> > +	struct MPT3SAS_ADAPTER *ioc = container_of(to_delayed_work(work),
> > +	    struct MPT3SAS_ADAPTER, dnr_unblock_work);
> > +	struct scsi_device *sdev;
> > +	unsigned long deadline, next = 0;
> > +
> > +	if (ioc->remove_host)
> > +		return;
> > +	if (ioc->shost_recovery || ioc->pci_error_recovery) {
> > +		queue_delayed_work(system_dfl_wq, &ioc->dnr_unblock_work, HZ);
> > +		return;
> > +	}
> 
> [Severity: Medium]
> Should _scsih_dnr_unblock_work() also check scsi_host_in_recovery(ioc->shost)
> and avoid unblocking targets marked sas_target->deleted, and use
> mod_delayed_work() here?
> 
> When a host reset finishes in mpt3sas_base_hard_reset_handler(),
> mpt3sas_scsih_reset_done_handler() marks unresponding targets with
> sas_target->deleted = 1 and queues MPT3SAS_REMOVE_UNRESPONDING_DEVICES, and
> ioc->shost_recovery is cleared to 0 while scsi_host_in_recovery(ioc->shost)
> can remain true until SCSI EH finishes. During SHOST_RECOVERY, scsih_qcmd()
> relies on sas_device_priv_data->block remaining set so TEST_UNIT_READY
> commands complete immediately with UNIT_ATTENTION / I_T_NEXUS_LOSS_OCCURRED,
> which is why _mpt3sas_fw_work() waits for both scsi_host_in_recovery() and
> ioc->shost_recovery before running _scsih_remove_unresponding_devices():
> 
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:_mpt3sas_fw_work()
> 	case MPT3SAS_REMOVE_UNRESPONDING_DEVICES:
> 		while (scsi_host_in_recovery(ioc->shost) ||
> 					 ioc->shost_recovery) {
> 			if (ioc->remove_host || ioc->fw_events_cleanup)
> 				goto out;
> 			ssleep(1);
> 		}
> 		_scsih_remove_unresponding_devices(ioc);
> 
> Because _scsih_dnr_unblock_work() only checks ioc->shost_recovery and
> _scsih_dnr_unblock_sdev() does not check sas_target->deleted (unlike
> _scsih_ublock_io_all_device()), a block deadline expiring after
> ioc->shost_recovery clears can unblock pre-reset or deleted devices while
> SHOST_RECOVERY or post-reset device removal is still pending.
> 
> Also, if _scsih_block_io_device() queues dnr_unblock_work with a full timeout
> at the same time _scsih_dnr_unblock_work() enters the recovery deferral
> branch, queue_delayed_work(..., HZ) will be a no-op because the work item is
> already pending, pushing the 1-second retry out to the full timeout unless
> mod_delayed_work() is used.
> 
> [1] https://lore.kernel.org/r/20261030-sas-abba-v1-1-b8cdd714a37c@kernel.org

I've added a scsi_host_in_recovery() to the deferral condition for v3,
and fixed up the mod_delayed_work() issue.

I declined to skip sas_target->deleted entries, as it could cause us to
hit the deadlock that this series fixes again. Unblocking a deleted
target is harmless anyway, so it's best to avoid doing that, I think.
-- 
Jeff Layton <jlayton@kernel.org>