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

[sashiko-bot@kernel.org]

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