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

[sashiko-bot@kernel.org]

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 <jtaubepe@redhat.com>

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?

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?

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?

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?

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, 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/20260812181300.3712426-1-jtaubepe@redhat.com?part=7