netdev - Re: [Intel-wired-lan] [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+tcoAP7Zk7A4pzK-za+_NMoX11SGR3ubtY6R+aaywoEq_H+g@mail.gmail.com>
Date: Mon, 21 Jul 2025 17:15:41 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
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: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow
 of budget in zerocopy mode

Hi Paul,

On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@...gen.mpg.de> wrote:
>
> Dear Jason,
>
>
> Thank you for your patch.

Thanks for your quick response and review :)

>
> Am 21.07.25 um 10:33 schrieb Jason Xing:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > The issue can happen when the budget number of descs are consumed. As
>
> Instead of “The issue”, I’d use “An underflow …”.

Will change it.

>
> > 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.
>
> What is “it”?

It means 'underflow of budget' behavior.

>
> > In this case where all the budget are used up, it means zc function
>
> *is* used?

Got it.

>
> > should return false to let the poll run again because normally we
> > might have more data to process.
>
> Do you have a reproducer, you could add to the commit message?

Sorry, I didn't have a reproducer. I cooked this patch after analyzing
the whole logic (because recently I'm reading the zc xmit
implementation among various drivers.)

>
> > 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) {
>
> So, if the while loop should not be entered with budget being 0, then
> the line could  be changed to `while (--budget > 0) {`? But then it
> wouldn’t be called for budget being 1.

Right, so it shouldn't be '--budget'.

>
> A for loop might be the better choice for a loop with budget as counting
> variable?

Sorry, I didn't follow you.

>
> >               struct stmmac_metadata_request meta_req;
> >               struct xsk_tx_metadata *meta = NULL;
> >               dma_addr_t dma_addr;
> > @@ -2681,6 +2681,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >
> >               tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
> >               entry = tx_q->cur_tx;
> > +
> > +             budget--;
> >       }
> >       u64_stats_update_begin(&txq_stats->napi_syncp);
> >       u64_stats_add(&txq_stats->napi.tx_set_ic_bit, tx_set_ic_bit);
>
> Excuse my ignorance, but I do not yet see the problem that the while
> loop is entered and buffer is set to 0. Is it later the return condition?

Let me give a simple example. Supposing the budget is one initially,
at the first round, the budget will be zero. Later, after this desc
being processed, the 'while (budget-- > 0)' statement will be accessed
again, and then the budget will be decreased by one which is u32(0 -
1), namely, UINT_MAX. !!UINT_MAX is true while the expected return
value is false (!!0, 0 is the expected budget value).

i40e_clean_tx_irq() handles this correctly, FYI.

Thanks,
Jason

>
>      return !!budget && work_done;
>
>
> Kind regards,
>
> Paul