netdev - Re: [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoB9U-YnJ7MPn7FQ4+ZsW5cgQXE3Tks-7=kGMhUE6nNprg@mail.gmail.com>
Date: Tue, 22 Jul 2025 07:05:35 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Stanislav Fomichev <stfomichev@...il.com>
Cc: anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com, 
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, bjorn@...nel.org, 
	magnus.karlsson@...el.com, maciej.fijalkowski@...el.com, 
	jonathan.lemon@...il.com, sdf@...ichev.me, ast@...nel.org, 
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com, 
	mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, 
	linux-stm32@...md-mailman.stormreply.com, bpf@...r.kernel.org, 
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in
 zerocopy mode

On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
<stfomichev@...il.com> wrote:
>
> On 07/21, Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > The issue can happen when the budget number of descs are consumed. As
> > long as the budget is decreased to zero, it will again go into
> > while (budget-- > 0) statement and get decreased by one, so the
> > underflow issue can happen. It will lead to returning true whereas the
> > expected value should be false.
> >
> > In this case where all the budget are used up, it means zc function
> > should return false to let the poll run again because normally we
> > might have more data to process.
> >
> > Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f350a6662880..ea5541f9e9a6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >
> >       budget = min(budget, stmmac_tx_avail(priv, queue));
> >
> > -     while (budget-- > 0) {
> > +     while (budget > 0) {
>
> There is a continue on line 2621.

Thanks for catching this!

> Should we do 'for (; budget > 0; budget--)'
> instead? And maybe the same for ixgbe [0]?

Not really. I think I can move the 'budget--' just before the
'continue' part. If we convert it to use 'for' loop and then we end up
with one of 'break' statements, the budget still gets accidently
increased by one whereas ixgbe driver even doesn't handle the desc
this time. IIUC, it should not happen, right?

>
> 0: https://lore.kernel.org/netdev/20250720091123.474-3-kerneljasonxing@gmail.com/

The same logic as above can be applied here as well. There are three
'break' statements in ixgbe_xmit_zc().

Hence, IMHO, I prefer to use while(...) in this case but I ought to
adjust the position of budget--.

Thanks,
Jason