Re: [PATCH v5 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:

New issues:
- [Critical] Data Race / List Corruption and Use-After-Free due to incorrectly dropping fnic_lock before sending ABTS.

Pre-existing issues:
- [High] Use-After-Free in the timer callback due to missing timer_delete_sync() before freeing memory.
--

commit da372c5b4e5660f32542c6cb182b777f4f2e3531
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 16e2f0add5ce6..c4de6606c0b2f 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c

[ ... ]

> @@ -1248,6 +1252,23 @@ void nvfnic_ls_rsp_recv(struct fnic_iport_s *iport,
>  	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the timer cancellation in
nvfnic_ls_rsp_recv() have a use-after-free race with the timer callback?

In nvfnic_ls_rsp_recv(), the code calls the non-synchronous timer_delete():

    timer_delete(&nvfnic_ls_req->ls_req_timer);
    ...
    spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
    lsreq->done(lsreq, status);

Because timer_delete() doesn't wait for the timer callback to finish on other
CPUs, if nvfnic_ls_req_timeout() is concurrently executing and spinning to
acquire fnic_lock, couldn't the lock be dropped here, allowing lsreq->done()
to immediately free the memory, while the timer callback proceeds to access it?

[ ... ]

> @@ -1290,6 +1310,19 @@ void nvfnic_ls_req_timeout(struct timer_list *t)
>  		FNIC_NVME_DBG(KERN_ERR, fnic,
>  			      "tport: 0x%x lsreq: 0x%x sending abort\n",
>  			      tport->fcid, nvfnic_ls_req->oxid);
> +		nvfnic_ls_req->state = FNIC_LS_REQ_CMD_ABTS_PENDING;
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
> +
> +		if (fdls_send_ls_req_abts(iport, tport, nvfnic_ls_req->oxid) == 0) {

[Severity: Critical]
Does dropping fnic_lock here before calling fdls_send_ls_req_abts() expose the
driver to list corruption?

fdls_send_ls_req_abts() calls fnic_send_fcoe_frame(), which eventually calls
fdls_send_fcoe_frame(). That function appears to perform a lockless 
list_add_tail(&frame_elem->links, &fnic->tx_queue), relying on the caller
to hold fnic_lock.

Additionally, does dropping the lock here expose the tport pointer to
concurrent deletion, potentially causing a use-after-free?

> +			timeout = FNIC_LS_REQ_TMO_MSECS(ls_req->timeout);
> +			mod_timer(&nvfnic_ls_req->ls_req_timer,
> +				  round_jiffies(jiffies + msecs_to_jiffies(timeout)));
> +			return;
> +		}

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