Re: [PATCH] ipset: Update byte and packet counters regardless of whether they match

[Jozsef Kadlecsik <kadlec@netfilter.org>]

Hi Stefano MithilMithil,

On Mon, 24 Feb 2020, Stefano Brivio wrote:

> In ip_set_match_extensions(), for sets with counters, we take care of 
> updating counters themselves by calling ip_set_update_counter(), and of 
> checking if the given comparison and values match, by calling 
> ip_set_match_counter() if needed.
> 
> However, if a given comparison on counters doesn't match the configured 
> values, that doesn't mean the set entry itself isn't matching.
> 
> This fix restores the behaviour we had before commit 4750005a85f7 
> ("netfilter: ipset: Fix "don't update counters" mode when counters used 
> at the matching"), without reintroducing the issue fixed there: back 
> then, mtype_data_match() first updated counters in any case, and then 
> took care of matching on counters.
> 
> Now, if the IPSET_FLAG_SKIP_COUNTER_UPDATE flag is set,
> ip_set_update_counter() will anyway skip counter updates if desired.
> 
> The issue observed is illustrated by this reproducer:
> 
>   ipset create c hash:ip counters
>   ipset add c 192.0.2.1
>   iptables -I INPUT -m set --match-set c src --bytes-gt 800 -j DROP
> 
> if we now send packets from 192.0.2.1, bytes and packets counters
> for the entry as shown by 'ipset list' are always zero, and, no
> matter how many bytes we send, the rule will never match, because
> counters themselves are not updated.

Sorry, but I disagree. ipset behaves the same as iptables itself: the 
counters are increased when the whole rule matches and that includes the 
counter comparison as well. I think it's less counter-intuitive that one 
can create never matching rules than to explain that "counter matching is 
a non-match for the point of view of 'when the rule matches, update the 
counter'".

What's really missing is a decrement-counters flag: that way one could 
store different "quotas" for the elements in a set.

Best regards,
Jozsef
 
> Reported-by: Mithil Mhatre <mmhatre@redhat.com>
> Fixes: 4750005a85f7 ("netfilter: ipset: Fix "don't update counters" mode when counters used at the matching")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  net/netfilter/ipset/ip_set_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 69c107f9ba8d..b140e38d9333 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -649,13 +649,14 @@ ip_set_match_extensions(struct ip_set *set, const struct ip_set_ext *ext,
>  	if (SET_WITH_COUNTER(set)) {
>  		struct ip_set_counter *counter = ext_counter(data, set);
>  
> +		ip_set_update_counter(counter, ext, flags);
> +
>  		if (flags & IPSET_FLAG_MATCH_COUNTERS &&
>  		    !(ip_set_match_counter(ip_set_get_packets(counter),
>  				mext->packets, mext->packets_op) &&
>  		      ip_set_match_counter(ip_set_get_bytes(counter),
>  				mext->bytes, mext->bytes_op)))
>  			return false;
> -		ip_set_update_counter(counter, ext, flags);
>  	}
>  	if (SET_WITH_SKBINFO(set))
>  		ip_set_get_skbinfo(ext_skbinfo(data, set),
> -- 
> 2.25.0
> 
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary