linux-kernel - Re: [PATCH] ata: libata-eh: Clear scsicmd->result when setting SAM_STAT_CHECK

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZtoGYshL26jTuTOj@ryzen.lan>
Date: Thu, 5 Sep 2024 21:28:34 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>
Cc: Damien Le Moal <dlemoal@...nel.org>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Hannes Reinecke <hare@...e.de>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: libata-eh: Clear scsicmd->result when setting
 SAM_STAT_CHECK_CONDITION

On Thu, Sep 05, 2024 at 02:40:28PM +0200, Niklas Cassel wrote:
> Anyway, I think I came up with an even nicer patch to clear the driver byte.
> 
> Change ata_scsi_set_sense():
> -to set SENSE_DATA_VALID
> -to clear driver byte (if scsicmd)
> 
> For the cases that calls:
> -scsi_build_sense_buffer()
> (because they don't want to set the SAM_STAT_CHECK_CONDITION)
> or
> -scsi_build_sense()
> without using ata_scsi_set_sense():
> create a new function/functions (e.g. ata_build_sense_keep_status())
> -sets SENSE_DATA_VALID
> -clears driver byte (if scsicmd)
> 
> Will send a PATCH/RFC series today or tomorrow.

Thinking even more about this:

ata_qc_schedule_eh() will have called scsi_timeout(), which sets
DID_TIME_OUT, since libata does not have an abort handler.

Thus, the command when first entering EH will have DID_TIME_OUT set.
Right now, we clear it as the final thing in ata_scsi_qc_complete().
You are suggesting that we clear it when storing sense data.

However:
There can always be commands that was sent via EH, that will not
have added sense data, e.g. for CDL policy 0xD commands that were
completed in the same IRQ (where some commands might have sense
data, but not all), same with NCQ error, one command will be the
one that triggered the NCQ error, rest will not have sense data.
Thus we will always need to clear the DID_TIME_OUT bit for commands
that were sent via EH...

I think what we could do, is to clear it when first entering EH,
instead of at the end of EH. This is probably the best solution,
because then we can remove the:
cmd->result &= 0x0000ffff;
from ata_scsi_qc_complete(), which is executed both for commands
that went via EH and command that did not go via EH.

And instead clear it in the beginning of EH, so DID_TIME_OUT will
only get cleared for commands that went via EH. (Because only
commands that went via EH will have DID_TIME_OUT set in the first
place.)

So my proposal is simply this:

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7de97ee8e78b..450e9bd96c97 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -630,6 +630,15 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
        list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
                struct ata_queued_cmd *qc;

+               /*
+                * If the scmd was added to EH, via ata_qc_schedule_eh() ->
+                * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will
+                * have set DID_TIME_OUT (since libata does not have an abort
+                * handler). Thus to clear the DID_TIME_OUT, we clear the host
+                * byte (but keep the SCSI ML and status byte).
+                */
+               scmd->result &= 0x0000ffff;
+
                ata_qc_for_each_raw(ap, qc, i) {
                        if (qc->flags & ATA_QCFLAG_ACTIVE &&
                            qc->scsicmd == scmd)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3a442f564b0d..6a90062c8b55 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1680,9 +1680,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
                        set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
        } else if (is_error && !have_sense) {
                ata_gen_ata_sense(qc);
-       } else {
-               /* Keep the SCSI ML and status byte, clear host byte. */
-               cmd->result &= 0x0000ffff;
        }

        ata_qc_done(qc);

Testing is appreciated :)

Thoughts?

Kind regards,
Niklas