On Tue, 25 Feb 2020 10:16:40 +0100 (CET) Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> 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
