ata: libata-scsi: Use correct device no in ata_find_dev()

PATCH Superseded Damien Le Moal 2 participants 6 messages May 23, 2023 – May 24, 2023 libata

Share & tools

Damien Le Moal

author maintainer

2023-05-23 02:32 UTC #0 lore raw

PATCH v2 1/1 ata: libata-scsi: Use correct device no in ata_find_dev()

For devices not attached to a port multiplier and managed directly by libata, the device number passed to ata_find_dev() must always be lower than the maximum number of devices returned by ata_link_max_devices(). That is 1 for SATA devices or 2 for an IDE link with master+slave devices. This device number is the scsi device ID which matches these constraint as the ID are generated per port and so never exceed the link maximum.

However, for libsas managed devices, scsi device IDs are assigned per scsi host, leading to device IDs for SATA devices that can be well in excess of libata per-link maximum number of devices. This results in ata_find_dev() always returning NULL for libsas managed devices except for the first device of the host with ID (device number) 0. This issue is visible by executing hdparm command, which fails:

hdparm -i /dev/sdX
/dev/sdX:
HDIO_GET_IDENTITY failed: No message of desired type

Fix this by rewriting ata_find_dev() to ignore the device number for non-pmp attached devices with a link with at most 1 device, that is SATA devices on SATA ports. For these, device number 0 is always used to return the correct ata_device struct of the port link. This change excludes IDE master/slave setups (maximum number of devices per link is 2) and port-multiplier attached devices.

Reported-by Xingui Yang <[email protected]>

Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---

Changes from v1:
* Simplify code change (remove uneeded check and remove switch-case)
* Reword and improve comments in ata_find_dev()
* Reword commit message

 drivers/ata/libata-scsi.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

drivers/ata/libata-scsi.c +25 -7

index 7bb12deab70c..d936506a8af9 100644
@@ -2696,16 +2696,34 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
 static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
 {
-	if (!sata_pmp_attached(ap)) {
-		if (likely(devno >= 0 &&
-			   devno < ata_link_max_devices(&ap->link)))
+	/*
+	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
+	 * or 2 (IDE master + slave). However, the former case includes
+	 * libsas hosted devices which are numbered per host, leading
+	 * to devno potentially being larger than 0 but with each ata device
+	 * having its own ata port and ata link. To accommodate these, ignore
+	 * devno and always use device number 0.
+	 */
+	if (likely(!sata_pmp_attached(ap))) {
+		int link_max_devices = ata_link_max_devices(&ap->link);
+
+		if (link_max_devices == 1)
+			return &ap->link.device[0];
+
+		if (devno < link_max_devices)
 			return &ap->link.device[devno];
-	} else {
-		if (likely(devno >= 0 &&
-			   devno < ap->nr_pmp_links))
-			return &ap->pmp_link[devno].device[0];
+
+		return NULL;
 	}
 
+	/*
+	 * For PMP-attached devices, the device number corresponds to C
+	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link
+	 * for the device.
+	 */
+	if (devno < ap->nr_pmp_links)
+		return &ap->pmp_link[devno].device[0];
+
 	return NULL;
 }

Signature · 1 line

2.40.1

Jason Yan

2023-05-23 07:05 UTC #1 lore raw

Quoted 57 lines · Damien Le Moal

For devices not attached to a port multiplier and managed directly by libata, the device number passed to ata_find_dev() must always be lower than the maximum number of devices returned by ata_link_max_devices(). That is 1 for SATA devices or 2 for an IDE link with master+slave devices. This device number is the scsi device ID which matches these constraint as the ID are generated per port and so never exceed the link maximum.
However, for libsas managed devices, scsi device IDs are assigned per scsi host, leading to device IDs for SATA devices that can be well in excess of libata per-link maximum number of devices. This results in ata_find_dev() always returning NULL for libsas managed devices except for the first device of the host with ID (device number) 0. This issue is visible by executing hdparm command, which fails:
hdparm -i /dev/sdX
/dev/sdX:
HDIO_GET_IDENTITY failed: No message of desired type
Fix this by rewriting ata_find_dev() to ignore the device number for non-pmp attached devices with a link with at most 1 device, that is SATA devices on SATA ports. For these, device number 0 is always used to return the correct ata_device struct of the port link. This change excludes IDE master/slave setups (maximum number of devices per link is 2) and port-multiplier attached devices.
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
Changes from v1:
* Simplify code change (remove uneeded check and remove switch-case)
* Reword and improve comments in ata_find_dev()
* Reword commit message
  drivers/ata/libata-scsi.c | 32 +++++++++++++++++++++++++-------
  1 file changed, 25 insertions(+), 7 deletions(-)
drivers/ata/libata-scsi.c +6 -3
index 7bb12deab70c..d936506a8af9 100644
@@ -2696,16 +2696,34 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
  static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
  {
-	if (!sata_pmp_attached(ap)) {
-		if (likely(devno >= 0 &&
-			   devno < ata_link_max_devices(&ap->link)))
+	/*
+	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
+	 * or 2 (IDE master + slave). However, the former case includes
+	 * libsas hosted devices which are numbered per host, leading
+	 * to devno potentially being larger than 0 but with each ata device
+	 * having its own ata port and ata link. To accommodate these, ignore

drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2714

Quoted 10 lines

drivers/ata/libata-scsi.c +9 -0

+	 * devno and always use device number 0.
+	 */
+	if (likely(!sata_pmp_attached(ap))) {
+		int link_max_devices = ata_link_max_devices(&ap->link);
+
+		if (link_max_devices == 1)
+			return &ap->link.device[0];
+
+		if (devno < link_max_devices)
  			return &ap->link.device[devno];

I wonder if you can change the type of devno to 'unsigned int'? At a closer look I found the user can control this value and may pass in a bogus channel or id.

proc_scsi_write

    =>scsi_add_single_device
        =>ata_scsi_user_scan
            =>ata_find_dev

Even though the channel or id is unsigned int, It still can be turned into a negative value when assigned to an 'int'.

Thanks,
Jason

Quoted 20 lines

drivers/ata/libata-scsi.c +10 -4

-	} else {
-		if (likely(devno >= 0 &&
-			   devno < ap->nr_pmp_links))
-			return &ap->pmp_link[devno].device[0];
+
+		return NULL;
  	}
 
+	/*
+	 * For PMP-attached devices, the device number corresponds to C
+	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link
+	 * for the device.
+	 */
+	if (devno < ap->nr_pmp_links)
+		return &ap->pmp_link[devno].device[0];
+
  	return NULL;
  }

Damien Le Moal

author maintainer

2023-05-23 11:52 UTC #2 lore raw

Quoted 73 lines · Jason Yan

Quoted 57 lines · Damien Le Moal
For devices not attached to a port multiplier and managed directly by libata, the device number passed to ata_find_dev() must always be lower than the maximum number of devices returned by ata_link_max_devices(). That is 1 for SATA devices or 2 for an IDE link with master+slave devices. This device number is the scsi device ID which matches these constraint as the ID are generated per port and so never exceed the link maximum.
However, for libsas managed devices, scsi device IDs are assigned per scsi host, leading to device IDs for SATA devices that can be well in excess of libata per-link maximum number of devices. This results in ata_find_dev() always returning NULL for libsas managed devices except for the first device of the host with ID (device number) 0. This issue is visible by executing hdparm command, which fails:
hdparm -i /dev/sdX
/dev/sdX:
HDIO_GET_IDENTITY failed: No message of desired type
Fix this by rewriting ata_find_dev() to ignore the device number for non-pmp attached devices with a link with at most 1 device, that is SATA devices on SATA ports. For these, device number 0 is always used to return the correct ata_device struct of the port link. This change excludes IDE master/slave setups (maximum number of devices per link is 2) and port-multiplier attached devices.
Reported-by Xingui Yang <[email protected]>
Fixes 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
Changes from v1:
* Simplify code change (remove uneeded check and remove switch-case)
* Reword and improve comments in ata_find_dev()
* Reword commit message
  drivers/ata/libata-scsi.c | 32 +++++++++++++++++++++++++-------
  1 file changed, 25 insertions(+), 7 deletions(-)
drivers/ata/libata-scsi.c +6 -3
index 7bb12deab70c..d936506a8af9 100644
@@ -2696,16 +2696,34 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
  static struct ata_device *ata_find_dev(struct ata_port *ap, int devno)
  {
-	if (!sata_pmp_attached(ap)) {
-		if (likely(devno >= 0 &&
-			   devno < ata_link_max_devices(&ap->link)))
+	/*
+	 * For the non PMP case, link_max_devices is 1 (e.g. SATA case),
+	 * or 2 (IDE master + slave). However, the former case includes
+	 * libsas hosted devices which are numbered per host, leading
+	 * to devno potentially being larger than 0 but with each ata device
+	 * having its own ata port and ata link. To accommodate these, ignore
drivers/ata/libata-scsi.c static unsigned int atapi_xlat(struct ata_queued_cmd *qc) · line 2714
Quoted 10 lines
drivers/ata/libata-scsi.c +9 -0
+	 * devno and always use device number 0.
+	 */
+	if (likely(!sata_pmp_attached(ap))) {
+		int link_max_devices = ata_link_max_devices(&ap->link);
+
+		if (link_max_devices == 1)
+			return &ap->link.device[0];
+
+		if (devno < link_max_devices)
  			return &ap->link.device[devno];
I wonder if you can change the type of devno to 'unsigned int'? At a closer look I found the user can control this value and may pass in a bogus channel or id.

Quoted 4 lines

proc_scsi_write

     =>scsi_add_single_device
         =>ata_scsi_user_scan
             =>ata_find_dev

Reading more about scsi_add_single_device(), the comment says "Note: this seems to be aimed exclusively at SCSI parallel busses.". So I don't think we should worry about it. But then I also do not understand why libata is wired to this at all. Cannot have ATA device on a parallel SCSI bus...

On my system, I cannot get

echo "scsi add-single-device X 0 100 0" >/proc/scsi/scsi

to do anything and so I do not see how ata_scsi_user_scan can ever be called...

Quoted 27 lines

Even though the channel or id is unsigned int, It still can be turned into a negative value when assigned to an 'int'.

Thanks,
Jason

Quoted 20 lines

drivers/ata/libata-scsi.c +10 -4

-	} else {
-		if (likely(devno >= 0 &&
-			   devno < ap->nr_pmp_links))
-			return &ap->pmp_link[devno].device[0];
+
+		return NULL;
  	}
 
+	/*
+	 * For PMP-attached devices, the device number corresponds to C
+	 * (channel) of SCSI [H:C:I:L], indicating the port pmp link
+	 * for the device.
+	 */
+	if (devno < ap->nr_pmp_links)
+		return &ap->pmp_link[devno].device[0];
+
  	return NULL;
  }

Signature · 2 lines

Damien Le Moal
Western Digital Research

Jason Yan

2023-05-23 12:30 UTC #3 lore raw

Quoted 4 lines · Damien Le Moal

Quoted 4 lines
I wonder if you can change the type of devno to 'unsigned int'? At a closer look I found the user can control this value and may pass in a bogus channel or id.

Quoted 8 lines

Quoted 4 lines
proc_scsi_write
      =>scsi_add_single_device
          =>ata_scsi_user_scan
              =>ata_find_dev
Reading more about scsi_add_single_device(), the comment says "Note: this seems to be aimed exclusively at SCSI parallel busses.". So I don't think we should worry about it. But then I also do not understand why libata is wired to this at all. Cannot have ATA device on a parallel SCSI bus...

The comment is kind of obsolete. It is not limited to SCSI parallel busses only.

Quoted 7 lines

On my system, I cannot get
echo "scsi add-single-device X 0 100 0" >/proc/scsi/scsi
to do anything and so I do not see how ata_scsi_user_scan can ever be called...

Did you enabled CONFIG_SCSI_PROC_FS ? I started a qemu and it still works.

[root@localhost ~]# cat /proc/scsi/scsi
Attached devices:
Host: scsi1 Channel: 00 Id: 00 Lun: 00
Vendor: QEMU Model: QEMU DVD-ROM Rev: 2.5+
Type: CD-ROM ANSI SCSI revision: 05
[root@localhost ~]# lsscsi
[1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.5+ /dev/sr0
[root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi remove-single-device 1 0 0 0" > /proc/scsi/scsi

[  639.747836] ata2.00: disable device

[root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi add-single-device 1 0 0 0" > /proc/scsi/scsi [root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# lsscsi
[1:0:0:0] cd/dvd QEMU QEMU DVD-ROM 2.5+ /dev/sr0
[root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi add-single-device 1 0 100 0" > /proc/scsi/scsi -bash: echo: write error: Invalid argument
[root@localhost ~]#
[root@localhost ~]#

For a wrong scsi nubmer "1 0 100 0", it returns an error now. If our patch is applied, it will return ok and will add "1 0 0 0" instead, I guess.

Thanks,
Jason

Jason Yan

2023-05-23 12:41 UTC #4 lore raw

Quoted 17 lines · Jason Yan

Quoted 4 lines · Damien Le Moal
Quoted 4 lines
I wonder if you can change the type of devno to 'unsigned int'? At a closer look I found the user can control this value and may pass in a bogus channel or id.
Quoted 8 lines
Quoted 4 lines
proc_scsi_write
      =>scsi_add_single_device
          =>ata_scsi_user_scan
              =>ata_find_dev
Reading more about scsi_add_single_device(), the comment says "Note: this seems to be aimed exclusively at SCSI parallel busses.". So I don't think we should worry about it. But then I also do not understand why libata is wired to this at all. Cannot have ATA device on a parallel SCSI bus...
The comment is kind of obsolete. It is not limited to SCSI parallel busses only.
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
On my system, I cannot get
echo "scsi add-single-device X 0 100 0" >/proc/scsi/scsi
to do anything and so I do not see how ata_scsi_user_scan can ever be called...
Did you enabled CONFIG_SCSI_PROC_FS ? I started a qemu and it still works.

Forgot to say that if you do not want to enable this config, you can still do this instead:

echo "0 0 0" > /sys/class/scsi_host/host1/scan

Damien Le Moal

author maintainer

2023-05-24 01:31 UTC #5 lore raw

In reply to Jason Yan

Quoted 59 lines · Jason Yan

Quoted 8 lines · Damien Le Moal
Quoted 8 lines
I wonder if you can change the type of devno to 'unsigned int'? At a closer look I found the user can control this value and may pass in a bogus channel or id.
proc_scsi_write
      =>scsi_add_single_device
          =>ata_scsi_user_scan
              =>ata_find_dev
Quoted 7 lines
Reading more about scsi_add_single_device(), the comment says "Note:
this seems
to be aimed exclusively at SCSI parallel busses.". So I don't think we should
worry about it. But then I also do not understand why libata is wired to this at
all. Cannot have ATA device on a parallel SCSI bus...
The comment is kind of obsolete. It is not limited to SCSI parallel busses only.
Quoted 8 lines
On my system, I cannot get
echo "scsi add-single-device X 0 100 0" >/proc/scsi/scsi
to do anything and so I do not see how ata_scsi_user_scan can ever be called...
Did you enabled CONFIG_SCSI_PROC_FS ? I started a qemu and it still works.
[root@localhost ~]# cat /proc/scsi/scsi
Attached devices:
Host: scsi1 Channel: 00 Id: 00 Lun: 00
Vendor: QEMU     Model: QEMU DVD-ROM     Rev: 2.5+
Type:   CD-ROM                           ANSI SCSI revision: 05 [root@localhost ~]# lsscsi
[1:0:0:0]    cd/dvd QEMU     QEMU DVD-ROM     2.5+ /dev/sr0 [root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi remove-single-device 1 0 0 0" > /proc/scsi/scsi
[ 639.747836] ata2.00: disable device
[root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi add-single-device 1 0 0 0" > /proc/scsi/scsi [root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# lsscsi
[1:0:0:0]    cd/dvd QEMU     QEMU DVD-ROM     2.5+ /dev/sr0 [root@localhost ~]#
[root@localhost ~]#
[root@localhost ~]# echo "scsi add-single-device 1 0 100 0" > /proc/scsi/scsi
-bash: echo: write error: Invalid argument
[root@localhost ~]#
[root@localhost ~]#

Quoted 3 lines

For a wrong scsi nubmer "1 0 100 0", it returns an error now. If our patch is applied, it will return ok and will add "1 0 0 0" instead, I guess.

Indeed. Using ata_find_dev() in ata_scsi_user_scan() as it is is not broken. And ata_scsi_user_scan() is also likely broken in subtle ways for libsas due to the ata port being determined from the scsi host, which does not seem to be how libsas manages things. Need to dig further.

Quoted 4 lines

Thanks,
Jason

Signature · 2 lines

Damien Le Moal
Western Digital Research