Re: [PATCH net-next v4 1/2] net: stmmac: fix TX coalesce race and div-by-zero in XDP/legacy path

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

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

On Oct 05, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 commit message says the stmmac_xdp_xmit_xdpf() divide-by-
>   zero is "only theoretical", but a privileged user can trigger it and…
> 
> Pre-existing issues:
> - [Low] __stmmac_set_coalesce() publishes tx_coal_frames[q] and
>   tx_coal_timer[q] as two separate stores and shares no synchronisation…
> 
> --
> 
> net: stmmac: fix TX coalesce race and div-by-zero in XDP/legacy path
> 
> This patch wraps every access to tx_coal_frames[] in READ_ONCE() or
> WRITE_ONCE(), since ethtool writes the array while the TX fast paths read
> it without a lock. It also guards the modulo in stmmac_xdp_xmit_xdpf()
> against a zero tx_coal_frames[queue].
> 
> > Both issues were found by code inspection while extending the XDP TX
> > path with multi-buff support, and neither has ever been triggered, so
> > they are only theoretical. Tested on the Qualcomm RB3-gen2 board.
> 
> [Severity: Medium]
> 
> Is the division by zero in stmmac_xdp_xmit_xdpf() really only
> theoretical?

with 'theoretical' I mean I spotted the issue with code inspection and I have
never observed it in practice. I agree this is a real issue.

> 
> __stmmac_set_coalesce() only rejects the case where both values are zero:
> 
> 	if ((ec->tx_coalesce_usecs == 0) &&
> 	    (ec->tx_max_coalesced_frames == 0))
> 		return -EINVAL;
> 
> So "ethtool -C ethX tx-frames 0" is accepted. tx-usecs keeps its default
> of STMMAC_COAL_TX_TIMER, and stmmac_ethtool_ops advertises
> ETHTOOL_COALESCE_MAX_FRAMES. After that, tx_coal_frames[queue] is 0.
> 
> >From then on, before this patch, every XDP_TX verdict
> (stmmac_xdp_xmit_back()->stmmac_xdp_xmit_xdpf()) and every ndo_xdp_xmit
> redirect into the device would evaluate:
> 
> 	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> 
> On x86, for example the dwmac-intel platforms that advertise XDP
> basic and redirect, this raises a divide error in NAPI softirq context.
> 
> On arm64, UDIV with a zero divisor returns 0 instead of trapping. That
> may be why the RB3-gen2 board never hit it.
> 
> CAP_NET_ADMIN can trigger this with standard tools. Could the commit
> message describe it as a reachable crash rather than a theoretical one?
> Would it also make sense to send the div-by-zero fix separately from the
> READ_ONCE()/WRITE_ONCE() annotations?
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > index 1cf0f8820b337..30e0d499326d8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> 
> [ ... ]
> 
> > @@ -887,14 +888,14 @@ static int __stmmac_set_coalesce(struct net_device *dev,
> >  		int i;
> >  
> >  		for (i = 0; i < tx_cnt; i++) {
> > -			priv->tx_coal_frames[i] =
> > -				ec->tx_max_coalesced_frames;
> > +			WRITE_ONCE(priv->tx_coal_frames[i],
> > +				   ec->tx_max_coalesced_frames);
> >  			priv->tx_coal_timer[i] =
> >  				ec->tx_coalesce_usecs;
> >  		}
> >  	} else if (queue < tx_cnt) {
> > -		priv->tx_coal_frames[queue] =
> > -			ec->tx_max_coalesced_frames;
> > +		WRITE_ONCE(priv->tx_coal_frames[queue],
> > +			   ec->tx_max_coalesced_frames);
> >  		priv->tx_coal_timer[queue] =
> >  			ec->tx_coalesce_usecs;
> >  	}
> 
> [Severity: Low]
> 
> This is a pre-existing issue and was not introduced by this patch.
> tx_coal_frames[] and tx_coal_timer[] are still published as two separate
> stores, with no synchronisation against the TX path.

as pointed out by sashiko, this is a pre-existing issue and it needs to be
fixed with a dedicated patch.

Regards,
Lorenzo

> 
> Can a concurrent stmmac_xmit() see a mixed pair? Take a change from
> (frames=N, usecs=0) to (frames=0, usecs=U). The TX path could read the
> new frames value:
> 
> 	tx_coal = READ_ONCE(priv->tx_coal_frames[queue]);
> 	...
> 	else if (!tx_coal)
> 		set_ic = false;
> 
> It could then read the old timer value in stmmac_tx_timer_arm():
> 
> 	u32 tx_coal_timer = priv->tx_coal_timer[queue];
> 
> 	if (!tx_coal_timer)
> 		return;
> 
> That frame would then get neither an IC interrupt nor the completion
> timer. Its completion would wait for the next TX or NAPI activity on that
> queue. The same window exists for stmmac_tso_xmit() and
> stmmac_xdp_xmit_xdpf().
> 
> READ_ONCE()/WRITE_ONCE() make each field consistent on its own, which is
> all the commit message claims. The window across the two fields is still
> there.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-b4-stmmac-xmit-mb-v4-0-664e0c3eb289%40oss.qualcomm.com

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