Re: [PATCH v5 1/2] scsi: target: Complete an aborted command at most once

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: target: double transport_complete_callback() via CMD_T_STOP
- [High] scsi: target: unguarded abort queueing in transport_generic_request_failure()
--

commit 9f0a0fc8e2f216b4d0f91c9797ea14d9a91154f8
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: Complete an aborted command at most once

This commit adds the CMD_T_ABORT_QUEUED flag under t_state_lock in
target_cmd_interrupted() so that an aborted command only runs its completion
callback and queues target_abort_work once.

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 927861c30afc4..4896a92392e0e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -882,24 +882,45 @@ static void target_abort_work(struct work_struct *work)
>  	target_handle_abort(cmd);
>  }
>  
> +/*
> + * Returns true if the command has been aborted or stopped, in which case the
> + * caller must not carry on with normal completion.  For an aborted command the
> + * work that must not run twice -- the completion callback and the queueing of
> + * target_abort_work -- is done once, under CMD_T_ABORT_QUEUED.
> + */
>  static bool target_cmd_interrupted(struct se_cmd *cmd)
>  {
> +	bool queue_abort = false, stop = false;
> +	unsigned long flags;
>  	int post_ret;
>  
> +	spin_lock_irqsave(&cmd->t_state_lock, flags);
>  	if (cmd->transport_state & CMD_T_ABORTED) {
> -		if (cmd->transport_complete_callback)
> -			cmd->transport_complete_callback(cmd, false, &post_ret);
> +		if (cmd->transport_state & CMD_T_ABORT_QUEUED) {
> +			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +			return true;
> +		}
> +		cmd->transport_state |= CMD_T_ABORT_QUEUED;
> +		queue_abort = true;
> +	} else if (cmd->transport_state & CMD_T_STOP) {
> +		stop = true;
> +	}
> +	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
> +	if (!queue_abort && !stop)
> +		return false;
> +
> +	if (cmd->transport_complete_callback)
> +		cmd->transport_complete_callback(cmd, false, &post_ret);

[Severity: High]
Can cmd->transport_complete_callback() still be called more than once when a
command hits the CMD_T_STOP path first?

The commit message notes that leaving the CMD_T_STOP branch unguarded is
harmless because complete_all() is idempotent, and explains that a first call
to target_cmd_interrupted() can take the CMD_T_STOP branch before CMD_T_ABORTED
is set afterwards.

However, target_cmd_interrupted() executes cmd->transport_complete_callback()
when either queue_abort or stop is true, without clearing the callback pointer
or guarding its execution. In drivers/target/target_core_sbc.c, both
compare_and_write_callback() and compare_and_write_post() release dev->caw_sem
without clearing cmd->transport_complete_callback:

drivers/target/target_core_sbc.c:compare_and_write_callback() {
    ...
out:
	/*
	 * In the MISCOMPARE or failure case, unlock ->caw_sem obtained in
	 * sbc_compare_and_write() before the original READ I/O submission.
	 */
	up(&dev->caw_sem);
	sg_free_table(&write_tbl);
	return ret;
}

If a first call to target_cmd_interrupted() takes the CMD_T_STOP branch and a
subsequent call takes either the CMD_T_ABORTED branch (after CMD_T_ABORTED is
set) or the CMD_T_STOP branch a second time, won't
cmd->transport_complete_callback() run twice and call up(&dev->caw_sem) a
second time?

> +
> +	if (queue_abort) {
>  		INIT_WORK(&cmd->work, target_abort_work);
>  		queue_work(target_completion_wq, &cmd->work);
> -		return true;
> -	} else if (cmd->transport_state & CMD_T_STOP) {
> -		if (cmd->transport_complete_callback)
> -			cmd->transport_complete_callback(cmd, false, &post_ret);
> +	} else {
>  		complete_all(&cmd->t_transport_stop_comp);
> -		return true;
>  	}
>  
> -	return false;
> +	return true;
>  }

[Severity: High]
Does transport_generic_request_failure() in
drivers/target/target_core_transport.c also need to test and set
CMD_T_ABORT_QUEUED under cmd->t_state_lock?

Commit 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET handling
synchronous") added the same abort handling pattern to both
target_cmd_interrupted() and transport_generic_request_failure():

drivers/target/target_core_transport.c:transport_generic_request_failure() {
    ...
	if (cmd->transport_complete_callback)
		cmd->transport_complete_callback(cmd, false, &post_ret);

	if (cmd->transport_state & CMD_T_ABORTED) {
		INIT_WORK(&cmd->work, target_abort_work);
		queue_work(target_completion_wq, &cmd->work);
		return;
	}
    ...
}

Because transport_generic_request_failure() does not check or set
CMD_T_ABORT_QUEUED under cmd->t_state_lock, if CMD_T_ABORTED is set after
target_cmd_interrupted() returns false in target_execute_cmd(), and execution
then fails in target_write_prot_action() or __target_execute_cmd() (for
example, when tcmu_queue_cmd() sees CMD_T_ABORTED and returns
TCM_CHECK_CONDITION_ABORT_CMD, or when target_scsi3_ua_check() fails):

drivers/target/target_core_transport.c:__target_execute_cmd() {
    ...
err:
	spin_lock_irq(&cmd->t_state_lock);
	cmd->transport_state &= ~CMD_T_SENT;
	spin_unlock_irq(&cmd->t_state_lock);

	transport_generic_request_failure(cmd, ret);
}

transport_generic_request_failure() will invoke
cmd->transport_complete_callback(), queue target_abort_work on cmd->work, and
return early without setting CMD_T_ABORT_QUEUED or SCF_SENT_CHECK_CONDITION.

If an iSCSI ERL=2 TMR TASK_REASSIGN then runs
iscsit_task_reassign_complete_write() in
drivers/target/iscsi/iscsi_target_tmr.c, it sees ICF_GOT_LAST_DATAOUT set and
CMD_T_SENT cleared, and calls target_execute_cmd() ->
target_cmd_interrupted():

drivers/target/iscsi/iscsi_target_tmr.c:iscsit_task_reassign_complete_write() {
    ...
	if (cmd->cmd_flags & ICF_GOT_LAST_DATAOUT) {
		if (!(cmd->se_cmd.transport_state & CMD_T_SENT)) {
			pr_debug("WRITE ITT: 0x%08x: t_state: %d"
				" never sent to transport\n",
				cmd->init_task_tag, cmd->se_cmd.t_state);
			target_execute_cmd(se_cmd);
			return 0;
		}
    ...
}

Similarly, fabric error paths such as tcm_qla2xxx_write_pending(),
tcm_qla2xxx_handle_data_work(), and isert_rdma_read_done() call
transport_generic_request_failure() directly. Could these paths reach both
transport_generic_request_failure() and target_cmd_interrupted() (or
transport_generic_request_failure() twice) for the same aborted command,
re-initializing cmd->work while queued and running
cmd->transport_complete_callback() and target_handle_abort() a second time?

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