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

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2026-11-06 at 20:57 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] scsi: mpt3sas: use-after-free of sdev->hostdata in unblock
>   helpers
> - [Medium] scsi: mpt3sas: race setting SDEV_OFFLINE outside block_lock
>   on unblock failure
> - [Medium] scsi: mpt3sas: _scsih_block_io_all_device() leaves sdevs
>   blocked without deadline
> --
> 
> commit ba9687a8025a542963a3bc4d91c8e8b094a21e74
> Author: Jeff Layton <jlayton@kernel.org>
> 
> scsi: mpt3sas: Time out delayed not-responding device blocks
> 
> This commit adds a delayed work item and per-device deadline to release
> delayed not-responding sdev blocks after the firmware's device missing
> delay plus 30 seconds, preventing scans from deadlocking on sas_host->lock.
> 
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > index 60c9cf679a8a7..acdeece20140f 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> 
> [ ... ]
> 
> > @@ -3958,7 +3989,20 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
> >  	int count = 0;
> >  	u8 tr_method = 0;
> >  	u8 tr_timeout = 30;
> > +	unsigned long flags;
> >  
> > +	/* polling one device can outlast another's deadline */
> > +	if (!no_turs && issue_scsi_cmd_to_bringup_drive) {
> > +		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 &&
> > +			    sas_device_priv_data->sas_target &&
> > +			    !sas_device_priv_data->sas_target->deleted)
> > +				sas_device_priv_data->block_deadline = 0;
> > +			spin_unlock_irqrestore(&ioc->block_lock, flags);
> > +		}
> > +	}
> >  
> >  	shost_for_each_device(sdev, ioc->shost) {
> >  		sas_device_priv_data = sdev->hostdata;
> > @@ -3975,10 +4019,18 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
> >  		if ((no_turs) || (!issue_scsi_cmd_to_bringup_drive)) {
> >  			sdev_printk(KERN_WARNING, sdev, "device_unblocked handle(0x%04x)\n",
> >  				sas_device_priv_data->sas_target->handle);
> > -			_scsih_internal_device_unblock(sdev, sas_device_priv_data);
> > +			spin_lock_irqsave(&ioc->block_lock, flags);
> > +			/* dnr_unblock_work may have released it */
> > +			if (sas_device_priv_data->block)
> > +				_scsih_internal_device_unblock(sdev,
> > +				    sas_device_priv_data);
> > +			spin_unlock_irqrestore(&ioc->block_lock, flags);
> >  			continue;
> >  		}
> >  
> > +		/* may have been blocked since the first pass */
> > +		if (!_scsih_claim_block(ioc, sdev))
> > +			continue;
> >  		do {
> >  			pcie_device = mpt3sas_get_pdev_by_handle(ioc, sas_target->handle);
> >  			if (pcie_device && (!ioc->tm_custom_handling) &&
> > @@ -3997,11 +4049,13 @@ _scsih_ublock_io_all_device(struct MPT3SAS_ADAPTER *ioc, u8 no_turs)
> >  		} while ((rc == DEVICE_RETRY || rc == DEVICE_START_UNIT ||
> >  		    rc == DEVICE_STOP_UNIT || rc == DEVICE_RETRY_UA)
> >  			&& count++ < command_retry_count);
> > +		spin_lock_irqsave(&ioc->block_lock, flags);
> >  		sas_device_priv_data->block = 0;
> >  		if (rc != DEVICE_READY)
> >  			sas_device_priv_data->deleted = 1;
> >  
> >  		_scsih_internal_device_unblock(sdev, sas_device_priv_data);
> > +		spin_unlock_irqrestore(&ioc->block_lock, flags);
> >  
> >  		if (rc != DEVICE_READY) {
> >  			sdev_printk(KERN_WARNING, sdev, "%s: device_offlined,\n"
> 
> [Severity: High]
> Can _scsih_ublock_io_all_device() and _scsih_ublock_io_device_wait() cause a
> use-after-free on sas_device_priv_data and sas_target?
> 
> shost_for_each_device() only holds a reference on sdev, not sdev->hostdata.
> When _scsih_dnr_unblock_work() unblocks an sdev that was blocked during a
> concurrent sysfs scan and its probe fails in scsi_probe_and_add_lun(), or
> when a device is removed via sysfs, __scsi_remove_device() calls
> scsih_sdev_destroy(), which clears sdev->hostdata under ioc->block_lock and
> frees sas_device_priv_data:
> 
> CPU0 (firmware_event_thread)      CPU1 (scan / device removal)
> _scsih_ublock_io_all_device()
>   sas_device_priv_data =
>     sdev->hostdata;
>   _scsih_claim_block(ioc, sdev)
>   _scsih_wait_for_device_...()
>                                   __scsi_remove_device(sdev)
>                                     scsih_sdev_destroy(sdev)
>                                       spin_lock_irqsave(&ioc->block_lock)
>                                       sdev->hostdata = NULL;
>                                       spin_unlock_irqrestore(&ioc->block_lock)
>                                       kfree(sas_device_priv_data);
>   spin_lock_irqsave(&ioc->block_lock)
>   sas_device_priv_data->block = 0;  <-- use-after-free
> 
> Both functions also read sdev->hostdata and dereference
> sas_device_priv_data->sas_target at the top of their second
> shost_for_each_device() loops without holding ioc->block_lock, and in the
> no_turs branch _scsih_ublock_io_all_device() takes ioc->block_lock and
> accesses the cached sas_device_priv_data pointer without re-checking
> sdev->hostdata.  Re-fetching sdev->hostdata (and sas_target) under
> ioc->block_lock before each access and after the polling loop would avoid
> touching freed hostdata.
> 

Real issue. Still, worth fixing while we're in here.

> > @@ -4060,24 +4114,30 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
> >  	u8 tr_timeout = 30;
> >  	u8 tr_method = 0;
> >  	int count = 0;
> > +	unsigned long flags;
> >  
> >  	/* moving devices from SDEV_OFFLINE to SDEV_BLOCK */
> >  	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;
> >  		sas_target = sas_device_priv_data->sas_target;
> >  		if (!sas_target)
> > -			continue;
> > +			goto next;
> >  		if (sas_target->sas_address != sas_address ||
> >  		    sas_target->port != port)
> > -			continue;
> > +			goto next;
> > +		/* polling one device can outlast another's deadline */
> > +		sas_device_priv_data->block_deadline = 0;
> >  		if (sdev->sdev_state == SDEV_OFFLINE) {
> >  			sas_device_priv_data->block = 1;
> >  			sas_device_priv_data->deleted = 0;
> >  			scsi_device_set_state(sdev, SDEV_RUNNING);
> >  			scsi_internal_device_block_nowait(sdev);
> >  		}
> > +next:
> > +		spin_unlock_irqrestore(&ioc->block_lock, flags);
> >  	}
> >  
> >  	/* moving devices from SDEV_BLOCK to SDEV_RUNNING state */
> > @@ -4091,7 +4151,7 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
> >  		if (sas_target->sas_address != sas_address ||
> >  		    sas_target->port != port)
> >  			continue;
> > -		if (!sas_device_priv_data->block)
> > +		if (!_scsih_claim_block(ioc, sdev))
> >  			continue;
> >  
> >  		do {
> > @@ -4129,10 +4189,12 @@ _scsih_ublock_io_device_wait(struct MPT3SAS_ADAPTER *ioc, u64 sas_address,
> >  		    rc == DEVICE_STOP_UNIT || rc == DEVICE_RETRY_UA)
> >  			&& count++ <= command_retry_count);
> >  
> > +		spin_lock_irqsave(&ioc->block_lock, flags);
> >  		sas_device_priv_data->block = 0;
> >  		if (rc != DEVICE_READY)
> >  			sas_device_priv_data->deleted = 1;
> >  		scsi_internal_device_unblock_nowait(sdev, SDEV_RUNNING);
> > +		spin_unlock_irqrestore(&ioc->block_lock, flags);
> >  
> >  		if (rc != DEVICE_READY) {
> >  			sdev_printk(KERN_WARNING, sdev,
> 
> [Severity: Medium]
> When rc != DEVICE_READY in _scsih_ublock_io_device_wait() (and in
> _scsih_ublock_io_all_device()), can dropping ioc->block_lock before calling
> scsi_device_set_state(sdev, SDEV_OFFLINE) race with _scsih_block_io_device()?
> 
> CPU0 (firmware_event_thread)      CPU1 (<hardirq>)
> _scsih_ublock_io_device_wait()
>   spin_lock_irqsave(&ioc->block_lock)
>   sas_device_priv_data->block = 0;
>   scsi_internal_device_unblock_nowait(
>     sdev, SDEV_RUNNING)
>   spin_unlock_irqrestore(&ioc->block_lock)
>                                   _scsih_block_io_device()
>                                     spin_lock_irqsave(&ioc->block_lock)
>                                     /* sdev_state is SDEV_RUNNING */
>                                     _scsih_internal_device_block(sdev, ...)
>                                     sas_device_priv_data->block_deadline = ...;
>                                     spin_unlock_irqrestore(&ioc->block_lock)
>   ...
>   scsi_device_set_state(sdev,
>     SDEV_OFFLINE)  <-- overwrites SDEV_BLOCK; queue stays stopped
> 
> Because sdev->sdev_state is SDEV_RUNNING while ioc->block_lock is dropped,
> _scsih_block_io_device() does not see SDEV_OFFLINE and stops the request
> queue, sets block = 1, and arms block_deadline.  CPU0 then sets
> sdev->sdev_state to SDEV_OFFLINE with the queue still stopped (so
> scsi_queue_rq() cannot fail queued I/O with BLK_STS_IOERR) and an active
> block_deadline that will later transition the offline device back to
> SDEV_RUNNING without a TUR check.  Setting SDEV_OFFLINE before releasing
> ioc->block_lock would close this window.
> 
> [ ... ]
> 

There's a real race window here, but I think the claimed result is
wrong. Still, this is simple to fix.

> > @@ -4213,21 +4279,119 @@ _scsih_block_io_all_device(struct MPT3SAS_ADAPTER *ioc)
> >  {
> >  	struct MPT3SAS_DEVICE *sas_device_priv_data;
> >  	struct scsi_device *sdev;
> > +	unsigned long flags;
> >  
> >  	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;
> > -		if (sas_device_priv_data->block)
> > -			continue;
> > +			goto next;
> > +		if (sas_device_priv_data->block) {
> > +			/* the caller unblocks it; dnr_unblock_work must not */
> > +			sas_device_priv_data->block_deadline = 0;
> > +			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);
> >  	}
> > +}
> 
> [Severity: Medium]
> Can _scsih_block_io_all_device() leave devices blocked with
> block_deadline == 0 so that neither the caller nor _scsih_dnr_unblock_work()
> unblocks them before device removal?
> 
> _scsih_block_io_all_device() blocks every sdev (or clears its
> block_deadline) without checking sas_target->deleted, whereas its paired
> _scsih_ublock_io_all_device() skips targets with sas_target->deleted set.
> In addition, its caller _scsih_sas_broadcast_primitive_event() skips
> _scsih_ublock_io_all_device() altogether when host recovery starts:
> 
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_sas_broadcast_primitive_event() {
>     ...
> 	ioc->broadcast_aen_busy = 0;
> 	if (!ioc->shost_recovery)
> 		_scsih_ublock_io_all_device(ioc, 1);
> 	mutex_unlock(&ioc->tm_cmds.mutex);
> }
> 
> After host reset, _scsih_remove_unresponding_devices() runs on
> firmware_event_thread and removes unresponding devices (taking
> sas_host->lock and shost->scan_mutex) before it reaches
> _scsih_ublock_io_all_device(ioc, 0).  If a concurrent sysfs scan holds
> sas_host->lock and waits on I/O to one of these sdevs whose block_deadline
> was cleared to 0, _scsih_dnr_unblock_work() will not unblock it and the scan
> deadlock recurs.

Real issue, but this predates the set AFAICT. Still it's also worth
fixing.

There's also another issue here, I think where repeated DNRs can extend
the block indefinitely and still cause a hang. We should just bound the
timeout instead.

I'll send a v4.
-- 
Jeff Layton <jlayton@kernel.org>