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

[Stefano Brivio <sbrivio@redhat.com>]

Hi Jozsef,

On Thu, 27 Feb 2020 21:37:10 +0100 (CET)
Jozsef Kadlecsik <kadlec@netfilter.org> wrote:

> Hi Stefano,
> 
> 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.

> Also, there's no protection against overflow in the counters. I know
> firewalls with ipset, 10gb interfaces and long uptimes, so it's not
> completely a theoretical issue.

With 10GbE, 64-bit counters can cover more than:
  2 ^ 64 / (10 * 1000 * 1000 * 1000 / 8) = 14757395259 seconds
that is,
  14757395259 / (60 * 60 * 24) = 170803 days
that is,
  170803 / 365 = 468 years

...is that a real issue?

> > > > > I almost added to my previous mail that the 'ge' and 'gt' matches 
> > > > > are not really useful at the moment...  
> > > > 
> > > > ...yes, I can't think of any other use for those either.    
> > > 
> > > Those could really be useful if the counters could be decremented. 
> > > Otherwise I think the counter matching in the sets is not as useful as 
> > > it seems to be.  
> > 
> > Still, if counters are updated with just matching element, but not 
> > necessarily matching rule, they should be as useful as in the hypothesis 
> > of introducing a "decrementing" feature -- one just needs to adjust the 
> > rule logic to that.  
> 
> That's true.
> 
> > > > > The other possibility is to force counter update. I.e. instead of
> > > > > 
> > > > > iptables -I INPUT -m set --match-set c src --bytes-gt 800 -j DROP
> > > > > 
> > > > > something like
> > > > > 
> > > > > iptables -I INPUT -m set --match-set c src --update-counters \
> > > > > 	--bytes-gt 800 -j DROP
> > > > > 
> > > > > but that also requires some internal changes to store a new flag, because 
> > > > > at the moment only "! --update-counters" is supported. So there'd be then 
> > > > > a fine-grained control over how the counters are updated:
> > > > > 
> > > > > - no --update-counters flag: update counters only if the whole rule 
> > > > >   matches, including the counter matches
> > > > > - --update-counters flag: update counters if counter matching is false    
> > > > 
> > > > ...this should probably be "in any case", also if it's true.    
> > > 
> > > Yes, but now I don't really like the name itself: --force-update-counters
> > > or something like that would be more clear.
> > >   
> > > > > - ! --update-counters flag: don't update counters    
> > > > 
> > > > I think that would fix the issue as well, I'm just struggling to find a
> > > > sensible use case for the "no --update-counters" case -- especially one
> > > > where there would be a substantial issue with the change I proposed.    
> > > 
> > > The no update counter flag was introduced to handle when one needs to 
> > > match in the same set multiple times, i.e. there are multiple rules with 
> > > the same set. Like you need to match in the raw/mangle/filter tables as 
> > > well. Unfortunately I can't recall the usercase.  
> > 
> > Okay, but what you're describing is the "! --update-counters" option. 
> > That works, didn't work before 4750005a85f7, but would still work with 
> > this change.
> > 
> > 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.

-- 
Stefano