NACK: [SRU][F][PATCH 1/1] scsi: lpfc: Move NPIV's transport unregistration to after resource clean up

[Massimiliano Pellizzer]

On Thu, 7 Nov 2024 at 02:59, Guoqing Jiang <guoqing.jiang at canonical.com> wrote:
>
>
>
> On 11/6/24 14:39, Massimiliano Pellizzer wrote:
> > From: Justin Tee <justin.tee at broadcom.com>
> >
> > [ Upstream commit 4ddf01f2f1504fa08b766e8cfeec558e9f8eef6c ]
> >
> > There are cases after NPIV deletion where the fabric switch still believes
> > the NPIV is logged into the fabric.  This occurs when a vport is
> > unregistered before the Remove All DA_ID CT and LOGO ELS are sent to the
> > fabric.
> >
> > Currently fc_remove_host(), which calls dev_loss_tmo for all D_IDs including
> > the fabric D_ID, removes the last ndlp reference and frees the ndlp rport
> > object.  This sometimes causes the race condition where the final DA_ID and
> > LOGO are skipped from being sent to the fabric switch.
> >
> > Fix by moving the fc_remove_host() and scsi_remove_host() calls after DA_ID
> > and LOGO are sent.
> >
> > Signed-off-by: Justin Tee <justin.tee at broadcom.com>
> > Link: https://lore.kernel.org/r/20240305200503.57317-3-justintee8345@gmail.com
> > Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
> > Signed-off-by: Sasha Levin <sashal at kernel.org>
> > (backported from commit f2c7f029051edc4b394bb48edbe2297575abefe0 linux-5.15.y)
> > [mpellizzer: since the fix commit moves the function calls
> > fc_remove_host and scsi_remove_host at the end of lpfc_vport_delete the
> > variable ns_ndlp_referenced, and the logic around it, become meaningless;
> > the same logic has been removed by e9b1108316b9b in mainline, however
> > e9b1108316b9b is a huge commit which is not worth backporting for the
> > CVE fix.]
>
> I would prefer not add partial change in e9b1108316b9b to this patch,
> pls add another patch for e9b1108316b9b if it is valuable or drop the
> partial change from the patch. Thanks, Guoqing
> > CVE-2024-36952
> > Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> > ---
> >   drivers/scsi/lpfc/lpfc_vport.c | 28 +++-------------------------
> >   1 file changed, 3 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/scsi/lpfc/lpfc_vport.c b/drivers/scsi/lpfc/lpfc_vport.c
> > index d0296f7cf45fc..409d5bc405f4d 100644
> > --- a/drivers/scsi/lpfc/lpfc_vport.c
> > +++ b/drivers/scsi/lpfc/lpfc_vport.c
> > @@ -606,7 +606,6 @@ lpfc_vport_delete(struct fc_vport *fc_vport)
> >       struct Scsi_Host *shost = lpfc_shost_from_vport(vport);
> >       struct lpfc_hba   *phba = vport->phba;
> >       long timeout;
> > -     bool ns_ndlp_referenced = false;
> >
> >       if (vport->port_type == LPFC_PHYSICAL_PORT) {
> >               lpfc_printf_vlog(vport, KERN_ERR, LOG_VPORT,
> > @@ -656,22 +655,6 @@ lpfc_vport_delete(struct fc_vport *fc_vport)
> >
> >       lpfc_debugfs_terminate(vport);
> >
> > -     /*
> > -      * The call to fc_remove_host might release the NameServer ndlp. Since
> > -      * we might need to use the ndlp to send the DA_ID CT command,
> > -      * increment the reference for the NameServer ndlp to prevent it from
> > -      * being released.
> > -      */
> > -     ndlp = lpfc_findnode_did(vport, NameServer_DID);
> > -     if (ndlp && NLP_CHK_NODE_ACT(ndlp)) {
> > -             lpfc_nlp_get(ndlp);
> > -             ns_ndlp_referenced = true;
> > -     }
> > -
> > -     /* Remove FC host and then SCSI host with the vport */
> > -     fc_remove_host(shost);
> > -     scsi_remove_host(shost);
> > -
> >       ndlp = lpfc_findnode_did(phba->pport, Fabric_DID);
> >
> >       /* In case of driver unload, we shall not perform fabric logo as the
> > @@ -774,14 +757,9 @@ lpfc_vport_delete(struct fc_vport *fc_vport)
> >
> >   skip_logo:
> >
> > -     /*
> > -      * If the NameServer ndlp has been incremented to allow the DA_ID CT
> > -      * command to be sent, decrement the ndlp now.
> > -      */
> > -     if (ns_ndlp_referenced) {
> > -             ndlp = lpfc_findnode_did(vport, NameServer_DID);
> > -             lpfc_nlp_put(ndlp);
> > -     }
> > +     /* Remove FC host to break driver binding. */
> > +     fc_remove_host(shost);
> > +     scsi_remove_host(shost);
> >
> >       lpfc_cleanup(vport);
> >       lpfc_sli_host_down(vport);
>

Sending a v2 soon, thanks.

-- 
Massimiliano Pellizzer