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

On Wed, Aug 12, 2026 at 2:35 PM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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?

Will fix

>
> 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?

Will fix

> 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/[email protected]?part=11
>