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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling timer_delete_sync() while holding fnic_lock causes an AB-BA deadlock.
- [High] Dropping the spinlock inside list_for_each_entry_safe() exposes the cached 'next' pointer to concurrent deletion, leading to Use-After-Free.
- [High] Lockless access to io_req_table elements in _terminate_tport_ios() races with IO completions, causing Use-After-Free.
- [High] Unhandled NVMe abort failures permanently leak fnic_io_req and hang the NVMe core.
- [High] Freeing tag_map and io_req_table in nvfnic_nvme_unload before disabling interrupts causes Use-After-Free on late IO completions.
- [High] Early return on nvme_fc_unregister_localport failure in nvfnic_nvme_unload leaks memory and guarantees Use-After-Free of the fnic structure.
- [Medium] Unused memory pool io_req_pool is leaked on module unload for NVMe initiators.
- [Medium] The fnic_tport_s structure memory is leaked if NVMe remote port unregistration fails or times out.
- [Medium] Valid DMA mappings at address 0 will not be unmapped, causing an IOMMU resource leak.

Pre-existing issues:
- [Critical] Using a shared struct member (fnic->lock_flags) with spin_lock_irqsave() corrupts CPU IRQ states.
--

commit bbeed4ae296bbd0a10dc168bc3c202bd9dce7297
Author: Karan Tilak Kumar <kartilak@cisco.com>
Subject: scsi: fnic: Add the NVMe/FC transport path

This patch adds the NVMe/FC transport path to the fnic driver. It includes
NVMe local-port and remote-port registration, I/O submission, DMA mapping,
tag management, completion, abort handling, and cleanup logic.

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

> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> [ ... ]
> @@ -44,6 +44,8 @@ int nvfnic_dma_map_sgl(struct fnic *fnic, struct fnic_io_req *io_req,
>  void nvfnic_dma_unmap_sgl(struct fnic *fnic, struct fnic_io_req *io_req)
>  {
>  	if (io_req->sgl_list_pa)
>  		dma_unmap_single(&fnic->pdev->dev, io_req->sgl_list_pa,
>  				 sizeof(io_req->sgl_list[0]) * io_req->sgl_cnt,
>  				 DMA_TO_DEVICE);
>  }

[Severity: Medium]
Could this leak IOMMU mappings if the DMA mapping happens to be at address 0?
On many architectures, 0 is a perfectly valid DMA address, so evaluating
it directly for truthiness might falsely skip unmapping a legitimate mapping.

> [ ... ]
> @@ -288,6 +290,7 @@ void nvfnic_fcpio_nvme_fast_cmpl_handler(struct fnic *fnic,
>  			    desc->hdr.type, desc->hdr.status, desc->hdr._resvd,
>  			    id);
>  		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
>  		return;
>  	}
>  	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

[Severity: Critical]
This is a pre-existing issue, but does using a shared structure member
(fnic->lock_flags) for saving IRQ state corrupt CPU IRQ states? 

When spin_lock_irqsave() is called, it saves the local CPU's IRQ state into
the provided variable. Since multiple CPUs could hit this concurrently and
overwrite the shared lock_flags, unlocking may restore corrupted state
and cause hard lockups.

> [ ... ]
> @@ -530,6 +533,7 @@ void nvfnic_fcpio_nvme_itmf_cmpl_handler(struct fnic *fnic,
>  		io_req->fcp_req->status = NVME_SC_ABORT_REQ;
>  
>  		nvfnic_release_nvme_ioreq_buf(iport, io_req);
>  		if (io_req->done)
>  			io_req->done(io_req);
>  		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
>  		return;
>  	}
>  	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
>  }

[Severity: High]
If the abort command fails (e.g., returning FCPIO_TIMEOUT), does this path
accidentally skip calling io_req->done()? It looks like the function just
returns without it, meaning the midlayer is never notified of the completion,
which could permanently hang the block layer queue and leak the IO request
buffers.

> [ ... ]
> @@ -582,6 +586,7 @@ void nvfnic_cleanup_tport_io(struct fnic *fnic, struct fnic_tport_s *tport)
>  
>  	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;
>  		}
>  		list_del(&nvfnic_ls_req->list);
>  		lsreq->private = NULL;
>  		oxid = nvfnic_ls_req->oxid;
>  		fdls_free_oxid(&fnic->iport, oxid, &nvfnic_ls_req->oxid);
>  		nvfnic_ls_req->state = FNIC_LS_REQ_CMD_COMPLETE;
>  		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  		timer_delete_sync(&nvfnic_ls_req->ls_req_timer);
>  		lsreq->done(lsreq, -ENXIO);
>  		spin_lock_irqsave(&fnic->fnic_lock, flags);
>  	}

[Severity: High]
Can dropping fnic_lock inside the list_for_each_entry_safe() loop cause a
use-after-free? 

The list_for_each_entry_safe() macro caches the 'next' pointer before
executing the loop body. Because the lock is temporarily dropped, a
concurrent timeout or completion handler could execute and free that cached
'next' element before the loop reacquires the lock and advances to it.

> [ ... ]
> @@ -647,6 +652,7 @@ bool _terminate_tport_ios(struct sbitmap *map, unsigned int tag,
>  	struct fnic *fnic = iport->fnic;
>  	struct fnic_io_req *io_req;
>  
>  	io_req = fnic->sw_copy_wq[0].io_req_table[tag];
>  	if (!io_req)
>  		return true;
>  
>  	if (io_req->tport != tport)
>  		return true;

[Severity: High]
Is it safe to access io_req_table here without holding fnic_lock? An IO
completion could concurrently execute nvfnic_fcpio_cmpl(), complete the IO,
and hand the memory back to the block layer where it gets freed. We might
dereference a freed io_req or io_req->tport during this check.

> [ ... ]
> @@ -704,6 +710,7 @@ void nvfnic_nvme_unload(struct fnic *fnic)
>  	fnic_flush_tport_event_list(fnic);
>  	fnic_delete_fcp_tports(fnic);
>  
>  	if (iport->flags & FNIC_LPORT_NVME_REGISTERED) {
>  		ret = nvme_fc_unregister_localport(fnic->iport.nv_lport);
>  		if (ret) {
>  			FNIC_NVME_DBG(KERN_ERR, fnic,
>  				"Unregister nvme localport failed: %d\n", ret);
>  			spin_lock_irqsave(&fnic->fnic_lock, flags);
>  			fnic->nvme_lport_unreg_done = NULL;
>  			spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  			return;
>  		}

[Severity: High]
If nvme_fc_unregister_localport() fails, does returning early here cause a
use-after-free? 

Returning early skips the rest of the local teardown and yields control back
to fnic_remove(), which proceeds to free the fnic structure entirely. Because
the local port remains registered, the NVMe FC transport could continue making
callbacks to the freed fnic structure.

> [ ... ]
> @@ -735,6 +742,7 @@ void nvfnic_nvme_unload(struct fnic *fnic)
>  	fnic->nvme_lport_unreg_done = NULL;
>  	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  
>  	nvfnic_flush_nvme_io_list(fnic);
>  	sbitmap_free(&fnic->nvfnic_tag_map);
>  	for (hwq = 0; hwq < fnic->wq_copy_count; hwq++)
>  		kfree(fnic->sw_copy_wq[hwq].io_req_table);
>  }

[Severity: High]
Does freeing nvfnic_tag_map and the io_req_table array here cause a
use-after-free if a late completion interrupt fires? Hardware interrupts and
completion queues aren't disabled until fnic_cleanup() is called later in the
removal process, so an active ISR might still try to access these structures.

[Severity: Medium]
Also, does this function forget to destroy fnic->io_req_pool? It appears
fnic_scsi_unload_cleanup() destroys it for FCP initiators, but NVMe-only
initiators might skip that step and leak the pool entirely on module unload.

> [ ... ]
> @@ -826,6 +834,7 @@ void nvfnic_ls_rsp_recv(struct fnic_iport_s *iport,
>  	nvfnic_ls_req->state = FNIC_LS_REQ_CMD_COMPLETE;
>  
>  	list_del_init(&nvfnic_ls_req->list);
>  	lsreq->private = NULL;
>  	fdls_free_oxid(iport, oxid, &nvfnic_ls_req->oxid);
>  	timer_delete_sync(&nvfnic_ls_req->ls_req_timer);

[Severity: High]
Can calling timer_delete_sync() while holding fnic_lock cause an AB-BA
deadlock? 

timer_delete_sync() will spin waiting for the timer callback to finish, but if
the callback (nvfnic_ls_req_timeout()) is already executing concurrently on
another CPU, it will attempt to acquire fnic_lock and spin indefinitely.

> [ ... ]
> @@ -1164,6 +1173,7 @@ void nvfnic_delete_tport(struct fnic_iport_s *iport,
>  	tport->flags |= FNIC_FDLS_TPORT_DELETED;
>  	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  	ret = nvme_fc_unregister_remoteport(tport->nv_rport);
>  	if (ret) {
>  		FNIC_NVME_DBG(KERN_ERR, fnic,
>  			    "tport: 0x%x unregister failed %d\n",
>  			    tport->fcid, ret);
>  		spin_lock_irqsave(&fnic->fnic_lock, flags);
>  		tport->tport_del_done = NULL;
>  		return;
>  	}

[Severity: Medium]
Are we leaking the tport structure on failure here? If 
nvme_fc_unregister_remoteport() returns an error, the function returns early
without freeing tport.

> [ ... ]
> @@ -1186,6 +1196,7 @@ void nvfnic_delete_tport(struct fnic_iport_s *iport,
>  		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  		nvfnic_cleanup_tport_io(fnic, tport);
>  		spin_lock_irqsave(&fnic->fnic_lock, flags);
>  		tport->flags &= ~FNIC_FDLS_NVME_TPORT_CLEANUP_PENDING;
>  	} else {
>  		while (!(tport->flags & FNIC_TPORT_CAN_BE_FREED) && (count < 8)) {
>  			count++;

[Severity: Medium]
Similarly, if wait_for_completion_timeout() times out, does the !time_remain
branch bypass the kfree(tport) call that happens in the else block, leaking
the memory?

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