Re: [PATCH nft] evaluate: allow to re-use existing metered set

[Pablo Neira Ayuso <pablo@netfilter.org>]

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
> 
>