Re: [PATCH v2] scsi: libsas: add lun number check in .slave_alloc callback

John Garry <john.garry@xxxxxxxxxx> · Wed, 9 Jun 2021 10:40:45 +0100

On 09/06/2021 10:36, 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 devices 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:255]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdjb

After REPORT LUN command issued to the offline device fail, it tries
to do a sequential scan and probe all devices whose lun is not 0
successfully.

To fix the problem, we try to do same things as commit 2fc62e2ac350
("[SCSI] libsas: disable scanning lun > 0 on ata devices"), which
will prevent the device whose lun number is not zero probe into system.

Reported-by: Wu Bo <wubo40@xxxxxxxxxx>
Suggested-by: John Garry <john.garry@xxxxxxxxxx>
Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
---
  drivers/scsi/aic94xx/aic94xx_init.c    | 1 +
  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 1 +
  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 1 +
  drivers/scsi/isci/init.c               | 1 +
  drivers/scsi/libsas/sas_scsi_host.c    | 9 +++++++++
  drivers/scsi/mvsas/mv_init.c           | 1 +
  drivers/scsi/pm8001/pm8001_init.c      | 1 +

Do we also need to modify aix79xx in a similar fashion?

I just noticed that libsas.h already has a prototype for 
sas_slave_alloc() - any idea why?

  8 files changed, 16 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index a195bfe9eccc..7a78606598c4 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -53,6 +53,7 @@ static struct scsi_host_template aic94xx_sht = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler	= sas_eh_device_reset_handler,
  	.eh_target_reset_handler	= sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 3e359ac752fd..15eaac3a4eb6 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1771,6 +1771,7 @@ static struct scsi_host_template sht_v1_hw = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler = sas_eh_device_reset_handler,
  	.eh_target_reset_handler = sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 46f60fc2a069..9df1639ffa65 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3584,6 +3584,7 @@ static struct scsi_host_template sht_v2_hw = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler = sas_eh_device_reset_handler,
  	.eh_target_reset_handler = sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index e95408314078..36ec3901cfd4 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -3155,6 +3155,7 @@ static struct scsi_host_template sht_v3_hw = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler = sas_eh_device_reset_handler,
  	.eh_target_reset_handler = sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index c452849e7bb4..ffd33e5decae 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -167,6 +167,7 @@ static struct scsi_host_template isci_sht = {
  	.eh_abort_handler		= sas_eh_abort_handler,
  	.eh_device_reset_handler        = sas_eh_device_reset_handler,
  	.eh_target_reset_handler        = sas_eh_target_reset_handler,
+	.slave_alloc			= sas_slave_alloc,
  	.target_destroy			= sas_target_destroy,
  	.ioctl				= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 1bf939818c98..ee44a0d7730b 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -911,6 +911,14 @@ void sas_task_abort(struct sas_task *task)
  		blk_abort_request(sc->request);
  }
  
+int sas_slave_alloc(struct scsi_device *sdev)
+{
+	if (dev_is_sata(sdev_to_domain_dev(sdev)) && sdev->lun)
+		return -ENXIO;
+
+	return 0;
+}
+
  void sas_target_destroy(struct scsi_target *starget)
  {
  	struct domain_device *found_dev = starget->hostdata;
@@ -957,5 +965,6 @@ EXPORT_SYMBOL_GPL(sas_task_abort);
  EXPORT_SYMBOL_GPL(sas_phy_reset);
  EXPORT_SYMBOL_GPL(sas_eh_device_reset_handler);
  EXPORT_SYMBOL_GPL(sas_eh_target_reset_handler);
+EXPORT_SYMBOL_GPL(sas_slave_alloc);
  EXPORT_SYMBOL_GPL(sas_target_destroy);
  EXPORT_SYMBOL_GPL(sas_ioctl);

hmmm... apart from this patchset, it's standard to put the export beside 
the function. Maybe we can change that later.


diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 6aa2697c4a15..b03c0f35d7b0 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -46,6 +46,7 @@ static struct scsi_host_template mvs_sht = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler = sas_eh_device_reset_handler,
  	.eh_target_reset_handler = sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index af09bd282cb9..313248c7bab9 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -101,6 +101,7 @@ static struct scsi_host_template pm8001_sht = {
  	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
  	.eh_device_reset_handler = sas_eh_device_reset_handler,
  	.eh_target_reset_handler = sas_eh_target_reset_handler,
+	.slave_alloc		= sas_slave_alloc,
  	.target_destroy		= sas_target_destroy,
  	.ioctl			= sas_ioctl,
  #ifdef CONFIG_COMPAT








