Re: [PATCH v2 1/2] scsi: libiscsi: only complete a task that has been sent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi: libiscsi: premature ISCSI_TASK_RUNNING before xmit ref
  acquisition
--

commit 7682c2a848ebe294348224903c5fc89275f307e1
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: libiscsi: only complete a task that has been sent

This commit uses smp_store_release() to mark a task ISCSI_TASK_RUNNING in
iscsi_prep_scsi_cmd_pdu() and iscsi_prep_mgmt_task(), and checks for
ISCSI_TASK_RUNNING with smp_load_acquire() in the RX path before
completing a management or rejected task.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 2d04448432a77..83d726043d3e8 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -429,7 +429,8 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
>  	if (session->tt->init_task && session->tt->init_task(task))
>  		return -EIO;
>  
> -	task->state = ISCSI_TASK_RUNNING;
> +	/* Pairs with the acquire in the RX task lookups. */
> +	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
>  	session->cmdsn++;
>  
>  	conn->scsicmd_pdus_cnt++;
> @@ -702,7 +703,8 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
>  	if ((hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_LOGOUT)
>  		session->state = ISCSI_STATE_LOGGING_OUT;
>  
> -	task->state = ISCSI_TASK_RUNNING;
> +	/* Pairs with the acquire in the RX task lookups. */
> +	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
>  	ISCSI_DBG_SESSION(session, "mgmtpdu [op 0x%x hdr->itt 0x%x "
>  			  "datalen %d]\n", hdr->opcode & ISCSI_OPCODE_MASK,
>  			  hdr->itt, task->data_count);

[Severity: High]
Can the RX path still complete and free task after
smp_store_release(&task->state, ISCSI_TASK_RUNNING) before the transmit
path finishes using it or acquires its reference?

iscsi_data_xmit() runs under session->frwd_lock and calls
iscsi_prep_mgmt_task() (or iscsi_prep_scsi_cmd_pdu()) while task->refcount
is still 1, before iscsi_xmit_task() takes the transmit reference.  Since
__iscsi_complete_pdu() and iscsi_handle_reject() run concurrently under
session->back_lock, an unsolicited response or Reject naming that ITT can
arrive in between:

kworker (CPU0)                    <softirq> (CPU1)
iscsi_data_xmit()
  spin_lock_bh(&session->frwd_lock)
  iscsi_prep_mgmt_task(conn, task)
    smp_store_release(&task->state,
                      ISCSI_TASK_RUNNING)
                                  __iscsi_complete_pdu()
                                    spin_lock(&session->back_lock)
                                    smp_load_acquire(&task->state)
                                      == ISCSI_TASK_RUNNING
                                    iscsi_complete_task(task, ...)
                                      __iscsi_put_task(session, task)
                                        iscsi_free_task(task)
    ISCSI_DBG_SESSION(...)
      hdr->opcode                 <-- use-after-free if cleanup_task
                                      freed task->hdr
  iscsi_xmit_task(..., task, false)
    iscsi_get_task(task)          <-- fails (refcount == 0),
                                      triggers WARN_ON_ONCE(1)

Even if iscsi_get_task() runs before the RX completion, iscsi_xmit_task()
drops session->frwd_lock around session->tt->xmit_task(task); if RX
completes the task during that window and xmit_task() returns an error
such as -EAGAIN, iscsi_xmit_task() unconditionally calls
iscsi_get_task(task) and saves the completed task in conn->task for
retransmission:

drivers/scsi/libiscsi.c:iscsi_xmit_task() {
    ...
	spin_unlock_bh(&session->frwd_lock);
	rc = session->tt->xmit_task(task);
	spin_lock_bh(&session->frwd_lock);
	if (!rc) {
		/* done with this task */
		task->last_xfer = jiffies;
	} else {
		/*
		 * get an extra ref that is released next time we access it
		 * as conn->task above.
		 */
		iscsi_get_task(task);
		conn->task = task;
	}
    ...
}

In addition, for transports without ihost->workq, iscsi_send_mgmt_task()
and iscsi_queuecommand() call iscsi_prep_mgmt_task() or
iscsi_prep_scsi_cmd_pdu() and invoke session->tt->xmit_task(task) directly
without taking an extra task reference at all, so an RX completion after
ISCSI_TASK_RUNNING is stored frees task while xmit_task() is running (and
if xmit_task() fails, the caller puts task a second time).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113233938.397415-1-yhlee@isslab.korea.ac.kr?part=1