linux-kernel - Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

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]

Message-ID: <e25962a1-cd0c-47c9-9e10-008c475f22cb@kernel.org>
Date: Fri, 21 Jun 2024 09:05:33 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Niklas Cassel <cassel@...nel.org>, Igor Pylypiv <ipylypiv@...gle.com>
Cc: Tejun Heo <tj@...nel.org>, Hannes Reinecke <hare@...e.de>,
 linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when
 ATA ERR/DF are set

On 6/20/24 21:55, Niklas Cassel wrote:
>> Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
>> since the flag is always set by ata_scsi_pass_thru() as you pointed out.
>> Do we still want to add the check even though we know that it is always
>> set by ata_scsi_pass_thru()?
>>
>> If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
>> flag instead? Currently it is used for ahci only but looks like it can be
>> expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
>> because it is not always setting the status/error values:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586
>>
>> For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
>> because fill_result_tf() is being called for failed commands regardless of
>> the ATA_QCFLAG_RESULT_TF flag:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873
>>
>> In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
>> fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.
>>
>> With that said I'm not sure if it makes sense to update all of the ATA
>> error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.
>>
>> What are your thoughts on this?
> 
> I see your point, we will fill the result if there is an error,
> even if ATA_QCFLAG_RESULT_TF wasn't set.
> 
> Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
> after it has called ap->ops->qc_fill_rtf(qc);

Yes, let's do that.

> Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.

And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set
the result tf for all commands. I fail to see why this is conditional to that flag.


-- 
Damien Le Moal
Western Digital Research