Re: [PATCH v5 01/13] scsi: fnic: Make debug logging protocol independent

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Shared struct member used as the flags argument in spin_lock_irqsave() causing IRQ state corruption.
- [High] Lockless modification of fnic_priv(sc)->flags after dropping wq_copy_lock leads to data races and potential Use-After-Free.
--

commit d6c65edf1de54a8d582352945f8226579f06a57d
Author: Karan Tilak Kumar <kartilak@cisco.com>
Subject: scsi: fnic: Make debug logging protocol independent

Make the fnic debug macros take struct fnic instead of struct Scsi_Host so
FCP and NVMe initiator roles can share the same logging interface. Route FCP
messages through shost_printk() and NVMe messages through printk(). Convert
existing debug call sites to pass the fnic instance directly.

> diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
> index 063eb864a5cd0..ca592bc3f6182 100644
> --- a/drivers/scsi/fnic/fnic_fcs.c
> +++ b/drivers/scsi/fnic/fnic_fcs.c
> @@ -1014,7 +1014,7 @@ void fnic_delete_fcp_tports(struct fnic *fnic)
>  
>  	spin_lock_irqsave(&fnic->fnic_lock, flags);
>  	list_for_each_entry_safe(tport, next, &fnic->iport.tport_list, links) {
> -		FNIC_FCS_DBG(KERN_INFO, fnic->host, fnic->fnic_num,
> +		FNIC_FCS_DBG(KERN_INFO, fnic,
>  					 "removing fcp rport fcid: 0x%x", tport->fcid);
>  		fdls_set_tport_state(tport, FDLS_TGT_STATE_OFFLINING);
>  		fnic_del_tport_timer_sync(fnic, tport);

[Severity: High]
This is a pre-existing issue, but can the call to fnic_del_tport_timer_sync()
corrupt the CPU's IRQ state here?

In fnic_delete_fcp_tports(), the lock is acquired using a local flags
variable:

	spin_lock_irqsave(&fnic->fnic_lock, flags);

However, inside fnic_del_tport_timer_sync(), the lock is released and
reacquired using the shared fnic->lock_flags struct member:

	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);
	timer_delete_sync(&tport->retry_timer);
	spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);

Would restoring the IRQ state from the uninitialized or stale fnic->lock_flags
leave interrupts incorrectly enabled or disabled? The same pattern also exists
in fnic_tport_event_handler() where it acquires the lock using a local flags
variable but later calls fdls_delete_tport().

[ ... ]

> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 6ee3c559e129c..0759540f66753 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -1255,7 +1254,7 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic, unsigned int cq_inde
>  		atomic64_inc(&fnic_stats->io_stats.ioreq_null);
>  		spin_unlock_irqrestore(&fnic->wq_copy_lock[hwq], flags);
>  		fnic_priv(sc)->flags |= FNIC_IO_ABT_TERM_REQ_NULL;
> -		shost_printk(KERN_ERR, fnic->host,
> +		fnic_printk(KERN_ERR, fnic,
>  			  "itmf_cmpl io_req is null - "

[Severity: High]
This is a pre-existing issue, but could this lockless modification cause a data
race or a use-after-free?

In fnic_fcpio_itmf_cmpl_handler(), fnic_priv(sc)->flags is modified
immediately after dropping the wq_copy_lock. If io_req is NULL, it indicates
the IO request has already been cleaned up. Once cleaned up, the block layer
may have already completed the SCSI command via scsi_done() and freed or
recycled the scsi_cmnd structure.

Is it possible that modifying the command's flags locklessly here will result
in memory corruption of a newly recycled or freed command? This pattern also
appears to exist in fnic_abort_cmd() and fnic_pending_aborts_iter().

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