linux-kernel - Re: [RFC PATCH] ata: libata-eh: Retry the cmnd when normal complete occurrd after scsi timeout

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 28 Dec 2022 17:59:10 +0000
From:   Niklas Cassel <Niklas.Cassel@....com>
To:     Wu Bo <wubo40@...wei.com>
CC:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        "linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "qiuchangqi.qiu@...wei.com" <qiuchangqi.qiu@...wei.com>
Subject: Re: [RFC PATCH] ata: libata-eh: Retry the cmnd when normal complete
 occurrd after scsi timeout

On Wed, Dec 28, 2022 at 05:36:56PM +0800, Wu Bo wrote:
> From: wubo <wubo40@...wei.com>
> 
> Hi,
> 
> Now SCSI middle layer EH and normal IO handler can only choose one of them,
> after the SCSI command is completed normally after scsi timeout period,
> Should this scenario be given a chance to retry?
> 
> Signed-off-by: wubo <wubo40@...wei.com>
> ---
>  drivers/ata/libata-eh.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce..8d1856f 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -617,14 +617,8 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>  					qc->flags |= ATA_QCFLAG_FAILED;
>  					nr_timedout++;
>  				}
> -			} else {
> -				/* Normal completion occurred after
> -				 * SCSI timeout but before this point.
> -				 * Successfully complete it.
> -				 */
> -				scmd->retries = scmd->allowed;
> +			} else
>  				scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
> -			}
>  		}
>

Hello Wu,


The function comment looks like this:
	/* For new EH, all qcs are finished in one of three ways -
	 * normal completion, error completion, and SCSI timeout.
	 * Both completions can race against SCSI timeout.  When normal
	 * completion wins, the qc never reaches EH.  When error
	 * completion wins, the qc has ATA_QCFLAG_FAILED set.
	 *
	 * When SCSI timeout wins, things are a bit more complex.
	 * Normal or error completion can occur after the timeout but
	 * before this point.  In such cases, both types of
	 * completions are honored.  A scmd is determined to have
	 * timed out iff its associated qc is active and not failed.
	 */

And the code looks like this:

			if (i < ATA_MAX_QUEUE) {
				/* the scmd has an associated qc */
				if (!(qc->flags & ATA_QCFLAG_FAILED)) {
					/* which hasn't failed yet, timeout */
					qc->err_mask |= AC_ERR_TIMEOUT;
					qc->flags |= ATA_QCFLAG_FAILED;
					nr_timedout++;
				}
			} else {
				/* Normal completion occurred after
				 * SCSI timeout but before this point.
				 * Successfully complete it.
				 */
				scmd->retries = scmd->allowed;
				scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
			}



If SCSI timeout wins, but there came an error completion after,
then we will go into the if (i < ATA_MAX_QUEUE) case, but we will
not enter the if !(qc->flags & ATA_QCFLAG_FAILED), as ATA_QCFLAG_FAILED
will already be set by the irq handler. This QC will be completed
by the ata_scsi_port_error_handler(), which gets to run just after
this function has returned:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/ata/libata-eh.c#L546



The else clause you are modifying however is for the case where SCSI timeout
wins, but there came a normal completion occurred after the SCSI timeout.

In more detail, what happens first is that scsi_timeout() gets called,
and if scsi timeout wins, it sets SCMD_STATE_COMPLETE:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_error.c#L355
and then schedules EH for that command using scsi_eh_scmd_add().

What happens next in this specific case is that the IRQ handler is called,
takes the ap->lock (which is also taken is this function so that it can run
at the same time as the IRQ handler), then the IRQ handler calls
__ata_qc_complete() for the QC, however, when scsi_done() is finally called
in ata_qc_done() (from IRQ context), it will not be a no-op, since
SCMD_STATE_COMPLETE is already set:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_lib.c#L1623

Since scsi_done() never finished the scsi_cmd, we need to finish it here,
in the else clause, by calling scsi_eh_finish_cmd().

When the EH queue is flushed, it will check if scsi_cmd_retry_allowed()
and if it is, the command will be retried, otherwise it will call scsi_finish()
on the command:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_error.c#L2150

Considering that we want to only finish the scmd here, libata sets
scmd->retries = scmd->allowed; such that the check:
	return ++cmd->retries <= cmd->allowed;
in scsi_cmd_retry_allowed() will evaulate to false.


So TL;DR:
It is absolutely essential to set scmd->retries = scmd->allowed;
in this else clause, as that is the only reason why this command will
be finished instead of retried.
Since this else clause is for a command that timed out, but got completed
successfully via the IRQ handler after timing out (so the QC got freed),
we only need to finish the scmd. Retrying the scmd is wrong in this case.


Kind regards,
Niklas