[PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[Guenter Roeck]

If the `ata_qc_for_each_raw()` loop finishes without finding a matching
`scmd`, `qc` will hold a pointer to the last element examined, which is
accociated with `i == ATA_MAX_QUEUE - 1`. This element can match
`ap->deferred_qc`. If that happens, the condition `qc == ap->deferred_qc`
evaluates to true despite the loop not breaking on a scmd match.
In that case, the error handler mistakenly intercepts a command that
completed normally after an unrelated SCSI timeout, returning a timeout
error instead of success.

Fix the problem by checking for i < ATA_MAX_QUEUE in addition to
qc == ap->deferred_qc.

The problem was found by an experimental code review agent based on
gemini-3.1-pro while reviewing backports into v6.18.y.

Assisted-by: Gemini:gemini-3.1-pro
Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Niklas Cassel <cassel@kernel.org>
Fixes: eddb98ad9364 ("ata: libata-eh: correctly handle deferred qc timeouts")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/ata/libata-eh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b373cceb95d2..44fddfbb7629 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -647,7 +647,7 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 				break;
 		}
 
-		if (qc == ap->deferred_qc) {
+		if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc) {
 			/*
 			 * This is a deferred command that timed out while
 			 * waiting for the command queue to drain. Since the qc
-- 
2.45.2

Re: [PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[Damien Le Moal]

On 3/6/26 11:48, Guenter Roeck wrote:
> If the `ata_qc_for_each_raw()` loop finishes without finding a matching
> `scmd`, `qc` will hold a pointer to the last element examined, which is
> accociated with `i == ATA_MAX_QUEUE - 1`. This element can match
> `ap->deferred_qc`. If that happens, the condition `qc == ap->deferred_qc`
> evaluates to true despite the loop not breaking on a scmd match.
> In that case, the error handler mistakenly intercepts a command that
> completed normally after an unrelated SCSI timeout, returning a timeout
> error instead of success.
> 
> Fix the problem by checking for i < ATA_MAX_QUEUE in addition to
> qc == ap->deferred_qc.
> 
> The problem was found by an experimental code review agent based on
> gemini-3.1-pro while reviewing backports into v6.18.y.

Please remove all the quotes:

If the ata_qc_for_each_raw() loop finishes without finding a matching SCSI
command for any QC, the variable qc will hold a pointer to the last element
examined, which has the tag i == ATA_MAX_QUEUE - 1. This qc can match the port
deferred QC (ap->deferred_qc).

If that happens, the condition qc == ap->deferred_qc evaluates to true despite
the loop not breaking with a match on the SCSI command for this QC. In that
case, the error handler mistakenly intercepts a command that not completed yet
and that has not timed out, and thus erroneously returning a timeout error.

Fix the problem by checking for i < ATA_MAX_QUEUE in addition to
qc == ap->deferred_qc.

The problem was found by an experimental code review agent based on
gemini-3.1-pro while reviewing backports into v6.18.y.

> Assisted-by: Gemini:gemini-3.1-pro
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Niklas Cassel <cassel@kernel.org>
> Fixes: eddb98ad9364 ("ata: libata-eh: correctly handle deferred qc timeouts")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

With the above, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[Niklas Cassel]

On Fri, Mar 06, 2026 at 12:33:21PM +0900, Damien Le Moal wrote:
> On 3/6/26 11:48, Guenter Roeck wrote:
> > If the `ata_qc_for_each_raw()` loop finishes without finding a matching
> > `scmd`, `qc` will hold a pointer to the last element examined, which is
> > accociated with `i == ATA_MAX_QUEUE - 1`. This element can match
> > `ap->deferred_qc`. If that happens, the condition `qc == ap->deferred_qc`
> > evaluates to true despite the loop not breaking on a scmd match.
> > In that case, the error handler mistakenly intercepts a command that
> > completed normally after an unrelated SCSI timeout, returning a timeout
> > error instead of success.
> > 
> > Fix the problem by checking for i < ATA_MAX_QUEUE in addition to
> > qc == ap->deferred_qc.
> > 
> > The problem was found by an experimental code review agent based on
> > gemini-3.1-pro while reviewing backports into v6.18.y.
> 
> Please remove all the quotes:
> 
> If the ata_qc_for_each_raw() loop finishes without finding a matching SCSI
> command for any QC, the variable qc will hold a pointer to the last element
> examined, which has the tag i == ATA_MAX_QUEUE - 1. This qc can match the port
> deferred QC (ap->deferred_qc).
> 
> If that happens, the condition qc == ap->deferred_qc evaluates to true despite
> the loop not breaking with a match on the SCSI command for this QC. In that
> case, the error handler mistakenly intercepts a command that not completed yet

Could even write:
"that has not been issued yet"

(while it is true that it has not been completed yet, it has not even been
issued to the drive yet.)


> and that has not timed out, and thus erroneously returning a timeout error.
> 
> Fix the problem by checking for i < ATA_MAX_QUEUE in addition to
> qc == ap->deferred_qc.
> 
> The problem was found by an experimental code review agent based on
> gemini-3.1-pro while reviewing backports into v6.18.y.
> 
> > Assisted-by: Gemini:gemini-3.1-pro
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Cc: Niklas Cassel <cassel@kernel.org>
> > Fixes: eddb98ad9364 ("ata: libata-eh: correctly handle deferred qc timeouts")
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> With the above, feel free to add:
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Thanks !
> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[Niklas Cassel]

On Thu, Mar 05, 2026 at 06:48:05PM -0800, Guenter Roeck wrote:
> If the `ata_qc_for_each_raw()` loop finishes without finding a matching
> `scmd`, `qc` will hold a pointer to the last element examined, which is
> accociated with `i == ATA_MAX_QUEUE - 1`. This element can match
> `ap->deferred_qc`. If that happens, the condition `qc == ap->deferred_qc`
> evaluates to true despite the loop not breaking on a scmd match.
> In that case, the error handler mistakenly intercepts a command that
> completed normally after an unrelated SCSI timeout, returning a timeout
> error instead of success.
>
> [...]

Applied commit log changes from Damien and myself.


Applied to libata/linux.git (libata-for-7.0-fixes), thanks!

[1/1] ata: libata-eh: Fix detection of deferred qc timeouts
      https://git.kernel.org/libata/linux/c/ee0e6e69

Kind regards,
Niklas

Re: [PATCH] ata: libata-eh: Fix detection of deferred qc timeouts

[Guenter Roeck]

On 3/6/26 01:05, Niklas Cassel wrote:
> On Thu, Mar 05, 2026 at 06:48:05PM -0800, Guenter Roeck wrote:
>> If the `ata_qc_for_each_raw()` loop finishes without finding a matching
>> `scmd`, `qc` will hold a pointer to the last element examined, which is
>> accociated with `i == ATA_MAX_QUEUE - 1`. This element can match
>> `ap->deferred_qc`. If that happens, the condition `qc == ap->deferred_qc`
>> evaluates to true despite the loop not breaking on a scmd match.
>> In that case, the error handler mistakenly intercepts a command that
>> completed normally after an unrelated SCSI timeout, returning a timeout
>> error instead of success.
>>
>> [...]
> 
> Applied commit log changes from Damien and myself.
> 
> 
> Applied to libata/linux.git (libata-for-7.0-fixes), thanks!
> 

Thanks!

Guenter