Re: [PATCH] ata: libata: cancel pending work after clearing deferred_qc

[Igor Pylypiv <ipylypiv@google.com>]

On Tue, Mar 03, 2026 at 11:03:42AM +0100, Niklas Cassel wrote:
> Syzbot reported a WARN_ON() in ata_scsi_deferred_qc_work(), caused by
> ap->ops->qc_defer() returning non-zero before issuing the deferred qc.
> 
> ata_scsi_schedule_deferred_qc() is called during each command completion.
> This function will check if there is a deferred QC, and if
> ap->ops->qc_defer() returns zero, meaning that it is possible to queue the
> deferred qc at this time (without being deferred), then it will queue the
> work which will issue the deferred qc.
> 
> Once the work get to run, which can potentially be a very long time after
> the work was scheduled, there is a WARN_ON() if ap->ops->qc_defer() returns
> non-zero.
> 
> While we hold the ap->lock both when assigning and clearing deferred_qc,
> and the work itself holds the ap->lock, the code currently does not cancel
> the work after clearing the deferred qc.
> 
> This means that the following scenario can happen:
> 1) One or several NCQ commands are queued.
> 2) A non-NCQ command is queued, gets stored in ap->deferred_qc.
> 3) Last NCQ command gets completed, work is queued to issue the deferred
>    qc.
> 4) Timeout or error happens, ap->deferred_qc is cleared. The queued work is
>    currently NOT canceled.
> 5) Port is reset.
> 6) One or several NCQ commands are queued.
> 7) A non-NCQ command is queued, gets stored in ap->deferred_qc.
> 8) Work is finally run. Yet at this time, there is still NCQ commands in
>    flight.
> 
> The work in 8) really belongs to the non-NCQ command in 2), not to the
> non-NCQ command in 7). The reason why the work is executed when it is not
> supposed to, is because it was never canceled when ap->deferred_qc was
> cleared in 4). Thus, ensure that we always cancel the work after clearing
> ap->deferred_qc.
> 
> Another potential fix would have been to let ata_scsi_deferred_qc_work() do
> nothing if ap->ops->qc_defer() returns non-zero. However, canceling the
> work when clearing ap->deferred_qc seems slightly more logical, as we hold
> the ap->lock when clearing ap->deferred_qc, so we know that the work cannot
> be holding the lock. (The function could be waiting for the lock, but that
> is okay since it will do nothing if ap->deferred_qc is not set.)
> 
> Reported-by: syzbot+bcaf842a1e8ead8dfb89@syzkaller.appspotmail.com
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Fixes: eddb98ad9364 ("ata: libata-eh: correctly handle deferred qc timeouts")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thanks,
Igor