Re: [PATCH net 3/6] net: stmmac: drop multi-buffer frames in XDP mode

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch cites only `Fixes: 5fabb01207a2` ("net: stmmac: Add
  initial XDP support").

Pre-existing issues:
- [High] Without an XDP program, stmmac_rx() still re-parses the rest of
  a frame as a new frame when napi_build_skb() fails on a non-last…
- [Medium] stmmac_rx() adds its per-poll local counters to the device-
  wide `priv->xstats.rx_dropped` and `priv->xstats.rx_errors` (plain…

--

net: stmmac: drop multi-buffer frames in XDP mode

This patch makes stmmac_rx() drop frames that span more than one
descriptor, or that use the second buffer of a descriptor, while an XDP
program is attached. Discarded buffers are no longer returned to the page
pool, so stmmac_rx_refill() can reuse them in place. The re-parse of the
remaining descriptors after an XDP_CONSUMED verdict is also removed.

> Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")

[Severity: Low]
Should this also carry a Fixes tag for 88095728511e ("net: stmmac: fix rx
Scatter-Gather support")? Two of the failures described in the commit
message seem to come from that commit, not from the initial XDP support.

First, the buf1_len underflow. Before 88095728511e, stmmac_rx_buf1_len()
returned min_t(unsigned int, dma_buf_sz, plen) for the last descriptor. A
tail re-parsed after XDP_CONSUMED therefore got dma_buf_sz bytes.

88095728511e changed the return to:

	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);

It also moved the XDP-mode FCS strip into the if (!skb) block:

	if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
		buf1_len -= ETH_FCS_LEN;

Together, these two changes let a 0-3 byte buf1_len underflow.

Second, the second-buffer case. Before 88095728511e, stmmac_rx_buf2_len()
started with:

	if (!priv->sph_active)
		return 0;

stmmac_xdp_set_prog() clears sph_active while XDP is enabled, so buf2_len
was always 0 in XDP mode. That check is now:

	if (!stmmac_rx_check_buf2_cap(priv))
		return 0;

So "GMAC4/XGMAC always provide a second data buffer per descriptor" has
only been true since 88095728511e. The new buf2_len drop fixes a condition
that commit introduced.

The last patch in this series ("net: stmmac: fix XDP RX buffer DMA sync
length") already carries Fixes: 88095728511e.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3fa2c15222d88..30a2774d722b7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5903,18 +5903,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> -		if (unlikely(error && (status & rx_not_ls)))
> -			goto read_again;
> +		/* We do not support SG in XDP mode */
> +		if (unlikely(!error && (status & rx_not_ls) &&
> +			     stmmac_xdp_is_enabled(priv))) {
> +			rx_dropped++;
> +			error = 1;
> +		}
>  
>  		if (unlikely(error)) {
> +			if (status & rx_not_ls)
> +				goto read_again;
> +
>  			dev_kfree_skb(skb);
>  			goto next;
>  		}

[Severity: High]
This is a pre-existing issue, and this patch did not introduce it. The new
error-and-drain handling only runs when stmmac_xdp_is_enabled() is true.
Without an XDP program, can the same re-parse still happen when
napi_build_skb() fails on a non-last descriptor?

The failure branch in stmmac_rx() leaves skb == NULL and error == 0:

	skb = napi_build_skb(page_address(buf->page),
			     rx_q->napi_skb_frag_size);
	if (!skb) {
		page_pool_recycle_direct(rx_q->page_pool,
					 buf->page);
		buf->page = NULL;
		rx_dropped++;
		count++;
		goto drain_data;
	}
	...
drain_data:
	if (likely(status & rx_not_ls))
		goto read_again;

The next descriptor of the same hardware frame then passes the error check
and is handled as the start of a new frame.

SPH is active by default on SPH-capable GMAC4/XGMAC when no XDP program is
attached. In that case stmmac_rx_buf1_len() returns 0 for this descriptor:

	if (priv->sph_active && len)
		return 0;

napi_build_skb() then builds an skb with an empty linear area, and the
payload is only in a buf2 frag. pskb_trim(skb, len) does nothing, because
the stale len is larger than skb->len.

With the default hw_vlan_en, nothing pulls the header into the linear area
before this call chain:

	eth_type_trans()
	  eth_skb_pull_mac()
	    skb_pull_inline(skb, ETH_HLEN)
	      __skb_pull()
	        BUG()

Under memory pressure, could an on-link peer panic the machine this way by
sending a frame larger than one buffer? GMAC_CONFIG_JE is set in
GMAC_CORE_INIT, so such frames are accepted even at MTU 1500.

Without SPH, the stack instead gets a frame whose Ethernet header comes
from the middle of the peer's payload.

This path is still unchanged at the end of the series. The VLAN rework in
"net: stmmac: fix VLAN tag stripping on short and non-linear RX frames"
adds pskb_may_pull() only to the software stmmac_rx_vlan() path.

Would it drain the frame correctly to set error = 1 in the
napi_build_skb() failure branch, as this patch does for XDP mode?

> @@ -5926,6 +5931,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
>  		len += buf2_len;
>  
> +		/* Drop the packet if it is split over buf1 and buf2 on a
> +		 * single descriptor if the interface is running in XDP mode.
> +		 */
> +		if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
> +			rx_dropped++;
> +			goto next;
> +		}

[Severity: Medium]
This isn't a bug introduced by this patch. The new drop paths add to
rx_dropped, which stmmac_rx() folds into a device-wide counter at the end:

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

These are plain unsigned long fields in struct stmmac_extra_stats. They
are updated with a non-atomic read-modify-write, and no shared lock is
held.

Each RX queue has its own NAPI instance. Can two queues polled on
different CPUs lose each other's increments here? The napi_syncp section
just before this only protects the per-queue rxq_stats.

The race goes back to 133466c3bbe171 ("net: stmmac: use per-queue 64 bit
statistics where necessary").

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com