Re: [PATCH v5 12/13] scsi: fnic: Expose NVMe transport state in debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The NVMe debugfs file is created and exposed to user space before the data structures and locks it depends on are initialized, creating a race condition that can trigger a NULL pointer dereference and uninitialized spinlock access.
--

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

scsi: fnic: Expose NVMe transport state in debugfs

Create an NVMe debugfs directory with a per-host nvmef_info file,
reporting local-port and target-port identifiers for NVMe initiator
instances.

> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index df8c8ebbc32b9..245db1b992b51 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
[ ... ]
> @@ -933,6 +933,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		err = -EOPNOTSUPP;
>  		goto err_out_fnic_role;
>  	case VFCF_FC_NVME_INITIATOR:
> +		fnic_nvmef_debugfs_init(fnic);

[Severity: High]
Is it safe to create and expose the debugfs file nvmef_info this early in
the probe sequence?

At this point in fnic_probe(), the fnic structure has been allocated with
kzalloc(), but critical structures like fnic_lock and iport->tport_list
haven't been initialized yet. They appear to be initialized later in the
function or in fnic_fdls_init().

If a process concurrently opens and reads this debugfs file right after
fnic_nvmef_debugfs_init() returns, could it trigger a crash?

>  		fnic->role = FNIC_ROLE_NVME_INITIATOR;
>  		dev_info(&fnic->pdev->dev, "fnic: %d is NVME initiator\n",
>  			fnic->fnic_num);
[ ... ]
> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index e322dd4dcb787..a19dd9cf76cf9 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
[ ... ]
> @@ -188,6 +188,36 @@ void nvfnic_release_nvme_ioreq_buf(struct fnic_iport_s *iport,
>  			     fnic->io_sgl_pool[io_req->sgl_type]);
>  }
>  
> +int nvfnic_get_nvmef_info(struct fnic *fnic, struct fnic_nvmef_info *info)
> +{
> +	int len = 0;
> +	struct fnic_iport_s *iport = &fnic->iport;
> +	int buf_size = info->buf_size;
> +	struct fnic_tport_s *tport;
> +	struct fnic_tport_s *next;
> +	unsigned long flags;
> +
> +	if (buf_size <= 0)
> +		return 0;
> +
> +	len += scnprintf(info->info_buffer + len, buf_size - len,
> +			 "lport wwpn 0x%llx wwnn 0x%llx fcid 0x%06x\n",
> +			 iport->wwpn, iport->wwnn, iport->fcid);
> +
> +	spin_lock_irqsave(&fnic->fnic_lock, flags);
> +	list_for_each_entry_safe(tport, next, &iport->tport_list, links) {

[Severity: High]
If nvfnic_get_nvmef_info() is executed concurrently before the fnic
struct is fully initialized, will this attempt to acquire an
uninitialized fnic_lock?

Furthermore, if iport->tport_list is still zeroed from the kzalloc(),
the list_for_each_entry_safe() macro expects a properly initialized
list head. Could passing a zeroed list head cause a NULL pointer
dereference when computing the next pointer for the first entry?

> +		if (len >= buf_size - 1)
> +			break;
> +
> +		len += scnprintf(info->info_buffer + len, buf_size - len,
> +				 "tport wwpn 0x%llx wwnn 0x%llx fcid 0x%06x\n",
> +				 tport->wwpn, tport->wwnn, tport->fcid);
> +	}
> +	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
> +
> +	return len;
> +}
> +

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