Re: [PATCH] scsi: libsas: check lun number valid for ata device in sas_queuecommand()

[John Garry <john.garry@huawei.com>]

On 01/06/2021 08:04, Yufen Yu wrote:
> We found that offline a ata device on hisi sas control and then
> scanning the host can probe 255 not exist devices into system.
> It can be reproduced easily as following:
> 
> Some ata devices on hisi sas v3 control:
>    [root@localhost ~]# lsscsi
>    [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
>    [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
>    [2:0:2:0]    disk    SEAGATE  ST600MM0006      B001  /dev/sdc
> 
>   1) echo "offline" > /sys/block/sdb/device/state
>   2) echo "- - -" > /sys/class/scsi_host/host2/scan
> 
> Then, we can see another 255 not exist device in system:
>    [root@localhost ~]# lsscsi
>    [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
>    [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
>    [2:0:1:1]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdh
>    ...
>    [2:0:1:254]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdja
>    [2:0:1:255]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdjb
> 
> When we try to scan the host, REPORT LUN command issued to the
> offline device (sdb) will return fail. Then it tries to do a
> sequential scan. Since only one ata device, any INQUIRY command
> will be issued to the only device (i.e. lun 0) and return success,
> no matter the lun number. So, the device whose lun number is not
> zero will also be probed into system.
> 
> We fix the probe by checking lun number valid in sas_queuecommand.
> Any lun number is not equal '0' will return fail.
> 
> Signed-off-by: Wu Bo <wubo40@huawei.com>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>

As mentioned privately, we could alternatively add a check in a slave 
alloc callback, like:

int sas_slave_alloc(struct scsi_device *sdev)
{
	if (dev_is_sata(sdev_to_domain_dev(sdev)) && sdev->lun)
		return -ENXIO;

	return 0;
}

This avoids touching the queuecommand fastpath. But not sure it is worth it.

BTW, please mention that we are doing similar to commit 2fc62e2ac, to 
cut down the commit log a bit.

> ---
>   drivers/scsi/libsas/sas_scsi_host.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index 1bf939818c98..62a01d11df96 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -174,6 +174,12 @@ int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   	}
>   
>   	if (dev_is_sata(dev)) {
> +		/* sas ata just have one lun */
> +		if (cmd->device->lun != 0) {
> +			cmd->result = (DID_BAD_TARGET << 16);
> +			cmd->scsi_done(cmd);
> +			return res;

goto out_done is better

> +		}
>   		spin_lock_irq(dev->sata_dev.ap->lock);
>   		res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
>   		spin_unlock_irq(dev->sata_dev.ap->lock);
>