Re: [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template

On Wed, Aug 12, 2026 at 2:28 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [High] The primary affected port (`attach_rport`) in a Link Integrity event is excluded from being marked as marginal.
> - [High] NVMe targets are erroneously placed into the 'marginal' state during Peer Congestion (PC) events.
> - [High] The `set_rport_marginal` callback is not invoked when the marginal state is manually cleared or set via sysfs.
>
> Pre-existing issues:
> - [Critical] The `fc_rport` pointer returned by `fc_find_rport_by_wwpn` is used without a reference count, leading to a Use-After-Free.
> - [Critical] Integer underflow in `fc_host_fpin_rcv` when `fpin_len` is small, leading to massive OOB read.
> --
>
> --- Patch [7]: [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template ---
> commit 978d2e847b2a738287194acf215a8cbbe297f65a
> Author: Jesse Taube <[email protected]>
>
> scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template
>
> Add fc_fpin_set_marginal function to evaluate the FPIN LI TLV
> information and set the 'marginal' path status for all
> affected nvme rports. Then call set_rport_marginal() callback added
> to fc_function_template.
>
> The implementation of set_rport_marginal() is almost identical to
> set_rport_dev_loss_tmo implementation.
>
> > diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> > index 02c58f4591d83..df1edf3c23fa8 100644
> > --- a/drivers/scsi/scsi_transport_fc.c
> > +++ b/drivers/scsi/scsi_transport_fc.c
> > @@ -737,6 +737,25 @@ fc_cn_stats_update(u16 event_type, struct fc_fpin_stats *stats)
> >       }
> >  }
> >
> > +static void fc_fpin_set_marginal(struct Scsi_Host *shost, struct fc_rport *rport)
> > +{
> > +     struct fc_internal *i = to_fc_internal(shost->transportt);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(shost->host_lock, flags);
> > +
> > +     if (rport->port_state == FC_PORTSTATE_ONLINE &&
> > +         rport->roles & FC_PORT_ROLE_NVME_TARGET) {
> > +             rport->port_state = FC_PORTSTATE_MARGINAL;
> > +             spin_unlock_irqrestore(shost->host_lock, flags);
> > +             if (i->f->set_rport_marginal)
> > +                     i->f->set_rport_marginal(rport, true);
>
> [Severity: Critical]
> This is a pre-existing issue, but does passing this rport pointer out to the
> driver callback outside of the lock risk a use-after-free?

Yes, the solution would be to move the lock to after set_rport_marginal, but
this holds a lock over a driver callback is not ideal. fc_rport does not have a
lock either, so this is also not possible. The solution is to let the
driver callback
check. There is also a Time of use time of check issue here with port_state
which can also be solved like this.

>
> The rport pointer passed into fc_fpin_set_marginal() is retrieved by
> fc_find_rport_by_wwpn(). Looking at fc_find_rport_by_wwpn(), it returns the
> pointer without incrementing its reference count (e.g., via get_device()).
>
> If a concurrent fc_remote_port_delete() runs, the rport memory could be
> freed as soon as the internal list lock is dropped. When the host_lock is
> dropped here, the unreferenced rport pointer is then passed to
> set_rport_marginal(), potentially dereferencing freed memory.
>
> > +             return;
> > +     }
> > +
> > +     spin_unlock_irqrestore(shost->host_lock, flags);
> > +}
> > +
> >  static void
> >  fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> >                          struct fc_rport *attach_rport, u16 event_type,
> > @@ -764,6 +783,7 @@ fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> >                       if (rport == attach_rport)
> >                               continue;
> >                       stats_update(event_type, &rport->fpin_stats);
> > +                     fc_fpin_set_marginal(shost, rport);
>
> [Severity: High]
> Does this continue statement unintentionally bypass the newly added
> fc_fpin_set_marginal() call for the primary attach_rport?

This is intentional; only pnames should be updated.

>
> When a Link Integrity FPIN is processed via fc_fpin_li_stats_update(), it
> locates the attach_rport and passes it to fc_fpin_pname_stats_update().
> Because of this continue, the primary port experiencing the Link Integrity
> failure won't be transitioned to the marginal state.
>
> [Severity: High]
> Will this also place NVMe targets into the marginal state during Peer
> Congestion (PC) events?

Yes, will fix.

>
> fc_fpin_peer_congn_stats_update() processes Peer Congestion events by
> calling fc_fpin_pname_stats_update(). Since fc_fpin_set_marginal() is now
> called unconditionally here, it seems ports involved in transient congestion
> events might be incorrectly marked as marginal, potentially causing
> unwarranted path failovers.
>
> >               }
> >       }
> >  }
> > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > index 9f30625aa0d30..bc7c3f8323992 100644
> > --- a/include/scsi/scsi_transport_fc.h
> > +++ b/include/scsi/scsi_transport_fc.h
> > @@ -684,6 +684,7 @@ struct fc_host_attrs {
> >  struct fc_function_template {
> >       void    (*get_rport_dev_loss_tmo)(struct fc_rport *);
> >       void    (*set_rport_dev_loss_tmo)(struct fc_rport *, u32);
> > +     void    (*set_rport_marginal)(struct fc_rport *rport, bool marginal);
>
> [Severity: High]
> Should this callback also be invoked when the marginal state is manually
> cleared or set via sysfs?

No, the AI clearly doesn't understand the code.

>
> When a user writes 'Online' to the port_state sysfs attribute, it calls
> fc_rport_set_marginal_state() which modifies rport->port_state, but it does
> not call this new set_rport_marginal() callback.
>
> Will this leave the Low-Level Device Driver (LLDD) permanently out of sync
> with the transport class state when a path is recovered manually?
>
> >
> >       void    (*get_starget_node_name)(struct scsi_target *);
> >       void    (*get_starget_port_name)(struct scsi_target *);
>
> [Severity: Critical]
> This is a pre-existing issue,

 pre-existing issue will not fix.

> but is there an integer underflow in
> fc_host_fpin_rcv() when fpin_len is small?
>
> In fc_host_fpin_rcv(), bytes_remain is calculated without validating that
> fpin_len is large enough:
>
>     bytes_remain = fpin_len - offsetof(struct fc_els_fpin, fpin_desc);
>     bytes_remain = min_t(u32, bytes_remain, be32_to_cpu(fpin->desc_len));
>
> Since offsetof(struct fc_els_fpin, fpin_desc) is 8, if an FPIN payload is
> received with fpin_len < 8, the subtraction underflows to a massive u32
> value. The parsing loop will then read well past the end of fpin_buf,
> causing an out-of-bounds read.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
>