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

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

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

On Oct 27, netdev-bot+sashiko@kernel.org wrote:
> 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.

ack, I will fix the commit message in v2

> 
> > 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?

This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.

> 
> 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;

This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.

Regards,
Lorenzo

> 
> 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

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