Re: [PATCH v6 10/13] scsi: fnic: Abort timed-out NVMe LS requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-after-free of `tport` in `nvfnic_ls_req_abort` due to concurrent target port deletion.
- [High] Immediate reuse of an Exchange ID (OXID) on ABTS timeout risks cross-talk with delayed target responses.
--

--- Patch [10]: [PATCH v6 10/13] scsi: fnic: Abort timed-out NVMe LS requests ---
commit 39245a565a721e78c479fc226cff0b04ea8531d0
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Abort timed-out NVMe LS requests

Add an FDLS helper that sends ABTS frames for outstanding NVMe LS
requests.

Use the active LS request OXID when building the ABTS frame, send it
through the FCoE transmit path, and call it from LS timeout and abort
handling.

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index 0b05dc9698be2..aa3c935d08155 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> @@ -1279,6 +1283,23 @@ void nvfnic_ls_rsp_recv(struct fnic_iport_s *iport,
>  	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
>  }
>  
> +static bool nvfnic_ls_req_cleanup(struct fnic_iport_s *iport,
> +				  struct nvmefc_ls_req *lsreq,
> +				  uint16_t oxid)
> +{
> +	struct nvfnic_ls_req *nvfnic_ls_req = lsreq->private;
> +
> +	if (!nvfnic_ls_req)
> +		return false;
> +
> +	lsreq->private = NULL;
> +	list_del(&nvfnic_ls_req->list);
> +	fdls_free_oxid(iport, oxid, &nvfnic_ls_req->oxid);

[Severity: High]
Is it safe to immediately reuse the OXID on an ABTS timeout, or should it
be quarantined for 2 * R_A_TOV?

When an NVMe LS request's ABTS frame times out, nvfnic_ls_req_timeout()
calls nvfnic_ls_req_cleanup(), which unconditionally calls fdls_free_oxid()
and returns the OXID to the active free pool.

According to Fibre Channel specifications, if an abort times out, the OXID
must be quarantined for 2 * R_A_TOV because a slow target might still send
a delayed response to the original aborted exchange.

If the OXID is reused immediately for a new request to the same target,
could the delayed response be incorrectly matched to the new request,
causing logic errors and potential data corruption?

> +	nvfnic_ls_req->state = FNIC_LS_REQ_CMD_COMPLETE;
> +
> +	return true;
> +}
> +

[ ... ]

> @@ -1648,10 +1675,24 @@ void nvfnic_ls_req_abort(struct nvme_fc_local_port *lport,
>  
>  	/* Mark the state and flags */
>  	nvfnic_ls_req->state = FNIC_LS_REQ_CMD_ABTS_PENDING;
> -	timeout = FNIC_LS_REQ_TMO_MSECS(lsreq->timeout);
> -	mod_timer(&nvfnic_ls_req->ls_req_timer,
> -		  round_jiffies(jiffies + msecs_to_jiffies(timeout)));
> +	oxid = nvfnic_ls_req->oxid;
> +
> +	ret = fdls_send_ls_req_abts(iport, tport, oxid);

[Severity: High]
Can this result in a use-after-free of tport if nvfnic_delete_tport() is
invoked concurrently?

If nvfnic_delete_tport() is invoked twice for the same target port (e.g.
concurrently from the port event handler and the unload/cleanup path), the
second call to nvme_fc_unregister_remoteport() will return -EINVAL since
it is already marked for deletion.

The error path in nvfnic_delete_tport() then calls
nvfnic_terminate_tport_ls_reqs(), which explicitly skips requests in the
FNIC_LS_REQ_CMD_ABTS_STARTED state, and immediately calls kfree(tport).

Concurrently, nvfnic_ls_req_abort() may be in timer_delete_sync() with the
lock dropped and the request in the ABTS_STARTED state. When
nvfnic_ls_req_abort() wakes up and reacquires the lock, it checks
lsreq->private (which was left intact by the cleanup path) and calls
fdls_send_ls_req_abts(iport, tport, oxid). This dereferences the tport that
was just freed.

> +	if (!ret) {
> +		timeout = FNIC_LS_REQ_TMO_MSECS(lsreq->timeout);
> +		mod_timer(&nvfnic_ls_req->ls_req_timer,
> +			  round_jiffies(jiffies + msecs_to_jiffies(timeout)));
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +		return;
> +	}
> +
> +	if (!nvfnic_ls_req_cleanup(iport, lsreq, oxid)) {
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +		return;
> +	}
> +
>  	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +	lsreq->done(lsreq, -EAGAIN);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724174811.5118-1-kartilak@cisco.com?part=10