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+tcoDo3p52+j8hahpDVAcWF-pGPAoGY5Z_=wkn25C=eH3FNA@mail.gmail.com>
Date: Tue, 22 Jul 2025 08:12:30 +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 Tue, Jul 22, 2025 at 7:05 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> 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?

Sorry, I was totally wrong. Your suggestions work and I will revise
them as you said :)

Thanks,
Jason