Hi Stefano,
On Tue, 3 Mar 2020, Stefano Brivio wrote:
> On Tue, 3 Mar 2020 10:36:53 +0100 (CET)
> Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> wrote:
>
> > On Fri, 28 Feb 2020, Stefano Brivio wrote:
> >
> > > On Thu, 27 Feb 2020 21:37:10 +0100 (CET)
> > > Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> wrote:
> > >
> > > > On Tue, 25 Feb 2020, Stefano Brivio wrote:
> > > >
> > > > > On Tue, 25 Feb 2020 21:37:45 +0100 (CET)
> > > > > Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> 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. :)
The situation is far from ideal: the original mode (update counters
regardless of the outcome of the counter matches) worked for almost five
years. Then the 'Fix "don't update counters" mode...' patch changed it so
that the result of the counter matches was taken into account, for about
two years. I don't know how many user is expecting either the original or
the changed behaviour, but better not change it again. Also, the grammar
seems to be simpler this way :-).
> 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:
Could you write a manpage update to describe properly the features?
> > 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.
In my opinion the two flags are mutually exclusive, therefore I dropped
the checking in ip_set_update_counter(). IPSET_FLAG_SKIP_COUNTER_UPDATE is
taken into account in ip_set_match_extensions() now.
> > - ip_set_update_counter(counter, ext, flags);
> > +
> > + if (!(flags & (IPSET_FLAG_UPDATE_COUNTERS_FIRST|
>
> Nit: whitespace before |.
I'll fix it, thanks!
Best regards,
Jozsef
-
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
