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

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

[Niklas Cassel]

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

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

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On 24/08/12 05:15PM, 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).
> 

Tested-by: Christian Heusel <christian@heusel.eu>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Niklas Cassel]

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".

Thus, perhaps it is safest to just drop this patch, and let users of
passthrough commands (SG_IO) simply learn how to parse sense data properly,
since there will/can always be someone complaining. My personal feeling is
that passthrough commands should simply follow the storage standard exactly,
and if a user space application does adhere to the standard, tough luck,
why are you using passthrough commands instead of regular commands then?
Passthrough commands by definition follow a specific storage standard,
and not the Linux kernel block layer API.


Kind regards,
Niklas

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

[Niklas Cassel]

On Mon, Aug 12, 2024 at 08:43:07PM +0200, Niklas Cassel wrote:
> 
> Thus, perhaps it is safest to just drop this patch, and let users of
> passthrough commands (SG_IO) simply learn how to parse sense data properly,
> since there will/can always be someone complaining. My personal feeling is
> that passthrough commands should simply follow the storage standard exactly,
> and if a user space application does adhere to the standard, tough luck,

s/does adhere/does not adhere/

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

[Martin K. Petersen]

Niklas,

> My personal feeling is that passthrough commands should simply follow
> the storage standard exactly, and if a user space application does
> adhere to the standard, tough luck, why are you using passthrough
> commands instead of regular commands then? Passthrough commands by
> definition follow a specific storage standard, and not the Linux
> kernel block layer API.

Yeah. I'm not sure how much of a problem non-ATA passthrough is for
libata-attached devices. But it does seem messy that userland is making
these kinds of assumptions.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Hannes Reinecke]

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

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

[Niklas Cassel]

On Tue, Aug 13, 2024 at 08:37:42AM +0200, Hannes Reinecke wrote:
> 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.

It is actually multiple programs, namely hdparm, hddtemp and udisks.

It is unfortunate that these applications do not handle sense data correctly,
and they would break even on older kernels if your do a MODE SELECT to change
the D_SENSE bit to one.

However, right now I don't see any other option than to revert commit
28ab9769117c ("ata: libata-scsi: Honor the D_SENSE bit for CK_COND=1 and
no error"), so that we are no longer spec compliant with SAT-6. This commit
got backported to stable, so now there is a bunch of users complaining that
hddtemp etc. is no longer working.

Perhaps we could re-visit this code to be spec compliant again in the
future (after the bad programs have been fixed).


Kind regards,
Niklas

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

[Christoph Hellwig]

On Tue, Aug 13, 2024 at 11:41:28AM +0200, Niklas Cassel wrote:
> Perhaps we could re-visit this code to be spec compliant again in the
> future (after the bad programs have been fixed).

I doubt it.  They are part of the core low-level userspace suite and
even when they are fixed the old version will be around roughly forever.
So I think we are (unfortunately) stuck here.

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

[Niklas Cassel]

On Tue, Aug 13, 2024 at 02:15:49PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 13, 2024 at 11:41:28AM +0200, Niklas Cassel wrote:
> > Perhaps we could re-visit this code to be spec compliant again in the
> > future (after the bad programs have been fixed).
> 
> I doubt it.  They are part of the core low-level userspace suite and
> even when they are fixed the old version will be around roughly forever.
> So I think we are (unfortunately) stuck here.

Agreed... even if it makes me a bit sad to intentionally not be spec
compliant...

Sent a patch that replaces the patch in $subject:
https://lore.kernel.org/linux-ide/20240813131900.1285842-2-cassel@kernel.org/T/#u


Kind regards,
Niklas