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@xxxxxxxxxx>
>> Fixes: 41bda9c98035 ("libata-link: update hotplug to handle PMP links")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
>> ---
>>
>> 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;
>> }
>>
>>
--
Damien Le Moal
Western Digital Research
