Re: "nft reset counters" bug on 32-bit systems

Andreas Fried <[email protected]> wrote:
> On 9/10/25 20:08, Florian Westphal wrote:
> > I'd suggest to turn nft_counter_reset() into a variant of nft_counter_fetch()
> > that uses local_xchg() instead of reads.
> > 
> > Expensive but reset requests should be rare and its much clearer as to
> > what is happening vs. this
> > fetch-and-then-add-negative-total-to-one-pcpu-counter.
> 
> I'm afraid I'm missing something here. Wouldn't this be too expensive?

No, but doesn't work due to not being atomic wrt. userspace reading
stats in parallel.

> nft_counter_fetch() can read the stats from other CPUs without issues,
> but that won't work for writing, i.e. local_xchg(), right? local64_t is
> only atomic with respect to one CPU, so would we need to schedule work
> on each CPU to read and reset the counters?

No, it simply won't work.  We need to continue to use the existing
scheme to make sure userspace observers either the counter before reset,
0, or a new updated value that happened right after the reset.

With my proposal, userspace can observe incorrect value when one cpu
is doing the reset, as for each xchg it can observe 'shrinking'
counters.

> In d84701ecbcd6ad63faa7a9c18ad670d1c4d561c0, Pablo Neira points out that
> cmpxchg will not work unless all other functions also use it, and that's
> too slow.

A-ha. Yes, its not atomic wrt. read path.

Someone needs to add u64_stats_add64() so this can be fixed
for 32bit platforms in a followup patch.

local_add() already does the right thing for us, so the stats api is the
only missing piece of the puzzle.