Re: [PATCH] scsi: libfc: fix directory server rport memory leak

[Guangshuo Li]

Hi Hannes,

On Mon, 21 Sept 2026 at 16:48, Hannes Reinecke <hare@xxxxxxx> wrote:
>
> On 9/19/26 7:34 PM, Guangshuo Li wrote:
> > fc_rport_recv_plogi_req() creates an rport before allocating the frame
> > used for the PLOGI LS_ACC response.
> >
> > fc_rport_create() does not add FC_FID_DIR_SERV rports to the discovery
> > rport list. If fc_frame_alloc() fails while handling a PLOGI from the
> > directory server, the function returns without starting the rport state
> > machine or dropping the initial rport reference.
> >
> > Since the directory server rport is not present in the discovery list,
> > there is no later teardown path that can find the object and release
> > that reference. The allocated fc_rport_priv is therefore leaked.
> >
> > Record when the failed frame allocation leaves an unlisted directory
> > server rport behind and drop its initial reference after releasing the
> > rport mutex. Keep the existing lifetime unchanged for ordinary rports,
> > which remain owned by the discovery list.
> >
> > The issue was identified by a static analysis tool I developed and
> > confirmed by manual review.
> >
> > Fixes: 3ac6f98f4113 ("[SCSI] libfc: correctly handle incoming PLOGI request.")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> >   drivers/scsi/libfc/fc_rport.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> > index c25979d96808..884b233c2f72 100644
> > --- a/drivers/scsi/libfc/fc_rport.c
> > +++ b/drivers/scsi/libfc/fc_rport.c
> > @@ -1848,6 +1848,7 @@ static void fc_rport_recv_plogi_req(struct fc_lport *lport,
> >       struct fc_els_flogi *pl;
> >       struct fc_seq_els_data rjt_data;
> >       u32 sid;
> > +     bool drop_rdata = false;
> >
> >       lockdep_assert_held(&lport->lp_mutex);
> >
> > @@ -1940,8 +1941,10 @@ static void fc_rport_recv_plogi_req(struct fc_lport *lport,
> >        * Send LS_ACC.  If this fails, the originator should retry.
> >        */
> >       fp = fc_frame_alloc(lport, sizeof(*pl));
> > -     if (!fp)
> > +     if (!fp) {
> > +             drop_rdata = sid == FC_FID_DIR_SERV;
> >               goto out;
> > +     }
> >
> >       fc_plogi_fill(lport, fp, ELS_LS_ACC);
> >       fc_fill_reply_hdr(fp, rx_fp, FC_RCTL_ELS_REP, 0);
> > @@ -1949,6 +1952,8 @@ static void fc_rport_recv_plogi_req(struct fc_lport *lport,
> >       fc_rport_enter_prli(rdata);
> >   out:
> >       mutex_unlock(&rdata->rp_mutex);
> > +     if (drop_rdata)
> > +             kref_put(&rdata->kref, fc_rport_destroy);
> >       fc_frame_free(rx_fp);
> >       return;
> >
>
> Wouldn't it be better to always create an rport for the directory
> server?
> That would make the teardown path far easier.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@xxxxxxx                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Yes, that sounds cleaner.

I kept the current directory-server special case to minimize the change, but
using the normal rport lifetime would avoid this extra cleanup logic.

Do you mean adding the directory-server rport to the regular rport list as
well, so the existing teardown path can release it?

Thanks,
Guangshuo