[PATCH] ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

[PATCH] ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

[hengyul]

From: Hengyu Liang <hengyul@cs.unc.edu>

Since commit 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands
on time out"), atapi_qc_complete() sets SAM_STAT_CHECK_CONDITION for a
failed command only if cmd->result is zero, so that the DID_TIME_OUT or
DID_REQUEUE host byte of a terminated command is preserved.

However, cmd->result can also be non-zero for a regular failed ATAPI
command completed through libata EH: ata_eh_analyze_tf() calls
ata_eh_decide_disposition(), that is, scsi_check_sense(), on the sense
data obtained with REQUEST SENSE, and scsi_check_sense() sets the SCSI
midlayer internal byte of the result for some sense codes, e.g.
SCSIML_STAT_TGT_FAILURE for ILLEGAL REQUEST with ASC 0x20 (INVALID
COMMAND OPERATION CODE) or 0x24 (INVALID FIELD IN CDB), or
SCSIML_STAT_MED_ERROR for MEDIUM ERROR with ASC 0x11 (UNRECOVERED READ
ERROR). For such commands, atapi_qc_complete() keeps the result as is and
the command completes with a GOOD status byte, even though it failed and
valid sense data is available.

As a result, passthrough users see these failed ATAPI commands as
successful. For example, with the emulated IDE CD-ROM drive of QEMU and
no medium loaded, a PLAY AUDIO MSF command issued with SG_IO completes
with status 0x00 (and no DRIVER_SENSE) instead of 0x02 (CHECK CONDITION)
with sense key ILLEGAL REQUEST, ASC/ASCQ 0x20/0x00, and the CDROM ioctls
CDROMPLAYTRKIND, CDROMSUBCHNL, CDROMPAUSE, CDROMRESUME, CDROMPLAYMSF and
CDROM_GET_MCN return 0 instead of -ENOMEDIUM or -EOPNOTSUPP.

Fix this by preserving the result only if its host byte is set, and by
otherwise completing the command with SAM_STAT_CHECK_CONDITION, as was
done before that commit.

Fixes: 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands on time out")
Cc: stable@vger.kernel.org
Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
 drivers/ata/libata-scsi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8f9aa97a519d..dc5978019eae 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3059,7 +3059,14 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
 			qc->dev->sdev->locked = 0;
 
-		if (cmd->result)
+		/*
+		 * Only preserve the result of a command that was terminated
+		 * with a host byte set, e.g. DID_TIME_OUT. A failed command
+		 * may also have the SCSI midlayer internal byte set by
+		 * scsi_check_sense(), which must not prevent reporting the
+		 * CHECK CONDITION status.
+		 */
+		if (get_host_byte(cmd) != DID_OK)
 			ata_scsi_qc_done(qc, false, 0);
 		else
 			ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
-- 
2.53.0

Re: [PATCH] ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: midlayer byte wiped in atapi_qc_complete() breaks error translation
--

commit a67971887516df13f132f00266219f908c09e9aa
Author: Hengyu Liang <hengyul@cs.unc.edu>

ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

This commit modifies atapi_qc_complete() to preserve cmd->result only if the
host byte is set. For other errors, it sets the status to
SAM_STAT_CHECK_CONDITION to avoid losing the CHECK CONDITION status for
failed ATAPI commands.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d2160ee7ca7d4..436ddf5ed4748 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3020,7 +3020,14 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
>  			qc->dev->sdev->locked = 0;
>  
> -		if (cmd->result)
> +		/*
> +		 * Only preserve the result of a command that was terminated
> +		 * with a host byte set, e.g. DID_TIME_OUT. A failed command
> +		 * may also have the SCSI midlayer internal byte set by
> +		 * scsi_check_sense(), which must not prevent reporting the
> +		 * CHECK CONDITION status.
> +		 */
> +		if (get_host_byte(cmd) != DID_OK)
>  			ata_scsi_qc_done(qc, false, 0);
>  		else
>  			ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this assignment destroy the upper 24 bits of cmd->result, completely
erasing the midlayer byte?

When an ATAPI command fails, ata_eh_analyze_tf() fetches sense data and
delegates to scsi_check_sense(), which translates sense keys (e.g.,
MEDIUM_ERROR) by setting the SCSI midlayer internal byte in cmd->result.

When atapi_qc_complete() calls ata_scsi_qc_done() with set_result = true:

drivers/ata/libata-scsi.c:ata_scsi_qc_done() {
    ...
	if (set_result)
		cmd->result = scmd_result;
    ...
}

This directly overwrites the 32-bit field and silently erases the midlayer
byte previously set by scsi_check_sense().

Consequently, scsi_result_to_blk_status() sees an empty midlayer byte and
translates legitimate ATAPI block errors to generic BLK_STS_IOERR (-EIO)
instead of the specific status (like BLK_STS_MEDIUM), breaking block layer
error translation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924180414.2102946-1-hengyul@cs.unc.edu?part=1

Re: [PATCH] ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

[Niklas Cassel]

Hello Hengyu,

On Thu, Sep 24, 2026 at 02:04:14PM -0400, hengyul@cs.unc.edu wrote:
> From: Hengyu Liang <hengyul@cs.unc.edu>
> 
> Since commit 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands
> on time out"), atapi_qc_complete() sets SAM_STAT_CHECK_CONDITION for a
> failed command only if cmd->result is zero, so that the DID_TIME_OUT or
> DID_REQUEUE host byte of a terminated command is preserved.
> 
> However, cmd->result can also be non-zero for a regular failed ATAPI
> command completed through libata EH: ata_eh_analyze_tf() calls
> ata_eh_decide_disposition(), that is, scsi_check_sense(), on the sense
> data obtained with REQUEST SENSE, and scsi_check_sense() sets the SCSI
> midlayer internal byte of the result for some sense codes, e.g.
> SCSIML_STAT_TGT_FAILURE for ILLEGAL REQUEST with ASC 0x20 (INVALID
> COMMAND OPERATION CODE) or 0x24 (INVALID FIELD IN CDB), or
> SCSIML_STAT_MED_ERROR for MEDIUM ERROR with ASC 0x11 (UNRECOVERED READ
> ERROR). For such commands, atapi_qc_complete() keeps the result as is and
> the command completes with a GOOD status byte, even though it failed and
> valid sense data is available.
> 
> As a result, passthrough users see these failed ATAPI commands as
> successful. For example, with the emulated IDE CD-ROM drive of QEMU and
> no medium loaded, a PLAY AUDIO MSF command issued with SG_IO completes
> with status 0x00 (and no DRIVER_SENSE) instead of 0x02 (CHECK CONDITION)
> with sense key ILLEGAL REQUEST, ASC/ASCQ 0x20/0x00, and the CDROM ioctls
> CDROMPLAYTRKIND, CDROMSUBCHNL, CDROMPAUSE, CDROMRESUME, CDROMPLAYMSF and
> CDROM_GET_MCN return 0 instead of -ENOMEDIUM or -EOPNOTSUPP.
> 
> Fix this by preserving the result only if its host byte is set, and by
> otherwise completing the command with SAM_STAT_CHECK_CONDITION, as was
> done before that commit.
> 
> Fixes: 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands on time out")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---

I think it is better if you set only the status byte with set_status_byte()
instead of overwriting cmd->result, so that the SCSI midlayer byte set by
scsi_check_sense() is kept.

Something like:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8f9aa97a519d..fb5517d51095 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3059,10 +3059,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
                if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
                        qc->dev->sdev->locked = 0;
 
-               if (cmd->result)
-                       ata_scsi_qc_done(qc, false, 0);
-               else
-                       ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
+               /*
+                * Report CHECK CONDITION unless the command was terminated
+                * with a host byte set. Only set the status byte, so that the
+                * SCSI midlayer internal byte, which scsi_check_sense() may
+                * have set from the sense data, is preserved.
+                */
+               if (get_host_byte(cmd) == DID_OK)
+                       set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
+               ata_scsi_qc_done(qc, false, 0);
                goto schedule_deferred;
        }