Re: [PATCH v2] ata: libata-scsi: Use correct device no in ata_find_dev()

[Damien Le Moal <dlemoal@kernel.org>]

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

-- 
Damien Le Moal
Western Digital Research