* [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands
@ 2026-10-27 18:01 Dmitry Bogdanov
2026-10-27 18:01 ` [PATCH 1/2] Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands" Dmitry Bogdanov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Dmitry Bogdanov @ 2026-10-27 18:01 UTC (permalink / raw)
To: Martin Petersen, Maurizio Lombardi, Laurence Oberman,
target-devel
Cc: michael.christie, linux-scsi, linux, Dmitry Bogdanov
ISCSI has some old leftovers in the code that prevents passing the aborted
command to the target core to make the actual abort. Such commands hang
until connection is closed.
So remove those leftovers.
Dmitry Bogdanov (2):
Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING
commands"
scsi: target: iscsi: fix hanging WRITE_PENDING commands
drivers/target/iscsi/iscsi_target.c | 19 -------------------
drivers/target/iscsi/iscsi_target_erl1.c | 8 --------
2 files changed, 27 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands" 2026-10-27 18:01 [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands Dmitry Bogdanov @ 2026-10-27 18:01 ` Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands Dmitry Bogdanov 2026-10-27 20:08 ` [PATCH 0/2] target: iscsi: fix hanged " Laurence Oberman 2 siblings, 0 replies; 8+ messages in thread From: Dmitry Bogdanov @ 2026-10-27 18:01 UTC (permalink / raw) To: Martin Petersen, Maurizio Lombardi, Laurence Oberman, target-devel Cc: michael.christie, linux-scsi, linux, Dmitry Bogdanov This reverts commit d5869dae5080e976d4b03cc33eb7ceb527f242bf as it introduces double completion of the command and other issues. Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> --- drivers/target/iscsi/iscsi_target.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 124ff269b8e75..62ada3a52210e 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1533,10 +1533,8 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf, */ if (se_cmd->transport_state & CMD_T_ABORTED) { if (hdr->flags & ISCSI_FLAG_CMD_FINAL && - --cmd->outstanding_r2ts < 1) { + --cmd->outstanding_r2ts < 1) iscsit_stop_dataout_timer(cmd); - target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED); - } return iscsit_dump_data_payload(conn, payload_length, 1); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands 2026-10-27 18:01 [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 1/2] Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands" Dmitry Bogdanov @ 2026-10-27 18:01 ` Dmitry Bogdanov 2026-10-27 18:18 ` sashiko-bot 2026-10-27 20:08 ` [PATCH 0/2] target: iscsi: fix hanged " Laurence Oberman 2 siblings, 1 reply; 8+ messages in thread From: Dmitry Bogdanov @ 2026-10-27 18:01 UTC (permalink / raw) To: Martin Petersen, Maurizio Lombardi, Laurence Oberman, target-devel Cc: michael.christie, linux-scsi, linux, Dmitry Bogdanov When Data comes for an aborted WRITE command in WRITE_PENDING state it is silently received and the command does not pass to the target. That leads to hanging of other session with LUN_RESET aborting that command. Pass the aborted WRITE commands when all Data has been gathered. Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> --- drivers/target/iscsi/iscsi_target.c | 17 ----------------- drivers/target/iscsi/iscsi_target_erl1.c | 8 -------- 2 files changed, 25 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 62ada3a52210e..03d618cd76721 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1518,23 +1518,6 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf, if (hdr->flags & ISCSI_FLAG_CMD_FINAL) iscsit_stop_dataout_timer(cmd); - return iscsit_dump_data_payload(conn, payload_length, 1); - } - } else { - /* - * For the normal solicited data path: - * - * Check for a delayed TASK_ABORTED status and dump any - * incoming data out payload if one exists. Also, when the - * ISCSI_FLAG_CMD_FINAL is set to denote the end of the current - * data out sequence, we decrement outstanding_r2ts. Once - * outstanding_r2ts reaches zero, go ahead and send the delayed - * TASK_ABORTED status. - */ - if (se_cmd->transport_state & CMD_T_ABORTED) { - if (hdr->flags & ISCSI_FLAG_CMD_FINAL && - --cmd->outstanding_r2ts < 1) - iscsit_stop_dataout_timer(cmd); return iscsit_dump_data_payload(conn, payload_length, 1); } diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c index d8ca06e697d63..44b851e4a602d 100644 --- a/drivers/target/iscsi/iscsi_target_erl1.c +++ b/drivers/target/iscsi/iscsi_target_erl1.c @@ -908,8 +908,6 @@ int iscsit_execute_cmd(struct iscsit_cmd *cmd, int ooo) return 0; } spin_unlock_bh(&cmd->istate_lock); - if (cmd->se_cmd.transport_state & CMD_T_ABORTED) - return 0; return transport_send_check_condition_and_sense(se_cmd, cmd->sense_reason, 0); } @@ -927,9 +925,6 @@ int iscsit_execute_cmd(struct iscsit_cmd *cmd, int ooo) if (!(cmd->cmd_flags & ICF_NON_IMMEDIATE_UNSOLICITED_DATA)) { - if (cmd->se_cmd.transport_state & CMD_T_ABORTED) - return 0; - iscsit_set_dataout_sequence_values(cmd); conn->conn_transport->iscsit_get_dataout(conn, cmd, false); } @@ -942,9 +937,6 @@ int iscsit_execute_cmd(struct iscsit_cmd *cmd, int ooo) if ((cmd->data_direction == DMA_TO_DEVICE) && !(cmd->cmd_flags & ICF_NON_IMMEDIATE_UNSOLICITED_DATA)) { - if (cmd->se_cmd.transport_state & CMD_T_ABORTED) - return 0; - iscsit_set_unsolicited_dataout(cmd); } return target_submit(&cmd->se_cmd); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands 2026-10-27 18:01 ` [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands Dmitry Bogdanov @ 2026-10-27 18:18 ` sashiko-bot 2026-10-27 22:55 ` Dmitry Bogdanov 0 siblings, 1 reply; 8+ messages in thread From: sashiko-bot @ 2026-10-27 18:18 UTC (permalink / raw) To: Dmitry Bogdanov, target-devel; +Cc: linux-scsi Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] scsi: target: iscsi: double completion in iscsit_release_commands_from_conn() -- commit 31a87057639fce26dc0f2f4bbcd6ddacf96b85a5 Author: Dmitry Bogdanov <d.bogdanov@yadro.com> scsi: target: iscsi: fix hanging WRITE_PENDING commands This commit removes early CMD_T_ABORTED checks in the iSCSI Data-Out and command execution paths so that aborted WRITE commands in WRITE_PENDING state are passed to the target core once all Data-Out PDUs have been received. > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index 62ada3a52210e..03d618cd76721 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -1518,23 +1518,6 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf, > if (hdr->flags & ISCSI_FLAG_CMD_FINAL) > iscsit_stop_dataout_timer(cmd); > > - return iscsit_dump_data_payload(conn, payload_length, 1); > - } > - } else { > - /* > - * For the normal solicited data path: > - * > - * Check for a delayed TASK_ABORTED status and dump any > - * incoming data out payload if one exists. Also, when the > - * ISCSI_FLAG_CMD_FINAL is set to denote the end of the current > - * data out sequence, we decrement outstanding_r2ts. Once > - * outstanding_r2ts reaches zero, go ahead and send the delayed > - * TASK_ABORTED status. > - */ > - if (se_cmd->transport_state & CMD_T_ABORTED) { > - if (hdr->flags & ISCSI_FLAG_CMD_FINAL && > - --cmd->outstanding_r2ts < 1) > - iscsit_stop_dataout_timer(cmd); [Severity: High] Could passing an aborted WRITE_PENDING command to target_execute_cmd() cause a double abort completion and use-after-free if the connection closes afterward in iscsit_release_commands_from_conn()? With this check removed in __iscsit_check_dataout_hdr(), an aborted WRITE command in TRANSPORT_WRITE_PENDING state receives its remaining Data-Out PDUs and calls target_execute_cmd() in iscsit_check_dataout_payload() (or in iscsit_execute_cmd() for out-of-order CmdSNs): drivers/target/iscsi/iscsi_target.c:iscsit_check_dataout_payload() { ... iscsit_stop_dataout_timer(cmd); if (ooo_cmdsn) return 0; target_execute_cmd(&cmd->se_cmd); return 0; ... } In target_execute_cmd(), target_cmd_interrupted() is called before cmd->t_state is updated to TRANSPORT_PROCESSING: drivers/target/target_core_transport.c:target_execute_cmd() { if (target_cmd_interrupted(cmd)) return; spin_lock_irq(&cmd->t_state_lock); cmd->t_state = TRANSPORT_PROCESSING; cmd->transport_state |= CMD_T_ACTIVE | CMD_T_SENT; spin_unlock_irq(&cmd->t_state_lock); ... } Because CMD_T_ABORTED is set, target_cmd_interrupted() queues target_abort_work on target_completion_wq and returns true without changing cmd->t_state: drivers/target/target_core_transport.c:target_cmd_interrupted() { ... if (cmd->transport_state & CMD_T_ABORTED) { if (cmd->transport_complete_callback) cmd->transport_complete_callback(cmd, false, &post_ret); INIT_WORK(&cmd->work, target_abort_work); queue_work(target_completion_wq, &cmd->work); return true; } ... } As a result, cmd->se_cmd.t_state remains TRANSPORT_WRITE_PENDING. When CMD_T_TAS is set (for example, when aborted by a LUN_RESET on another session with emulate_tas enabled), target_handle_abort() queues SAM_STAT_TASK_ABORTED status and drops one cmd_kref reference via transport_cmd_check_stop_to_fabric(), leaving the command on conn->conn_cmd_list in TRANSPORT_WRITE_PENDING state until ExpStatSN acknowledges the status (or, when CMD_T_TAS is not set, until target_abort_work runs iscsit_aborted_task()). If the connection closes while the command is still on conn->conn_cmd_list, iscsit_release_commands_from_conn() checks for TRANSPORT_WRITE_PENDING: drivers/target/iscsi/iscsi_target.c:iscsit_release_commands_from_conn() { ... if (cmd->se_cmd.t_state == TRANSPORT_WRITE_PENDING) { /* * We never submitted the cmd to LIO core, so we have * to tell LIO to perform the completion process. */ spin_unlock_irq(&se_cmd->t_state_lock); target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED); continue; } ... } Wouldn't this call target_complete_cmd() -> target_cmd_interrupted() a second time on the same command, re-initializing and re-queueing cmd->work for target_abort_work while iscsit_release_commands_from_conn() also frees the command via iscsit_free_cmd(), leading to workqueue corruption, cmd_kref underflow, and use-after-free of struct iscsit_cmd? -- Sashiko AI review · https://sashiko.dev/#/patchset/20261027180123.13163-1-d.bogdanov@yadro.com?part=2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands 2026-10-27 18:18 ` sashiko-bot @ 2026-10-27 22:55 ` Dmitry Bogdanov 0 siblings, 0 replies; 8+ messages in thread From: Dmitry Bogdanov @ 2026-10-27 22:55 UTC (permalink / raw) To: sashiko-reviews; +Cc: target-devel, linux-scsi On Tue, Oct 27, 2026 at 06:18:12PM +0000, sashiko-bot@kernel.org wrote: > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [High] scsi: target: iscsi: double completion in iscsit_release_commands_from_conn() > -- > > commit 31a87057639fce26dc0f2f4bbcd6ddacf96b85a5 > Author: Dmitry Bogdanov <d.bogdanov@yadro.com> > > scsi: target: iscsi: fix hanging WRITE_PENDING commands > > This commit removes early CMD_T_ABORTED checks in the iSCSI Data-Out and > command execution paths so that aborted WRITE commands in WRITE_PENDING state > are passed to the target core once all Data-Out PDUs have been received. > > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > > index 62ada3a52210e..03d618cd76721 100644 > > --- a/drivers/target/iscsi/iscsi_target.c > > +++ b/drivers/target/iscsi/iscsi_target.c > > @@ -1518,23 +1518,6 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf, > > if (hdr->flags & ISCSI_FLAG_CMD_FINAL) > > iscsit_stop_dataout_timer(cmd); > > > > - return iscsit_dump_data_payload(conn, payload_length, 1); > > - } > > - } else { > > - /* > > - * For the normal solicited data path: > > - * > > - * Check for a delayed TASK_ABORTED status and dump any > > - * incoming data out payload if one exists. Also, when the > > - * ISCSI_FLAG_CMD_FINAL is set to denote the end of the current > > - * data out sequence, we decrement outstanding_r2ts. Once > > - * outstanding_r2ts reaches zero, go ahead and send the delayed > > - * TASK_ABORTED status. > > - */ > > - if (se_cmd->transport_state & CMD_T_ABORTED) { > > - if (hdr->flags & ISCSI_FLAG_CMD_FINAL && > > - --cmd->outstanding_r2ts < 1) > > - iscsit_stop_dataout_timer(cmd); > > [Severity: High] > Could passing an aborted WRITE_PENDING command to target_execute_cmd() cause > a double abort completion and use-after-free if the connection closes afterward > in iscsit_release_commands_from_conn()? Yes, it can cause. But my patch neither was intended to address that problem neither increases its probability. WRITE_PENDING always may be aborted, in parallel from other session, right before target_cmd_interrupted() in target_execute_cmd(). And that is easily fixed by changing cmd->t_state in target_execute_cmd() before target_cmd_interrupted() to take the cmd ownership by the target core: ----------------- diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 2c9f4b58c024f..14ea1a9d7fa40 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2561,6 +2561,10 @@ static bool target_handle_task_attr(struct se_cmd *cmd) void target_execute_cmd(struct se_cmd *cmd) { trace_target_execute_cmd(cmd); + + spin_lock_irq(&cmd->t_state_lock); + cmd->t_state = TRANSPORT_PROCESSING; + spin_unlock_irq(&cmd->t_state_lock); /* * Determine if frontend context caller is requesting the stopping of * this command for frontend exceptions. @@ -2571,7 +2575,6 @@ void target_execute_cmd(struct se_cmd *cmd) return; spin_lock_irq(&cmd->t_state_lock); - cmd->t_state = TRANSPORT_PROCESSING; cmd->transport_state |= CMD_T_ACTIVE | CMD_T_SENT; spin_unlock_irq(&cmd->t_state_lock); --------------- > > With this check removed in __iscsit_check_dataout_hdr(), an aborted WRITE > command in TRANSPORT_WRITE_PENDING state receives its remaining Data-Out PDUs > and calls target_execute_cmd() in iscsit_check_dataout_payload() (or in > iscsit_execute_cmd() for out-of-order CmdSNs): > > drivers/target/iscsi/iscsi_target.c:iscsit_check_dataout_payload() { > ... > iscsit_stop_dataout_timer(cmd); > if (ooo_cmdsn) > return 0; > target_execute_cmd(&cmd->se_cmd); > return 0; > ... > } > > In target_execute_cmd(), target_cmd_interrupted() is called before > cmd->t_state is updated to TRANSPORT_PROCESSING: > > drivers/target/target_core_transport.c:target_execute_cmd() { > if (target_cmd_interrupted(cmd)) > return; > > spin_lock_irq(&cmd->t_state_lock); > cmd->t_state = TRANSPORT_PROCESSING; > cmd->transport_state |= CMD_T_ACTIVE | CMD_T_SENT; > spin_unlock_irq(&cmd->t_state_lock); > ... > } > > Because CMD_T_ABORTED is set, target_cmd_interrupted() queues > target_abort_work on target_completion_wq and returns true without changing > cmd->t_state: > > drivers/target/target_core_transport.c:target_cmd_interrupted() { > ... > if (cmd->transport_state & CMD_T_ABORTED) { > if (cmd->transport_complete_callback) > cmd->transport_complete_callback(cmd, false, &post_ret); > INIT_WORK(&cmd->work, target_abort_work); > queue_work(target_completion_wq, &cmd->work); > return true; > } > ... > } > > As a result, cmd->se_cmd.t_state remains TRANSPORT_WRITE_PENDING. When > CMD_T_TAS is set (for example, when aborted by a LUN_RESET on another session > with emulate_tas enabled), target_handle_abort() queues SAM_STAT_TASK_ABORTED > status and drops one cmd_kref reference via > transport_cmd_check_stop_to_fabric(), leaving the command on > conn->conn_cmd_list in TRANSPORT_WRITE_PENDING state until ExpStatSN > acknowledges the status (or, when CMD_T_TAS is not set, until > target_abort_work runs iscsit_aborted_task()). > > If the connection closes while the command is still on conn->conn_cmd_list, > iscsit_release_commands_from_conn() checks for TRANSPORT_WRITE_PENDING: > > drivers/target/iscsi/iscsi_target.c:iscsit_release_commands_from_conn() { > ... > if (cmd->se_cmd.t_state == TRANSPORT_WRITE_PENDING) { > /* > * We never submitted the cmd to LIO core, so we have > * to tell LIO to perform the completion process. > */ > spin_unlock_irq(&se_cmd->t_state_lock); > target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED); > continue; > } > ... > } > > Wouldn't this call target_complete_cmd() -> target_cmd_interrupted() a second > time on the same command, re-initializing and re-queueing cmd->work for > target_abort_work while iscsit_release_commands_from_conn() also frees the > command via iscsit_free_cmd(), leading to workqueue corruption, cmd_kref > underflow, and use-after-free of struct iscsit_cmd? > > -- > Sashiko AI review · https://sashiko.dev/#/patchset/20261027180123.13163-1-d.bogdanov@yadro.com?part=2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands 2026-10-27 18:01 [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 1/2] Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands" Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands Dmitry Bogdanov @ 2026-10-27 20:08 ` Laurence Oberman 2026-10-28 14:02 ` Dmitry Bogdanov 2 siblings, 1 reply; 8+ messages in thread From: Laurence Oberman @ 2026-10-27 20:08 UTC (permalink / raw) To: Dmitry Bogdanov, Martin Petersen, Maurizio Lombardi, target-devel Cc: michael.christie, linux-scsi, linux On Tue, 2026-10-27 at 21:01 +0300, Dmitry Bogdanov wrote: > ISCSI has some old leftovers in the code that prevents passing the > aborted > command to the target core to make the actual abort. Such commands > hang > until connection is closed. > So remove those leftovers. > > Dmitry Bogdanov (2): > Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING > commands" > scsi: target: iscsi: fix hanging WRITE_PENDING commands > > drivers/target/iscsi/iscsi_target.c | 19 ------------------- > drivers/target/iscsi/iscsi_target_erl1.c | 8 -------- > 2 files changed, 27 deletions(-) Hello Dmitri Thanks for posting this. I have a series in flight for the same hang: https://lore.kernel.org/linux-scsi/20261027122837.2148074-1-loberman@redhat.com/ Is the revert maybe aimed at the wrong layer, and the sashiko review of your 2/2 makes the case better than I can: with the fabric check removed, an aborted command reaches target_execute_cmd(), target_cmd_interrupted() queues target_abort_work and returns without changing t_state, the command stays on conn_cmd_list in TRANSPORT_WRITE_PENDING, and iscsit_release_commands_from_conn() completes it a second time if the connection closes. That is the same double completion you are reverting d5869dae5080 for. It is not a defect in Maurizio's patch and it will not be fixed by replacing it -- target_cmd_interrupted() records nothing when it queues an abort, so any site that completes an aborted WRITE_PENDING command can be followed by another one. I found this from vmcores on a customer system before either of us posted, so no argument from me that it is real. Patch 3 of my series fixes it there, under t_state_lock, for both queueing sites in target_core_transport.c and for every fabric. With that in place your 2/2 is safe and d5869dae5080 does not need reverting for this reason -- which matters, because the revert is not free: that commit carries CVE-2026-97951 and is in 5.10.y, 5.15.y, 6.1.y, 6.6.y, 6.12.y, 6.18.y and 7.2.y. Two questions on 2/2 independent of all that. Removing the sense_reason early return in iscsit_execute_cmd() means transport_send_check_condition_and_sense() now runs for an aborted command. It guards only on SCF_SENT_CHECK_CONDITION, never checks CMD_T_ABORTED, and ends in queue_status(); target_handle_abort() also calls queue_status() for a CMD_T_TAS command, and each drops a reference. Can an aborted command with a sense reason send both a CHECK CONDITION and a TASK ABORTED, and drop two references? And with the early return gone, a LUN_RESET aborting a large WRITE waits in target_put_cmd_and_wait() for the whole remaining data transfer. That is bounded where today it is not, but on a large transfer it may still trip the hung task timeout that made this visible. On the overlap: your 2/2 and patch 4 of my series touch the same lines, so only one can apply, and I have no attachment to mine if yours is the better shape. Patches 1 to 3 are core-side and conflict with nothing you have posted. Martin, would you prefer those three as their own series, given both candidate iSCSI fixes need patch 3 to be correct? I am happy to test either or both: I have a reporter's environment that reproduces this reliably and 300-cycle runs on it. Many Thanks Laurence ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands 2026-10-27 20:08 ` [PATCH 0/2] target: iscsi: fix hanged " Laurence Oberman @ 2026-10-28 14:02 ` Dmitry Bogdanov 2026-10-28 14:30 ` Laurence Oberman 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Bogdanov @ 2026-10-28 14:02 UTC (permalink / raw) To: Laurence Oberman Cc: Martin Petersen, Maurizio Lombardi, target-devel, michael.christie, linux-scsi, linux Hi Laurence, On Tue, Oct 27, 2026 at 04:08:08PM -0400, Laurence Oberman wrote: > > On Tue, 2026-10-27 at 21:01 +0300, Dmitry Bogdanov wrote: > > ISCSI has some old leftovers in the code that prevents passing the > > aborted > > command to the target core to make the actual abort. Such commands > > hang > > until connection is closed. > > So remove those leftovers. > > > > Dmitry Bogdanov (2): > > Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING > > commands" > > scsi: target: iscsi: fix hanging WRITE_PENDING commands > > > > drivers/target/iscsi/iscsi_target.c | 19 ------------------- > > drivers/target/iscsi/iscsi_target_erl1.c | 8 -------- > > 2 files changed, 27 deletions(-) > > Hello Dmitri > Thanks for posting this. I have a series in flight for the same hang: > > https://lore.kernel.org/linux-scsi/20261027122837.2148074-1-loberman@redhat.com/ I know, that is why both you and Maurizio in To: field. I want from you some comments. Thank you for review. > > Is the revert maybe aimed at the wrong layer, and the sashiko review of > your 2/2 makes the case better than I can: with the fabric check > removed, an aborted command reaches target_execute_cmd(), > target_cmd_interrupted() queues target_abort_work and returns without > changing t_state, the command stays on conn_cmd_list in > TRANSPORT_WRITE_PENDING, and iscsit_release_commands_from_conn() > completes it a second time if the connection closes. As I said in response to sashiko-bot, that is the current (always be) behavior. It is not introduced by my patch. > That is the same double completion you are reverting d5869dae5080 for. > It > is not a defect in Maurizio's patch and it will not be fixed by > replacing > it -- target_cmd_interrupted() records nothing when it queues an abort, > so > any site that completes an aborted WRITE_PENDING command can be > followed > by another one. I found this from vmcores on a customer system before > either of us posted, so no argument from me that it is real. Not the same, d5869dae5080 greatly increases the probability of the double completion by calling target_complete_cmd() which is allowed to be called only once for the cmd. Calling it twice is wrong by nature. Fabric module shall call either target_execute_cmd() either transport_generic_request_failure(). target_complete_cmd() is for backstore modules. That target_complete_cmd() at the iscsi connection release is not so good too. But it was intended to be the last(and the only) call from fabric driver for the command never be in the target core. > > Patch 3 of my series fixes it there, under t_state_lock, for both > queueing sites in target_core_transport.c and for every fabric. With > that > in place your 2/2 is safe and d5869dae5080 does not need reverting for > this reason -- which matters, because the revert is not free: that > commit > carries CVE-2026-97951 and is in 5.10.y, 5.15.y, 6.1.y, 6.6.y, 6.12.y, > 6.18.y and 7.2.y. My intention is to not create a situation when there a race is possible instead of fixing those races. > Two questions on 2/2 independent of all that. > > Removing the sense_reason early return in iscsit_execute_cmd() means > transport_send_check_condition_and_sense() now runs for an aborted > command. It guards only on SCF_SENT_CHECK_CONDITION, never checks > CMD_T_ABORTED, and ends in queue_status(); target_handle_abort() also > calls queue_status() for a CMD_T_TAS command, and each drops a > reference. > Can an aborted command with a sense reason send both a CHECK CONDITION > and a TASK ABORTED, and drop two references? No, transport_send_check_condition_and_sense will change t_state, send status and decrease cmd->refcnt and then unblock wait_for_completion of ABORT/LUN_RESET. Both the commnand and ABORT of that command will be completed sucessfully. Noone calls target_handle_abort(). > And with the early return gone, a LUN_RESET aborting a large WRITE > waits in target_put_cmd_and_wait() for the whole remaining data > transfer. That is bounded where today it is not, but on a large > transfer it may still trip the hung task timeout that made this > visible. That is a idea for a future discussion and improvmens. This patchset is narrowed to just to fix the stuck of WRITE_PENDING commands. The abort of WRITE_PENDIG command in the state waiting for DataOuts currently will wait for the data completion. So, my patch again does not do worse. > On the overlap: your 2/2 and patch 4 of my series touch the same lines, > so only one can apply, and I have no attachment to mine if yours is the > better shape. Patches 1 to 3 are core-side and conflict with nothing > you have posted. > > Martin, would you prefer those three as their own series, > given both candidate iSCSI fixes need patch 3 to be correct? > > I am happy to test either or both: I have a reporter's environment that > reproduces this reliably and 300-cycle runs on it. My patch is more than 2 years old in customers setups, but certainly it would be great to test it in other environments too. BR, Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands 2026-10-28 14:02 ` Dmitry Bogdanov @ 2026-10-28 14:30 ` Laurence Oberman 0 siblings, 0 replies; 8+ messages in thread From: Laurence Oberman @ 2026-10-28 14:30 UTC (permalink / raw) To: Dmitry Bogdanov Cc: Martin Petersen, Maurizio Lombardi, target-devel, michael.christie, linux-scsi, linux On Wed, 2026-10-28 at 17:02 +0300, Dmitry Bogdanov wrote: > Hi Laurence, > > On Tue, Oct 27, 2026 at 04:08:08PM -0400, Laurence Oberman wrote: > > > > On Tue, 2026-10-27 at 21:01 +0300, Dmitry Bogdanov wrote: > > > ISCSI has some old leftovers in the code that prevents passing > > > the > > > aborted > > > command to the target core to make the actual abort. Such > > > commands > > > hang > > > until connection is closed. > > > So remove those leftovers. > > > > > > Dmitry Bogdanov (2): > > > Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING > > > commands" > > > scsi: target: iscsi: fix hanging WRITE_PENDING commands > > > > > > drivers/target/iscsi/iscsi_target.c | 19 ------------------ > > > - > > > drivers/target/iscsi/iscsi_target_erl1.c | 8 -------- > > > 2 files changed, 27 deletions(-) > > > > Hello Dmitri > > Thanks for posting this. I have a series in flight for the same > > hang: > > > > https://lore.kernel.org/linux-scsi/20261027122837.2148074-1-loberman@redhat.com/ > > I know, that is why both you and Maurizio in To: field. I want from > you some comments. Thank you for review. > > > > > Is the revert maybe aimed at the wrong layer, and the sashiko > > review of > > your 2/2 makes the case better than I can: with the fabric check > > removed, an aborted command reaches target_execute_cmd(), > > target_cmd_interrupted() queues target_abort_work and returns > > without > > changing t_state, the command stays on conn_cmd_list in > > TRANSPORT_WRITE_PENDING, and iscsit_release_commands_from_conn() > > completes it a second time if the connection closes. > > As I said in response to sashiko-bot, that is the current (always be) > behavior. > It is not introduced by my patch. > > > That is the same double completion you are reverting d5869dae5080 > > for. > > It > > is not a defect in Maurizio's patch and it will not be fixed by > > replacing > > it -- target_cmd_interrupted() records nothing when it queues an > > abort, > > so > > any site that completes an aborted WRITE_PENDING command can be > > followed > > by another one. I found this from vmcores on a customer system > > before > > either of us posted, so no argument from me that it is real. > > Not the same, d5869dae5080 greatly increases the probability of the > double completion by calling target_complete_cmd() which is allowed > to be > called only once for the cmd. Calling it twice is wrong by nature. > Fabric > module shall call either target_execute_cmd() either > transport_generic_request_failure(). target_complete_cmd() is for > backstore modules. That target_complete_cmd() at the iscsi > connection release is not so good too. But it was intended to be the > last(and the only) call from fabric driver for the command never be > in > the target core. > > > > > Patch 3 of my series fixes it there, under t_state_lock, for both > > queueing sites in target_core_transport.c and for every fabric. > > With > > that > > in place your 2/2 is safe and d5869dae5080 does not need reverting > > for > > this reason -- which matters, because the revert is not free: that > > commit > > carries CVE-2026-97951 and is in 5.10.y, 5.15.y, 6.1.y, 6.6.y, > > 6.12.y, > > 6.18.y and 7.2.y. > > My intention is to not create a situation when there a race is > possible > instead of fixing those races. > > > Two questions on 2/2 independent of all that. > > > > Removing the sense_reason early return in iscsit_execute_cmd() > > means > > transport_send_check_condition_and_sense() now runs for an aborted > > command. It guards only on SCF_SENT_CHECK_CONDITION, never checks > > CMD_T_ABORTED, and ends in queue_status(); target_handle_abort() > > also > > calls queue_status() for a CMD_T_TAS command, and each drops a > > reference. > > Can an aborted command with a sense reason send both a CHECK > > CONDITION > > and a TASK ABORTED, and drop two references? > > No, transport_send_check_condition_and_sense will change t_state, > send > status and decrease cmd->refcnt and then unblock wait_for_completion > of > ABORT/LUN_RESET. Both the commnand and ABORT of that command will be > completed sucessfully. > Noone calls target_handle_abort(). > > > And with the early return gone, a LUN_RESET aborting a large WRITE > > waits in target_put_cmd_and_wait() for the whole remaining data > > transfer. That is bounded where today it is not, but on a large > > transfer it may still trip the hung task timeout that made this > > visible. > > That is a idea for a future discussion and improvmens. This patchset > is > narrowed to just to fix the stuck of WRITE_PENDING commands. > > The abort of WRITE_PENDIG command in the state waiting for DataOuts > currently will wait for the data completion. So, my patch again does > not > do worse. > > > On the overlap: your 2/2 and patch 4 of my series touch the same > > lines, > > so only one can apply, and I have no attachment to mine if yours is > > the > > better shape. Patches 1 to 3 are core-side and conflict with > > nothing > > you have posted. > > > > Martin, would you prefer those three as their own series, > > given both candidate iSCSI fixes need patch 3 to be correct? > > > > I am happy to test either or both: I have a reporter's environment > > that > > reproduces this reliably and 300-cycle runs on it. > > My patch is more than 2 years old in customers setups, but certainly > it would be great to test it in other environments too. > > BR, > Dmitry OK Great, reverting d5869dae5080 and applying yours and getting it tested. If it passes than we can revisit my patches 1-3 for a later series. On testing: I will build revert + your 2/2 on a RHEL9.8 based distro kernel and put it through the reporter's environment -- an Oracle RAC cluster where this reproduces reliably and where the earlier candidate fixes failed at 95 and 196 LUN_RESET cycles. The pass mark is 300 cycles, which is what my patch 4 cleared twice. I will deliberately leave my core patches out of that build so the result is about your series alone. Thanks for all your reviews and assistance. Laurence ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-10-28 14:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-27 18:01 [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 1/2] Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands" Dmitry Bogdanov 2026-10-27 18:01 ` [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands Dmitry Bogdanov 2026-10-27 18:18 ` sashiko-bot 2026-10-27 22:55 ` Dmitry Bogdanov 2026-10-27 20:08 ` [PATCH 0/2] target: iscsi: fix hanged " Laurence Oberman 2026-10-28 14:02 ` Dmitry Bogdanov 2026-10-28 14:30 ` Laurence Oberman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox