Re: [PATCH v6 2/2] scsi: libsas: terminate deferred commands on time out

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A race condition between the SCSI timeout handler and ata_scsi_requeue_deferred_qc(ap, NULL) causes timed-out deferred commands to be silently completed as SUCCESS, leading to data corruption.
--

--- Patch [2]: [PATCH v6 2/2] scsi: libsas: terminate deferred commands on time out ---
commit e25a34ed23fb53d891efaea25a8a4f4ff07ee231
Author: Damien Le Moal <[email protected]>

scsi: libsas: terminate deferred commands on time out

This patch implements the eh_timed_out SCSI host template operation for libsas.
It avoids unnecessary SCSI EH trigger wait times for deferred non-NCQ commands
by utilizing ata_scsi_retry_deferred_qc() for SATA devices, mirroring the
method used in libata-scsi.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b02b4ca5c09e2..dfd7fc2c8cb66 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1762,8 +1762,7 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
>  		queue_work(system_highpri_wq, &link->deferred_qc_work);
>  }
>  
> -static void ata_scsi_retry_deferred_qc(struct ata_port *ap,
> -				       struct scsi_cmnd *scmd)
> +void ata_scsi_retry_deferred_qc(struct ata_port *ap, struct scsi_cmnd *scmd)
>  {
>  	unsigned long flags;
>  
> @@ -1771,6 +1770,7 @@ static void ata_scsi_retry_deferred_qc(struct ata_port *ap,
>  	ata_scsi_requeue_deferred_qc(ap, scmd);
>  	spin_unlock_irqrestore(ap->lock, flags);
>  }
> +EXPORT_SYMBOL_GPL(ata_scsi_retry_deferred_qc);

[Severity: Critical]
This is a pre-existing issue, but does exporting and reusing
ata_scsi_retry_deferred_qc() expose us further to a race condition with
ata_scsi_requeue_deferred_qc()?

If a deferred SATA command times out, the block layer marks its request as
MQ_RQ_COMPLETE and enters scsi_timeout(). Concurrently, if EH is scheduled for
another command on the same port, ata_eh_set_pending() can call
ata_scsi_requeue_deferred_qc(ap, NULL).

When called with timedout_scmd == NULL, this clears link->deferred_qc and calls
ata_scsi_qc_done():

drivers/ata/libata-scsi.c:ata_scsi_requeue_deferred_qc() {
    ...
    if (!timedout_scmd) {
        ...
        ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
        continue;
    }
    ...
}

ata_scsi_qc_done() zeroes qc->flags via ata_qc_free(), and calls scsi_done(),
which the block layer ignores due to the concurrent timeout. The timeout path
then adds the command to EH.

When ata_scsi_cmd_error_handler() runs, it fails to find the QC because
qc->flags is 0, assuming normal successful completion:

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
        } 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);
        }
    ...
}

Can this race cause timed-out deferred commands to be silently completed as
SUCCESS, leading to silent data corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2