| Message ID | 20230523023219.291253-1-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ata: libata-scsi: Use correct device no in ata_find_dev() | expand |
On 2023/5/23 10:32, Damien Le Moal wrote: > 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 <yangxingui@huawei.com> > Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > > 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(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 7bb12deab70c..d936506a8af9 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -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]; 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 > - } 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; > } > >
On 5/23/23 16:05, Jason Yan wrote: > On 2023/5/23 10:32, Damien Le Moal wrote: >> 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 <yangxingui@huawei.com> >> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> >> 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(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 7bb12deab70c..d936506a8af9 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -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]; > > 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 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... > > 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 > >> - } 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; >> } >> >>
On 2023/5/23 19:52, Damien Le Moal wrote: >> 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 > 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. > > 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
On 2023/5/23 20:29, Jason Yan wrote: > On 2023/5/23 19:52, Damien Le Moal wrote: >>> 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 >> 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. > >> >> 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
On 5/23/23 21:29, Jason Yan wrote: > On 2023/5/23 19:52, Damien Le Moal wrote: >>> 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 >> 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. > >> >> 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. 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. > > > Thanks, > Jason
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 7bb12deab70c..d936506a8af9 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -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; }
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 <yangxingui@huawei.com> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- 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(-)