Re: [PATCH] ata: libata-core: Return sense data in descriptor format by default

[Hannes Reinecke <hare@suse.de>]

On 8/12/24 20:43, Niklas Cassel wrote:
> On Mon, Aug 12, 2024 at 05:15:18PM +0200, Niklas Cassel wrote:
>> Sense data can be in either fixed format or descriptor format.
>>
>> SAT-6 revision 1, 10.4.6 Control mode page, says that if the D_SENSE bit
>> is set to zero (i.e., fixed format sense data), then the SATL should
>> return fixed format sense data for ATA PASS-THROUGH commands.
>>
>> A lot of user space programs incorrectly assume that the sense data is in
>> descriptor format, without checking the RESPONSE CODE field of the
>> returned sense data (to see which format the sense data is in).
>>
>> The libata SATL has always kept D_SENSE set to zero by default.
>> (It is however possible to change the value using a MODE SELECT command.)
>>
>> For failed ATA PASS-THROUGH commands, we correctly generated sense data
>> according to the D_SENSE bit. However, because of a bug, sense data for
>> successful ATA PASS-THROUGH commands was always generated in the
>> descriptor format.
>>
>> This was fixed to consistently respect D_SENSE for both failed and
>> successful ATA PASS-THROUGH commands in commit 28ab9769117c ("ata:
>> libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error").
>>
>> After commit 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for
>> CK_COND=1 and no error"), we started receiving bug reports that we broke
>> these user space programs (these user space programs must never have
>> encountered a failing command, as the sense data for failing commands has
>> always correctly respected D_SENSE, which by default meant fixed format).
>>
>> Since a lot of user space programs seem to assume that the sense data is
>> in descriptor format (without checking the type), let's simply change the
>> default to have D_SENSE set to one by default.
>>
>> That way:
>> -Broken user space programs will see no regression.
>> -Both failed and successful ATA PASS-THROUGH commands will respect D_SENSE,
>>   as per SAT-6 revision 1.
>> -Apparently it seems way more common for user space applications to assume
>>   that the sense data is in descriptor format, rather than fixed format.
>>   (A user space program should of course support both, and check the
>>   RESPONSE CODE field to see which format the returned sense data is in.)
>>
>> Cc: stable@vger.kernel.org # 4.19+
>> Reported-by: Stephan Eisvogel <eisvogel@seitics.de>
>> Reported-by: Christian Heusel <christian@heusel.eu>
>> Closes: https://lore.kernel.org/linux-ide/0bf3f2f0-0fc6-4ba5-a420-c0874ef82d64@heusel.eu/
>> Fixes: 28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and no error")
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>   drivers/ata/libata-core.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index c7752dc80028..590bebe1354d 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5368,6 +5368,13 @@ void ata_dev_init(struct ata_device *dev)
>>   	 */
>>   	spin_lock_irqsave(ap->lock, flags);
>>   	dev->flags &= ~ATA_DFLAG_INIT_MASK;
>> +
>> +	/*
>> +	 * A lot of user space programs incorrectly assume that the sense data
>> +	 * is in descriptor format, without checking the RESPONSE CODE field of
>> +	 * the returned sense data (to see which format the sense data is in).
>> +	 */
>> +	dev->flags |= ATA_DFLAG_D_SENSE;
>>   	dev->horkage = 0;
>>   	spin_unlock_irqrestore(ap->lock, flags);
>>   
>> -- 
>> 2.46.0
>>
> 
> This patch will change so that the sense data will be generated in descriptor
> format (by default) for passthrough (SG_IO) commands, not just SG_IO ATA
> PASS-THROUGH commands.
> 
> Non-passthrough (SG_IO) commands are not relavant, as they will go via
> scsi_finish_command(), which calls scsi_normalize_sense() before interpreting
> the sense data, and for non-passthrough commands, the sense data is not
> propagated to the user. (The SK/ASC/ASCQ is only printed to the log, and this
> print will be the same as before.)
> 
> However, it is possible to send any command as passthrough (SG_IO), not only
> ATA PASS-THROUGH (ATA-16 / ATA-12 commands).
> 
> So there will be a difference (by default) for SG_IO (passthrough) commands
> that are not ATA PASS-THROUGH commands (ATA-16 / ATA-12 commands).
> (E.g. if you send a regular SCSI read/write command via SG_IO to an ATA device,
> and if that command generates sense data, the default sense data format would
> be different.)
> 
> Is this a concern?
> 
> I have a feeling that some user space program that blindly assumes that the
> sense data will be in fixed format (for e.g. a command that does an invalid
> read) using SG_IO will start to complain because of a "regression".
> 
I really hate it when people start generalising which in fact was an 
occurrence with a single program, namely hdparm.

Which indeed is ancient, and I'm only slightly surprised that things
broke here.

But all other programs I know of do attempt to handle sense codes, so
really I don't have an issue with this change.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich