Re: [PATCH v6 06/13] scsi: fnic: Add the NVMe/FC transport path

[sashiko-bot@kernel.org]

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

New issues:
- [High] The NVMe FC transport template `nvfnic_port` omits the mandatory `.ls_req` callback, causing `nvme_fc_register_localport()` to unconditionally fail and preventing NVMe initialization.
- [High] NVMe I/O completion and ITMF handlers are completely disconnected from the CQ polling loop, causing all NVMe I/Os to hang indefinitely.
- [High] The `ls_req_timer` in `struct nvfnic_ls_req` is armed without ever being initialized.
- [High] Concurrent execution of `nvfnic_delete_tport` results in stack corruption, double free, and use-after-free of the `tport` structure.
- [High] A TOCTOU race between abort WQ enqueue and fast completions causes IO requests to permanently hang.
- [High] `tport->ls_req_list` is not initialized with `INIT_LIST_HEAD()`, causing a kernel panic when iterated.
- [High] `ls_req` is left on `tport->ls_req_list` during abort, leading to Use-After-Free and list corruption when `tport` is freed.
- [High] Timeout during local port unregistration leads to a use-after-free of the `nv_tmpl` and `fnic` structures.
- [Medium] `fnic_queue_wq_copy_desc_itmf` leaks uninitialized descriptor memory to firmware when `lun` is NULL.

Pre-existing issues:
- [High] Dropping a spinlock inside a `list_for_each_entry_safe` loop invalidates the safe iteration, leading to list corruption and kernel panic.
- [High] The driver incorrectly uses a shared struct member (`fnic->lock_flags`) for saving CPU-local IRQ state during `spin_lock_irqsave`.
--

--- Patch [6]: [PATCH v6 06/13] scsi: fnic: Add the NVMe/FC transport path ---
commit 320b83a04180e41753d3bc332a151c194da1ab86
Author: Karan Tilak Kumar <kartilak@cisco.com>
Subject: scsi: fnic: Add the NVMe/FC transport path

This commit introduces the NVMe over FC transport path for the fnic driver.
It adds logic for local-port and remote-port registration, I/O submission,
tag management, completion handling, and cleanup operations.

Closes: https://lore.kernel.org/oe-kbuild-all/202605280430.wTYAqI3A-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202605280619.pmobiDWp-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202605280519.Jd4fmgAZ-lkp@intel.com/

> diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
> --- a/drivers/scsi/fnic/fnic_fcs.c
> +++ b/drivers/scsi/fnic/fnic_fcs.c
[ ... ]
> @@ -1020,6 +1019,8 @@ void fnic_delete_fcp_tports(struct fnic *fnic)
>  		fnic_del_tport_timer_sync(fnic, tport);
>  		if (IS_FNIC_FCP_INITIATOR(fnic))
>  			fnic_fdls_remove_tport(&fnic->iport, tport, flags);
> +		else if (IS_FNIC_NVME_INITIATOR(fnic))
> +			nvfnic_delete_tport(&fnic->iport, tport, flags);

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock inside the
list_for_each_entry_safe() loop corrupt the list?

In fnic_delete_fcp_tports(), the loop holds fnic_lock and calls
nvfnic_delete_tport(), which explicitly drops fnic_lock. If a concurrent
task frees the cached next pointer while the lock is dropped, the loop will
dereference freed memory when it resumes.

[ ... ]
> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/scsi/fnic/fnic_nvme.c
[ ... ]
> +void nvfnic_fcpio_nvme_fast_cmpl_handler(struct fnic *fnic,
> +				       struct fcpio_fw_req *desc)
> +{
[ ... ]
> +	if (io_req->cmd_state == FNIC_IOREQ_ABTS_PENDING) {
> +		/*
> +		 * set the FNIC_IO_DONE so that this doesn't get
> +		 * flagged as 'out of order' if it was not aborted
> +		 */
> +		io_req->cmd_flags |= FNIC_IO_DONE;
> +		io_req->cmd_flags |= FNIC_IO_ABTS_PENDING;

[Severity: High]
Will this sequence cause an I/O request to hang permanently?

In nvfnic_fcpio_nvme_fast_cmpl_handler(), if the IO state is
FNIC_IOREQ_ABTS_PENDING, the completion is discarded. If nvfnic_fcpio_abort()
then fails to enqueue the abort (for example, if the WQ is full), it restores
the state back to FNIC_IOREQ_CMD_PENDING and returns. Since the hardware
completion was already dropped, the I/O would remain pending forever.

[ ... ]
> +void nvfnic_local_port_delete(struct nvme_fc_local_port *lport)
> +{
> +	struct fnic_iport_s *iport = (struct fnic_iport_s *) lport->private;
> +	struct fnic *fnic = iport->fnic;
> +	unsigned long flags = 0;
> +
> +	FNIC_NVME_DBG(KERN_INFO, fnic, "lport delete 0x%x\n",
> +		      iport->fcid);
> +
> +	spin_lock_irqsave(&fnic->fnic_lock, flags);

[Severity: High]
Could a timeout during unload lead to a use-after-free here?

If the wait_for_completion_timeout() in nvfnic_nvme_unload() expires, the
driver frees iport->nv_tmpl and eventually the fnic structure. However, if
the NVMe midlayer later invokes the localport_delete callback
(nvfnic_local_port_delete()), it will dereference the freed iport and fnic
memory and attempt to acquire the freed fnic_lock.

[ ... ]
> +void
> +nvfnic_cleanup_tport_io(struct fnic *fnic, struct fnic_tport_s *tport)
> +{
> +	unsigned long flags;
> +	struct nvfnic_ls_req *nvfnic_ls_req, *next;
> +	struct nvmefc_ls_req *lsreq;
> +	uint16_t oxid;
> +	LIST_HEAD(done_reqs);
> +
> +	spin_lock_irqsave(&fnic->fnic_lock, flags);
> +	list_for_each_entry_safe(nvfnic_ls_req, next,
> +				 &(tport->ls_req_list), list) {
> +		lsreq = nvfnic_ls_req->ls_req;
> +		if (!lsreq || (lsreq->private == NULL)) {
> +			FNIC_NVME_DBG(KERN_INFO, fnic,
> +				"fnic_cleanup_tport_io lsreq NULL\n");
> +			continue;
> +		}
> +		if (nvfnic_ls_req->state == FNIC_LS_REQ_CMD_ABTS_STARTED) {
> +			FNIC_NVME_DBG(KERN_INFO, fnic,
> +				"fnic_cleanup_tport_io lsreq abort started\n");
> +			continue;
> +		}

[Severity: High]
Does skipping list_del_init() for aborting requests lead to a use-after-free
of the tport?

If the request state is FNIC_LS_REQ_CMD_ABTS_STARTED, the continue statement
skips removing the request from tport->ls_req_list. Once the tport is freed,
the subsequent abort completion or timeout callback will call list_del() on
nvfnic_ls_req->list, corrupting the freed tport memory.

[ ... ]
> +void
> +nvfnic_terminate_tport_ls_reqs(struct fnic *fnic, struct fnic_tport_s *tport)
> +{
> +	struct nvmefc_ls_req *lsreq;
> +	struct nvfnic_ls_req *nvfnic_ls_req, *next;
> +	int count = 0;
> +	uint16_t oxid;
> +	LIST_HEAD(done_reqs);
> +
> +	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

[Severity: High]
This is a pre-existing issue, but does using the shared fnic->lock_flags
corrupt CPU-local IRQ state?

Calling spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags) overwrites the
shared structure member. If multiple CPUs contend for this lock, they will
overwrite each other's saved IRQ states, leading to an incorrect state
restoration upon unlock.

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

[Severity: High]
Will iterating this list trigger a kernel panic?

The tport structure is allocated with kzalloc(), but tport->ls_req_list is
never initialized with INIT_LIST_HEAD(). When list_for_each_entry_safe()
executes, it will dereference a NULL next pointer.

[ ... ]
> +struct
> +nvme_fc_port_template nvfnic_port = {
> +	.localport_delete = nvfnic_local_port_delete,
> +	.remoteport_delete = nvfnic_remote_port_delete,
> +	.create_queue = nvfnic_create_queue,
> +	.delete_queue = NULL,
> +	.ls_req = NULL,

[Severity: High]
Does the omitted ls_req callback cause the local port registration to always
fail?

The nvfnic_port template explicitly sets .ls_req to NULL. When
nvfnic_add_lport() calls nvme_fc_register_localport(), the NVMe transport
validates the presence of the ls_req callback and will unconditionally return
-EINVAL if it is missing. This prevents NVMe initialization entirely.

[ ... ]
> +void nvfnic_nvme_unload(struct fnic *fnic)
> +{
> +	int ret = 0;
> +	struct fnic_iport_s *iport = &fnic->iport;
> +	unsigned long flags;
> +	unsigned int time_wait =  FNIC_NVME_LPORT_REMOVE_WAIT;
> +	unsigned int time_remain;
> +	DECLARE_COMPLETION_ONSTACK(nvme_lport_unreg_done);
[ ... ]
> +			time_remain = wait_for_completion_timeout(
> +				fnic->nvme_lport_unreg_done,
> +				msecs_to_jiffies(time_wait));
> +			if (!time_remain) {
> +				FNIC_NVME_DBG(KERN_ERR, fnic,
> +					      "Local port removal timed out\n");
> +				WARN_ON(1);
> +			}
> +			iport->flags &= ~FNIC_LPORT_NVME_REGISTERED;
> +			kfree(iport->nv_tmpl);

[Severity: High]
Could a timeout here lead to a use-after-free?

If the wait_for_completion_timeout() in nvfnic_nvme_unload() expires, the
driver frees iport->nv_tmpl and eventually the fnic structure. However, if
the NVMe midlayer later invokes the localport_delete callback
(nvfnic_local_port_delete()), it will dereference the freed iport and fnic
memory and attempt to acquire the freed fnic_lock.

[ ... ]
> +void nvfnic_ls_req_abort(struct nvme_fc_local_port *lport,
> +		   struct nvme_fc_remote_port *rport,
> +		   struct nvmefc_ls_req *lsreq)
> +{
[ ... ]
> +	/* 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)));

[Severity: High]
Could arming this timer trigger a kernel panic?

In nvfnic_ls_req_abort(), mod_timer() is called on
nvfnic_ls_req->ls_req_timer. However, timer_setup() is never called for this
timer during the request's lifecycle. Calling mod_timer() on an uninitialized
timer structure leads to list corruption.

[ ... ]
> +void nvfnic_delete_tport(struct fnic_iport_s *iport,
> +						struct fnic_tport_s *tport,
> +						unsigned long flags)
> +{
[ ... ]
> +	if (!(tport->flags & FNIC_FDLS_NVME_REGISTERED)) {
> +		FNIC_NVME_DBG(KERN_ERR, fnic,
> +			"0x%x: tport: 0x%x not registered. Freeing\n",
> +		    iport->fcid, tport->fcid);
> +		list_del(&tport->links);
> +		kfree(tport);
> +		return;
> +	}
> +
> +	tport->tport_del_done = &tm_done;

[Severity: High]
Is it possible for concurrent teardown paths to corrupt the stack and
double-free the tport?

The FNIC_FDLS_NVME_REGISTERED flag is checked but never cleared. If called
concurrently, both threads will assign their local stack address of tm_done
to tport->tport_del_done. If the second thread fails in
nvme_fc_unregister_remoteport(), it will free the tport and return, while
the first thread is still waiting on the completion using the freed tport.

> +
> +	tport->flags |= FNIC_FDLS_TPORT_DELETED;
> +	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> +	ret = nvme_fc_unregister_remoteport(tport->nv_rport);

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock here corrupt the
list iteration in the caller?

In fnic_delete_fcp_tports(), a list_for_each_entry_safe() loop holds
fnic_lock and calls nvfnic_delete_tport(). By explicitly dropping fnic_lock
here, if a concurrent task frees the cached next pointer while the lock is
dropped, the loop will dereference freed memory when it resumes.

[ ... ]
> diff --git a/drivers/scsi/fnic/fnic_res.h b/drivers/scsi/fnic/fnic_res.h
> --- a/drivers/scsi/fnic/fnic_res.h
> +++ b/drivers/scsi/fnic/fnic_res.h
> @@ -126,7 +126,8 @@ static inline void fnic_queue_wq_copy_desc_itmf(struct vnic_wq_copy *wq,
>  	desc->u.itmf.tm_req = tm_req;       /* SCSI Task Management request */
>  	desc->u.itmf.t_tag = tm_id;         /* tag of fcpio to be aborted */
>  	desc->u.itmf._resvd = 0;
> -	memcpy(desc->u.itmf.lun, lun, LUN_ADDRESS);  /* LUN address */
> +	if (lun)
> +		memcpy(desc->u.itmf.lun, lun, LUN_ADDRESS);  /* LUN address */
>  	desc->u.itmf._resvd1 = 0;
>  	hton24(desc->u.itmf.d_id, d_id);    /* FC vNIC only: Target D_ID */

[Severity: Medium]
Does skipping the memcpy when lun is NULL leak uninitialized memory to the
firmware?

If lun is NULL, the 8-byte lun array in the descriptor is not zeroed. Because
vnic_wq_copy_next_desc() returns memory directly from the ring buffer, this
leaks whatever residual data was previously in the descriptor.

[ ... ]
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c

[Severity: High]
Are the newly added NVMe completion handlers unreachable from the completion
queue polling loop?

When the hardware completes an NVMe I/O, fnic_fcpio_cmpl_handler() processes
the CQ entry. Because there are no switch cases for FCPIO_NVME_ERSP_HW_CMPL
or FCPIO_NVME_ERSP_FW_CMPL, they hit the default case and are discarded.
Consequently, NVMe commands are never completed back to the midlayer, causing
I/Os to hang indefinitely.

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