[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: target: double transport_complete_callback() via CMD_T_STOP
- [High] scsi: target: unguarded abort queueing in transport_generic_request_failure()
--

commit 9f0a0fc8e2f216b4d0f91c9797ea14d9a91154f8
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: Complete an aborted command at most once

This commit adds the CMD_T_ABORT_QUEUED flag under t_state_lock in
target_cmd_interrupted() so that an aborted command only runs its completion
callback and queues target_abort_work once.

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 927861c30afc4..4896a92392e0e 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);

[Severity: High]
Can cmd->transport_complete_callback() still be called more than once when a
command hits the CMD_T_STOP path first?

The commit message notes that leaving the CMD_T_STOP branch unguarded is
harmless because complete_all() is idempotent, and explains that a first call
to target_cmd_interrupted() can take the CMD_T_STOP branch before CMD_T_ABORTED
is set afterwards.

However, target_cmd_interrupted() executes cmd->transport_complete_callback()
when either queue_abort or stop is true, without clearing the callback pointer
or guarding its execution. In drivers/target/target_core_sbc.c, both
compare_and_write_callback() and compare_and_write_post() release dev->caw_sem
without clearing cmd->transport_complete_callback:

drivers/target/target_core_sbc.c:compare_and_write_callback() {
    ...
out:
	/*
	 * In the MISCOMPARE or failure case, unlock ->caw_sem obtained in
	 * sbc_compare_and_write() before the original READ I/O submission.
	 */
	up(&dev->caw_sem);
	sg_free_table(&write_tbl);
	return ret;
}

If a first call to target_cmd_interrupted() takes the CMD_T_STOP branch and a
subsequent call takes either the CMD_T_ABORTED branch (after CMD_T_ABORTED is
set) or the CMD_T_STOP branch a second time, won't
cmd->transport_complete_callback() run twice and call up(&dev->caw_sem) a
second time?

> +
> +	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;
>  }

[Severity: High]
Does transport_generic_request_failure() in
drivers/target/target_core_transport.c also need to test and set
CMD_T_ABORT_QUEUED under cmd->t_state_lock?

Commit 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET handling
synchronous") added the same abort handling pattern to both
target_cmd_interrupted() and transport_generic_request_failure():

drivers/target/target_core_transport.c:transport_generic_request_failure() {
    ...
	if (cmd->transport_complete_callback)
		cmd->transport_complete_callback(cmd, false, &post_ret);

	if (cmd->transport_state & CMD_T_ABORTED) {
		INIT_WORK(&cmd->work, target_abort_work);
		queue_work(target_completion_wq, &cmd->work);
		return;
	}
    ...
}

Because transport_generic_request_failure() does not check or set
CMD_T_ABORT_QUEUED under cmd->t_state_lock, if CMD_T_ABORTED is set after
target_cmd_interrupted() returns false in target_execute_cmd(), and execution
then fails in target_write_prot_action() or __target_execute_cmd() (for
example, when tcmu_queue_cmd() sees CMD_T_ABORTED and returns
TCM_CHECK_CONDITION_ABORT_CMD, or when target_scsi3_ua_check() fails):

drivers/target/target_core_transport.c:__target_execute_cmd() {
    ...
err:
	spin_lock_irq(&cmd->t_state_lock);
	cmd->transport_state &= ~CMD_T_SENT;
	spin_unlock_irq(&cmd->t_state_lock);

	transport_generic_request_failure(cmd, ret);
}

transport_generic_request_failure() will invoke
cmd->transport_complete_callback(), queue target_abort_work on cmd->work, and
return early without setting CMD_T_ABORT_QUEUED or SCF_SENT_CHECK_CONDITION.

If an iSCSI ERL=2 TMR TASK_REASSIGN then runs
iscsit_task_reassign_complete_write() in
drivers/target/iscsi/iscsi_target_tmr.c, it sees ICF_GOT_LAST_DATAOUT set and
CMD_T_SENT cleared, and calls target_execute_cmd() ->
target_cmd_interrupted():

drivers/target/iscsi/iscsi_target_tmr.c:iscsit_task_reassign_complete_write() {
    ...
	if (cmd->cmd_flags & ICF_GOT_LAST_DATAOUT) {
		if (!(cmd->se_cmd.transport_state & CMD_T_SENT)) {
			pr_debug("WRITE ITT: 0x%08x: t_state: %d"
				" never sent to transport\n",
				cmd->init_task_tag, cmd->se_cmd.t_state);
			target_execute_cmd(se_cmd);
			return 0;
		}
    ...
}

Similarly, fabric error paths such as tcm_qla2xxx_write_pending(),
tcm_qla2xxx_handle_data_work(), and isert_rdma_read_done() call
transport_generic_request_failure() directly. Could these paths reach both
transport_generic_request_failure() and target_cmd_interrupted() (or
transport_generic_request_failure() twice) for the same aborted command,
re-initializing cmd->work while queued and running
cmd->transport_complete_callback() and target_handle_abort() a second time?

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

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

[Laurence Oberman]

On Mon, 2026-10-26 at 19:56 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] scsi: target: double transport_complete_callback() via
> CMD_T_STOP
> - [High] scsi: target: unguarded abort queueing in
> transport_generic_request_failure()
> --
> 
> commit 9f0a0fc8e2f216b4d0f91c9797ea14d9a91154f8
> Author: Laurence Oberman <loberman@redhat.com>
> 
> scsi: target: Complete an aborted command at most once
> 
> This commit adds the CMD_T_ABORT_QUEUED flag under t_state_lock in
> target_cmd_interrupted() so that an aborted command only runs its
> completion
> callback and queues target_abort_work once.
> 
> > diff --git a/drivers/target/target_core_transport.c
> > b/drivers/target/target_core_transport.c
> > index 927861c30afc4..4896a92392e0e 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);
> 
> [Severity: High]
> Can cmd->transport_complete_callback() still be called more than once
> when a
> command hits the CMD_T_STOP path first?
> 
> The commit message notes that leaving the CMD_T_STOP branch unguarded
> is
> harmless because complete_all() is idempotent, and explains that a
> first call
> to target_cmd_interrupted() can take the CMD_T_STOP branch before
> CMD_T_ABORTED
> is set afterwards.
> 
> However, target_cmd_interrupted() executes cmd-
> >transport_complete_callback()
> when either queue_abort or stop is true, without clearing the
> callback pointer
> or guarding its execution. In drivers/target/target_core_sbc.c, both
> compare_and_write_callback() and compare_and_write_post() release
> dev->caw_sem
> without clearing cmd->transport_complete_callback:
> 
> drivers/target/target_core_sbc.c:compare_and_write_callback() {
>     ...
> out:
> 	/*
> 	 * In the MISCOMPARE or failure case, unlock ->caw_sem
> obtained in
> 	 * sbc_compare_and_write() before the original READ I/O
> submission.
> 	 */
> 	up(&dev->caw_sem);
> 	sg_free_table(&write_tbl);
> 	return ret;
> }
> 
> If a first call to target_cmd_interrupted() takes the CMD_T_STOP
> branch and a
> subsequent call takes either the CMD_T_ABORTED branch (after
> CMD_T_ABORTED is
> set) or the CMD_T_STOP branch a second time, won't
> cmd->transport_complete_callback() run twice and call up(&dev-
> >caw_sem) a
> second time?
> 
> > +
> > +	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;
> >  }
> 
> [Severity: High]
> Does transport_generic_request_failure() in
> drivers/target/target_core_transport.c also need to test and set
> CMD_T_ABORT_QUEUED under cmd->t_state_lock?
> 
> Commit 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET
> handling
> synchronous") added the same abort handling pattern to both
> target_cmd_interrupted() and transport_generic_request_failure():
> 
> drivers/target/target_core_transport.c:transport_generic_request_fail
> ure() {
>     ...
> 	if (cmd->transport_complete_callback)
> 		cmd->transport_complete_callback(cmd, false,
> &post_ret);
> 
> 	if (cmd->transport_state & CMD_T_ABORTED) {
> 		INIT_WORK(&cmd->work, target_abort_work);
> 		queue_work(target_completion_wq, &cmd->work);
> 		return;
> 	}
>     ...
> }
> 
> Because transport_generic_request_failure() does not check or set
> CMD_T_ABORT_QUEUED under cmd->t_state_lock, if CMD_T_ABORTED is set
> after
> target_cmd_interrupted() returns false in target_execute_cmd(), and
> execution
> then fails in target_write_prot_action() or __target_execute_cmd()
> (for
> example, when tcmu_queue_cmd() sees CMD_T_ABORTED and returns
> TCM_CHECK_CONDITION_ABORT_CMD, or when target_scsi3_ua_check()
> fails):
> 
> drivers/target/target_core_transport.c:__target_execute_cmd() {
>     ...
> err:
> 	spin_lock_irq(&cmd->t_state_lock);
> 	cmd->transport_state &= ~CMD_T_SENT;
> 	spin_unlock_irq(&cmd->t_state_lock);
> 
> 	transport_generic_request_failure(cmd, ret);
> }
> 
> transport_generic_request_failure() will invoke
> cmd->transport_complete_callback(), queue target_abort_work on cmd-
> >work, and
> return early without setting CMD_T_ABORT_QUEUED or
> SCF_SENT_CHECK_CONDITION.
> 
> If an iSCSI ERL=2 TMR TASK_REASSIGN then runs
> iscsit_task_reassign_complete_write() in
> drivers/target/iscsi/iscsi_target_tmr.c, it sees ICF_GOT_LAST_DATAOUT
> set and
> CMD_T_SENT cleared, and calls target_execute_cmd() ->
> target_cmd_interrupted():
> 
> drivers/target/iscsi/iscsi_target_tmr.c:iscsit_task_reassign_complete
> _write() {
>     ...
> 	if (cmd->cmd_flags & ICF_GOT_LAST_DATAOUT) {
> 		if (!(cmd->se_cmd.transport_state & CMD_T_SENT)) {
> 			pr_debug("WRITE ITT: 0x%08x: t_state: %d"
> 				" never sent to transport\n",
> 				cmd->init_task_tag, cmd-
> >se_cmd.t_state);
> 			target_execute_cmd(se_cmd);
> 			return 0;
> 		}
>     ...
> }
> 
> Similarly, fabric error paths such as tcm_qla2xxx_write_pending(),
> tcm_qla2xxx_handle_data_work(), and isert_rdma_read_done() call
> transport_generic_request_failure() directly. Could these paths reach
> both
> transport_generic_request_failure() and target_cmd_interrupted() (or
> transport_generic_request_failure() twice) for the same aborted
> command,
> re-initializing cmd->work while queued and running
> cmd->transport_complete_callback() and target_handle_abort() a second
> time?

Let me wait on Bart's review but I do see validity in what Sashiko is
reporting. The complexity in this target code is really hard to deal
with but I am doing my best. I will also see if I can get AI to check
it fully first should I come up with a good response to the latest
sashiko review

Regards
Laurence

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

[Laurence Oberman]

On Mon, 2026-10-26 at 16:04 -0400, Laurence Oberman wrote:
> On Mon, 2026-10-26 at 19:56 +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2
> > potential
> > issue(s) to consider:
> > - [High] scsi: target: double transport_complete_callback() via
> > CMD_T_STOP
> > - [High] scsi: target: unguarded abort queueing in
> > transport_generic_request_failure()
> > --
> > 
> > commit 9f0a0fc8e2f216b4d0f91c9797ea14d9a91154f8
> > Author: Laurence Oberman <loberman@redhat.com>
> > 
> > scsi: target: Complete an aborted command at most once
> > 
> > This commit adds the CMD_T_ABORT_QUEUED flag under t_state_lock in
> > target_cmd_interrupted() so that an aborted command only runs its
> > completion
> > callback and queues target_abort_work once.
> > 
> > > diff --git a/drivers/target/target_core_transport.c
> > > b/drivers/target/target_core_transport.c
> > > index 927861c30afc4..4896a92392e0e 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);
> > 
> > [Severity: High]
> > Can cmd->transport_complete_callback() still be called more than
> > once
> > when a
> > command hits the CMD_T_STOP path first?
> > 
> > The commit message notes that leaving the CMD_T_STOP branch
> > unguarded
> > is
> > harmless because complete_all() is idempotent, and explains that a
> > first call
> > to target_cmd_interrupted() can take the CMD_T_STOP branch before
> > CMD_T_ABORTED
> > is set afterwards.
> > 
> > However, target_cmd_interrupted() executes cmd-
> > > transport_complete_callback()
> > when either queue_abort or stop is true, without clearing the
> > callback pointer
> > or guarding its execution. In drivers/target/target_core_sbc.c,
> > both
> > compare_and_write_callback() and compare_and_write_post() release
> > dev->caw_sem
> > without clearing cmd->transport_complete_callback:
> > 
> > drivers/target/target_core_sbc.c:compare_and_write_callback() {
> >     ...
> > out:
> > 	/*
> > 	 * In the MISCOMPARE or failure case, unlock ->caw_sem
> > obtained in
> > 	 * sbc_compare_and_write() before the original READ I/O
> > submission.
> > 	 */
> > 	up(&dev->caw_sem);
> > 	sg_free_table(&write_tbl);
> > 	return ret;
> > }
> > 
> > If a first call to target_cmd_interrupted() takes the CMD_T_STOP
> > branch and a
> > subsequent call takes either the CMD_T_ABORTED branch (after
> > CMD_T_ABORTED is
> > set) or the CMD_T_STOP branch a second time, won't
> > cmd->transport_complete_callback() run twice and call up(&dev-
> > > caw_sem) a
> > second time?
> > 
> > > +
> > > +	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;
> > >  }
> > 
> > [Severity: High]
> > Does transport_generic_request_failure() in
> > drivers/target/target_core_transport.c also need to test and set
> > CMD_T_ABORT_QUEUED under cmd->t_state_lock?
> > 
> > Commit 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET
> > handling
> > synchronous") added the same abort handling pattern to both
> > target_cmd_interrupted() and transport_generic_request_failure():
> > 
> > drivers/target/target_core_transport.c:transport_generic_request_fa
> > il
> > ure() {
> >     ...
> > 	if (cmd->transport_complete_callback)
> > 		cmd->transport_complete_callback(cmd, false,
> > &post_ret);
> > 
> > 	if (cmd->transport_state & CMD_T_ABORTED) {
> > 		INIT_WORK(&cmd->work, target_abort_work);
> > 		queue_work(target_completion_wq, &cmd->work);
> > 		return;
> > 	}
> >     ...
> > }
> > 
> > Because transport_generic_request_failure() does not check or set
> > CMD_T_ABORT_QUEUED under cmd->t_state_lock, if CMD_T_ABORTED is set
> > after
> > target_cmd_interrupted() returns false in target_execute_cmd(), and
> > execution
> > then fails in target_write_prot_action() or __target_execute_cmd()
> > (for
> > example, when tcmu_queue_cmd() sees CMD_T_ABORTED and returns
> > TCM_CHECK_CONDITION_ABORT_CMD, or when target_scsi3_ua_check()
> > fails):
> > 
> > drivers/target/target_core_transport.c:__target_execute_cmd() {
> >     ...
> > err:
> > 	spin_lock_irq(&cmd->t_state_lock);
> > 	cmd->transport_state &= ~CMD_T_SENT;
> > 	spin_unlock_irq(&cmd->t_state_lock);
> > 
> > 	transport_generic_request_failure(cmd, ret);
> > }
> > 
> > transport_generic_request_failure() will invoke
> > cmd->transport_complete_callback(), queue target_abort_work on cmd-
> > > work, and
> > return early without setting CMD_T_ABORT_QUEUED or
> > SCF_SENT_CHECK_CONDITION.
> > 
> > If an iSCSI ERL=2 TMR TASK_REASSIGN then runs
> > iscsit_task_reassign_complete_write() in
> > drivers/target/iscsi/iscsi_target_tmr.c, it sees
> > ICF_GOT_LAST_DATAOUT
> > set and
> > CMD_T_SENT cleared, and calls target_execute_cmd() ->
> > target_cmd_interrupted():
> > 
> > drivers/target/iscsi/iscsi_target_tmr.c:iscsit_task_reassign_comple
> > te
> > _write() {
> >     ...
> > 	if (cmd->cmd_flags & ICF_GOT_LAST_DATAOUT) {
> > 		if (!(cmd->se_cmd.transport_state & CMD_T_SENT)) {
> > 			pr_debug("WRITE ITT: 0x%08x: t_state: %d"
> > 				" never sent to transport\n",
> > 				cmd->init_task_tag, cmd-
> > > se_cmd.t_state);
> > 			target_execute_cmd(se_cmd);
> > 			return 0;
> > 		}
> >     ...
> > }
> > 
> > Similarly, fabric error paths such as tcm_qla2xxx_write_pending(),
> > tcm_qla2xxx_handle_data_work(), and isert_rdma_read_done() call
> > transport_generic_request_failure() directly. Could these paths
> > reach
> > both
> > transport_generic_request_failure() and target_cmd_interrupted()
> > (or
> > transport_generic_request_failure() twice) for the same aborted
> > command,
> > re-initializing cmd->work while queued and running
> > cmd->transport_complete_callback() and target_handle_abort() a
> > second
> > time?
> 
> Let me wait on Bart's review but I do see validity in what Sashiko is
> reporting. The complexity in this target code is really hard to deal
> with but I am doing my best. I will also see if I can get AI to check
> it fully first should I come up with a good response to the latest
> sashiko review
> 
> Regards
> Laurence


Hello Bart
I decided to send this along to get some guidance before I finalize
something to deal with Sashiko's latest report.

The Sashiko review of v5 found two things against patch 1. I checked
both against the tree and the second one is a real hole in the patch,
so v6 will follow/

1. transport_generic_request_failure() queues target_abort_work too.

There are exactly two sites in target_core_transport.c that queue
target_abort_work -- target_cmd_interrupted() and
transport_generic_request_failure() -- and v5 guarded only the first.
2c9fa49e100f added the same pattern to both, so guarding one is half a
fix, and the path the review gives is reachable:
target_cmd_interrupted()
returns false because CMD_T_ABORTED is not set yet, execution then
fails
and transport_generic_request_failure() queues the work with nothing
recorded, and a later target_execute_cmd() -- from
iscsit_task_reassign_complete_write(), say -- queues it again. That is
the
re-queue-while-queued this series is supposed to prevent, so patch 1 as
posted did not hold up its own subject line.

v6 factors the test-and-set into __target_claim_abort() and uses it at
both sites. Patch 1's subject is now "Queue the abort of a command at
most
once", which is what it actually guarantees.

2. transport_complete_callback() is still not one-shot.

Also correct. It runs on both branches of target_cmd_interrupted() and
again in transport_generic_request_failure(), so a CMD_T_STOP call
followed by a CMD_T_ABORTED call runs it twice, as does a failure
arriving
after an abort has already been claimed. For COMPARE AND WRITE that is
a
second up(&dev->caw_sem).

This is existing behaviour rather than something the series introduces
--
mainline runs the callback on both branches today -- but patch 1 cites
the
caw_sem double release as part of its rationale, so it is fair to say
the
fix does not match its own description. I have not tried to close it in
v6, because the options are a second flag alongside CMD_T_ABORT_QUEUED
or
clearing cmd->transport_complete_callback after the first success=false
call, and the latter affects target_complete_ok_work(), which calls the
same callback with success=true.

Bart, since the claim is your design: which way would you want that
done, and should it be part of this series or a follow-up? Patch 1
describes the gap explicitly at the end so it is not left implied.

Thanks as always
Laurence

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

[Bart Van Assche]

On 10/26/26 1:15 PM, Laurence Oberman wrote:
> Bart, since the claim is your design: which way would you want that
> done, and should it be part of this series or a follow-up? Patch 1
> describes the gap explicitly at the end so it is not left implied.

My proposal is as follows:
- Fix the COMPARE AND WRITE bug in the COMPARE AND WRITE code. Since it
   is an existing bug, make that fix the first patch in this series.
- Track whether caw_sem is held. Set a per-command flag when down()
   succeeds in sbc_compare_and_write(). Release the semaphore in
   compare_and_write_callback() / compare_and_write_post() only if that
   flag is set, and clear it when releasing.
- This covers a case that your options don't cover. It fixes the double
   release, and also the release of a semaphore that was never taken: an
   aborted COMPARE AND WRITE still in write-pending reaches the callback
   with buffers allocated but without holding caw_sem.

Other feedback:
* Since patch 2 has the Cc: stable tag and since patch 2 depends on
   patch 1, patch 1 should also have a Cc: stable tag.
* Please add a lockless check before taking t_state_lock in
   target_cmd_interrupted() because this function is called for every
   completion.
* The iscsit_stop_dataout_timer() call in patch 2 contradicts its own
   comment that no timer is armed.
* The other early returns for aborted commands in iscsit_execute_cmd()
   need an explanation of why they can't cause a hang.

Thanks,

Bart.