ATA port deferred qc fixes

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
  <TASK>
  __dump_stack lib/dump_stack.c:94 [inline]
  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
  process_scheduled_works kernel/workqueue.c:3358 [inline]
  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
  kthread+0x370/0x450 kernel/kthread.c:467
  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
  </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +0 -1

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&

Hi,

Quoted 57 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +4 -2

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 2 lines

drivers/ata/libata-eh.c +1 -1

-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE).

If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

Quoted 47 lines · Guenter Roeck

Hi,

Quoted 44 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24 shift
exponent 4210818301 is too large for 64-bit type 'long long unsigned
int' ... Call Trace: <TASK> __dump_stack lib/dump_stack.c:94 [inline] 
dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120 ubsan_epilogue+0xa/0x30
lib/ubsan.c:233 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/
ubsan.c:494 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166 
ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679 
process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275 
process_scheduled_works kernel/workqueue.c:3358 [inline] 
worker_thread+0x5da/0xe40 kernel/workqueue.c:3439 kthread+0x370/0x450
kernel/kthread.c:467 ret_from_fork+0x754/0xd80 arch/x86/kernel/
process.c:158 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245 
</TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command

starvation") Signed-off-by: Damien Le Moal <[email protected]> Reviewed-
by: Hannes Reinecke <[email protected]> Reviewed-by: Igor Pylypiv <[email protected]> --- drivers/ata/libata-eh.c | 22 ++++++++++++++++++ +--- 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c index renamed from drivers/ata/libata-eh.c +0 -0

72a22b6c9682..b373cceb95d2 100644 --- a/drivers/ata/libata-eh.c +++ b/ drivers/ata/libata-eh.c @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, set_host_byte(scmd, DID_OK);

ata_qc_for_each_raw(ap, qc, i) { - if (qc->flags & ATA_QCFLAG_ACTIVE
&& - qc->scsicmd == scmd) + if (qc->scsicmd != scmd) +
continue; + if ((qc->flags & ATA_QCFLAG_ACTIVE) || + qc == ap-

Quoted 1 line

deferred_qc) break; }

Quoted 8 lines

drivers/ata/libata-eh.c index

Quoted 1 line

drivers/ata/libata-eh.c index renamed from drivers/ata/libata-eh.c +0 -1

-		if (i < ATA_MAX_QUEUE) { +		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE).

I think the AI is wrong here.

That last element assigned to QC will be ATA_MAX_QUEUE - 1.

Quoted 7 lines

If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...

On NCQ error, we complete the deferred QC as a failed command.

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

So... I actually think that the change suggested by the AI is something we want. (Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)

Quoted 9 lines

Quoted 3 lines

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

The request for the qc/scsi command was started from the block layer perspective and so can still timeout. So this is all valid.

BUT ATA_MAX_QUEUE qc (last in the loop if there is no match) is the one reserved for internal commands issued from EH. Internal QCs do not go through the deferred issue path, so even without checking for the index when there is no match, we have:
- qc is still a valid pointer (the array of QCs is ATA_MAX_QUEUE + 1 sized)
- We can never have qc == ap->deferred_qc.

Quoted 13 lines · Damien Le Moal

Quoted 9 lines

Quoted 3 lines

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

The request for the qc/scsi command was started from the block layer perspective and so can still timeout. So this is all valid.

Quoted 6 lines

BUT ATA_MAX_QUEUE qc (last in the loop if there is no match) is the one reserved for internal commands issued from EH. Internal QCs do not go through the deferred issue path, so even without checking for the index when there is no match, we have:
- qc is still a valid pointer (the array of QCs is ATA_MAX_QUEUE + 1 sized)
- We can never have qc == ap->deferred_qc.

This second part of my comment is obviously wrong. I need more coffee :)

The first part stands: a deferred qc that has not been issued can timeout.

Quoted 66 lines · Guenter Roeck

Hi,

Quoted 49 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +0 -1

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 10 lines

drivers/ata/libata-eh.c +5 -2

-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 
-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Quoted 2 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE).

I think the AI is wrong here.

That last element assigned to QC will be ATA_MAX_QUEUE - 1.

Quoted 5 lines

If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Quoted 3 lines

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...

On NCQ error, we complete the deferred QC as a failed command.

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

So... I actually think that the change suggested by the AI is something we want.
(Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)

Quoted 67 lines · Niklas Cassel

Quoted 66 lines · Guenter Roeck

Hi,

Quoted 49 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +0 -1

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 10 lines

drivers/ata/libata-eh.c +5 -2

-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 
-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Quoted 7 lines

Quoted 2 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE).

I think the AI is wrong here.

That last element assigned to QC will be ATA_MAX_QUEUE - 1.

I think that is what it means with "qc will hold a pointer to the last element examined". The "(i == ATA_MAX_QUEUE) part is a bit confusing.

I think what it is trying to say is that if i == ATA_MAX_QUEUE, qc would point to the last examined element, which would not have ATA_QCFLAG_ACTIVE set because otherwise it would have exited the loop. Yet, ap->deferred_qc could be set, and the if statement would be true even though i == ATA_MAX_QUEUE and there was no qc match.

Quoted 5 lines

Quoted 5 lines

If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

That is pretty much much repeating what I said above, without the confusing "(i == ATA_MAX_QUEUE)" part.

Quoted 4 lines

Quoted 3 lines

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

This part is beyond my understanding, primarily because I don't know what "qc->deferred" actually refers to.

Quoted 11 lines

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...

On NCQ error, we complete the deferred QC as a failed command.

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

Quoted 3 lines

So... I actually think that the change suggested by the AI is something we want.
(Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)

So should I send a patch, or do you want to handle it ? It might be better if you handle it since I don't know how to exactly describe the problem differently than the AI.

Thanks,
Guenter

Quoted 96 lines · Guenter Roeck

Quoted 67 lines · Niklas Cassel

Quoted 66 lines · Guenter Roeck

Hi,

Quoted 49 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +0 -1

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 10 lines

drivers/ata/libata-eh.c +5 -2

-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 
-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Quoted 7 lines

Quoted 2 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE).

I think the AI is wrong here.

That last element assigned to QC will be ATA_MAX_QUEUE - 1.

I think that is what it means with "qc will hold a pointer to the last element examined". The "(i == ATA_MAX_QUEUE) part is a bit confusing.

I think what it is trying to say is that if i == ATA_MAX_QUEUE, qc would point to the last examined element, which would not have ATA_QCFLAG_ACTIVE set because otherwise it would have exited the loop. Yet, ap->deferred_qc could be set, and the if statement would be true even though i == ATA_MAX_QUEUE and there was no qc match.

Quoted 5 lines

Quoted 5 lines

If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

That is pretty much much repeating what I said above, without the confusing "(i == ATA_MAX_QUEUE)" part.

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command?

This part is beyond my understanding, primarily because I don't know what "qc->deferred" actually refers to.

There are 2 types of ATA commands: queueable ones (NCQ == Native Command Queueing) and non-queueable ones (legacy/old ATA commands). The 2 types cannot be mixed. When NCQ commands are on-going, you cannot issue a non-NCQ command, and vice-versa. This has always been handled with command requeueing in libata-scsi (since forever), but with blk-mq introduction, there was a potential command starvation issue for non-NCQ commands that has existed for a long time.

We fixed that recently by keeping on hand any non-NCQ command that must wait for on-going NCQ commands to complete first. This is ap->deferred_qc.

Quoted 15 lines

Quoted 14 lines

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...

On NCQ error, we complete the deferred QC as a failed command.

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

So... I actually think that the change suggested by the AI is something we want.
(Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)

Quoted 3 lines

So should I send a patch, or do you want to handle it ? It might be better if you handle it since I don't know how to exactly describe the problem differently than the AI.

Send a patch. Write a commit message based on the information I sent in my previous email. We can correct the commit message if needed.

Hi,

Quoted 57 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +4 -2

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 2 lines

drivers/ata/libata-eh.c +1 -1

-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE). If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command? Should we verify that the loop actually found a match, for instance by checking if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)?

It does seem to be a real problem to me, but I don't know the code well enough to be sure. Please take a look and let me know if the problem is real. If so, I'll be happy to submit a patch to fix it. If not, please let me know what the agent is missing.

Quoted 60 lines · Guenter Roeck

Hi,

Quoted 57 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
  <TASK>
  __dump_stack lib/dump_stack.c:94 [inline]
  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
  process_scheduled_works kernel/workqueue.c:3358 [inline]
  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
  kthread+0x370/0x450 kernel/kthread.c:467
  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
  </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +4 -2

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
  		set_host_byte(scmd, DID_OK);
 
  		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
  				break;
  		}
 

Quoted 5 lines

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 2 lines

drivers/ata/libata-eh.c +1 -1

-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Thanks for that.

Quoted 11 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE). If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command? Should we verify that the loop actually found a match, for instance by checking if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)?

Yeah. Something like this. But that condition cannot actually happen. i == ATA_MAX_QUEUE correspond to internal QCs, and these never go through the deferred path.

Quoted 4 lines

It does seem to be a real problem to me, but I don't know the code well enough to be sure. Please take a look and let me know if the problem is real. If so, I'll be happy to submit a patch to fix it. If not, please let me know what the agent is missing.

See above. That is not a real problem, but it would still be good to check. I think your change is fine. Car to send a proper patch ?

Quoted 81 lines · Damien Le Moal

Quoted 60 lines · Guenter Roeck

Hi,

Quoted 57 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
  <TASK>
  __dump_stack lib/dump_stack.c:94 [inline]
  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
  process_scheduled_works kernel/workqueue.c:3358 [inline]
  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
  kthread+0x370/0x450 kernel/kthread.c:467
  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
  </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +4 -2

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
  		set_host_byte(scmd, DID_OK);
 
  		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
  				break;
  		}
 

Quoted 5 lines

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 2 lines

drivers/ata/libata-eh.c +1 -1

-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Thanks for that.

Quoted 11 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE). If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command? Should we verify that the loop actually found a match, for instance by checking if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)?

Quoted 4 lines

Yeah. Something like this. But that condition cannot actually happen. i == ATA_MAX_QUEUE correspond to internal QCs, and these never go through the deferred path.

Pardon my ignorance, but doesn't i == ATA_MAX_QUEUE mean that qc points to the _previously_ examined qc, i.e., the one associated with i == ATA_MAX_QUEUE - 1 ? Are you saying that this qc will always always be internal and never match ap->deferred_qc ?

Quoted 8 lines

Quoted 4 lines

It does seem to be a real problem to me, but I don't know the code well enough to be sure. Please take a look and let me know if the problem is real. If so, I'll be happy to submit a patch to fix it. If not, please let me know what the agent is missing.

See above. That is not a real problem, but it would still be good to check. I think your change is fine. Car to send a proper patch ?

I can try, but if the AI analysis is missing the point I don't really know how to describe/explain it. Can you give me some guidance ?

Thanks,
Guenter

Quoted 87 lines · Guenter Roeck

Quoted 81 lines · Damien Le Moal

Quoted 60 lines · Guenter Roeck

Hi,

Quoted 57 lines · Damien Le Moal

A deferred qc may timeout while waiting for the device queue to drain to be submitted. In such case, since the qc is not active, ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which frees the qc. But as the port deferred_qc field still references this finished/freed qc, the deferred qc work may eventually attempt to call ata_qc_issue() against this invalid qc, leading to errors such as reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
  <TASK>
  __dump_stack lib/dump_stack.c:94 [inline]
  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
  process_scheduled_works kernel/workqueue.c:3358 [inline]
  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
  kthread+0x370/0x450 kernel/kthread.c:467
  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
  </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred one, and in such case, clear the port deferred_qc field and finish the SCSI command with DID_TIME_OUT.

Reported-by [email protected]

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

---
  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

drivers/ata/libata-eh.c +4 -2

index 72a22b6c9682..b373cceb95d2 100644
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
  		set_host_byte(scmd, DID_OK);
 
  		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
  				break;
  		}
 

Quoted 5 lines

drivers/ata/libata-eh.c void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, · line 650

Quoted 2 lines

drivers/ata/libata-eh.c +1 -1

-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following comment.

Thanks for that.

Quoted 11 lines

If the ata_qc_for_each_raw() loop finishes without finding a matching scmd, qc will hold a pointer to the last element examined (i == ATA_MAX_QUEUE). If this last element happens to be ap->deferred_qc, the condition qc == ap->deferred_qc evaluates to true despite the loop not breaking on a match.

Could this mistakenly intercept a command that completed normally after a SCSI timeout, returning a timeout error instead of success? Would this also incorrectly clear ap->deferred_qc, dropping the deferred command? Should we verify that the loop actually found a match, for instance by checking if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)?

Quoted 4 lines

Yeah. Something like this. But that condition cannot actually happen. i == ATA_MAX_QUEUE correspond to internal QCs, and these never go through the deferred path.

Quoted 4 lines

Pardon my ignorance, but doesn't i == ATA_MAX_QUEUE mean that qc points to the _previously_ examined qc, i.e., the one associated with i == ATA_MAX_QUEUE - 1 ? Are you saying that this qc will always always be internal and never match ap->deferred_qc ?

Arg !!!! Yes ! I screwed up. Checking ata_qc_for_each_raw(), the tag is checked before setting the qc pointer. So yes, you are absolutely correct, and we have a problem.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

It does seem to be a real problem to me, but I don't know the code well enough to be sure. Please take a look and let me know if the problem is real. If so, I'll be happy to submit a patch to fix it. If not, please let me know what the agent is missing.

See above. That is not a real problem, but it would still be good to check. I think your change is fine. Car to send a proper patch ?

Quoted 2 lines

I can try, but if the AI analysis is missing the point I don't really know how to describe/explain it. Can you give me some guidance ?

I think the AI is correct. That "if" without the index check can lead to false positives and we can endup timeout-failing a deferred qc that has not timed out yet.

Send a patch please. As mentioned, we can touch-up the commit message if needed.

Quoted 3 lines

I can try, but if the AI analysis is missing the point I don't really know how to describe/explain it. Can you give me some guidance ?

I think the AI is correct. That "if" without the index check can lead to false positives and we can endup timeout-failing a deferred qc that has not timed out yet.

Quoted 6 lines

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

So... I actually think that the change suggested by the AI is something we want.

cancel_work_sync() is a sleeping function so it cannot be called with the spin lock of a port being held. Move the call to this function in ata_port_detach() after EH completes, with the port lock released, together with other work cancellation calls.

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-core.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

cancel_work_sync() is a sleeping function so it cannot be called with the spin lock of a port being held. Move the call to this function in ata_port_detach() after EH completes, with the port lock released, together with other work cancellation calls.

Fixes 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

Signed-off-by Damien Le Moal <[email protected]>

The first patch addresses a use-after-free issue when a deferred qc times out. The second patch avoids a call to a potentially sleeping function while a port spinlock is held.

Changes from v1:
- Corrected typo in patch 1 message, improved comment in code and added a WARN_ON_ONCE() call to verify that a timed out qc is not active.
- Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the port lock held. This call is in fact removed: it is not needed as ata_scsi_requeue_deferred_qc() is called in EH, which is always run when removing a port.

Damien Le Moal (2):
ata: libata-eh: correctly handle deferred qc timeouts
ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

The first patch addresses a use-after-free issue when a deferred qc times out. The second patch avoids a call to a potentially sleeping function while a port spinlock is held.

Changes from v1:
- Corrected typo in patch 1 message, improved comment in code and added a WARN_ON_ONCE() call to verify that a timed out qc is not active.
- Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the port lock held. This call is in fact removed: it is not needed as ata_scsi_requeue_deferred_qc() is called in EH, which is always run when removing a port.

Damien Le Moal (2):
ata: libata-eh: correctly handle deferred qc timeouts
ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS

Quoted 12 lines · Damien Le Moal

The first patch addresses a use-after-free issue when a deferred qc times out. The second patch avoids a call to a potentially sleeping function while a port spinlock is held.

Changes from v1:
- Corrected typo in patch 1 message, improved comment in code and added a WARN_ON_ONCE() call to verify that a timed out qc is not active.
- Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the port lock held. This call is in fact removed: it is not needed as ata_scsi_requeue_deferred_qc() is called in EH, which is always run when removing a port.

Quoted 8 lines

Damien Le Moal (2):
ata: libata-eh: correctly handle deferred qc timeouts
ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

Hello, this is my first message on the LKML so please forgive me for likely not following etiquette.

I have what might be a regression from somewhere in the recent series of NCQ / QC patches.

I have a peculiar setup that might reveal the source of the bug. I'm using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1 SATA port to 5 drives.
The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda. The platform does not (yet) support FIS-Based Switching (FBS), so it is using Command-Based Switching (CBS). FBS support may land in Linux 7.1.

Quoted 2 lines

kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS

When switching from kernel 6.18.13 to 6.19.7, drive access became extremely slow, causing lock timeouts at the filesystem level. I solved the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.

I also looked through the recent LPM patches, but the drives' performance didn't improve after changing LPM settings for the scsi host in sysfs.

Let me know if I need to provide more details or logs. I can see COMRESET a lot in each device's SMART logs but they aren't timestamped so I haven't proved that they are recent. Perhaps the slow drive switching is causing command starvation.

Thank you,
Tommy

Quoted 12 lines · Damien Le Moal

The first patch addresses a use-after-free issue when a deferred qc times out. The second patch avoids a call to a potentially sleeping function while a port spinlock is held.

Changes from v1:
- Corrected typo in patch 1 message, improved comment in code and added a WARN_ON_ONCE() call to verify that a timed out qc is not active.
- Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the port lock held. This call is in fact removed: it is not needed as ata_scsi_requeue_deferred_qc() is called in EH, which is always run when removing a port.

Quoted 8 lines

Damien Le Moal (2):
ata: libata-eh: correctly handle deferred qc timeouts
ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

Hello, this is my first message on the LKML so please forgive me for likely not following etiquette.

I have what might be a regression from somewhere in the recent series of NCQ / QC patches.

I have a peculiar setup that might reveal the source of the bug. I'm using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1 SATA port to 5 drives.
The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda. The platform does not (yet) support FIS-Based Switching (FBS), so it is using Command-Based Switching (CBS). FBS support may land in Linux 7.1.

Quoted 2 lines

kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS

When switching from kernel 6.18.13 to 6.19.7, drive access became extremely slow, causing lock timeouts at the filesystem level. I solved the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.

I also looked through the recent LPM patches, but the drives' performance didn't improve after changing LPM settings for the scsi host in sysfs.

Let me know if I need to provide more details or logs. I can see COMRESET a lot in each device's SMART logs but they aren't timestamped so I haven't proved that they are recent. Perhaps the slow drive switching is causing command starvation.

Quoted 43 lines · Tommy Kelly

Quoted 12 lines · Damien Le Moal

The first patch addresses a use-after-free issue when a deferred qc times out. The second patch avoids a call to a potentially sleeping function while a port spinlock is held.

Changes from v1:
- Corrected typo in patch 1 message, improved comment in code and added a WARN_ON_ONCE() call to verify that a timed out qc is not active.
- Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the port lock held. This call is in fact removed: it is not needed as ata_scsi_requeue_deferred_qc() is called in EH, which is always run when removing a port.

Quoted 8 lines

Damien Le Moal (2):
ata: libata-eh: correctly handle deferred qc timeouts
ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

Hello, this is my first message on the LKML so please forgive me for likely not following etiquette.

I have what might be a regression from somewhere in the recent series of NCQ / QC patches.

I have a peculiar setup that might reveal the source of the bug. I'm using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1 SATA port to 5 drives.
The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda. The platform does not (yet) support FIS-Based Switching (FBS), so it is using Command-Based Switching (CBS). FBS support may land in Linux 7.1.

Quoted 2 lines

kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS

When switching from kernel 6.18.13 to 6.19.7, drive access became extremely slow, causing lock timeouts at the filesystem level. I solved the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.

Quoted 8 lines

I also looked through the recent LPM patches, but the drives' performance didn't improve after changing LPM settings for the scsi host in sysfs.

Let me know if I need to provide more details or logs. I can see COMRESET a lot in each device's SMART logs but they aren't timestamped so I haven't proved that they are recent. Perhaps the slow drive switching is causing command starvation.

Hello Tommy,

Igor recently found a bug in the deferred QC feature:
https://lore.kernel.org/linux-ide/[email protected]/

His fix patch has been queued up on libata/for-next, so perhaps you could try the libata/for-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next

And see if you can still reproduce the problem there.

Kind regards,
Niklas

SUBSYSTEM=="scsi", DRIVERS=="sd", ATTR{queue_depth}="1"

(udev-worker)[549]: sdb: Spawned process 'ata_id --export /dev/sdb' [635] is taking longer than 59s to complete. systemd-udevd[473]: sda: Worker [551] processing SEQNUM=3406 is taking a long time. systemd-udevd[473]: sdd: Worker [542] processing SEQNUM=3414 is taking a long time. systemd-udevd[473]: sdc: Worker [562] processing SEQNUM=3408 is taking a long time. systemd-udevd[473]: sde: Worker [550] processing SEQNUM=3412 is taking a long time. systemd-udevd[473]: sdb: Worker [549] processing SEQNUM=3410 is taking a long time. systemd[1]: dev-sdc.device: Job dev-sdc.device/start timed out.
systemd[1]: Timed out waiting for device dev-sdc.device - /dev/sdc.
systemd[1]: dev-sdc.device: Job dev-sdc.device/start failed with result 'timeout'.
systemd[1]: Unnecessary job was removed for dev-sda.device - /dev/sda.
systemd[1]: Unnecessary job was removed for dev-sdd.device - /dev/sdd.
systemd[1]: Unnecessary job was removed for dev-sdb.device - /dev/sdb.
systemd[1]: Unnecessary job was removed for dev-sde.device - /dev/sde.

systemd[1]: dev-sde.device: Job dev-sde.device/start timed out. systemd[1]: Timed out waiting for device dev-sde.device - /dev/sde. systemd[1]: dev-sde.device: Job dev-sde.device/start failed with result 'timeout'. systemd-udevd[856]: sde: Worker [869] processing SEQNUM=3696 killed. systemd-udevd[856]: sdd: Worker [871] processing SEQNUM=3693 killed. systemd-udevd[856]: sdc: Worker [875] processing SEQNUM=3690 killed. systemd-udevd[856]: sdc: Worker [875] terminated by signal 9 (KILL). systemd-udevd[856]: sde: Worker [869] terminated by signal 9 (KILL).

pcp-pmie[1784]: High per disk average queue length 6280%await[sdd]@hostname

kernel: ahci-dwc fc800000.sata: supply ahci not found, using dummy regulator
kernel: ahci-dwc fc800000.sata: supply phy not found, using dummy regulator
kernel: ahci-dwc fc800000.sata: supply target not found, using dummy regulator
kernel: ahci-dwc fc800000.sata: PMPn is limited up to 5 ports
kernel: ahci-dwc fc800000.sata: forcing port_map 0x0 -> 0x1
kernel: ahci-dwc fc800000.sata: AHCI vers 0001.0300, 32 command slots, 6 Gbps, platform mode
kernel: ahci-dwc fc800000.sata: 1/1 ports implemented (port mask 0x1)
kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS
kernel: scsi host0: ahci-dwc
kernel: ata1: SATA max UDMA/133 mmio [mem 0xfc800000-0xfc800fff] port 0x100 irq 77 lpm-pol 0
kernel: ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
kernel: ata1.15: Port Multiplier 1.2, 0x197b:0x5755 r0, 5 ports, feat 0x5/0xf
kernel: ata1.03: hard resetting link
kernel: ata1.03: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
kernel: ata1.03: ATA-10: ADATA SU655, V8X01c62, max UDMA/133
kernel: ata1.03: 468862128 sectors, multi 1: LBA48 NCQ (depth 32)
kernel: ata1.03: Features: DIPM
kernel: ata1.03: configured for UDMA/133

kernel: scsi 0:3:0:0: Direct-Access ATA ADATA SU655 1c62 PQ: 0 ANSI: 5
kernel: sd 0:3:0:0: Attached scsi generic sg3 type 0
kernel: sd 0:3:0:0: [sdd] 468862128 512-byte logical blocks: (240 GB/224 GiB)
kernel: sd 0:3:0:0: [sdd] Write Protect is off
kernel: sd 0:3:0:0: [sdd] Mode Sense: 00 3a 00 00
kernel: sd 0:3:0:0: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
kernel: sd 0:3:0:0: [sdd] Preferred minimum I/O size 512 bytes
kernel: sd 0:3:0:0: [sdd] Attached SCSI disk

Hi Niklas and Damien,

Thanks for the responses.
I managed to build libata/for-next into rpms for my Fedora CoreOS system.

I am new to this, but perhaps linux is misunderstanding the queue depth.
There are "32 command slots" for the PMP, but each drive advertises "NCQ (depth 32)" as well, but perhaps there is only actually 32/5=6 slots per drive. Not sure if that matters wrt the bug.

The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda. The platform does not (yet) support FIS-Based Switching (FBS), so it is using Command-Based Switching (CBS). FBS support may land in Linux 7.1.