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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Dropping fnic_lock before network transmission and accessing the NVMe LS request on failure leads to Use-After-Free and double completions.

Pre-existing issues:
- [High] Calling timer_delete_sync() while holding fnic->fnic_lock causes an ABBA deadlock with the timer callback.
- [High] Dropping a spinlock inside list_for_each_entry_safe invalidates the 'next' pointer.
- [High] Saving spin_lock_irqsave interrupt state into a shared struct member corrupts interrupt state.
--

commit af4855e82320c32dd587460223ab9977658df024
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 014660725373b..9cc8f0baf09cb 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does calling timer_delete_sync while
holding fnic->fnic_lock in nvfnic_ls_rsp_recv cause an ABBA deadlock?

If the timer callback unconditionally requests fnic_lock, waiting for the
timer to complete while holding the lock will deadlock:

nvfnic_ls_rsp_recv()
    spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
    ...
    timer_delete_sync(&nvfnic_ls_req->ls_req_timer);

[ ... ]

> @@ -1274,6 +1294,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) {
> +			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;
> +		}
> +		FNIC_NVME_DBG(KERN_ERR, fnic,
> +			      "tport: 0x%x lsreq: 0x%x cannot send abort\n",
> +			      tport->fcid, oxid);
> +		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

[Severity: High]
This is a pre-existing issue, but does saving the interrupt state into the
shared fnic->lock_flags struct member risk corrupting CPU IRQ states
during spinlock contention?

Using a global variable for IRQ state instead of a local stack variable
can lead to corruption when the lock is contended.

[ ... ]

> @@ -1579,10 +1607,25 @@ 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;
> +	oxid = nvfnic_ls_req->oxid;
>  	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);
> +
> +	ret = fdls_send_ls_req_abts(iport, tport, oxid);
> +	if (!ret)
> +		return;
> +
> +	timer_delete_sync(&nvfnic_ls_req->ls_req_timer);

[Severity: Critical]
Can dropping fnic_lock before network transmission and then accessing
nvfnic_ls_req after transmission failure lead to a use-after-free and
double completions?

Since memory ownership is transferred to the timer when mod_timer is
called, the timer could fire while the lock is dropped. This could free
nvfnic_ls_req before we reach timer_delete_sync.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock inside
list_for_each_entry_safe invalidate the next pointer?

In drivers/scsi/fnic/fnic_nvme.c:nvfnic_cleanup_tport_io():

list_for_each_entry_safe(nvfnic_ls_req, next, &(tport->ls_req_list), list)

If fnic_lock is dropped inside this loop, concurrent modifications to the
list can invalidate the next pointer, leading to a use-after-free.

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