Re: [PATCH 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 1 potential issue(s) to consider:
- [High] iscsi: workqueue corruption via double completion of aborted cmd
--

commit 1cef49db01d5c1eb5e9670fc6f016dfd14ddd5cd
Author: Laurence Oberman <loberman@redhat.com>

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

iscsit_check_dataout_payload() hands a command to iscsit_get_dataout() to
request the next burst whenever iscsit_check_post_dataout() returns
DATAOUT_SEND_R2T, without checking CMD_T_ABORTED first. This patch guards
for aborted commands by stopping the dataout timer and completing the command.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index aab9237a67768..87b215b18034e 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1669,6 +1669,25 @@ 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) {
[ ... ]
> +		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> +			if (cmd->outstanding_r2ts < 1) {
> +				iscsit_stop_dataout_timer(cmd);
> +				target_complete_cmd(&cmd->se_cmd,
> +						    SAM_STAT_TASK_ABORTED);

[Severity: High]
Can calling target_complete_cmd() directly on an aborted command bypass the
synchronization wrapper iscsit_complete_aborted_cmd()?

Because this bypasses iscsit_claim_completion_locked(), could concurrent
session teardown or other abort paths also claim the command and call
target_complete_cmd()?

This concurrent modification could lead to target_cmd_interrupted() executing
INIT_WORK(&cmd->work, target_abort_work) and queue_work() concurrently on an
already active work item, which might destroy the workqueue linked list.

There is also a preexisting location in the header parsing path that exposes
the same unsynchronized completion race, which appears to have been copied
here and left unfixed:

drivers/target/iscsi/iscsi_target.c:__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);
			}
    ...
}

Would it be safer to use iscsit_complete_aborted_cmd() in both places to
ensure the completion is safely claimed?

> +			}
> +			return 0;
> +		}
> +
>  		iscsit_set_dataout_sequence_values(cmd);

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