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] [day] [month] [year] [list]
Message-ID: <d3218307-51fd-4731-b2aa-06ce4735d90c@kernel.org>
Date: Tue, 8 Apr 2025 08:52:44 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>, Niklas Cassel <cassel@...nel.org>
Cc: linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ata: libata-scsi: Set INFORMATION sense data field
 consistently

On 2025/04/08 3:18, Igor Pylypiv wrote:
> On Mon, Apr 07, 2025 at 10:52:19AM +0200, Niklas Cassel wrote:
>> On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote:
>>>
>>> Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands
>>> already contains the ATA LBA in bytes [6..11] so it seems redundant to
>>> also include the same in the Information sense data descriptor.   
>>
>> Damien and I talked about this.
>>
>> Since this patch only affects non-PT commands, what it this patch actually
>> solving?
> 
> For ATA PASS-THROUGH + fixed format sense data + NCQ autosense, this patch
> eliminates reduntant writes to set the INFORMATION field and the VALID bit.
> 
> First write: scsi_set_sense_information() sets the INFORMATION field
> to ATA LBA (which is incorrect for ATA PASS-THROUGH).
> 
> Second write: ata_scsi_set_passthru_sense_fields() sets the INFORMATION
> field to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.
> 
> User space should not see an issue because second write overwrites
> the incorrect data from the first write. I think we should still fix
> this in case someone updates the code to remove the second write in
> the future.

OK. This now makes more sense. Please add all this description to the commit
message to clarify WHAT you are fixing, and clearly explain how the first
(useless) INFORMATION field write is removed.


>> So unless SCSI core does some specific handling based on the INFORMATION
>> field (and AFAICT, SCSI core only looks at SK/ASC/ASCQ), I can't see how
>> this patch can actually solve anything.
> 
> +1 the patch does not seem to solve any issues for non-PT commands besides
> a spec compliance which is not visible to a user.

If the above sequence applies to both passthrugh and regular IOs, then there is
no spec violation since the second write does set the INFORMATION filed to a
correct value.

> Deleting ata_scsi_set_sense_information() should work as well. If SCSI core
> does not use the INFORMATION field perhaps there is no need to set it at all?
> This will eliminate the reduntant writes for ATA PASS-THROUGH as well.

I do not think the scsi core use that field at all. But libata should still
behave correctly and act as a spec compliant SAT. So if SAT says that this field
should be set, then let's initialize it. If SAT says otherwise, let's do that.
I have not checked SAT specs. Have you ?


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ