Re: [PATCH net-next v3] net: stmmac: add XDP multi-buff support for TX side

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

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

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The patch fixes a divide-by-zero crash in the XDP TX path
>   without saying so and without a Fixes: tag.
> - [Low] In stmmac_xdp_xmit_xdpf(), the new coalescing check reads
>   priv->tx_coal_frames[queue] twice with plain loads: once for the zero…
> 
> --
> 
> net: stmmac: add XDP multi-buff support for TX side
> 
> This commit extends stmmac_xdp_xmit_xdpf() so it can transmit XDP frames
> that carry fragments. The frame head and each frag get their own TX
> descriptor. The driver now also advertises NDO_XMIT_SG for the redirect
> target.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3ad9252bf6aea..f1e9ac2f98598 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5241,77 +5241,118 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
> >  				struct xdp_frame *xdpf, bool dma_map)
> >  {
> 
> [ ... ]
> 
> > +	tx_q->tx_count_frames += num_frames;
> >  
> > -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> > -		set_ic = true;
> > -	else
> > -		set_ic = false;
> > +	if (priv->tx_coal_frames[queue] &&
> > +	    (tx_q->tx_count_frames % priv->tx_coal_frames[queue]) < num_frames) {
> 
> [Severity: Medium]
> The old code here divided by priv->tx_coal_frames[queue] without checking
> it for zero. That code came in with be8b38a722e6 ("net: stmmac: Add support
> for XDP_TX action").
> 
> Can tx_coal_frames[queue] be 0 here? __stmmac_set_coalesce() only rejects
> the request when both values are zero:
> 
> 	if ((ec->tx_coalesce_usecs == 0) &&
> 	    (ec->tx_max_coalesced_frames == 0))
> 		return -EINVAL;
> 
> So a command like "ethtool -C ethX tx-usecs 100 tx-frames 0" is accepted
> and stores 0 in priv->tx_coal_frames[]. Before this patch, the next XDP_TX
> or ndo_xdp_xmit frame would then do a modulo by zero in softirq context.
> 
> The new priv->tx_coal_frames[queue] guard fixes that, but the commit
> message only describes multi-buff TX support. stmmac_xmit(),
> stmmac_tso_xmit() and stmmac_xdp_xmit_zc() already have this check.
> 
> Could the fix be split into its own patch with a
> "Fixes: be8b38a722e6" tag, so it can reach stable?

I guess we can avoid to add the 'Fixes tag' here (and so to backport this
patch) since the issue seems quite a corner case.

> 
> [Severity: Low]
> This check does two plain loads of priv->tx_coal_frames[queue]: one for the
> zero test and one for the divisor.
> 
> __stmmac_set_coalesce() writes this field under RTNL. It does not hold the
> TX queue lock that stmmac_xdp_xmit() and stmmac_xdp_xmit_back() take, and
> it can store 0.
> 
> If the compiler reloads the field between the two reads and a 0 is stored
> in that window, can the modulo still divide by zero? Would taking a single
> snapshot help, for example:
> 
> 	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);
> 
> and then using coal for both the test and the division?
> 
> stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() read the field
> twice in the same way.

I agree there is a small race here, but the same issue is present in
stmmac_xmit() and in stmmac_tso_xmit(). I think we can fix them with a single
dedicated patch.

Regards,
Lorenzo

> 
> > +		struct stmmac_txq_stats *txq_stats;
> >  
> > -	if (set_ic) {
> > +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> > +		stmmac_set_tx_ic(priv, desc);
> >  		tx_q->tx_count_frames = 0;
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-b4-stmmac-xmit-mb-v3-1-ca08f029e81c%40oss.qualcomm.com

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