Re: [PATCH 2/2] scsi: target: iscsi: fix hanging WRITE_PENDING commands

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

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