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

[Niklas Cassel <cassel@kernel.org>]

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