[nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Phil Sutter]

This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
flag if dynamic updates requested"), implementing the alternative
mentioned in the comment it added.

Reason is the inconsistent behaviour when applying the same ruleset
twice: In the first call, the set lacking 'dynamic' flag does not exist
and is therefore added to the cache. Consequently, both the 'add set'
command and the set statement point at the same set object. In the
second call, a set with same name exists already, so the object created
for 'add set' command is not added to cache and consequently not updated
with the missing flag. The kernel thus rejects the NEWSET request as the
existing set differs from the new one.

Fixes: 8d443adfcc8c1 ("evaluate: attempt to set_eval flag if dynamic updates requested")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
*Note*: This is the best I could come up with. If you see a better way
to fix this (i.e., fix up cmd->set from stmt_evaluate_set()), please
speak up. :)
---
 src/evaluate.c                                | 13 ++------
 .../testcases/sets/dumps/dynset_missing.nft   | 12 -------
 tests/shell/testcases/sets/dynset_missing     | 32 -------------------
 3 files changed, 3 insertions(+), 54 deletions(-)
 delete mode 100644 tests/shell/testcases/sets/dumps/dynset_missing.nft
 delete mode 100755 tests/shell/testcases/sets/dynset_missing

diff --git a/src/evaluate.c b/src/evaluate.c
index fe15d7ace5dde..9fb768cbd4c9f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4139,7 +4139,6 @@ static int stmt_evaluate_log(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_set(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	struct set *this_set;
 	struct stmt *this;
 
 	expr_set_context(&ctx->ectx, NULL, 0);
@@ -4148,6 +4147,9 @@ static int stmt_evaluate_set(struct eval_ctx *ctx, struct stmt *stmt)
 	if (stmt->set.set->etype != EXPR_SET_REF)
 		return expr_error(ctx->msgs, stmt->set.set,
 				  "Expression does not refer to a set");
+	if (!(stmt->set.set->set->flags & NFT_SET_EVAL))
+		return expr_error(ctx->msgs, stmt->set.set,
+				  "Referenced set lacks \"dynamic\" flag");
 
 	if (stmt_evaluate_key(ctx, stmt,
 			      stmt->set.set->set->key->dtype,
@@ -4169,15 +4171,6 @@ static int stmt_evaluate_set(struct eval_ctx *ctx, struct stmt *stmt)
 					  "statement must be stateful");
 	}
 
-	this_set = stmt->set.set->set;
-
-	/* Make sure EVAL flag is set on set definition so that kernel
-	 * picks a set that allows updates from the packet path.
-	 *
-	 * Alternatively we could error out in case 'flags dynamic' was
-	 * not given, but we can repair this here.
-	 */
-	this_set->flags |= NFT_SET_EVAL;
 	return 0;
 }
 
diff --git a/tests/shell/testcases/sets/dumps/dynset_missing.nft b/tests/shell/testcases/sets/dumps/dynset_missing.nft
deleted file mode 100644
index 6c8ed323bdc94..0000000000000
--- a/tests/shell/testcases/sets/dumps/dynset_missing.nft
+++ /dev/null
@@ -1,12 +0,0 @@
-table ip test {
-	set dlist {
-		type ipv4_addr
-		size 65535
-		flags dynamic
-	}
-
-	chain output {
-		type filter hook output priority filter; policy accept;
-		udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
-	}
-}
diff --git a/tests/shell/testcases/sets/dynset_missing b/tests/shell/testcases/sets/dynset_missing
deleted file mode 100755
index fdf5f49edb9c6..0000000000000
--- a/tests/shell/testcases/sets/dynset_missing
+++ /dev/null
@@ -1,32 +0,0 @@
-#!/bin/bash
-
-set -e
-
-$NFT -f /dev/stdin <<EOF
-table ip test {
-	chain output { type filter hook output priority 0;
-	}
-}
-EOF
-
-# misses 'flags dynamic'
-$NFT 'add set ip test dlist {type ipv4_addr; }'
-
-# picks rhash backend because 'size' was also missing.
-$NFT 'add rule ip test output udp dport 1234 update @dlist { ip daddr } counter'
-
-tmpfile=$(mktemp)
-
-trap "rm -rf $tmpfile" EXIT
-
-# kernel has forced an 64k upper size, i.e. this restore file
-# has 'size 65536' but no 'flags dynamic'.
-$NFT list ruleset > $tmpfile
-
-# this restore works, because set is still the rhash backend.
-$NFT -f $tmpfile # success
-$NFT flush ruleset
-
-# fails without commit 'attempt to set_eval flag if dynamic updates requested',
-# because set in $tmpfile has 'size x' but no 'flags dynamic'.
-$NFT -f $tmpfile
-- 
2.40.0

Re: [nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Pablo Neira Ayuso]

On Wed, May 03, 2023 at 12:50:22PM +0200, Phil Sutter wrote:
> This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
> flag if dynamic updates requested"), implementing the alternative
> mentioned in the comment it added.
> 
> Reason is the inconsistent behaviour when applying the same ruleset
> twice: In the first call, the set lacking 'dynamic' flag does not exist
> and is therefore added to the cache. Consequently, both the 'add set'
> command and the set statement point at the same set object. In the
> second call, a set with same name exists already, so the object created
> for 'add set' command is not added to cache and consequently not updated
> with the missing flag. The kernel thus rejects the NEWSET request as the
> existing set differs from the new one.

# cat test.nft
flush ruleset

table ip test {
        set dlist {
                type ipv4_addr
                size 65535
        }

        chain output {
                type filter hook output priority filter; policy accept;
                udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
        }
}
# nft -f test.nft
# nft -f test.nft
# nft list ruleset
table ip test {
        set dlist {
                type ipv4_addr
                size 65535
                flags dynamic
        }

        chain output {
                type filter hook output priority filter; policy accept;
                udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
        }
}

What are the steps to reproduce this? Did I misunderstand?

Re: [nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Phil Sutter]

Hi Pablo,

On Wed, May 10, 2023 at 12:54:28PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 03, 2023 at 12:50:22PM +0200, Phil Sutter wrote:
> > This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
> > flag if dynamic updates requested"), implementing the alternative
> > mentioned in the comment it added.
> > 
> > Reason is the inconsistent behaviour when applying the same ruleset
> > twice: In the first call, the set lacking 'dynamic' flag does not exist
> > and is therefore added to the cache. Consequently, both the 'add set'
> > command and the set statement point at the same set object. In the
> > second call, a set with same name exists already, so the object created
> > for 'add set' command is not added to cache and consequently not updated
> > with the missing flag. The kernel thus rejects the NEWSET request as the
> > existing set differs from the new one.
> 
> # cat test.nft
> flush ruleset

Just remove this 'flush ruleset' call, then it should trigger.

Cheers, Phil

Re: [nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Pablo Neira Ayuso]

On Wed, May 10, 2023 at 01:48:01PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, May 10, 2023 at 12:54:28PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, May 03, 2023 at 12:50:22PM +0200, Phil Sutter wrote:
> > > This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
> > > flag if dynamic updates requested"), implementing the alternative
> > > mentioned in the comment it added.
> > > 
> > > Reason is the inconsistent behaviour when applying the same ruleset
> > > twice: In the first call, the set lacking 'dynamic' flag does not exist
> > > and is therefore added to the cache. Consequently, both the 'add set'
> > > command and the set statement point at the same set object. In the
> > > second call, a set with same name exists already, so the object created
> > > for 'add set' command is not added to cache and consequently not updated
> > > with the missing flag. The kernel thus rejects the NEWSET request as the
> > > existing set differs from the new one.
> > 
> > # cat test.nft
> > flush ruleset
> 
> Just remove this 'flush ruleset' call, then it should trigger.

I cannot reproduce it yet :(

# cat test.nft
table ip test {
        set dlist {
                type ipv4_addr
                size 65535
        }

        chain output {
                type filter hook output priority filter; policy accept;
                udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
        }
}
# nft -f test.nft
# nft -f test.nft
# nft list ruleset
table ip test {
        set dlist {
                type ipv4_addr
                size 65535
                flags dynamic
        }

        chain output {
                type filter hook output priority filter; policy accept;
                udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
                udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
        }
}

Maybe I need a specific kernel? I am trying with latest.

Re: [nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Phil Sutter]

Hi Pablo,

On Thu, May 18, 2023 at 11:41:10AM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 10, 2023 at 01:48:01PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, May 10, 2023 at 12:54:28PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, May 03, 2023 at 12:50:22PM +0200, Phil Sutter wrote:
> > > > This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
> > > > flag if dynamic updates requested"), implementing the alternative
> > > > mentioned in the comment it added.
> > > > 
> > > > Reason is the inconsistent behaviour when applying the same ruleset
> > > > twice: In the first call, the set lacking 'dynamic' flag does not exist
> > > > and is therefore added to the cache. Consequently, both the 'add set'
> > > > command and the set statement point at the same set object. In the
> > > > second call, a set with same name exists already, so the object created
> > > > for 'add set' command is not added to cache and consequently not updated
> > > > with the missing flag. The kernel thus rejects the NEWSET request as the
> > > > existing set differs from the new one.
> > > 
> > > # cat test.nft
> > > flush ruleset
> > 
> > Just remove this 'flush ruleset' call, then it should trigger.
> 
> I cannot reproduce it yet :(

This is very odd.

> # cat test.nft
> table ip test {
>         set dlist {
>                 type ipv4_addr
>                 size 65535
>         }
> 
>         chain output {
>                 type filter hook output priority filter; policy accept;
>                 udp dport 1234 update @dlist { ip daddr } counter packets 0 bytes 0
>         }
> }
> # nft -f test.nft
> # nft -f test.nft

On my side, this second call emits:

| /tmp/test.nft:2:6-10: Error: Could not process rule: File exists
|         set dlist {
|             ^^^^^

> Maybe I need a specific kernel? I am trying with latest.

IIUC, it is not kernel-related. I can reproduce with:

nftables @ d486c9e626405 ("datatype: add hint error handler")
libnftnl @ libnftnl-1.2.5
linux @ b50a8b0d57ab1 ("net: openvswitch: Use struct_size()")

Cheers, Phil

Re: [nft PATCH] evaluate: Reject set stmt refs to sets without dynamic flag

[Pablo Neira Ayuso]

On Thu, May 18, 2023 at 11:41:10AM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 10, 2023 at 01:48:01PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, May 10, 2023 at 12:54:28PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, May 03, 2023 at 12:50:22PM +0200, Phil Sutter wrote:
> > > > This is a revert of commit 8d443adfcc8c1 ("evaluate: attempt to set_eval
> > > > flag if dynamic updates requested"), implementing the alternative
> > > > mentioned in the comment it added.
> > > > 
> > > > Reason is the inconsistent behaviour when applying the same ruleset
> > > > twice: In the first call, the set lacking 'dynamic' flag does not exist
> > > > and is therefore added to the cache. Consequently, both the 'add set'
> > > > command and the set statement point at the same set object. In the
> > > > second call, a set with same name exists already, so the object created
> > > > for 'add set' command is not added to cache and consequently not updated
> > > > with the missing flag. The kernel thus rejects the NEWSET request as the
> > > > existing set differs from the new one.
> > > 
> > > # cat test.nft
> > > flush ruleset
> > 
> > Just remove this 'flush ruleset' call, then it should trigger.
> 
> I cannot reproduce it yet :(

I can reproduce it. For the record, I have applied this alternate fix:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230518125806.11100-1-pablo@netfilter.org/