Re: [PATCH net-next v1 10/11] eth: fbnic: Support RX buffers it cannot split

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] eth: fbnic: incomplete split-buffer checks in
  fbnic_bd_one_desc() callers
- [Medium] eth: fbnic: race on FBNIC_QUEUE_RDE_CTL0 in
  fbnic_config_drop_mode()
--

commit 054b7a97f2c49f63f4a411460a28df28be50d728
Author: Björn Töpel <bjorn@kernel.org>

eth: fbnic: Support RX buffers it cannot split

This commit adds support for RX memory providers without MP_CAP_FRAG by
assigning a single reference per buffer, programming per-queue header and
payload thresholds so each descriptor uses a whole buffer, and updating
link-up drop mode configuration to preserve per-queue header split settings.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index bfaa284e497eb..d96c957d6fbad 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

> +	/* MAX_HDR leaves no room for a second header in a whole buffer, so
> +	 * the device closes it, with PAGE_FIN, after the first.
> +	 */
> +	pkt->add_frag_failed = false;
> +	if (unlikely(!fbnic_bd_one_desc(nv, &qt->sub0, hdr_pg_idx,
> +					FIELD_GET(FBNIC_RCD_AL_PAGE_FIN,
> +						  rcd)))) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
>  	netmem = fbnic_page_pool_get_head(qt, hdr_pg_idx);
>  	pkt->head = netmem;
>  
> @@ -1032,7 +1063,6 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>  			 len - FBNIC_RX_PAD, true);
>  
>  	pkt->hwtstamp = 0;
> -	pkt->add_frag_failed = false;
>  }
>  
>  static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
> @@ -1046,8 +1076,20 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>  	netmem_ref netmem;
>  	bool added;
>  
> +	if (unlikely(!fbnic_bd_one_desc(nv, &qt->sub1, pg_idx, !pg_off))) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}

[Severity: Medium]
Can a split RX completion descriptor still pass fbnic_bd_one_desc() on both
the header and payload rings?

In fbnic_add_rx_frag(), fbnic_bd_one_desc() checks !pg_off but does not
check FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) (or the subpage offset from
fbnic_rcd_bd_page_offset()). If the device splits a whole payload buffer
across multiple descriptors, the first descriptor has pg_off == 0 and
PAGE_FIN == 0, so it passes fbnic_bd_one_desc() and consumes the buffer's
single netmem reference while the hardware still holds the buffer open for
subsequent descriptors.

Conversely, in fbnic_pkt_prepare(), fbnic_bd_one_desc() checks only
PAGE_FIN. When the first descriptor of a split header buffer has
PAGE_FIN == 0, fbnic_pkt_prepare() returns before fbnic_page_pool_get_head()
decrements pagecnt_bias. Because pagecnt_bias stays 1 and the starting
offset is not checked, the trailing descriptor for that buffer (with
PAGE_FIN == 1 at a non-zero offset) passes fbnic_bd_one_desc().

[ ... ]

> @@ -2672,6 +2724,14 @@ static void fbnic_enable_bdq(struct fbnic_ring *hpq, struct fbnic_ring *ppq)
>  	fbnic_ring_wr32(hpq, FBNIC_QUEUE_BDQ_CTL, bdq_ctl);
>  }
>  
> +static u32 fbnic_drop_mode(struct fbnic_net *fbn, bool tx_pause)
> +{
> +	if (!tx_pause && fbn->num_rx_queues > 1)
> +		return FBNIC_QUEUE_RDE_CTL0_DROP_IMMEDIATE;
> +
> +	return FBNIC_QUEUE_RDE_CTL0_DROP_NEVER;
> +}
> +
>  static void fbnic_config_drop_mode_rcq(struct fbnic_napi_vector *nv,
>  				       struct fbnic_ring *rcq, bool tx_pause,
>  				       bool hdr_split)
> @@ -2679,10 +2739,7 @@ static void fbnic_config_drop_mode_rcq(struct fbnic_napi_vector *nv,
>  	struct fbnic_net *fbn = netdev_priv(nv->napi.dev);
>  	u32 drop_mode, rcq_ctl;
>  
> -	if (!tx_pause && fbn->num_rx_queues > 1)
> -		drop_mode = FBNIC_QUEUE_RDE_CTL0_DROP_IMMEDIATE;
> -	else
> -		drop_mode = FBNIC_QUEUE_RDE_CTL0_DROP_NEVER;
> +	drop_mode = fbnic_drop_mode(fbn, tx_pause);
>  
>  	/* Specify packet layout */
>  	rcq_ctl = FIELD_PREP(FBNIC_QUEUE_RDE_CTL0_DROP_MODE_MASK, drop_mode) |
> @@ -2693,20 +2750,32 @@ static void fbnic_config_drop_mode_rcq(struct fbnic_napi_vector *nv,
>  	fbnic_ring_wr32(rcq, FBNIC_QUEUE_RDE_CTL0, rcq_ctl);
>  }
>  
> +static u32 fbnic_rcq_hds_thresh(const struct fbnic_net *fbn,
> +				const struct fbnic_ring *rcq)
> +{
> +	const struct fbnic_q_triad *qt;
> +
> +	qt = container_of_const(rcq, struct fbnic_q_triad, cmpl);
> +	return qt->hdr_max ? : fbn->hds_thresh;
> +}
> +
> +/* Only the drop mode: a queue start may write the rest of the register */
>  void fbnic_config_drop_mode(struct fbnic_net *fbn, bool txp)
>  {
> -	bool hds;
> +	struct fbnic_ring *rcq;
> +	u32 drop_mode, rcq_ctl;
>  	int i, t;
>  
> -	hds = fbn->hds_thresh < FBNIC_HDR_BYTES_MIN;
> -
> +	drop_mode = fbnic_drop_mode(fbn, txp);
>  	for (i = 0; i < fbn->num_napi; i++) {
>  		struct fbnic_napi_vector *nv = fbn->napi[i];
>  
>  		for (t = 0; t < nv->rxt_count; t++) {
> -			struct fbnic_q_triad *qt = &nv->qt[nv->txt_count + t];
> -
> -			fbnic_config_drop_mode_rcq(nv, &qt->cmpl, txp, hds);
> +			rcq = &nv->qt[nv->txt_count + t].cmpl;
> +			rcq_ctl = fbnic_ring_rd32(rcq, FBNIC_QUEUE_RDE_CTL0);
> +			FIELD_MODIFY(FBNIC_QUEUE_RDE_CTL0_DROP_MODE_MASK,
> +				     &rcq_ctl, drop_mode);
> +			fbnic_ring_wr32(rcq, FBNIC_QUEUE_RDE_CTL0, rcq_ctl);
>  		}
>  	}
>  }

[Severity: Medium]
Can the read-modify-write of FBNIC_QUEUE_RDE_CTL0 in
fbnic_config_drop_mode() race with fbnic_enable_rcq() and overwrite the
per-queue EN_HDR_SPLIT setting?

kworker (phylink_resolve)         CPU0
fbnic_phylink_mac_link_up()       fbnic_queue_start()
  fbn->tx_pause = tx_pause          ...
  fbnic_config_drop_mode()            fbnic_enable_rcq()
    rcq_ctl = fbnic_ring_rd32(...)      fbnic_config_drop_mode_rcq()
                                          fbnic_ring_wr32(rcq,
                                            FBNIC_QUEUE_RDE_CTL0, ...)
    FIELD_MODIFY(..., &rcq_ctl, ...)
    fbnic_ring_wr32(rcq,
      FBNIC_QUEUE_RDE_CTL0, rcq_ctl)  <-- clobbers EN_HDR_SPLIT

fbnic_phylink_mac_link_up() runs from the phylink workqueue without
netdev_lock(dev), while fbnic_queue_start() (or fbnic_up() after
__fbnic_open() calls phylink_resume()) programs FBNIC_QUEUE_RDE_CTL0 via
fbnic_enable_rcq(). If the two interleave, fbnic_config_drop_mode() can
restore a stale EN_HDR_SPLIT, MIN_HROOM or MIN_TROOM value, or
fbnic_enable_rcq() can write a stale fbn->tx_pause drop mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110102039.1002605-1-bjorn@kernel.org?part=10