nft_counter_reset() calls u64_stats_add() with a negative value to reset
the counter. This will work on 64bit archs, hence the negative value
added will wrap as a 64bit value which then can wrap the stat counter as
well.

On 32bit archs, the added negative value will wrap as a 32bit value and
_not_ wrapping the stat counter properly. In most cases, this would just
lead to a very large 32bit value being added to the stat counter.

Fix by introducing u64_stats_sub().

Fixes: 4a1d3acd6ea8 ("netfilter: nft_counter: Use u64_stats_t for statistic")
Signed-off-by: Anders Grahn <anders.grahn@westermo.com>
---
 include/linux/u64_stats_sync.h | 10 ++++++++++
 net/netfilter/nft_counter.c    |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index 457879938fc1..9942d29b17e5 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -89,6 +89,11 @@ static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
 	local64_add(val, &p->v);
 }
 
+static inline void u64_stats_sub(u64_stats_t *p, unsigned long val)
+{
+	local64_sub(val, &p->v);
+}
+
 static inline void u64_stats_inc(u64_stats_t *p)
 {
 	local64_inc(&p->v);
@@ -130,6 +135,11 @@ static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
 	p->v += val;
 }
 
+static inline void u64_stats_sub(u64_stats_t *p, unsigned long val)
+{
+	p->v -= val;
+}
+
 static inline void u64_stats_inc(u64_stats_t *p)
 {
 	p->v++;
diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
index cc7325329496..0d70325280cc 100644
--- a/net/netfilter/nft_counter.c
+++ b/net/netfilter/nft_counter.c
@@ -117,8 +117,8 @@ static void nft_counter_reset(struct nft_counter_percpu_priv *priv,
 	nft_sync = this_cpu_ptr(&nft_counter_sync);
 
 	u64_stats_update_begin(nft_sync);
-	u64_stats_add(&this_cpu->packets, -total->packets);
-	u64_stats_add(&this_cpu->bytes, -total->bytes);
+	u64_stats_sub(&this_cpu->packets, total->packets);
+	u64_stats_sub(&this_cpu->bytes, total->bytes);
 	u64_stats_update_end(nft_sync);
 
 	local_bh_enable();
-- 
2.43.0

On Mon, Dec 15, 2025 at 01:12:57PM +0100, Anders Grahn wrote:
> nft_counter_reset() calls u64_stats_add() with a negative value to reset
> the counter. This will work on 64bit archs, hence the negative value
> added will wrap as a 64bit value which then can wrap the stat counter as
> well.
> 
> On 32bit archs, the added negative value will wrap as a 32bit value and
> _not_ wrapping the stat counter properly. In most cases, this would just
> lead to a very large 32bit value being added to the stat counter.
> 
> Fix by introducing u64_stats_sub().
> 
> Fixes: 4a1d3acd6ea8 ("netfilter: nft_counter: Use u64_stats_t for statistic")

Nit: there is a minor mismatch in the subject of the Fixes tag and
     git history: the trailing '.'
     I would go for this. But perhaps it doesn't matter.

Fixes: 4a1d3acd6ea8 ("netfilter: nft_counter: Use u64_stats_t for statistic.")

> Signed-off-by: Anders Grahn <anders.grahn@westermo.com>

...

Anders Grahn <anders.grahn@gmail.com> wrote:
> nft_counter_reset() calls u64_stats_add() with a negative value to reset
> the counter. This will work on 64bit archs, hence the negative value
> added will wrap as a 64bit value which then can wrap the stat counter as
> well.
> 
> On 32bit archs, the added negative value will wrap as a 32bit value and
> _not_ wrapping the stat counter properly. In most cases, this would just
> lead to a very large 32bit value being added to the stat counter.
> 
> Fix by introducing u64_stats_sub().
> 
> Fixes: 4a1d3acd6ea8 ("netfilter: nft_counter: Use u64_stats_t for statistic")
> Signed-off-by: Anders Grahn <anders.grahn@westermo.com>
> ---
>  include/linux/u64_stats_sync.h | 10 ++++++++++
>  net/netfilter/nft_counter.c    |  4 ++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
> index 457879938fc1..9942d29b17e5 100644
> --- a/include/linux/u64_stats_sync.h
> +++ b/include/linux/u64_stats_sync.h
> @@ -89,6 +89,11 @@ static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
>  	local64_add(val, &p->v);
>  }
>  
> +static inline void u64_stats_sub(u64_stats_t *p, unsigned long val)
> +{
> +	local64_sub(val, &p->v);
> +}

That still truncates val on 32bit.  Maybe use "s64 val"?

> That still truncates val on 32bit.  Maybe use "s64 val"?

Agree. Thanks for the feedback.

On 32bit archs, you're definitely right. It would truncate the counter value on
32bit. However, the problem I intended to fix was the fact that, previously, a
negative value was passed to u64_stats_add() which always wrapped.

Initially, I was a bit reluctant to use s64 for u64_stats_sub() as I wanted to
keep the signature the same as the existing u64_stats_add(). As u64_stats_add()
is used in a lot of places, I was not sure about the effect of this.

However, I can prepare a v2 with just u64_stats_sub(u64_stats_t *, s64).

On Mon, 15 Dec 2025 13:36:11 +0100
Florian Westphal <fw@strlen.de> wrote:

> Anders Grahn <anders.grahn@gmail.com> wrote:
> > nft_counter_reset() calls u64_stats_add() with a negative value to reset
> > the counter. This will work on 64bit archs, hence the negative value
> > added will wrap as a 64bit value which then can wrap the stat counter as
> > well.
> > 
> > On 32bit archs, the added negative value will wrap as a 32bit value and
> > _not_ wrapping the stat counter properly. In most cases, this would just
> > lead to a very large 32bit value being added to the stat counter.
> > 
> > Fix by introducing u64_stats_sub().
> > 
> > Fixes: 4a1d3acd6ea8 ("netfilter: nft_counter: Use u64_stats_t for statistic")
> > Signed-off-by: Anders Grahn <anders.grahn@westermo.com>
> > ---
> >  include/linux/u64_stats_sync.h | 10 ++++++++++
> >  net/netfilter/nft_counter.c    |  4 ++--
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
> > index 457879938fc1..9942d29b17e5 100644
> > --- a/include/linux/u64_stats_sync.h
> > +++ b/include/linux/u64_stats_sync.h
> > @@ -89,6 +89,11 @@ static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
> >  	local64_add(val, &p->v);
> >  }
> >  
> > +static inline void u64_stats_sub(u64_stats_t *p, unsigned long val)
> > +{
> > +	local64_sub(val, &p->v);
> > +}  
> 
> That still truncates val on 32bit.  Maybe use "s64 val"?
> 

It probably depends on the type of total->bytes and total->packets.
The 'bytes' value will wrap 32bits quickly, so may need to be 64bit anyway.
And if (elsewhere) there are this_cpu->bytes += val; total->bytes += val;
pairs you can't 'undo' the adds once total->bytes has wrapped.
So should any of these types be 'long' at all?

I sometimes think that 'long' should be pretty much never used in the
kernel because of the 32bit/64bit portability issues.
But that should have been sorted over 20 years ago.
Maybe M$ had it right keeping long as 32bits :-)

	David