Re: [PATCH v3 4/4] scsi: target: iscsi: Claim the completion on the DataOut submit path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: target: iscsi: CMD_T_ABORTED race in iscsit_check_dataout_payload()
--

commit 106a0e34d325c477082712ef599ef2736992fc34
Author: Laurence Oberman <loberman@redhat.com>

scsi: target: iscsi: Claim the completion on the DataOut submit path

This commit checks for CMD_T_ABORTED in iscsit_check_dataout_payload() before
submitting the command to target_execute_cmd(), completing already-aborted
commands via iscsit_complete_aborted_cmd() so that completion_claimed is set
and connection teardown does not complete the command a second time.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 83184de284825..e6113a549e053 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1699,6 +1699,16 @@ iscsit_check_dataout_payload(struct iscsit_cmd *cmd, struct iscsi_data *hdr,
>  		iscsit_stop_dataout_timer(cmd);
>  		if (ooo_cmdsn)
>  			return 0;
> +		/*
> +		 * target_execute_cmd() tests CMD_T_ABORTED before it touches
> +		 * t_state, so for an aborted command it would queue
> +		 * target_abort_work and return with the command still in
> +		 * TRANSPORT_WRITE_PENDING and the completion unclaimed.
> +		 */
> +		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> +			iscsit_complete_aborted_cmd(cmd);
> +			return 0;
> +		}
>  		target_execute_cmd(&cmd->se_cmd);

[Severity: High]
Can CMD_T_ABORTED still be set after the check in
iscsit_check_dataout_payload() but before target_execute_cmd() updates
t_state, leaving the completion unclaimed?

If CMD_T_ABORTED is not yet set at this check, iscsit_check_dataout_payload()
calls target_execute_cmd() with cmd->completion_claimed still false and
cmd->se_cmd.t_state still in TRANSPORT_WRITE_PENDING.

A concurrent task management request (such as ABORT_TASK via
core_tmr_abort_task() or LUN_RESET via core_tmr_drain_state_list()) calling
__target_check_io_state() can then set CMD_T_ABORTED before
target_execute_cmd() calls target_cmd_interrupted():

drivers/target/target_core_tmr.c:__target_check_io_state()
    spin_lock_irqsave(&se_cmd->t_state_lock, flags);
    ...
    se_cmd->transport_state |= CMD_T_ABORTED;

When target_execute_cmd() runs, it calls target_cmd_interrupted() before
taking t_state_lock to transition t_state 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 target_cmd_interrupted() sees CMD_T_ABORTED, it queues
target_abort_work on target_completion_wq and returns true, so
target_execute_cmd() returns with t_state still in TRANSPORT_WRITE_PENDING
and completion_claimed still false.

If the connection then drops, iscsit_release_commands_from_conn() sees
TRANSPORT_WRITE_PENDING and claims the completion:

drivers/target/iscsi/iscsi_target.c:iscsit_release_commands_from_conn()
    if (cmd->se_cmd.t_state == TRANSPORT_WRITE_PENDING) {
        bool claimed = iscsit_claim_completion_locked(cmd);

        spin_unlock_irq(&se_cmd->t_state_lock);
        if (claimed)
            target_complete_cmd(&cmd->se_cmd,
                                SAM_STAT_TASK_ABORTED);
        continue;
    }

Wouldn't that call target_complete_cmd() and re-queue cmd->se_cmd.work while
target_abort_work is already queued or running?

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