[PATCH nft 1/5] cache: assert filter when calling nft_cache_evaluate()

[PATCH nft 1/5] cache: assert filter when calling nft_cache_evaluate()

[Pablo Neira Ayuso]

nft_cache_evaluate() always takes a non-null filter, remove superfluous
checks when calculating cache requirements via flags.

Note that filter is still option from netlink dump path, since this can
be called from error path to provide hints.

Fixes: 08725a9dc14c ("cache: filter out rules by chain")
Fixes: b3ed8fd8c9f3 ("cache: missing family in cache filtering")
Fixes: 635ee1cad8aa ("cache: filter out sets and maps that are not requested")
Fixes: 3f1d3912c3a6 ("cache: filter out tables that are not requested")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/cache.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 72f2972f0259..8cddabdb7b98 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -212,18 +212,17 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 {
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
-		if (filter)
-			filter->list.family = cmd->handle.family;
+		filter->list.family = cmd->handle.family;
 		if (!cmd->handle.table.name) {
 			flags |= NFT_CACHE_TABLE;
 			break;
-		} else if (filter) {
+		} else {
 			filter->list.table = cmd->handle.table.name;
 		}
 		flags |= NFT_CACHE_FULL;
 		break;
 	case CMD_OBJ_CHAIN:
-		if (filter && cmd->handle.chain.name) {
+		if (cmd->handle.chain.name) {
 			filter->list.family = cmd->handle.family;
 			filter->list.table = cmd->handle.table.name;
 			filter->list.chain = cmd->handle.chain.name;
@@ -236,7 +235,7 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 		break;
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
-		if (filter && cmd->handle.table.name && cmd->handle.set.name) {
+		if (cmd->handle.table.name && cmd->handle.set.name) {
 			filter->list.family = cmd->handle.family;
 			filter->list.table = cmd->handle.table.name;
 			filter->list.set = cmd->handle.set.name;
@@ -256,8 +255,7 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 			flags |= NFT_CACHE_SETELEM;
 		break;
 	case CMD_OBJ_FLOWTABLE:
-		if (filter &&
-		    cmd->handle.table.name &&
+		if (cmd->handle.table.name &&
 		    cmd->handle.flowtable.name) {
 			filter->list.family = cmd->handle.family;
 			filter->list.table = cmd->handle.table.name;
@@ -314,8 +312,6 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 static unsigned int evaluate_cache_reset(struct cmd *cmd, unsigned int flags,
 					 struct nft_cache_filter *filter)
 {
-	assert(filter);
-
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
 	case CMD_OBJ_CHAIN:
@@ -482,6 +478,8 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 	unsigned int flags, batch_flags = NFT_CACHE_EMPTY;
 	struct cmd *cmd;
 
+	assert(filter);
+
 	list_for_each_entry(cmd, cmds, list) {
 		if (nft_handle_validate(cmd, msgs) < 0)
 			return -1;
-- 
2.30.2

[PATCH nft 2/5] cache: clean up evaluate_cache_del()

[Pablo Neira Ayuso]

Move NFT_CACHE_TABLE flag to default case to disentangle this.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/cache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 8cddabdb7b98..bed98bb71655 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -77,6 +77,7 @@ static unsigned int evaluate_cache_del(struct cmd *cmd, unsigned int flags)
 			 NFT_CACHE_SETELEM_MAYBE;
 		break;
 	default:
+		flags = NFT_CACHE_TABLE;
 		break;
 	}
 
@@ -500,8 +501,6 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 			break;
 		case CMD_DELETE:
 		case CMD_DESTROY:
-			flags |= NFT_CACHE_TABLE;
-
 			flags = evaluate_cache_del(cmd, flags);
 			break;
 		case CMD_GET:
-- 
2.30.2

[PATCH nft 3/5] cache: remove full cache requirement when echo flag is set on

[Pablo Neira Ayuso]

The echo flags does not use the cache infrastructure yet, it relies on
the monitor cache which follows the netlink_echo_callback() path.

Fixes: 01e5c6f0ed03 ("src: add cache level flags")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/cache.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index bed98bb71655..fce71eed3452 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -493,8 +493,6 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 		case CMD_INSERT:
 		case CMD_CREATE:
 			flags = evaluate_cache_add(cmd, flags);
-			if (nft_output_echo(&nft->output))
-				flags |= NFT_CACHE_FULL;
 			break;
 		case CMD_REPLACE:
 			flags = NFT_CACHE_FULL;
-- 
2.30.2

[PATCH nft 4/5] cache: relax requirement for replace rule command

[Pablo Neira Ayuso]

No need for full cache, this command relies on the rule handle which is
not validated from userspace. Cache requirements are similar to those
of add/create/delete rule commands.

This speeds up incremental updates with large rulesets.

Extend tests/coverage for rule replacement.

Fixes: 01e5c6f0ed03 ("src: add cache level flags")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/cache.c                                   |  2 +-
 .../testcases/rule_management/0004replace_0   |  8 ++-
 .../dumps/0004replace_0.json-nft              | 49 ++++++++++++++++++-
 .../rule_management/dumps/0004replace_0.nft   | 11 ++++-
 4 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index fce71eed3452..db7dfd96081d 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -495,7 +495,7 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 			flags = evaluate_cache_add(cmd, flags);
 			break;
 		case CMD_REPLACE:
-			flags = NFT_CACHE_FULL;
+			flags = NFT_CACHE_TABLE | NFT_CACHE_SET;
 			break;
 		case CMD_DELETE:
 		case CMD_DESTROY:
diff --git a/tests/shell/testcases/rule_management/0004replace_0 b/tests/shell/testcases/rule_management/0004replace_0
index c3329af500d3..18dc4a9fe30b 100755
--- a/tests/shell/testcases/rule_management/0004replace_0
+++ b/tests/shell/testcases/rule_management/0004replace_0
@@ -6,5 +6,9 @@
 set -e
 $NFT add table t
 $NFT add chain t c
-$NFT add rule t c accept	# should have handle 2
-$NFT replace rule t c handle 2 drop
+$NFT 'add set t s1 { type ipv4_addr; }'
+$NFT 'add set t s2 { type ipv4_addr; flags interval; }'
+$NFT add rule t c accept        # should have handle 4
+$NFT replace rule t c handle 4 drop
+$NFT replace rule t c handle 4 ip saddr { 1.1.1.1, 2.2.2.2 }
+$NFT replace rule t c handle 4 ip saddr @s2 ip daddr { 3.3.3.3, 4.4.4.4 }
diff --git a/tests/shell/testcases/rule_management/dumps/0004replace_0.json-nft b/tests/shell/testcases/rule_management/dumps/0004replace_0.json-nft
index 5d0b7d066e83..767e80f14ff2 100644
--- a/tests/shell/testcases/rule_management/dumps/0004replace_0.json-nft
+++ b/tests/shell/testcases/rule_management/dumps/0004replace_0.json-nft
@@ -22,6 +22,27 @@
         "handle": 0
       }
     },
+    {
+      "set": {
+        "family": "ip",
+        "name": "s1",
+        "table": "t",
+        "type": "ipv4_addr",
+        "handle": 0
+      }
+    },
+    {
+      "set": {
+        "family": "ip",
+        "name": "s2",
+        "table": "t",
+        "type": "ipv4_addr",
+        "handle": 0,
+        "flags": [
+          "interval"
+        ]
+      }
+    },
     {
       "rule": {
         "family": "ip",
@@ -30,7 +51,33 @@
         "handle": 0,
         "expr": [
           {
-            "drop": null
+            "match": {
+              "op": "==",
+              "left": {
+                "payload": {
+                  "protocol": "ip",
+                  "field": "saddr"
+                }
+              },
+              "right": "@s2"
+            }
+          },
+          {
+            "match": {
+              "op": "==",
+              "left": {
+                "payload": {
+                  "protocol": "ip",
+                  "field": "daddr"
+                }
+              },
+              "right": {
+                "set": [
+                  "3.3.3.3",
+                  "4.4.4.4"
+                ]
+              }
+            }
           }
         ]
       }
diff --git a/tests/shell/testcases/rule_management/dumps/0004replace_0.nft b/tests/shell/testcases/rule_management/dumps/0004replace_0.nft
index e20952ef573e..803c0debb737 100644
--- a/tests/shell/testcases/rule_management/dumps/0004replace_0.nft
+++ b/tests/shell/testcases/rule_management/dumps/0004replace_0.nft
@@ -1,5 +1,14 @@
 table ip t {
+	set s1 {
+		type ipv4_addr
+	}
+
+	set s2 {
+		type ipv4_addr
+		flags interval
+	}
+
 	chain c {
-		drop
+		ip saddr @s2 ip daddr { 3.3.3.3, 4.4.4.4 }
 	}
 }
-- 
2.30.2

[PATCH nft 5/5] cache: position does not require full cache

[Pablo Neira Ayuso]

position refers to the rule handle, it has similar cache requirements as
replace rule command, relax cache requirements.

Commit e5382c0d08e3 ("src: Support intra-transaction rule references")
uses position.id for index support which requires a full cache, but
only in such case.

Fixes: 01e5c6f0ed03 ("src: add cache level flags")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/cache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index db7dfd96081d..3f1324a2f98b 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -58,8 +58,7 @@ static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
 		flags |= NFT_CACHE_TABLE |
 			 NFT_CACHE_SET;
 
-		if (cmd->handle.index.id ||
-		    cmd->handle.position.id)
+		if (cmd->handle.index.id)
 			flags |= NFT_CACHE_FULL | NFT_CACHE_UPDATE;
 		break;
 	default:
-- 
2.30.2

Re: [PATCH nft 1/5] cache: assert filter when calling nft_cache_evaluate()

[Eric Garver]

On Thu, Aug 29, 2024 at 01:31:49PM +0200, Pablo Neira Ayuso wrote:
> nft_cache_evaluate() always takes a non-null filter, remove superfluous
> checks when calculating cache requirements via flags.
> 
> Note that filter is still option from netlink dump path, since this can
> be called from error path to provide hints.
> 
> Fixes: 08725a9dc14c ("cache: filter out rules by chain")
> Fixes: b3ed8fd8c9f3 ("cache: missing family in cache filtering")
> Fixes: 635ee1cad8aa ("cache: filter out sets and maps that are not requested")
> Fixes: 3f1d3912c3a6 ("cache: filter out tables that are not requested")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/cache.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Thanks Pablo.

For the series:

Tested-by: Eric Garver <eric@garver.life>

Re: [PATCH nft 1/5] cache: assert filter when calling nft_cache_evaluate()

[Pablo Neira Ayuso]

On Thu, Aug 29, 2024 at 10:35:30AM -0400, Eric Garver wrote:
> On Thu, Aug 29, 2024 at 01:31:49PM +0200, Pablo Neira Ayuso wrote:
> > nft_cache_evaluate() always takes a non-null filter, remove superfluous
> > checks when calculating cache requirements via flags.
> > 
> > Note that filter is still option from netlink dump path, since this can
> > be called from error path to provide hints.
> > 
> > Fixes: 08725a9dc14c ("cache: filter out rules by chain")
> > Fixes: b3ed8fd8c9f3 ("cache: missing family in cache filtering")
> > Fixes: 635ee1cad8aa ("cache: filter out sets and maps that are not requested")
> > Fixes: 3f1d3912c3a6 ("cache: filter out tables that are not requested")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  src/cache.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> Thanks Pablo.
> 
> For the series:
> 
> Tested-by: Eric Garver <eric@garver.life>

Pushed out, thanks