Re: [PATCH v2 2/2] scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iscsi: double completion due to incomplete CMD_T_ABORTED fix
- [Medium] iscsi: data race on cmd->outstanding_r2ts during abort
--

commit 538cfb5a3bbcee47d2aa78e30d5cf64fe5bed2fe
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: iscsi: Complete an aborted WRITE when a new R2T would be sent

This commit adds a check for aborted commands in the DataOut processing
path when a new R2T is requested. It ensures the command is safely
completed using iscsit_complete_aborted_cmd() to claim the completion.
This prevents a race condition where a connection drop could cause a
double-completion of the aborted command.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index aab9237a67768..3dad7f69f3bae 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1669,6 +1669,22 @@ iscsit_check_dataout_payload(struct iscsit_cmd *cmd, struct iscsi_data *hdr,
>  	if ((rc == DATAOUT_NORMAL) || (rc == DATAOUT_WITHIN_COMMAND_RECOVERY))
>  		return 0;
>  	else if (rc == DATAOUT_SEND_R2T) {
> +		/*
> +		 * A new R2T would never be answered for an aborted command.
> +		 * The CMD_T_ABORTED handling in __iscsit_check_dataout_hdr()
> +		 * does not cover this: it only runs for DataOut PDUs
> +		 * belonging to R2Ts that were already issued, and
> +		 * iscsit_dataout_update_r2t() has already decremented
> +		 * outstanding_r2ts for the burst that just finished.  Guard
> +		 * on outstanding_r2ts so that with MaxOutstandingR2T > 1 the
> +		 * completion is still left to the last R2T to drain.
> +		 */
> +		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> +			if (cmd->outstanding_r2ts < 1)

[Severity: High]
This patch introduces iscsit_complete_aborted_cmd() to fix a double-completion
race by safely claiming the completion. However, does this still leave two
other paths vulnerable to the same issue?

If cmd->outstanding_r2ts >= 1, the patch explicitly leaves completion to
__iscsit_check_dataout_hdr(). But __iscsit_check_dataout_hdr() uses
target_complete_cmd() directly without claiming the completion:

__iscsit_check_dataout_hdr() {
    ...
		if (se_cmd->transport_state & CMD_T_ABORTED) {
			if (hdr->flags & ISCSI_FLAG_CMD_FINAL &&
			    --cmd->outstanding_r2ts < 1) {
				iscsit_stop_dataout_timer(cmd);
				target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
			}
    ...
}

Similarly, if the final payload for an aborted command is received in this
function, it reaches DATAOUT_SEND_TO_TRANSPORT and calls target_execute_cmd(),
which also queues the abort work without claiming the completion:

iscsit_check_dataout_payload() {
    ...
	} else if (rc == DATAOUT_SEND_TO_TRANSPORT) {
        ...
		target_execute_cmd(&cmd->se_cmd);
		return 0;
    ...
}

Could a connection drop in either of these windows allow
iscsit_release_commands_from_conn() to complete the command a second time,
resulting in corruption of the target_completion_wq?


[Severity: Medium]
Is it safe to read cmd->outstanding_r2ts without holding cmd->r2t_lock here?

If iscsit_build_r2ts_for_cmd() encounters an allocation failure, the target
core queues the command to qf_work_queue. The worker thread will retry
building the R2T asynchronously via transport_write_pending_qf() ->
iscsit_build_r2ts_for_cmd(), incrementing the counter under cmd->r2t_lock:

iscsit_build_r2ts_for_cmd() {
    ...
		cmd->outstanding_r2ts++;
		first_r2t = 0;

		if (iscsit_add_r2t_to_list(cmd, offset, xfer_len, 0, 0) < 0) {
    ...
}

If the command is aborted concurrently, the RX thread processing incoming
DataOut PDUs will locklessly decrement the counter in
__iscsit_check_dataout_hdr() and locklessly read it here. Could this
concurrent non-atomic access cause lost updates, leading to a corrupted
counter and a leaked command?

> +				iscsit_complete_aborted_cmd(cmd);
> +			return 0;
> +		}
> +
>  		iscsit_set_dataout_sequence_values(cmd);
>  		conn->conn_transport->iscsit_get_dataout(conn, cmd, false);
>  	} else if (rc == DATAOUT_SEND_TO_TRANSPORT) {

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