[PATCH 0/2] scsi: target: iscsi: fix stalled aborted WRITE commands

[PATCH 0/2] scsi: target: iscsi: fix stalled aborted WRITE commands

[Laurence Oberman]

These two patches apply on top of 
commit d5869dae5080 ("scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands").

A WRITE command that is still in TRANSPORT_WRITE_PENDING has never been
passed to the backend, so when a TMR aborts it the iSCSI fabric is the one
that has to complete it. That commit fixed the case where the final
DataOut PDU for an already-issued R2T arrives after the abort. Two more
paths can leave such a command with nothing to complete it:

 - Patch 1 is the hang this work started from. A WRITE that arrives with
   immediate data, aborted after that data has been received but before
   the first R2T is built, is never completed by anything: no R2T has been
   sent so the initiator cannot send more data, and no DataOut timer is
   armed so the ERL=0 timeout cannot fire either. core_tmr_lun_reset()
   then blocks for as long as the connection stays up.

 - Patch 2 closes the equivalent window where a new R2T would have been
   sent for an already-aborted command.

Patch 1 also adds a one-shot completion claim. Completing the command in
iscsit_execute_cmd() introduces a second site which
iscsit_release_commands_from_conn() would also complete during connection
teardown, because t_state is still TRANSPORT_WRITE_PENDING there and
target_complete_cmd() on an aborted command records nothing that would say
otherwise. The claim makes sure only one of them does it.

How this was found and tested:

The hang was reported against an Oracle RAC cluster on two-session iSCSI
LIO targets, where one node's LUN_RESET aborts WRITEs belonging to the
other node's session. It was not reproducible in a lab, so the diagnosis
came from instrumented kernels in the reporter's environment, logging every
site that completes or waits on an aborted command.

Across two hosts every stall had the identical signature given in patch 1,
and the stall only ever ended when the initiator gave up and dropped the
connection. 217 of 222 LUN_RESET drains completed in under a second; the
rest took 46 to 54 seconds, which is where the hung-task panics came from.
One 53.7 second stall recovered on its own and the host ran for a further
19 hours.

Both patches, backported to a 5.14-based distro kernel, have since run 300
reset cycles with no hang, in two independent runs: one instrumented and
one with the tracing removed. The same reproducer previously failed at 95
and 196 cycles. The code here is equivalent to what was tested; only
comments differ.

One thing to flag for review: patch 2's window was never observed. The
instrumented kernels recorded no completions at that site in either run,
and the reproducer never reached it. It is included because the same
reasoning applies there, but it has no field evidence behind it, and I am
happy to drop it or send it separately if you would prefer.

An earlier attempt at this fix completed the command from
core_tmr_drain_state_list() instead. That is not safe: at that point the
initiator may still be sending DataOut for the command, and the result was
a double completion -- re-queued se_cmd->work, list_del corruption and a
refcount underflow. Mentioning it in case the same idea comes up in review.


Laurence Oberman (2):
  scsi: target: iscsi: Complete an aborted WRITE stuck before the first
    R2T
  scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be
    sent

 drivers/target/iscsi/iscsi_target.c      | 31 ++++++++++++++--
 drivers/target/iscsi/iscsi_target_erl1.c | 10 +++++-
 drivers/target/iscsi/iscsi_target_util.c | 46 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_util.h |  2 ++
 include/target/iscsi/iscsi_target_core.h |  5 +++
 5 files changed, 91 insertions(+), 3 deletions(-)

-- 
2.52.0

[PATCH 1/2] scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

[Laurence Oberman]

A WRITE that arrives with immediate data, and whose data length exceeds
FirstBurstLength, is left in TRANSPORT_WRITE_PENDING by
iscsit_execute_cmd() until the first R2T is built. If a LUN_RESET or
ABORT_TASK from another session sets CMD_T_ABORTED in that window,
iscsit_execute_cmd() returns without sending the R2T.

Nothing then completes the command. No R2T has been sent, so the
initiator cannot legally send more data; no DataOut timer was armed, so
the ERL=0 timeout cannot fire either. The command stays in
TRANSPORT_WRITE_PENDING with CMD_T_ABORTED set, and
core_tmr_drain_state_list() blocks in target_put_cmd_and_wait() on it for
as long as the connection stays up:

  INFO: task kworker/5:2 blocked for more than 46 seconds.
   target_put_cmd_and_wait+0x6d/0xb0 [target_core_mod]
   core_tmr_drain_state_list+0x2dc/0x380 [target_core_mod]
   core_tmr_lun_reset+0x93/0x1a0 [target_core_mod]

Complete it in iscsit_execute_cmd() instead. No DataOut path can race
with this: without an R2T the initiator cannot send further data for the
command, and the branch is only reached when no unsolicited DataOut is
expected either.

That introduces a second site which can complete a command that
iscsit_release_commands_from_conn() would also complete during connection
teardown, because t_state is still TRANSPORT_WRITE_PENDING there.
target_complete_cmd() on a command that already has CMD_T_ABORTED set
goes straight into target_cmd_interrupted(), which queues
target_abort_work and returns without setting CMD_T_COMPLETE and without
changing t_state, so nothing in target core records that a completion has
happened. A second target_complete_cmd() re-initialises and re-queues
se_cmd->work while it is still queued and drops a command reference that
was never taken. Add a one-shot claim, taken under se_cmd->t_state_lock,
so that only one of the two sites completes the command.

This was reported against an Oracle RAC cluster on two-session iSCSI LIO
targets, where one node's LUN_RESET aborts WRITEs belonging to the other
node's session. It was not reproducible in a lab, so the diagnosis came
from instrumented kernels in the reporter's environment. Across two
hosts, every stalled command had the same signature: DMA_TO_DEVICE,
TRANSPORT_WRITE_PENDING, CMD_T_ABORTED | CMD_T_TAS, outstanding_r2ts == 0,
write_data_done == first_burst_len, and an empty cmd_r2t_list. 217 of 222
LUN_RESET drains completed in under a second; the rest took 46 to 54
seconds, which is where the hung-task panics came from. One 53.7 second
stall recovered on its own and the host ran for a further 19 hours. The
stall only ever ended when the initiator gave up and dropped the
connection, which let iscsit_release_commands_from_conn() complete the
command.

Assisted-by: Claude LLM
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c      | 12 +++++--
 drivers/target/iscsi/iscsi_target_erl1.c | 10 +++++-
 drivers/target/iscsi/iscsi_target_util.c | 46 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_util.h |  2 ++
 include/target/iscsi/iscsi_target_core.h |  5 +++
 5 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 124ff269b8e7..aab9237a6776 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4198,10 +4198,18 @@ static void iscsit_release_commands_from_conn(struct iscsit_conn *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.
+			 * to tell LIO to perform the completion process --
+			 * unless iscsit_execute_cmd() already did.  t_state
+			 * stays TRANSPORT_WRITE_PENDING across that
+			 * completion, so the claim is the only thing that can
+			 * tell us.
 			 */
+			bool claimed = iscsit_claim_completion_locked(cmd);
+
 			spin_unlock_irq(&se_cmd->t_state_lock);
-			target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED);
+			if (claimed)
+				target_complete_cmd(&cmd->se_cmd,
+						    SAM_STAT_TASK_ABORTED);
 			continue;
 		}
 		spin_unlock_irq(&se_cmd->t_state_lock);
diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index d8ca06e697d6..cf72c79be355 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -927,8 +927,16 @@ 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)
+				/*
+				 * No R2T has been sent for this command yet,
+				 * so returning here leaves the initiator with
+				 * nothing to send and no DataOut timer armed.
+				 * Complete it, or nothing ever will.
+				 */
+				if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
+					iscsit_complete_aborted_cmd(cmd);
 					return 0;
+				}
 
 				iscsit_set_dataout_sequence_values(cmd);
 				conn->conn_transport->iscsit_get_dataout(conn, cmd, false);
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index c1888c42afdd..31248cb9e915 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -14,6 +14,7 @@
 #include <scsi/scsi_tcq.h>
 #include <scsi/iscsi_proto.h>
 #include <target/target_core_base.h>
+#include <target/target_core_backend.h>
 #include <target/target_core_fabric.h>
 #include <target/iscsi/iscsi_transport.h>
 
@@ -186,6 +187,51 @@ struct iscsit_cmd *iscsit_allocate_cmd(struct iscsit_conn *conn, int state)
 }
 EXPORT_SYMBOL(iscsit_allocate_cmd);
 
+/*
+ * A WRITE command still in TRANSPORT_WRITE_PENDING has never been passed to
+ * the backend, so when a TMR aborts it the fabric has to complete it.  More
+ * than one site can reach that conclusion for the same command, and
+ * target_complete_cmd() on a command that already has CMD_T_ABORTED set
+ * neither sets CMD_T_COMPLETE nor changes t_state, so target core keeps no
+ * state that would stop a second completion.  Serialise them here: whichever
+ * site claims the completion performs it and every other site leaves the
+ * command alone.
+ *
+ * Caller must hold cmd->se_cmd.t_state_lock.
+ */
+bool iscsit_claim_completion_locked(struct iscsit_cmd *cmd)
+{
+	lockdep_assert_held(&cmd->se_cmd.t_state_lock);
+
+	if (cmd->completion_claimed)
+		return false;
+
+	cmd->completion_claimed = true;
+	return true;
+}
+
+/* Complete an aborted command at most once.  Must not hold t_state_lock. */
+void iscsit_complete_aborted_cmd(struct iscsit_cmd *cmd)
+{
+	struct se_cmd *se_cmd = &cmd->se_cmd;
+	unsigned long flags;
+	bool claimed;
+
+	spin_lock_irqsave(&se_cmd->t_state_lock, flags);
+	claimed = iscsit_claim_completion_locked(cmd);
+	spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
+
+	if (!claimed)
+		return;
+
+	/*
+	 * No-op unless a DataOut timer was armed for this command; callers
+	 * that complete before the first R2T have none.
+	 */
+	iscsit_stop_dataout_timer(cmd);
+	target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
+}
+
 struct iscsi_seq *iscsit_get_seq_holder_for_datain(
 	struct iscsit_cmd *cmd,
 	u32 seq_send_order)
diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
index 7ae48a8a5cbf..8f3fe7428428 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -18,6 +18,8 @@ extern struct iscsi_r2t *iscsit_get_r2t_from_list(struct iscsit_cmd *);
 extern void iscsit_free_r2t(struct iscsi_r2t *, struct iscsit_cmd *);
 extern void iscsit_free_r2ts_from_list(struct iscsit_cmd *);
 extern struct iscsit_cmd *iscsit_allocate_cmd(struct iscsit_conn *, int);
+extern bool iscsit_claim_completion_locked(struct iscsit_cmd *cmd);
+extern void iscsit_complete_aborted_cmd(struct iscsit_cmd *cmd);
 extern struct iscsi_seq *iscsit_get_seq_holder_for_datain(struct iscsit_cmd *, u32);
 extern struct iscsi_seq *iscsit_get_seq_holder_for_r2t(struct iscsit_cmd *);
 extern struct iscsi_r2t *iscsit_get_holder_for_r2tsn(struct iscsit_cmd *, u32);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 51ca80abacf7..0707a9520810 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -496,6 +496,11 @@ struct iscsit_cmd {
 	u32			first_data_sg_off;
 	u32			kmapped_nents;
 	sense_reason_t		sense_reason;
+	/*
+	 * Set once the fabric has called target_complete_cmd() for this
+	 * command.  Protected by se_cmd.t_state_lock.
+	 */
+	bool			completion_claimed;
 }  ____cacheline_aligned;
 
 struct iscsi_tmr_req {
-- 
2.52.0

Re: [PATCH 1/2] scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iscsi: double completion and workqueue corruption for aborted WRITEs on DataOut path
--

commit 7b04f6989363db2c2b75d8c212ee852153982c8d
Author: Laurence Oberman <loberman@redhat.com>
    
scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

This patch resolves an issue where an aborted WRITE command that arrives with
immediate data exceeding FirstBurstLength gets stuck in TRANSPORT_WRITE_PENDING.
It accomplishes this by completing the command in iscsit_execute_cmd() and
introducing a new iscsit_claim_completion_locked() helper to avoid a double
completion race with iscsit_release_commands_from_conn().

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 124ff269b8e75..aab9237a67768 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -4198,10 +4198,18 @@ static void iscsit_release_commands_from_conn(struct iscsit_conn *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.
> +			 * to tell LIO to perform the completion process --
> +			 * unless iscsit_execute_cmd() already did.  t_state
> +			 * stays TRANSPORT_WRITE_PENDING across that
> +			 * completion, so the claim is the only thing that can
> +			 * tell us.
>  			 */
> +			bool claimed = iscsit_claim_completion_locked(cmd);
> +
>  			spin_unlock_irq(&se_cmd->t_state_lock);
> -			target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED);
> +			if (claimed)
> +				target_complete_cmd(&cmd->se_cmd,
> +						    SAM_STAT_TASK_ABORTED);

[Severity: High]
Does this code miss the DataOut paths?

Because target_complete_cmd() is called directly in __iscsit_check_dataout_hdr()
and target_execute_cmd() is called in iscsit_check_dataout_payload() for aborted
commands without setting the completion_claimed flag.

If a command is aborted on the DataOut path:

drivers/target/iscsi/iscsi_target.c:__iscsit_check_dataout_hdr() {
    ...
		if (se_cmd->transport_state & CMD_T_ABORTED) {
			if (hdr->flags & ISCSI_FLAG_CMD_FINAL &&
			    --cmd->outstanding_r2ts < 1) {
				iscsit_stop_dataout_timer(cmd);
				target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
			}
    ...
}

Because completion_claimed is never set, target_complete_cmd() delegates
to target_cmd_interrupted() which queues target_abort_work but leaves
the command in TRANSPORT_WRITE_PENDING.

If the connection drops before target_abort_work removes the command
from the connection list, iscsit_release_commands_from_conn() will find it,
iscsit_claim_completion_locked() will return true, and it will call
target_complete_cmd() a second time.

Can this double call to target_complete_cmd() cause target_cmd_interrupted()
to re-initialize and re-queue cmd->work while it is already pending on the
workqueue, leading to workqueue linked-list corruption and a use-after-free
when target_handle_abort() runs twice?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014143932.39453-1-loberman@redhat.com?part=1

[PATCH 2/2] scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

[Laurence Oberman]

iscsit_check_dataout_payload() hands a command to iscsit_get_dataout() to
request the next burst whenever iscsit_check_post_dataout() returns
DATAOUT_SEND_R2T, without checking CMD_T_ABORTED first.

If a TMR aborted the command while the burst that just finished was in
flight, no further DataOut will arrive: the initiator is about to be told
the command was aborted. The CMD_T_ABORTED handling in
__iscsit_check_dataout_hdr() does not cover this, because it only runs
for DataOut PDUs belonging to R2Ts that were already issued, and
outstanding_r2ts has already been decremented to zero for the burst that
completed. The command is left in TRANSPORT_WRITE_PENDING with nothing to
complete it.

Complete it here instead. Guard on outstanding_r2ts so that with
MaxOutstandingR2T > 1 the completion is still left to the last R2T to
drain in __iscsit_check_dataout_hdr().

Unlike patch 1 of this series, this window was never observed in the
field: instrumented kernels on two hosts recorded no completions at this
site across 300 reset cycles, and the reproducer that triggered the other
one never reached it. It is closed by code inspection, for symmetry with
the other paths that can leave an aborted WRITE_PENDING command stranded.

Assisted-by: Claude LLM
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index aab9237a6776..87b215b18034 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1669,6 +1669,25 @@ iscsit_check_dataout_payload(struct iscsit_cmd *cmd, struct iscsi_data *hdr,
 	if ((rc == DATAOUT_NORMAL) || (rc == DATAOUT_WITHIN_COMMAND_RECOVERY))
 		return 0;
 	else if (rc == DATAOUT_SEND_R2T) {
+		/*
+		 * A new R2T would never be answered for an aborted command.
+		 * The CMD_T_ABORTED handling in __iscsit_check_dataout_hdr()
+		 * does not cover this: it only runs for DataOut PDUs
+		 * belonging to R2Ts that were already issued, and
+		 * iscsit_dataout_update_r2t() has already decremented
+		 * outstanding_r2ts for the burst that just finished.  Guard
+		 * on outstanding_r2ts so that with MaxOutstandingR2T > 1 the
+		 * completion is still left to the last R2T to drain.
+		 */
+		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
+			if (cmd->outstanding_r2ts < 1) {
+				iscsit_stop_dataout_timer(cmd);
+				target_complete_cmd(&cmd->se_cmd,
+						    SAM_STAT_TASK_ABORTED);
+			}
+			return 0;
+		}
+
 		iscsit_set_dataout_sequence_values(cmd);
 		conn->conn_transport->iscsit_get_dataout(conn, cmd, false);
 	} else if (rc == DATAOUT_SEND_TO_TRANSPORT) {
-- 
2.52.0

Re: [PATCH 2/2] scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iscsi: workqueue corruption via double completion of aborted cmd
--

commit 1cef49db01d5c1eb5e9670fc6f016dfd14ddd5cd
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

iscsit_check_dataout_payload() hands a command to iscsit_get_dataout() to
request the next burst whenever iscsit_check_post_dataout() returns
DATAOUT_SEND_R2T, without checking CMD_T_ABORTED first. This patch guards
for aborted commands by stopping the dataout timer and completing the command.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index aab9237a67768..87b215b18034e 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1669,6 +1669,25 @@ iscsit_check_dataout_payload(struct iscsit_cmd *cmd, struct iscsi_data *hdr,
>  	if ((rc == DATAOUT_NORMAL) || (rc == DATAOUT_WITHIN_COMMAND_RECOVERY))
>  		return 0;
>  	else if (rc == DATAOUT_SEND_R2T) {
[ ... ]
> +		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> +			if (cmd->outstanding_r2ts < 1) {
> +				iscsit_stop_dataout_timer(cmd);
> +				target_complete_cmd(&cmd->se_cmd,
> +						    SAM_STAT_TASK_ABORTED);

[Severity: High]
Can calling target_complete_cmd() directly on an aborted command bypass the
synchronization wrapper iscsit_complete_aborted_cmd()?

Because this bypasses iscsit_claim_completion_locked(), could concurrent
session teardown or other abort paths also claim the command and call
target_complete_cmd()?

This concurrent modification could lead to target_cmd_interrupted() executing
INIT_WORK(&cmd->work, target_abort_work) and queue_work() concurrently on an
already active work item, which might destroy the workqueue linked list.

There is also a preexisting location in the header parsing path that exposes
the same unsynchronized completion race, which appears to have been copied
here and left unfixed:

drivers/target/iscsi/iscsi_target.c:__iscsit_check_dataout_hdr() {
    ...
		if (se_cmd->transport_state & CMD_T_ABORTED) {
			if (hdr->flags & ISCSI_FLAG_CMD_FINAL &&
			    --cmd->outstanding_r2ts < 1) {
				iscsit_stop_dataout_timer(cmd);
				target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
			}
    ...
}

Would it be safer to use iscsit_complete_aborted_cmd() in both places to
ensure the completion is safely claimed?

> +			}
> +			return 0;
> +		}
> +
>  		iscsit_set_dataout_sequence_values(cmd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014143932.39453-1-loberman@redhat.com?part=2