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

[sashiko-bot@kernel.org]

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