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

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

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;
>>   }
>>   
>>

-- 
Damien Le Moal
Western Digital Research