[PATCH v5 0/2] scsi: target: fix LUN_RESET hang on aborted iSCSI WRITEs

[PATCH v5 0/2] scsi: target: fix LUN_RESET hang on aborted iSCSI WRITEs

[Laurence Oberman]

These two patches fix a LUN_RESET hang on iSCSI targets, and the double
completion that fixing it exposes.

A WRITE command that is still in TRANSPORT_WRITE_PENDING has never been
passed to the backend, so when a TMR aborts it the fabric is the one that
has to complete it. Patch 2 completes it at the one site where nothing
does today. Patch 1 makes a second completion of an already-aborted
command a no-op, which is what makes that safe -- and which also fixes a
pre-existing double completion that is not specific to iSCSI.

 - Patch 1 guards the CMD_T_ABORTED branch of target_cmd_interrupted()
   with a one-shot flag taken under t_state_lock. Without it, a second
   target_complete_cmd() on an aborted command re-queues an se_cmd->work
   that is already queued and re-runs transport_complete_callback(), which
   for COMPARE AND WRITE releases dev->caw_sem a second time.

 - Patch 2 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.

Changes since v4:
 - Moved the one-shot guard from iSCSI into target_cmd_interrupted(), as
   Bart Van Assche asked for in review of v4, and to the design he
   proposed there. This replaces the iSCSI-private completion_claimed
   member and the two helpers, which are gone, and it covers sites the
   iSCSI version could not: it closes the window where CMD_T_ABORTED is
   set between a fabric check and target_cmd_interrupted(), it covers the
   deferred ICF_OOO_CMDSN path into target_execute_cmd(), and it applies
   to every fabric rather than iSCSI alone.
 - The guard is limited to the CMD_T_ABORTED branch. complete_all() on the
   CMD_T_STOP branch is idempotent, and a shared guard would suppress the
   queueing of target_abort_work if CMD_T_ABORTED were set after a first
   call took the CMD_T_STOP branch.
 - Dropped v4 patches 2 and 3, which claimed the completion at two further
   iSCSI sites. Patch 1 makes both unnecessary.
 - Patch 2's Fixes: tag now names commit 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET handling synchronous")
   rather than the TAS commit it cited in v4. The TAS commit left the site
   with nothing to complete the command; making the abort path wait on a
   fabric completion is what turned that into a hang. Both are in v5.0, so
   the stable range is unchanged, and the patch body describes both.
 - Patch 2 is otherwise the same change as v4 patch 1, without the claim.

v4: https://lore.kernel.org/linux-scsi/20261026150722.823941-1-loberman@redhat.com/
v3: https://lore.kernel.org/linux-scsi/20261026124344.745124-1-loberman@redhat.com/
v2: https://lore.kernel.org/linux-scsi/20261016175351.126373-1-loberman@redhat.com/
v1: https://lore.kernel.org/linux-scsi/20261014143932.39453-1-loberman@redhat.com/

How this was found:

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 2,
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.

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. That is the failure patch 1 now prevents outright, and
it is worth knowing that the same symptoms are reachable without any of
this series.

What is and is not tested:

Patch 2's change has run 300 LUN_RESET cycles with no hang, in two
independent runs on a 5.14-based distro kernel, one instrumented and one
with the tracing removed. The same reproducer previously failed at 95 and
196 cycles. In those kernels the double completion was prevented by the
iSCSI-private claim that v4 carried rather than by patch 1, so the hang
fix is validated but patch 1 is not: it is new code, and its design is
Bart's rather than mine. I would rather say that plainly than imply field
coverage it does not have.

The reporter's environment is a production cluster and the only place this
has ever reproduced, so I cannot offer an upstream -rc run from there. A
build of this series is going to them and I will report the result.

Laurence Oberman (2):
  scsi: target: Complete an aborted command at most once
  scsi: target: iscsi: Complete an aborted WRITE stuck before the first
    R2T

 drivers/target/iscsi/iscsi_target_erl1.c | 13 ++++++++-
 drivers/target/target_core_transport.c   | 37 +++++++++++++++++++-----
 include/target/target_core_base.h        |  1 +
 3 files changed, 42 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] scsi: target: Complete an aborted command at most once

[Laurence Oberman]

target_cmd_interrupted() queues target_abort_work for a command that has
CMD_T_ABORTED set and returns without setting CMD_T_COMPLETE and without
changing t_state, so target core records nothing that would stop a second
completion of the same command. A second call re-initialises and re-queues
cmd->work while the first is still queued or running, and calls
transport_complete_callback() again. For COMPARE AND WRITE that callback is
compare_and_write_callback(), which releases dev->caw_sem, so a repeated
call releases that semaphore twice.

More than one site can reach that conclusion for the same command. A WRITE
still in TRANSPORT_WRITE_PENDING has never been passed to the backend, so
the fabric has to complete it when a TMR aborts it, and for iSCSI both the
DataOut paths and iscsit_release_commands_from_conn() during connection
teardown will do so.

Guard the CMD_T_ABORTED branch with a one-shot CMD_T_ABORT_QUEUED flag,
tested and set under t_state_lock. The CMD_T_ABORTED test moves under that
lock as well, which is the lock __target_check_io_state() sets
CMD_T_ABORTED under, so there is no window between the test and the set.
Second and later calls are a no-op by design and still return true, so
callers do not proceed with normal completion. A fabric that completes an
aborted command exactly once never reaches the guard and is unaffected.

The CMD_T_STOP branch is deliberately left unguarded. complete_all() is
idempotent and t_transport_stop_comp is initialised once in
__target_init_cmd(), so a repeated call there is harmless. A guard shared
between the two branches would also be wrong: a first call taking the
CMD_T_STOP branch would then suppress the queueing of target_abort_work if
CMD_T_ABORTED were set afterwards, which is the hang this series started
from.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    Prerequisite for patch 2, which relies on a second completion of an
    aborted command being a no-op.  Both need to travel together to stable.
    
    I have not put a Fixes: tag on this one -- the double completion is older
    than the iSCSI hang and I am not certain which commit to name.  If you
    would like one here, please say which.

 drivers/target/target_core_transport.c | 37 ++++++++++++++++++++------
 include/target/target_core_base.h      |  1 +
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 927861c..4896a92 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -882,24 +882,45 @@ static void target_abort_work(struct work_struct *work)
 	target_handle_abort(cmd);
 }
 
+/*
+ * Returns true if the command has been aborted or stopped, in which case the
+ * caller must not carry on with normal completion.  For an aborted command the
+ * work that must not run twice -- the completion callback and the queueing of
+ * target_abort_work -- is done once, under CMD_T_ABORT_QUEUED.
+ */
 static bool target_cmd_interrupted(struct se_cmd *cmd)
 {
+	bool queue_abort = false, stop = false;
+	unsigned long flags;
 	int post_ret;
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (cmd->transport_state & CMD_T_ABORTED) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+		if (cmd->transport_state & CMD_T_ABORT_QUEUED) {
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+			return true;
+		}
+		cmd->transport_state |= CMD_T_ABORT_QUEUED;
+		queue_abort = true;
+	} else if (cmd->transport_state & CMD_T_STOP) {
+		stop = true;
+	}
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	if (!queue_abort && !stop)
+		return false;
+
+	if (cmd->transport_complete_callback)
+		cmd->transport_complete_callback(cmd, false, &post_ret);
+
+	if (queue_abort) {
 		INIT_WORK(&cmd->work, target_abort_work);
 		queue_work(target_completion_wq, &cmd->work);
-		return true;
-	} else if (cmd->transport_state & CMD_T_STOP) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+	} else {
 		complete_all(&cmd->t_transport_stop_comp);
-		return true;
 	}
 
-	return false;
+	return true;
 }
 
 static void target_complete(struct se_cmd *cmd, int success)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 002b0fc..7fa6733 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -537,6 +537,7 @@ struct se_cmd {
 #define CMD_T_STOP		(1 << 5)
 #define CMD_T_TAS		(1 << 10)
 #define CMD_T_FABRIC_STOP	(1 << 11)
+#define CMD_T_ABORT_QUEUED	(1 << 12)
 	spinlock_t		t_state_lock;
 	struct kref		cmd_kref;
 	struct completion	t_transport_stop_comp;
-- 
2.43.0

[PATCH v5 2/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]

Commit aaa00cc93c1d ("scsi: target/core: Fix TAS handling for aborted
commands") is what left this site with nothing to complete the command.
Before it, transport_check_aborted_status() sent the delayed TASK_ABORTED
status here; that commit replaced the call with a bare CMD_T_ABORTED test
and a return, on the basis that the target core would send TASK_ABORTED
from the ABORT TMF context instead. It became a hang once ABORT and LUN
RESET handling was made synchronous, because the abort path then waits in
target_put_cmd_and_wait() for a fabric completion that no longer comes.

Complete the command here 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.
If the connection drops before target_abort_work has run,
iscsit_release_commands_from_conn() will complete the same command again,
which the one-shot guard added in the previous patch makes a no-op.

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.

Fixes: 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET handling synchronous")
Cc: stable@vger.kernel.org
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target_erl1.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index d8ca06e..c78f312 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -12,6 +12,7 @@
 #include <linux/slab.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>
 
@@ -927,8 +928,18 @@ 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_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);
-- 
2.43.0