Hi József,
On Tue, 3 Mar 2020 10:36:53 +0100 (CET)
Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> wrote:
> Hi Stefano,
>
> 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. :)
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
