"nft reset counters" bug on 32-bit systems

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

on 32-bit systems, resetting netfilter counters sets them to 2**32
instead if their count was not 0:

# nft list counters
table ip foo {
	counter test1 {
		packets 4 bytes 240
	}
	counter test2 {
		packets 0 bytes 0
	}
}
# nft reset counters
table ip foo {
	counter test1 {
		packets 4 bytes 240
	}
	counter test2 {
		packets 0 bytes 0
	}
}
# nft list counters
table ip foo {
	counter test1 {
		packets 4294967296 bytes 4294967296
	}
	counter test2 {
		packets 0 bytes 0
	}
}

This was tested on an arm32 system running kernel 6.1.134 and nftables
1.0.9, but as far as I can see, the code is the same on current master.
Looking at nft_counter_reset(), this is a problem for systems where long
is 32 bits wide.

nft_counter_reset() wants to subtract the current total from the
counter, so it calls u64_stats_add() with a negative "val" argument
(e.g. -total->packets, in our case -4). But that argument is an unsigned
long (u32) being added to the u64 counter. That means that it actually
adds 0xfffffffc to the counter, giving 0x100000000, a.k.a. 2**32.

Seeing that u64_stats are used all over the place, any change might
break all sorts of other things. So I'm hesitating to suggest a patch,
just some observations:

The first solution would be a separate u64_stats_sub() function.
However, that still limits the functions to non-negative arguments, and
negative arguments still do unexpected things to the u64 counter.

It also seems to be OK to change u64_stats_add() to take s64 instead of
unsigned long. On 32-bit systems, it only makes the variable wider, so
there's no problem.

For 64-bit systems, this makes the variable one bit narrower because
it's now signed. This might be a problem, but there is some signedness
confusion going on anyway: The counter in u64_stats_t is a local64_t,
which turns out to be a wrapped s64, rather than the u64 used in 32-bit
u64_stats_t. The "val" parameter of u64_stats_add is unsigned long (i.e.
u64), but that boils down to atomic_long_add, which takes a (signed)
long (i.e. s64) parameter. So it seems like making u64_stats_add()
taking an s64 in the first place won't break.

Thanks for your help,
Andreas

-- 
Andreas Fried
 
emlix GmbH
Headquarters: Berliner Str. 12, 37073 Goettingen, Germany
Phone +49 (0)551 30664-0, e-mail info@xxxxxxxxx
District Court of Goettingen, Registry Number HR B 3160
Managing Directors: Heike Jordan, Dr. Uwe Kracke
VAT ID No. DE 205 198 055
Office Berlin: Panoramastr. 1, 10178 Berlin, Germany
Office Bonn: Bachstr. 6, 53115 Bonn, Germany
http://www.emlix.com

emlix - your embedded Linux partner





[Index of Archives]     [Linux Netfilter Development]     [Linux Kernel Networking Development]     [Netem]     [Berkeley Packet Filter]     [Linux Kernel Development]     [Advanced Routing & Traffice Control]     [Bugtraq]

  Powered by Linux