Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [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 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 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 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