linux-kernel - Re: [PATCH v2] ata: libata-scsi: Fix get identity data failed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 May 2023 18:44:13 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     Jason Yan <yanaijie@...wei.com>,
        yangxingui <yangxingui@...wei.com>, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, john.g.garry@...cle.com
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com, prime.zeng@...ilicon.com,
        kangfenglong@...wei.com
Subject: Re: [PATCH v2] ata: libata-scsi: Fix get identity data failed

On 5/22/23 17:00, Jason Yan wrote:
>> OK, so the issue is that __ata_scsi_find_dev() calls ata_find_dev() with devno
>> == scsidev->id. This leads to devno being 0, 1, 2 and 3 for connected drives
>> sdd, sd1, sdf and sdg, as shown by lsscsi. However, each drive has its own
>> port+link, with the link for each one having  ata_link_max_devices() == 1, so
>> ata_find_dev() works only for the first drive with scsidev->id == 0 and fails
>> for the others. A naive fix would be this:
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 7bb12deab70c..e4d6f17d7ccc 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2718,7 +2718,7 @@ static struct ata_device *__ata_scsi_find_dev(struct
>> ata_port *ap,
>>          if (!sata_pmp_attached(ap)) {
>>                  if (unlikely(scsidev->channel || scsidev->lun))
>>                          return NULL;
>> -               devno = scsidev->id;
>> +               devno = 0;
>>          } else {
>>                  if (unlikely(scsidev->id || scsidev->lun))
>>                          return NULL;
>>
>> And running this on my setup, it works. This makes libsas added ports/devices
>> look like AHCI ones, where all devices have ID 0 for the !pmp case.
>>
>> However, I am not sure this would be OK for all setups...
>>
>> John,
>>
>> Any idea if there is any cases where libsas managed drives would endup not being
>> correctly identified by this change ? As long as a device always has its own
>> port, I do not see any issue. But is there a case where we could have multiple
>> devices on the same port ? Per libata, max is 2, and that is only for the IDE
>> master/slave case. Otherwise, it is always 1.
>>
> 
> AFAIK, libsas does not support multiple devices on the same port. So 
> this change is ok for libsas.

Yes, for libsas it is OK. But as is, it will break master+slave IDE setups... So
the fix needs to be finer than this.

> 
>> Not that looking at the pmp case, I am not confident at all that the
>> identification is correct for libsas. But I do not think that anyone would ever
>> connect a pmp box to a libsas HBA...
>>
> 
> libsas's does not support pmp either, and I do not see any future plans 
> to support pmp.

Good. Dealing with that one is always painful.

> So the above change (needs a ATA_FLAG_SAS_HOST check) looks good to me.

Yes, this flag check is needed to avoid breaking IDE/pata.

> It's better to make libsas behave as other ata drivers so that we can 
> drop the ATA_FLAG_SAS_HOST check. But this need tons of work for libsas.

Yes, getting rid of this special casing with this flag would be really nice. It
should not be needed. I will try to write a proper fix not using it for now, to
facilitate removing the flag later.


-- 
Damien Le Moal
Western Digital Research