Re: [PATCH v4 11/13] scsi: fnic: Track NVMe transport statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Deadlock between fnic_lock and timer lock during NVMe LS response.
- [High] Use-After-Free of io_req when writing waitq_start_time.
- [High] Memory and active_ios leak when ITMF abort fails.
- [Medium] active_fw_reqs counter permanently drifts upwards on ERSP completions.
--

commit b68b40c66de8d347db7bd91c9ea2450605172e71
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Track NVMe transport statistics

Add counters for NVMe requests, responses, LS handling, aborts, and
wait-queue activity.

Update NVMe I/O, completion, LS response, LS abort, and abort paths to
maintain the new counters.

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index 9cc8f0baf09cb..2ccbfa0dfa92d 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c

[ ... ]

> @@ -193,6 +250,11 @@ inline int nvfnic_queue_wq_nvme_copy_desc(struct fnic *fnic,
>  					tport->max_payload_size, tport->r_a_tov,
>  					tport->e_d_tov);
>  
> +	atomic64_inc(&fnic->fnic_stats.fw_stats.active_fw_reqs);
> +	if (atomic64_read(&fnic->fnic_stats.fw_stats.active_fw_reqs) >
> +	    atomic64_read(&fnic->fnic_stats.fw_stats.max_fw_reqs))
> +		atomic64_set(&fnic->fnic_stats.fw_stats.max_fw_reqs,
> +		     atomic64_read(&fnic->fnic_stats.fw_stats.active_fw_reqs));
>  
>  	spin_unlock_irqrestore(&fnic->wq_copy_lock[idx], intr_flags);

[Severity: Medium]
Since this function increments active_fw_reqs for NVMe requests, does the
decrement in fnic_fcpio_cmpl_handler() in fnic_scsi.c need to be updated
to include FCPIO_NVME_ERSP_HW_CMPL?

Without updating the switch statement in fnic_fcpio_cmpl_handler(), will
the counter permanently drift upwards when NVMe commands complete with an
Extended Response?

[ ... ]

> @@ -766,11 +878,18 @@ void nvfnic_fcpio_nvme_itmf_cmpl_handler(struct fnic *fnic,
>  
>  	io_req->cmd_flags |= FNIC_IO_ABT_TERM_DONE;
>  
> +	if (!(io_req->cmd_flags & (FNIC_IO_ABORTED | FNIC_IO_DONE)))
> +		atomic64_inc(&misc_stats->no_icmnd_itmf_cmpls);
>  
>  	if (io_req->abts_state == FCPIO_SUCCESS) {
>  		io_req->fcp_req->transferred_length = 0;
>  		io_req->fcp_req->rcv_rsplen = 0;
>  		io_req->fcp_req->status = NVME_SC_ABORT_REQ;
> +		atomic64_dec(&fnic_stats->io_stats.active_ios);
> +		if (atomic64_read(&fnic->io_cmpl_skip))
> +			atomic64_dec(&fnic->io_cmpl_skip);
> +		else
> +			atomic64_inc(&fnic_stats->io_stats.io_completions);
>  
>  		nvfnic_release_nvme_ioreq_buf(iport, io_req);
>  		if (io_req->done)

[Severity: High]
Does this code leak memory and the active_ios counter when the abort status
is not FCPIO_SUCCESS?

If the abort fails, the function skips nvfnic_release_nvme_ioreq_buf(),
the io_req->done() callback, and the active_ios decrement before unlocking
and returning.

[ ... ]

> @@ -1067,11 +1191,14 @@ nvfnic_find_ls_req(struct fnic_tport_s *tport, uint16_t oxid)
>  void nvfnic_fcpio_cmpl(struct fnic_io_req *io_req)
>  {
>  	struct fnic *fnic = io_req->iport->fnic;
> +	struct fnic_stats *fnic_stats = &fnic->fnic_stats;
>  
>  	nvfnic_free_fcpio_tag(io_req->iport, io_req);
> +	atomic64_inc(&fnic_stats->io_stats.nvme_ios_queued_for_rsp);
>  
>  	llist_add(&io_req->nvfnic_io_cmpl, &fnic->nvme_io_event_llist);
>  	atomic_inc(&fnic->nvme_io_event_queued);
> +	atomic64_inc(&fnic_stats->io_stats.nvme_num_ios_in_waitq);
>  
>  	io_req->waitq_start_time = jiffies;
>  	queue_work(fnic_cmpl_queue, &fnic->nvme_io_cmpl_work);

[Severity: High]
Can this write to waitq_start_time cause a use-after-free?

The io_req is published to a lockless list via llist_add() before assigning
waitq_start_time. A concurrent worker thread could dequeue the request,
process the completion, and free the memory before this assignment occurs.

[ ... ]

> @@ -1215,6 +1348,7 @@ void nvfnic_ls_rsp_recv(struct fnic_iport_s *iport,
>  	}
>  
>  	nvfnic_ls_req->state = FNIC_LS_REQ_CMD_COMPLETE;
> +	atomic64_inc(&fnic_stats->nvme_stats.nvme_ls_responses);
>  
>  	list_del_init(&nvfnic_ls_req->list);
>  	lsreq->private = NULL;

[Severity: High]
Is it possible the timer_delete_sync() call right after this block triggers
a deadlock?

nvfnic_ls_rsp_recv() is called with fnic->fnic_lock held. The sync
cancellation blocks on the timer callback if it's running, but the callback
(nvfnic_ls_req_timeout()) unconditionally attempts to acquire the same
fnic->fnic_lock, creating an AB-BA deadlock scenario.

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