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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 25 Feb 2020 10:16:40 +0100 (CET)
Jozsef Kadlecsik <kadlec@netfilter.org> wrote:

> Hi Stefano,
> 
> On Tue, 25 Feb 2020, Stefano Brivio wrote:
> 
> > > > 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'".  
> > 
> > Note that this behaviour was modified two years ago: earlier, this was 
> > not the case (and by the way this is how we found out, as it broke a 
> > user setup).  
> 
> That's  really bad. Still, I think it was a bug earlier which was 
> then fixed. 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".

Now, I see the conceptual problem about matching: if the rule isn't
matching, and counters count matched packets, counters shouldn't
increase. But still, I think there are a number of facts to
be considered:

- the man page says (and has said for a number of years):

	If the packet is matched an element in the set, match only if
	the byte counter of the element is greater than the given value
	as well.

  which actually makes the problem undecidable: matching depends on
  matching itself. Trying some "common sense" interpretation, I would
  read this as:

	If the packet matches an *element* in the set, this *rule* will
	match only if the byte counter of the element is greater than
	the given value.

  that is, by separating the meaning of "element matching" from "rule
  matching", this starts making sense.

- I spent the past two hours trying to think of an actual case that was
  affected by 4750005a85f7, *other than the "main" bug it fixes*, that
  is, "! --update-counters" was ignored altogether, and I couldn't.

  Even if we had a --bytes-lt option, it would be counter-intuitive,
  because the counter would be updated until bytes are less than the
  threshold, and then the rule would stop matching, meaning that the
  user most probably thinks:

	"Drop matching packets as long as less than 800 bytes are sent"

  and what happens is:

	"Count and drop matching packets until 800 bytes are sent, then
	stop dropping and counting them"

  The only "functional" case I can think of is something like
  --bytes-lt 800 -j ACCEPT. User probably thinks:

	"Don't let more than 800 bytes go through"

  and what happens is:

	"Let up to 800 bytes, or 799 bytes plus one packet, go through,
	counting the bytes in packets that were let through"

  which isn't much different from the expectation.

- and then,

> > Other than this, I'm a bit confused. How could --packets-gt and
> > --bytes-gt be used, if counters don't increase as long as the rule
> > doesn't match?  
> 
> 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.

> > > What's really missing is a decrement-counters flag: that way one could 
> > > store different "quotas" for the elements in a set.  
> > 
> > I see, that would work as well.  
> 
> 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.

> - ! --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.

-- 
Stefano