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

[Stefano Brivio <sbrivio@redhat.com>]

Hi József,

On Tue, 3 Mar 2020 10:36:53 +0100 (CET)
Jozsef Kadlecsik <kadlec@netfilter.org> wrote:

> Hi Stefano,
> 
> On Fri, 28 Feb 2020, Stefano Brivio wrote:
> 
> > On Thu, 27 Feb 2020 21:37:10 +0100 (CET)
> > Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> >   
> > > On Tue, 25 Feb 2020, Stefano Brivio wrote:
> > >   
> > > > On Tue, 25 Feb 2020 21:37:45 +0100 (CET)
> > > > Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> > > >     
> > > > > On Tue, 25 Feb 2020, Stefano Brivio wrote:
> > > > >     
> > > > > > > The logic could be changed in the user rules from
> > > > > > > 
> > > > > > > iptables -I INPUT -m set --match-set c src --bytes-gt 800 -j DROP
> > > > > > > 
> > > > > > > to
> > > > > > > 
> > > > > > > iptables -I INPUT -m set --match-set c src --bytes-lt 800 -j ACCEPT
> > > > > > > [ otherwise DROP ]
> > > > > > > 
> > > > > > > but of course it might be not so simple, depending on how the rules are 
> > > > > > > built up.      
> > > > > > 
> > > > > > Yes, it would work, unless the user actually wants to check with the
> > > > > > same counter how many bytes are sent "in excess".      
> > > > > 
> > > > > You mean the counters are still updated whenever the element is matched in 
> > > > > the set and then one could check how many bytes were sent over the 
> > > > > threshold just by listing the set elements.    
> > > > 
> > > > Yes, exactly -- note that it was possible (and, I think, used) before.    
> > > 
> > > I'm still not really convinced about such a feature. Why is it useful to 
> > > know how many bytes would be sent over the "limit"?  
> > 
> > This is useful in case one wants different treatments for packets
> > according to a number of thresholds in different rules. For example,
> > 
> >     iptables -I INPUT -m set --match-set c src --bytes-lt 100 -j noise
> >     iptables -I noise -m set --match-set c src --bytes-lt 20000 -j download
> > 
> > and you want to log packets from chains 'noise' and 'download' with
> > different prefixes.  
> 
> What do you think about this patch?

Thanks, I think it gives a way to avoid the issue.

I'm still not convinced that keeping this disabled by default is the
best way to go (mostly because we had a kernel change affecting
semantics that were exported to userspace for a long time), but if
there's a need for the opposite of this option, introducing it as a
negation becomes linguistically awkward. :)

And anyway, it's surely better than not having this possibility at all.

Let me know if you want me to review (or try to draft) man page
changes. Just a few comments inline:

> diff --git a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> index 7545af4..6881329 100644
> --- a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> +++ b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> @@ -186,6 +186,9 @@ enum ipset_cmd_flags {
>  	IPSET_FLAG_MAP_SKBPRIO = (1 << IPSET_FLAG_BIT_MAP_SKBPRIO),
>  	IPSET_FLAG_BIT_MAP_SKBQUEUE = 10,
>  	IPSET_FLAG_MAP_SKBQUEUE = (1 << IPSET_FLAG_BIT_MAP_SKBQUEUE),
> +	IPSET_FLAG_BIT_UPDATE_COUNTERS_FIRST = 11,
> +	IPSET_FLAG_UPDATE_COUNTERS_FIRST =
> +		(1 << IPSET_FLAG_BIT_UPDATE_COUNTERS_FIRST),
>  	IPSET_FLAG_CMD_MAX = 15,
>  };
>  
> diff --git a/kernel/net/netfilter/ipset/ip_set_core.c b/kernel/net/netfilter/ipset/ip_set_core.c
> index 1df6536..423d0de 100644
> --- a/kernel/net/netfilter/ipset/ip_set_core.c
> +++ b/kernel/net/netfilter/ipset/ip_set_core.c
> @@ -622,10 +622,9 @@ ip_set_add_packets(u64 packets, struct ip_set_counter *counter)
>  
>  static void
>  ip_set_update_counter(struct ip_set_counter *counter,
> -		      const struct ip_set_ext *ext, u32 flags)
> +		      const struct ip_set_ext *ext)
>  {
> -	if (ext->packets != ULLONG_MAX &&
> -	    !(flags & IPSET_FLAG_SKIP_COUNTER_UPDATE)) {
> +	if (ext->packets != ULLONG_MAX) {

This means that UPDATE_COUNTERS_FIRST wins over SKIP_COUNTER_UPDATE. Is
that intended? Intuitively, I would say that "skip" is more imperative
than "do it *first*". Anyway, I guess they will be mutually exclusive.

>  		ip_set_add_bytes(ext->bytes, counter);
>  		ip_set_add_packets(ext->packets, counter);
>  	}
> @@ -649,13 +648,19 @@ 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);
>  
> +		if (flags & IPSET_FLAG_UPDATE_COUNTERS_FIRST)
> +			ip_set_update_counter(counter, ext);
> +
>  		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 (!(flags & (IPSET_FLAG_UPDATE_COUNTERS_FIRST|

Nit: whitespace before |.

> +			       IPSET_FLAG_SKIP_COUNTER_UPDATE)))
> +			ip_set_update_counter(counter, ext);
>  	}
>  	if (SET_WITH_SKBINFO(set))
>  		ip_set_get_skbinfo(ext_skbinfo(data, set),
> 
> Then the rules above would look like
> 
> ... -m set ... --update-counters-first --bytes-lt 100 -j noise
> ... -m set ... --update-counters-first --bytes-ge 100 -j download

Sorry for the typo in my previous example, I really meant:

  iptables -I INPUT -m set --match-set c src --bytes-gt 100 -j noise
  iptables -I noise -m set --match-set c src --bytes-gt 20000 -j download

that is, "noise" is more than "a single connection attempt", and
"download" is even more. But I think your example is equivalent for
this purpose.

> > > > What I meant is really the case where "--update-counters" (or 
> > > > "--force-update-counters") and "! --update-counters" are both 
> > > > absent: I don't see any particular advantage in the current 
> > > > behaviour for that case.  
> > > 
> > > The counters are used just for statistical purposes: reflect the 
> > > packets/bytes which were let through, i.e. matched the whole "rule". 
> > > In that case updating the counters before the counter value matching 
> > > is evaluated gives false results.  
> > 
> > Well, but for that, iptables/x_tables counters are available and (as far 
> > as I know) typically used.  
> 
> With "rules" I meant at ipset level (match element + packet/byte counters 
> as specified), i.e. counters for statistical purposes per set elements 
> level.

Ah, I see now, thanks for explaining.

-- 
Stefano