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

[sashiko-bot@kernel.org]

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.

> @@ -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.

[ ... ]

> @@ -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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sas-abba-v3-0-571b854594aa@kernel.org?part=2