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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 4582 bytes --]

On Oct 12, Nicolai Buchwitz wrote:
> Hi Lorenzo
> 
> On 12.10.2026 18:44, Lorenzo Bianconi wrote:
> > stmmac_rx() runs the XDP program once per descriptor buffer and uses
> > skb == NULL as the only marker for the start of a frame. A frame larger
> > than one buffer is split by the hardware across several descriptors;
> > GMAC4 enables jumbo frames unconditionally (GMAC_CONFIG_JE in
> > GMAC_CORE_INIT), so an on-link peer can deliver such a frame even when
> > the interface MTU is left at 1500.
> > 
> > When an XDP verdict ends a non-last descriptor, skb is cleared and the
> > next descriptor of the same hardware frame is parsed as a new frame.
> > For XDP_CONSUMED this happens via read_again with len preserved, so
> > stmmac_rx_buf1_len() then returns min(dma_buf_sz, plen - len) for the
> > last descriptor, which can be as small as 0-3 bytes. buf1_len -=
> > ETH_FCS_LEN underflows and xdp_prepare_buff() puts data_end far past
> > data; on XDP_PASS this produces an skb whose length is close to 4G that
> > reaches the network stack (skb->tail and skb->len are 32-bit and wrap
> > below skb->end, so skb_over_panic() does not fire). A 4+ byte tail is
> > instead offered to the XDP program and delivered as a standalone
> > mid-payload Ethernet frame on XDP_PASS. XDP_TX/XDP_REDIRECT instead
> > falls through to the next iteration, resetting the running length, so
> > the tail is re-parsed as a full-size new frame.
> > 
> > XDP does not support scatter-gather frames until the multi-buffer
> > rework, so drop the whole frame instead: mark it as errored as soon as
> > a non-last descriptor is seen while an XDP program is attached, drain
> > and release every descriptor - including the last - and account the
> > frame once in rx_dropped. Removing the re-parse lets both XDP verdict
> > branches simply complete the packet via next.
> > 
> > Note: the subsequent "net: stmmac: rework stmmac_rx to support XDP rx
> > multi-buff" accumulates the whole frame into a single xdp_buff and
> > supersedes this patch; the fix is kept so the net portion of the series
> > stands on its own.
> > 
> > Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 28
> > ++++++++++-------------
> >  1 file changed, 12 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 426bb68c9bbc..8c08d44fdbd3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5893,18 +5893,24 @@ static int stmmac_rx(struct stmmac_priv *priv,
> > int limit, u32 queue)
> > 
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx +
> > entry);
> > -		if (unlikely(status == discard_frame)) {
> > -			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> > -			buf->page = NULL;
> > +
> > +		if (unlikely(!error && status == discard_frame)) {
> >  			error = 1;
> >  			if (!priv->hwts_rx_en)
> >  				rx_errors++;
> >  		}
> > 
> > -		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;
> > +		}
> 
> AFAIU GMAC4 and XGMAC always use the second buffer of a descriptor. If
> so, at MTU 1500 a frame of up to 3072 bytes can fit in one descriptor,
> and this check would not catch it?
> 
> The XDP program would then only see the first buffer. With XDP_TX the
> frame is cut and sent too short. With XDP_PASS the second buffer is added
> after the program already decided.
> 
> Patch 4 fixes this. But if this patch should work on its own, should it
> also drop the frame when buf2_len is not zero? Or am I missing
> something?

Hi Nicolai,

IIUC your comment we are missing the case where we have a single descriptor
(rx_not_ls is not set in status) but the packet is splitted over buf1 and buf2,
right? Something like:

	...
	buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
	len += buf2_len;

	if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
		rx_dropped++;
		dev_kfree_skb(skb);
		goto next;
	}
	...

Is my understanding correct?

Regards,
Lorenzo

> 
> > [...]
> 
> Thanks,
> Nicolai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]