[PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit

[PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Fix two underflow issues around {stmmac_xdp|igb}_xmit_zc().

Jason Xing (2):
  stmmac: xsk: fix underflow of budget in zerocopy mode
  igb: xsk: solve underflow of nb_pkts in zerocopy mode

 drivers/net/ethernet/intel/igb/igb_xsk.c          | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.41.3

[Intel-wired-lan] [PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Fix two underflow issues around {stmmac_xdp|igb}_xmit_zc().

Jason Xing (2):
  stmmac: xsk: fix underflow of budget in zerocopy mode
  igb: xsk: solve underflow of nb_pkts in zerocopy mode

 drivers/net/ethernet/intel/igb/igb_xsk.c          | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.41.3

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.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@tencent.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) {
 		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);
-- 
2.41.3

[Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode

[Jason Xing]

From: Jason Xing <kernelxing@tencent.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@tencent.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) {
 		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);
-- 
2.41.3

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

[Paul Menzel]

Dear Jason,


Thank you for your patch.

Am 21.07.25 um 10:33 schrieb Jason Xing:
> From: Jason Xing <kernelxing@tencent.com>
> 
> The issue can happen when the budget number of descs are consumed. As

Instead of “The issue”, I’d use “An underflow …”.

> 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”?

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

*is* used?

> 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?

> Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> Signed-off-by: Jason Xing <kernelxing@tencent.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.

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

>   		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?

     return !!budget && work_done;


Kind regards,

Paul

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

[Jason Xing]

Hi Paul,

On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.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@tencent.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@tencent.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

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

[Willem de Bruijn]

Jason Xing wrote:
> Hi Paul,
> 
> On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.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@tencent.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.

A technicality, but this is (negative) overflow.

Underflow is a computation that results in a value that is too small
to be represented by the given type.

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

[Jason Xing]

On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > Hi Paul,
> >
> > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.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@tencent.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.
>
> A technicality, but this is (negative) overflow.
>
> Underflow is a computation that results in a value that is too small
> to be represented by the given type.

Interesting. Thanks for sharing this with me:)

I just checked the wikipedia[1] that says " Underflow can in part be
regarded as negative overflow of the exponent of the floating-point
value.". I assume this rule can also be applied in this case? I'm
hesitant to send the v3 patch tomorrow with this 'negative overflow'
term included.

[1]: https://en.wikipedia.org/wiki/Arithmetic_underflow

Thanks,
Jason

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

[Willem de Bruijn]

Jason Xing wrote:
> On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > Hi Paul,
> > >
> > > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.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@tencent.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.
> >
> > A technicality, but this is (negative) overflow.
> >
> > Underflow is a computation that results in a value that is too small
> > to be represented by the given type.
> 
> Interesting. Thanks for sharing this with me:)
> 
> I just checked the wikipedia[1] that says " Underflow can in part be
> regarded as negative overflow of the exponent of the floating-point
> value.". I assume this rule can also be applied in this case? I'm
> hesitant to send the v3 patch tomorrow with this 'negative overflow'
> term included.

My point is very pedantic. I think these cases are not underflow.

But it is often called that, understandably. So choose as you see fit.

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

[Jason Xing]

On Wed, Jul 23, 2025 at 1:29 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > Hi Paul,
> > > >
> > > > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.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@tencent.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.
> > >
> > > A technicality, but this is (negative) overflow.
> > >
> > > Underflow is a computation that results in a value that is too small
> > > to be represented by the given type.
> >
> > Interesting. Thanks for sharing this with me:)
> >
> > I just checked the wikipedia[1] that says " Underflow can in part be
> > regarded as negative overflow of the exponent of the floating-point
> > value.". I assume this rule can also be applied in this case? I'm
> > hesitant to send the v3 patch tomorrow with this 'negative overflow'
> > term included.
>
> My point is very pedantic. I think these cases are not underflow.
>
> But it is often called that, understandably. So choose as you see fit.

I see. Thanks for pointing that out. I will change it :)

Thanks,
Jason

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

[Stanislav Fomichev]

On 07/21, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.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@tencent.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. Should we do 'for (; budget > 0; budget--)'
instead? And maybe the same for ixgbe [0]?

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

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

[Stanislav Fomichev]

On 07/21, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.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@tencent.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. Should we do 'for (; budget > 0; budget--)'
instead? And maybe the same for ixgbe [0]?

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

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

[Jason Xing]

On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
<stfomichev@gmail.com> wrote:
>
> On 07/21, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.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@tencent.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

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

[Jason Xing]

On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
<stfomichev@gmail.com> wrote:
>
> On 07/21, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.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@tencent.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

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

[Jason Xing]

On Tue, Jul 22, 2025 at 7:05 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
> <stfomichev@gmail.com> wrote:
> >
> > On 07/21, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.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@tencent.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

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

[Jason Xing]

On Tue, Jul 22, 2025 at 7:05 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
> <stfomichev@gmail.com> wrote:
> >
> > On 07/21, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.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@tencent.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

[PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

There is no break time in the while() loop, so every time at the end of
igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
value always false. But theoretically, the result should be set after
calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
a good example.

Returning false means we're not done with transmission and we need one
more poll, which is exactly what igb_xmit_zc() always did before this
patch. After this patch, the return value depends on the nb_pkts value.
Two cases might happen then:
1. if (nb_pkts < budget), it means we process all the possible data, so
   return true and no more necessary poll will be triggered because of
   this.
2. if (nb_pkts == budget), it means we might have more data, so return
   false to let another poll run again.

Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index 5cf67ba29269..243f4246fee8 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
 	if (!nb_pkts)
 		return true;
 
-	while (nb_pkts-- > 0) {
+	while (i < nb_pkts) {
 		dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
 		xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
 
-- 
2.41.3

[Intel-wired-lan] [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

There is no break time in the while() loop, so every time at the end of
igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
value always false. But theoretically, the result should be set after
calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
a good example.

Returning false means we're not done with transmission and we need one
more poll, which is exactly what igb_xmit_zc() always did before this
patch. After this patch, the return value depends on the nb_pkts value.
Two cases might happen then:
1. if (nb_pkts < budget), it means we process all the possible data, so
   return true and no more necessary poll will be triggered because of
   this.
2. if (nb_pkts == budget), it means we might have more data, so return
   false to let another poll run again.

Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index 5cf67ba29269..243f4246fee8 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
 	if (!nb_pkts)
 		return true;
 
-	while (nb_pkts-- > 0) {
+	while (i < nb_pkts) {
 		dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
 		xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
 
-- 
2.41.3

Re: [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Simon Horman]

On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> There is no break time in the while() loop, so every time at the end of
> igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> value always false. But theoretically, the result should be set after
> calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> a good example.
> 
> Returning false means we're not done with transmission and we need one
> more poll, which is exactly what igb_xmit_zc() always did before this
> patch. After this patch, the return value depends on the nb_pkts value.
> Two cases might happen then:
> 1. if (nb_pkts < budget), it means we process all the possible data, so
>    return true and no more necessary poll will be triggered because of
>    this.
> 2. if (nb_pkts == budget), it means we might have more data, so return
>    false to let another poll run again.
> 
> Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index 5cf67ba29269..243f4246fee8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
>  	if (!nb_pkts)
>  		return true;
>  
> -	while (nb_pkts-- > 0) {
> +	while (i < nb_pkts) {

Hi Jason,

FWIIW, I think using a for loop is a more idiomatic way
of handling the relationship between i, nb_pkts, and
the iterations of this loop.

>  		dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
>  		xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
>  
> -- 
> 2.41.3
> 
> 

Re: [Intel-wired-lan] [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Simon Horman]

On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> There is no break time in the while() loop, so every time at the end of
> igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> value always false. But theoretically, the result should be set after
> calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> a good example.
> 
> Returning false means we're not done with transmission and we need one
> more poll, which is exactly what igb_xmit_zc() always did before this
> patch. After this patch, the return value depends on the nb_pkts value.
> Two cases might happen then:
> 1. if (nb_pkts < budget), it means we process all the possible data, so
>    return true and no more necessary poll will be triggered because of
>    this.
> 2. if (nb_pkts == budget), it means we might have more data, so return
>    false to let another poll run again.
> 
> Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index 5cf67ba29269..243f4246fee8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
>  	if (!nb_pkts)
>  		return true;
>  
> -	while (nb_pkts-- > 0) {
> +	while (i < nb_pkts) {

Hi Jason,

FWIIW, I think using a for loop is a more idiomatic way
of handling the relationship between i, nb_pkts, and
the iterations of this loop.

>  		dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
>  		xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
>  
> -- 
> 2.41.3
> 
> 

Re: [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Jason Xing]

Hi Simon,

On Mon, Jul 21, 2025 at 6:12 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > There is no break time in the while() loop, so every time at the end of
> > igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> > value always false. But theoretically, the result should be set after
> > calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> > a good example.
> >
> > Returning false means we're not done with transmission and we need one
> > more poll, which is exactly what igb_xmit_zc() always did before this
> > patch. After this patch, the return value depends on the nb_pkts value.
> > Two cases might happen then:
> > 1. if (nb_pkts < budget), it means we process all the possible data, so
> >    return true and no more necessary poll will be triggered because of
> >    this.
> > 2. if (nb_pkts == budget), it means we might have more data, so return
> >    false to let another poll run again.
> >
> > Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > index 5cf67ba29269..243f4246fee8 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
> >       if (!nb_pkts)
> >               return true;
> >
> > -     while (nb_pkts-- > 0) {
> > +     while (i < nb_pkts) {
>
> Hi Jason,
>
> FWIIW, I think using a for loop is a more idiomatic way
> of handling the relationship between i, nb_pkts, and
> the iterations of this loop.

Sure, I can turn it into 'for (i = 0; i < nb_pkts; i++)' in the next version.

Thanks,
Jason

Re: [Intel-wired-lan] [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode

[Jason Xing]

Hi Simon,

On Mon, Jul 21, 2025 at 6:12 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > There is no break time in the while() loop, so every time at the end of
> > igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> > value always false. But theoretically, the result should be set after
> > calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> > a good example.
> >
> > Returning false means we're not done with transmission and we need one
> > more poll, which is exactly what igb_xmit_zc() always did before this
> > patch. After this patch, the return value depends on the nb_pkts value.
> > Two cases might happen then:
> > 1. if (nb_pkts < budget), it means we process all the possible data, so
> >    return true and no more necessary poll will be triggered because of
> >    this.
> > 2. if (nb_pkts == budget), it means we might have more data, so return
> >    false to let another poll run again.
> >
> > Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > index 5cf67ba29269..243f4246fee8 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
> >       if (!nb_pkts)
> >               return true;
> >
> > -     while (nb_pkts-- > 0) {
> > +     while (i < nb_pkts) {
>
> Hi Jason,
>
> FWIIW, I think using a for loop is a more idiomatic way
> of handling the relationship between i, nb_pkts, and
> the iterations of this loop.

Sure, I can turn it into 'for (i = 0; i < nb_pkts; i++)' in the next version.

Thanks,
Jason