* nft meter add behavior change post translate-to-sets change
@ 2025-01-21 14:00 Florian Westphal
2025-01-21 21:20 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2025-01-21 14:00 UTC (permalink / raw)
To: netfilter-devel
TL;DR: since v1.1 meters work slightly different
and re-add after flush won't work:
cat > repro.sh <<EOF
NFT=src/nft
ip netns add N
ip netns exec N $NFT add table filter
ip netns exec N $NFT add chain filter input '{ type filter hook input priority 0 ; }'
ip netns exec N $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop
ip netns exec N $NFT list meters
# This used to remove the anon set, but not anymore
ip netns exec N $NFT flush chain filter input
# This will now fail:
ip netns exec N $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop
ip netns del N
EOF
This is caused by:
b8f8ddff ("evaluate: translate meter into dynamic set")
Should the last rule in above example work or not?
If it should I will turn the above into a formal test case and will
work on a fix, from a quick glance it should be possible to
handle the collision if the existing set has matching key length.
Thanks,
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: nft meter add behavior change post translate-to-sets change 2025-01-21 14:00 nft meter add behavior change post translate-to-sets change Florian Westphal @ 2025-01-21 21:20 ` Pablo Neira Ayuso 2025-01-21 21:33 ` Florian Westphal 0 siblings, 1 reply; 5+ messages in thread From: Pablo Neira Ayuso @ 2025-01-21 21:20 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel Hi Florian, On Tue, Jan 21, 2025 at 03:00:11PM +0100, Florian Westphal wrote: > TL;DR: since v1.1 meters work slightly different > and re-add after flush won't work: > > cat > repro.sh <<EOF > NFT=src/nft > > ip netns add N > ip netns exec N $NFT add table filter > ip netns exec N $NFT add chain filter input '{ type filter hook input priority 0 ; }' > ip netns exec N $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop > > ip netns exec N $NFT list meters > > # This used to remove the anon set, but not anymore > ip netns exec N $NFT flush chain filter input > > # This will now fail: > ip netns exec N $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop Ah, I can see what is going on here. > ip netns del N > EOF > > This is caused by: > b8f8ddff ("evaluate: translate meter into dynamic set") > > Should the last rule in above example work or not? > If it should I will turn the above into a formal test case and will > work on a fix, from a quick glance it should be possible to > handle the collision if the existing set has matching key length. I think it should be possible to address this case by allowing the meter statement to pick up an existing 'http1' set with the same key, this requires to extend b8f8ddff to deal with this. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: nft meter add behavior change post translate-to-sets change 2025-01-21 21:20 ` Pablo Neira Ayuso @ 2025-01-21 21:33 ` Florian Westphal 2025-01-22 9:18 ` [PATCH nft] evaluate: allow to re-use existing metered set Florian Westphal 0 siblings, 1 reply; 5+ messages in thread From: Florian Westphal @ 2025-01-21 21:33 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > ip netns del N > > EOF > > > > This is caused by: > > b8f8ddff ("evaluate: translate meter into dynamic set") > > > > Should the last rule in above example work or not? > > If it should I will turn the above into a formal test case and will > > work on a fix, from a quick glance it should be possible to > > handle the collision if the existing set has matching key length. > > I think it should be possible to address this case by allowing the > meter statement to pick up an existing 'http1' set with the same key, > this requires to extend b8f8ddff to deal with this. I'll have a look. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH nft] evaluate: allow to re-use existing metered set 2025-01-21 21:33 ` Florian Westphal @ 2025-01-22 9:18 ` Florian Westphal 2025-01-28 21:06 ` Pablo Neira Ayuso 0 siblings, 1 reply; 5+ messages in thread From: Florian Westphal @ 2025-01-22 9:18 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal, Yi Chen Blamed commit translates old meter syntax (which used to allocate an anonymous set) to dynamic sets. A side effect of this is that re-adding a meter rule after chain was flushed results in an error, unlike anonymous sets named sets are not impacted by the flush. Refine this: if a set of the same name exists and is compatible, then re-use it instead of returning an error. Also pick up the reproducer kindly provided by the reporter and place it in the shell test directory. Fixes: b8f8ddfff733 ("evaluate: translate meter into dynamic set") Reported-by: Yi Chen <yiche@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de> --- src/evaluate.c | 46 ++++++-- .../sets/dumps/meter_set_reuse.json-nft | 105 ++++++++++++++++++ .../testcases/sets/dumps/meter_set_reuse.nft | 11 ++ tests/shell/testcases/sets/meter_set_reuse | 20 ++++ 4 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft create mode 100644 tests/shell/testcases/sets/dumps/meter_set_reuse.nft create mode 100755 tests/shell/testcases/sets/meter_set_reuse diff --git a/src/evaluate.c b/src/evaluate.c index 919ef90707d9..50443df14df4 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -3356,7 +3356,7 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt) static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) { - struct expr *key, *set, *setref; + struct expr *key, *setref; struct set *existing_set; struct table *table; @@ -3367,7 +3367,9 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) return table_not_found(ctx); existing_set = set_cache_find(table, stmt->meter.name); - if (existing_set) + if (existing_set && + (!set_is_meter_compat(existing_set->flags) || + set_is_map(existing_set->flags))) return cmd_error(ctx, &stmt->location, "%s; meter '%s' overlaps an existing %s '%s' in family %s", strerror(EEXIST), @@ -3388,17 +3390,43 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) /* Declare an empty set */ key = stmt->meter.key; - set = set_expr_alloc(&key->location, NULL); - set->set_flags |= NFT_SET_EVAL; - if (key->timeout) - set->set_flags |= NFT_SET_TIMEOUT; + if (existing_set) { + if ((existing_set->flags & NFT_SET_TIMEOUT) && !key->timeout) + return expr_error(ctx->msgs, stmt->meter.key, + "existing set '%s' has timeout flag", + stmt->meter.name); + + if ((existing_set->flags & NFT_SET_TIMEOUT) == 0 && key->timeout) + return expr_error(ctx->msgs, stmt->meter.key, + "existing set '%s' lacks timeout flag", + stmt->meter.name); + + if (stmt->meter.size > 0 && existing_set->desc.size != stmt->meter.size) + return expr_error(ctx->msgs, stmt->meter.key, + "existing set '%s' has size %u, meter has %u", + stmt->meter.name, existing_set->desc.size, + stmt->meter.size); + } + + if (existing_set) { + setref = set_ref_expr_alloc(&key->location, existing_set); + } else { + struct expr *set; + + set = set_expr_alloc(&key->location, existing_set); + if (key->timeout) + set->set_flags |= NFT_SET_TIMEOUT; + + set->set_flags |= NFT_SET_EVAL; + setref = implicit_set_declaration(ctx, stmt->meter.name, + expr_get(key), NULL, set, 0); + if (setref) + setref->set->desc.size = stmt->meter.size; + } - setref = implicit_set_declaration(ctx, stmt->meter.name, - expr_get(key), NULL, set, 0); if (!setref) return -1; - setref->set->desc.size = stmt->meter.size; stmt->meter.set = setref; if (stmt_evaluate(ctx, stmt->meter.stmt) < 0) diff --git a/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft b/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft new file mode 100644 index 000000000000..ab4ac06184d0 --- /dev/null +++ b/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft @@ -0,0 +1,105 @@ +{ + "nftables": [ + { + "metainfo": { + "version": "VERSION", + "release_name": "RELEASE_NAME", + "json_schema_version": 1 + } + }, + { + "table": { + "family": "ip", + "name": "filter", + "handle": 0 + } + }, + { + "chain": { + "family": "ip", + "table": "filter", + "name": "input", + "handle": 0 + } + }, + { + "set": { + "family": "ip", + "name": "http1", + "table": "filter", + "type": [ + "inet_service", + "ipv4_addr" + ], + "handle": 0, + "size": 65535, + "flags": [ + "dynamic" + ] + } + }, + { + "rule": { + "family": "ip", + "table": "filter", + "chain": "input", + "handle": 0, + "expr": [ + { + "match": { + "op": "==", + "left": { + "payload": { + "protocol": "tcp", + "field": "dport" + } + }, + "right": 80 + } + }, + { + "set": { + "op": "add", + "elem": { + "concat": [ + { + "payload": { + "protocol": "tcp", + "field": "dport" + } + }, + { + "payload": { + "protocol": "ip", + "field": "saddr" + } + } + ] + }, + "set": "@http1", + "stmt": [ + { + "limit": { + "rate": 200, + "burst": 5, + "per": "second", + "inv": true + } + } + ] + } + }, + { + "counter": { + "packets": 0, + "bytes": 0 + } + }, + { + "drop": null + } + ] + } + } + ] +} diff --git a/tests/shell/testcases/sets/dumps/meter_set_reuse.nft b/tests/shell/testcases/sets/dumps/meter_set_reuse.nft new file mode 100644 index 000000000000..f911acaffb85 --- /dev/null +++ b/tests/shell/testcases/sets/dumps/meter_set_reuse.nft @@ -0,0 +1,11 @@ +table ip filter { + set http1 { + type inet_service . ipv4_addr + size 65535 + flags dynamic + } + + chain input { + tcp dport 80 add @http1 { tcp dport . ip saddr limit rate over 200/second burst 5 packets } counter packets 0 bytes 0 drop + } +} diff --git a/tests/shell/testcases/sets/meter_set_reuse b/tests/shell/testcases/sets/meter_set_reuse new file mode 100755 index 000000000000..94eccc1a7b82 --- /dev/null +++ b/tests/shell/testcases/sets/meter_set_reuse @@ -0,0 +1,20 @@ +#!/bin/bash + +set -e + +addrule() +{ + $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop +} + +$NFT add table filter +$NFT add chain filter input +addrule + +$NFT list meters + +# This used to remove the anon set, but not anymore +$NFT flush chain filter input + +# This re-add should work. +addrule -- 2.48.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nft] evaluate: allow to re-use existing metered set 2025-01-22 9:18 ` [PATCH nft] evaluate: allow to re-use existing metered set Florian Westphal @ 2025-01-28 21:06 ` Pablo Neira Ayuso 0 siblings, 0 replies; 5+ messages in thread From: Pablo Neira Ayuso @ 2025-01-28 21:06 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel, Yi Chen Hi Florian, LGTM, just a comestic comment, see below On Wed, Jan 22, 2025 at 10:18:04AM +0100, Florian Westphal wrote: > Blamed commit translates old meter syntax (which used to allocate an > anonymous set) to dynamic sets. > > A side effect of this is that re-adding a meter rule after chain was > flushed results in an error, unlike anonymous sets named sets are not > impacted by the flush. > > Refine this: if a set of the same name exists and is compatible, then > re-use it instead of returning an error. > > Also pick up the reproducer kindly provided by the reporter and place it > in the shell test directory. > > Fixes: b8f8ddfff733 ("evaluate: translate meter into dynamic set") > Reported-by: Yi Chen <yiche@redhat.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > src/evaluate.c | 46 ++++++-- > .../sets/dumps/meter_set_reuse.json-nft | 105 ++++++++++++++++++ > .../testcases/sets/dumps/meter_set_reuse.nft | 11 ++ > tests/shell/testcases/sets/meter_set_reuse | 20 ++++ > 4 files changed, 173 insertions(+), 9 deletions(-) > create mode 100644 tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft > create mode 100644 tests/shell/testcases/sets/dumps/meter_set_reuse.nft > create mode 100755 tests/shell/testcases/sets/meter_set_reuse > > diff --git a/src/evaluate.c b/src/evaluate.c > index 919ef90707d9..50443df14df4 100644 > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -3356,7 +3356,7 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt) > > static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) > { > - struct expr *key, *set, *setref; > + struct expr *key, *setref; > struct set *existing_set; > struct table *table; > > @@ -3367,7 +3367,9 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) > return table_not_found(ctx); > > existing_set = set_cache_find(table, stmt->meter.name); > - if (existing_set) > + if (existing_set && > + (!set_is_meter_compat(existing_set->flags) || > + set_is_map(existing_set->flags))) > return cmd_error(ctx, &stmt->location, > "%s; meter '%s' overlaps an existing %s '%s' in family %s", > strerror(EEXIST), > @@ -3388,17 +3390,43 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt) > > /* Declare an empty set */ > key = stmt->meter.key; > - set = set_expr_alloc(&key->location, NULL); > - set->set_flags |= NFT_SET_EVAL; > - if (key->timeout) > - set->set_flags |= NFT_SET_TIMEOUT; > + if (existing_set) { > + if ((existing_set->flags & NFT_SET_TIMEOUT) && !key->timeout) > + return expr_error(ctx->msgs, stmt->meter.key, > + "existing set '%s' has timeout flag", > + stmt->meter.name); > + > + if ((existing_set->flags & NFT_SET_TIMEOUT) == 0 && key->timeout) > + return expr_error(ctx->msgs, stmt->meter.key, > + "existing set '%s' lacks timeout flag", > + stmt->meter.name); > + > + if (stmt->meter.size > 0 && existing_set->desc.size != stmt->meter.size) > + return expr_error(ctx->msgs, stmt->meter.key, > + "existing set '%s' has size %u, meter has %u", > + stmt->meter.name, existing_set->desc.size, > + stmt->meter.size); > + } ^ remove this.. > + > + if (existing_set) { ^^^^^^^^^^^^^^^^^^^ and remove this too, then: > + setref = set_ref_expr_alloc(&key->location, existing_set); just stays on the existing branch. no need to send v2, just amend and push if you are happy with this. > + } else { > + struct expr *set; > + > + set = set_expr_alloc(&key->location, existing_set); > + if (key->timeout) > + set->set_flags |= NFT_SET_TIMEOUT; > + > + set->set_flags |= NFT_SET_EVAL; > + setref = implicit_set_declaration(ctx, stmt->meter.name, > + expr_get(key), NULL, set, 0); > + if (setref) > + setref->set->desc.size = stmt->meter.size; > + } > > - setref = implicit_set_declaration(ctx, stmt->meter.name, > - expr_get(key), NULL, set, 0); > if (!setref) > return -1; > > - setref->set->desc.size = stmt->meter.size; > stmt->meter.set = setref; > > if (stmt_evaluate(ctx, stmt->meter.stmt) < 0) > diff --git a/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft b/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft > new file mode 100644 > index 000000000000..ab4ac06184d0 > --- /dev/null > +++ b/tests/shell/testcases/sets/dumps/meter_set_reuse.json-nft > @@ -0,0 +1,105 @@ > +{ > + "nftables": [ > + { > + "metainfo": { > + "version": "VERSION", > + "release_name": "RELEASE_NAME", > + "json_schema_version": 1 > + } > + }, > + { > + "table": { > + "family": "ip", > + "name": "filter", > + "handle": 0 > + } > + }, > + { > + "chain": { > + "family": "ip", > + "table": "filter", > + "name": "input", > + "handle": 0 > + } > + }, > + { > + "set": { > + "family": "ip", > + "name": "http1", > + "table": "filter", > + "type": [ > + "inet_service", > + "ipv4_addr" > + ], > + "handle": 0, > + "size": 65535, > + "flags": [ > + "dynamic" > + ] > + } > + }, > + { > + "rule": { > + "family": "ip", > + "table": "filter", > + "chain": "input", > + "handle": 0, > + "expr": [ > + { > + "match": { > + "op": "==", > + "left": { > + "payload": { > + "protocol": "tcp", > + "field": "dport" > + } > + }, > + "right": 80 > + } > + }, > + { > + "set": { > + "op": "add", > + "elem": { > + "concat": [ > + { > + "payload": { > + "protocol": "tcp", > + "field": "dport" > + } > + }, > + { > + "payload": { > + "protocol": "ip", > + "field": "saddr" > + } > + } > + ] > + }, > + "set": "@http1", > + "stmt": [ > + { > + "limit": { > + "rate": 200, > + "burst": 5, > + "per": "second", > + "inv": true > + } > + } > + ] > + } > + }, > + { > + "counter": { > + "packets": 0, > + "bytes": 0 > + } > + }, > + { > + "drop": null > + } > + ] > + } > + } > + ] > +} > diff --git a/tests/shell/testcases/sets/dumps/meter_set_reuse.nft b/tests/shell/testcases/sets/dumps/meter_set_reuse.nft > new file mode 100644 > index 000000000000..f911acaffb85 > --- /dev/null > +++ b/tests/shell/testcases/sets/dumps/meter_set_reuse.nft > @@ -0,0 +1,11 @@ > +table ip filter { > + set http1 { > + type inet_service . ipv4_addr > + size 65535 > + flags dynamic > + } > + > + chain input { > + tcp dport 80 add @http1 { tcp dport . ip saddr limit rate over 200/second burst 5 packets } counter packets 0 bytes 0 drop > + } > +} > diff --git a/tests/shell/testcases/sets/meter_set_reuse b/tests/shell/testcases/sets/meter_set_reuse > new file mode 100755 > index 000000000000..94eccc1a7b82 > --- /dev/null > +++ b/tests/shell/testcases/sets/meter_set_reuse > @@ -0,0 +1,20 @@ > +#!/bin/bash > + > +set -e > + > +addrule() > +{ > + $NFT add rule ip filter input tcp dport 80 meter http1 { tcp dport . ip saddr limit rate over 200/second } counter drop > +} > + > +$NFT add table filter > +$NFT add chain filter input > +addrule > + > +$NFT list meters > + > +# This used to remove the anon set, but not anymore > +$NFT flush chain filter input > + > +# This re-add should work. > +addrule > -- > 2.48.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-28 21:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-21 14:00 nft meter add behavior change post translate-to-sets change Florian Westphal 2025-01-21 21:20 ` Pablo Neira Ayuso 2025-01-21 21:33 ` Florian Westphal 2025-01-22 9:18 ` [PATCH nft] evaluate: allow to re-use existing metered set Florian Westphal 2025-01-28 21:06 ` Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.