Re: [PATCH 11/13] scsi: lpfc: enable FPIN notification for NVMe

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Logging Formatting Defect: Error message is improperly split across two consecutive `dev_info()` calls.

Pre-existing issues:
- [High] Concurrency / Use-After-Free: Lockless access to `ndlp->nrport` and `nrport->remoteport` can lead to a Use-After-Free due to concurrent unregistration.
--

--- Patch [11]: [PATCH 11/13] scsi: lpfc: enable FPIN notification for NVMe ---
commit 6d41021b653095acd49d1f7828d07c1405835a16
Author: Jesse Taube <jtaubepe@redhat.com>

scsi: lpfc: enable FPIN notification for NVMe

Implement set_rport_marginal callback using nvme_fc_set_remoteport_fpin() to
enable FPIN notifications for NVMe.

> diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> index f4e8164b94abb..d54899cceed99 100644
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c

[ ... ]

> @@ -7134,6 +7134,30 @@ lpfc_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
>  #endif
>  }
>  
> +static void
> +lpfc_set_rport_marginal(struct fc_rport *rport, bool marginal)
> +{
> +	struct lpfc_rport_data *rdata = rport->dd_data;
> +	struct lpfc_nodelist *ndlp = rdata->pnode;
> +	struct lpfc_nvme_rport *nrport = NULL;
> +
> +	/* Break early if NVME_FC is not enabled */
> +	if (!IS_ENABLED(CONFIG_NVME_FC))
> +		return;
> +
> +	if (!ndlp) {
> +		dev_info(&rport->dev, "Cannot find remote node to ");
> +		dev_info(&rport->dev, "set rport marginal, port_id x%x\n",
> +			 rport->port_id);

[Severity: Low]
Does this code format the output correctly?

Because dev_info() prepends the device name and KERN_INFO to every
invocation, splitting the message across two calls without a newline on the
first will cause the prefix to be injected in the middle of the sentence in
the kernel log.

> +		return;
> +	}
> +
> +	nrport = lpfc_ndlp_get_nrport(ndlp);
> +
> +	if (nrport && nrport->remoteport)
> +		nvme_fc_set_remoteport_fpin(nrport->remoteport, marginal);

[Severity: High]
This is a pre-existing issue, but does this lockless retrieval of nrport via
lpfc_ndlp_get_nrport() safely protect against concurrent unregistration?

The macro performs a lockless double-fetch of ndlp->nrport without acquiring
ndlp->lock:

drivers/scsi/lpfc/lpfc_nvme.h:
#define lpfc_ndlp_get_nrport(ndlp)				\
	((!ndlp->nrport || (ndlp->fc4_xpt_flags & NVME_XPT_UNREG_WAIT))\
	? NULL : ndlp->nrport)

If a concurrent remote port loss triggers lpfc_nvme_unregister_port(), it
could free the NVMe remote port memory before setting ndlp->nrport = NULL.

Could this code then read a dangling pointer and pass nrport->remoteport
to nvme_fc_set_remoteport_fpin(), resulting in a Use-After-Free?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=11