Re: [PATCH v2 1/2] ata: libata-scsi: terminate deferred commands on time out

On Fri, Jul 10, 2026 at 12:24:30AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] `ata_scsi_port_eh_timed_out` fails to set `ATA_PFLAG_EH_PENDING`, which allows requeued deferred commands to be immediately re-issued and the timed-out command itself to be erroneously executed, defeating the patch's optimization and causing unnecessary hardware resets.
> 
> Pre-existing issues:
> - [Critical] A race condition between a deferred command's SCSI timeout and its requeue completion causes the command to be silently dropped and reported as successfully completed to the block layer, leading to silent data corruption.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] ata: libata-scsi: terminate deferred commands on time out ---
> commit a21f27abd9e61a32aa55fa1005071af9c225247b
> Author: Damien Le Moal <[email protected]>
> 
> ata: libata-scsi: terminate deferred commands on time out
> 
> If a command timeout occurs while we have a deferred non-NCQ command
> waiting to be issued, the SCSI EH task is never woken up. Avoid this
> unnecessary EH trigger additional wait time using the eh_timed_out SCSI
> host template operation to force a requeue of deferred QCs.
> 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> 
> [ ... ]
> 
> > @@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> >  	/*
> >  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
> >  	 * do not try to be smart about what to do with this deferred command
> > -	 * and simply requeue it by completing it with DID_REQUEUE.
> > +	 * and simply requeue it by completing it with DID_REQUEUE. The
> > +	 * exception here is if the deferred qc timed out, in which case, we
> > +	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
> >  	 */
> >  	ata_for_each_link(link, ap, PMP_FIRST) {
> >  		struct ata_queued_cmd *qc = link->deferred_qc;
> >  
> > -		if (qc) {
> > -			link->deferred_qc = NULL;
> > -			cancel_work(&link->deferred_qc_work);
> > -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> > -		}
> > +		if (!qc || qc->scsicmd == timed_out_scmd)
> > +			continue;
> > +
> > +		link->deferred_qc = NULL;
> > +		cancel_work(&link->deferred_qc_work);
> > +		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> 
> [Severity: Critical]
> This is a pre-existing issue, but won't the explicit invocation of this
> requeue logic on every command timeout make a regression more likely?
> 
> When ata_scsi_requeue_deferred_qc() completes a deferred command using
> ata_scsi_qc_done() with DID_REQUEUE, it unconditionally calls
> ata_qc_free(), which clears the active flag.
> 
> If the block layer timer for this command expires concurrently,
> scsi_timeout() can win the race against the scsi_done() completion. The
> block layer marks the command as timed out and adds it to the SCSI EH queue.
> 
> When SCSI EH runs ata_scsi_cmd_error_handler(), it iterates over all QCs to
> find the command. Since ata_qc_free() was already called and the deferred_qc
> pointer was set to NULL, it won't match the deferred check.
> 
> Does this cause the loop to finish without finding the command and fall
> through to the successful completion branch:
> 
> drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
>     ...
>     } else {
>         /* Normal completion occurred after
>          * SCSI timeout but before this point.
>          * Successfully complete it.
>          */
>         set_host_byte(scmd, DID_OK);
>         scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
>     }
> }
> 
> Could this result in the command being reported as successfully completed to
> the block layer, resulting in silent data corruption?

This specific race between ata_scsi_requeue_deferred_qc() and scsi_timeout()
has not existed for that long, because ata_scsi_requeue_deferred_qc() itself
is not that old.

Perhaps ata_scsi_requeue_deferred_qc(), instead of calling
ata_scsi_qc_done(qc, ...) directly to finish the command, should instead set
ATA_QCFLAG_RETRY (or similar) and call ata_qc_schedule_eh(qc, ...), such that
scsi_eh_scmd_add() will be called, and then EH will requeue the command instead
the completion path.

This way, there should be no race between ata_scsi_requeue_deferred_qc() ->
scsi_done() -> scsi_complete() -> scsi_queue_insert() and scsi_timeout(), as
it will be EH itself that requeues the command.


Note that while link->deferred_qc is set, we can only get a block layer timeout.
If the command was actually issued by the workqueue, link->deferred_qc will be
NULL, so the code in ata_scsi_cmd_error_handler() to handle the race between
normal completion and scsi_timeout() is not really applicable for link->deferred_qc.
(If link->deferred_qc is set, this qc cannot have raced against completion, only
against timeout.)


Kind regards,
Niklas