Re: [PATCH v6 08/13] scsi: fnic: Handle NVMe LS frames in FDLS

[sashiko-bot@kernel.org]

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

New issues:
- [High] Mismatched lock flags when temporarily dropping `fnic->fnic_lock` corrupt the CPU interrupt state.

Pre-existing issues:
- [High] Mempool-allocated FC frames are permanently leaked when hardware transmission fails.
- [High] Dropping a spinlock during `list_for_each_entry_safe` iteration allows concurrent list modifications, leading to use-after-free on loop resumption.
- [High] Missing synchronous cancellation of delayed works `oxid_reclaim_work` and `schedule_oxid_free_retry` causes Use-After-Free during module unload.
--

--- Patch [8]: [PATCH v6 08/13] scsi: fnic: Handle NVMe LS frames in FDLS ---
commit 1f929f12db7e96d3eff12397152cd2a0b81d2192
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Handle NVMe LS frames in FDLS

Classify NVMe LS request OXIDs, route NVMe LS responses and ABTS frames
through the FCS receive path, and reset NVMe exchanges when FDLS tears
down target ports.

Extend FDLS link-down and frame-processing paths so NVMe LS traffic
follows the same discovery and cleanup state machine as FCP traffic.

> diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c
> index e03256183ac63..30d9bcd3795f9 100644
> --- a/drivers/scsi/fnic/fdls_disc.c
> +++ b/drivers/scsi/fnic/fdls_disc.c
[ ... ]
> @@ -387,10 +394,25 @@ static bool fdls_is_oxid_tgt_req(uint16_t oxid)
>  	return true;
>  }
>  
> +static inline bool fdls_is_oxid_nvme_req(uint16_t oxid)
> +{
> +	return FNIC_FRAME_TYPE(oxid) == FNIC_FRAME_TYPE_NVME_LS;
> +}
> +
>  static void fdls_reset_oxid_pool(struct fnic_iport_s *iport)
>  {
>  	struct fnic_oxid_pool_s *oxid_pool = &iport->oxid_pool;
> +	struct reclaim_entry_s *reclaim_entry, *next;
>  
> +	cancel_delayed_work(&oxid_pool->oxid_reclaim_work);
> +	cancel_delayed_work(&oxid_pool->schedule_oxid_free_retry);

[Severity: High]
This is a pre-existing issue, but does this code leave active delayed 
works running after module unload?

During module unload, the driver needs to synchronously cancel these works
to prevent them from executing after the underlying fnic structure is freed.

Since this only attempts asynchronous cancellation via cancel_delayed_work(),
if the worker is already running or concurrently dispatched, it will continue
executing after kfree(fnic) is called in the module teardown.

Could this lead to a use-after-free when dereferencing oxid_pool? Should
cancel_delayed_work_sync() be used instead?

> +	list_for_each_entry_safe(reclaim_entry, next,
> +				 &oxid_pool->oxid_reclaim_list, links) {
> +		list_del(&reclaim_entry->links);
> +		kfree(reclaim_entry);
> +	}
> +	bitmap_clear(oxid_pool->pending_schedule_free, 0, FNIC_OXID_POOL_SZ);
> +	bitmap_clear(oxid_pool->bitmap, 0, FNIC_OXID_POOL_SZ);
>  	oxid_pool->next_idx = 0;
>  }
[ ... ]
> @@ -1288,6 +1310,10 @@ bool fdls_delete_tport(struct fnic_iport_s *iport, struct fnic_tport_s *tport)
>  		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
>  		fnic_rport_exch_reset(iport->fnic, tport->fcid);
>  		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

[Severity: High]
This is a pre-existing issue, but can this lock drop cause a use-after-free
during list traversal?

When iterating over iport->tport_list or fnic->tport_event_list using
list_for_each_entry_safe(), the macro caches the next element at the start 
of the loop. 

However, dropping fnic_lock here in fdls_delete_tport() allows concurrent
threads to acquire the lock and modify or free the cached next element.

When the lock is reacquired and the loop continues, will evaluating the 
loop condition access a freed next pointer?

> +	} else if (IS_FNIC_NVME_INITIATOR(fnic)) {
> +		spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);

[Severity: High]
Can this sequence corrupt the CPU interrupt state? 

Callers like fdls_tport_timer_callback() acquire the lock using a local 
flags variable:

    spin_lock_irqsave(&fnic->fnic_lock, flags);

However, when this NVMe block temporarily drops the lock, it uses the 
shared struct member fnic->lock_flags:

    spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);

Because fnic->lock_flags was never updated by the caller, doesn't this 
restore a stale, unrelated, or potentially garbage interrupt state?

> +		nvfnic_exch_reset(iport, tport);
> +		spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);
>  	}
>  
>  	if ((tport->flags & FNIC_FDLS_SCSI_REGISTERED) ||
[ ... ]
> diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
> index 94b7c150c08c2..b00672ef8b000 100644
> --- a/drivers/scsi/fnic/fnic_fcs.c
> +++ b/drivers/scsi/fnic/fnic_fcs.c
[ ... ]
> @@ -651,7 +667,7 @@ static int fnic_send_frame(struct fnic *fnic, void *frame, int frame_len)
>  		dma_unmap_single(&fnic->pdev->dev, pa, frame_len, DMA_TO_DEVICE);
>  		FNIC_FCS_DBG(KERN_INFO, fnic,
>  					 "vnic work queue descriptor is not available");
> -		ret = -1;
> +		ret = -ENXIO;
>  		goto fnic_send_frame_end;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this error path leak the 
mempool-allocated FC frame?

When fnic_send_frame() encounters a full hardware TX queue, it unmaps the 
DMA address and exits via fnic_send_frame_end:

    dma_unmap_single(&fnic->pdev->dev, pa, frame_len, DMA_TO_DEVICE);
    ret = -ENXIO;
    goto fnic_send_frame_end;

However, it fails to free the frame back to fnic->frame_pool.

Since callers like fdls_send_fcoe_frame() systematically ignore the return 
value, won't this frame be permanently leaked? 

Under network congestion, could these leaks rapidly exhaust the mempool and 
prevent the driver from processing discoveries or recovering?

> @@ -685,7 +701,6 @@ fdls_send_fcoe_frame(struct fnic *fnic, void *frame, int frame_size,

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