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@xxxxxxxxxx>
Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
---
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
