[PATCH nft 1/5] src: remove useless parameter from cache_flush()

[PATCH nft 1/5] src: remove useless parameter from cache_flush()

[Pablo Neira Ayuso]

Command type is never used in cache_flush().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h | 3 +--
 src/evaluate.c | 2 +-
 src/rule.c     | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index b41825d000d6..299485ffeeaa 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -639,8 +639,7 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 extern int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds);
 extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 			struct list_head *msgs);
-extern void cache_flush(struct nft_ctx *ctx, enum cmd_ops cmd,
-			struct list_head *msgs);
+extern void cache_flush(struct nft_ctx *ctx, struct list_head *msgs);
 extern void cache_release(struct nft_cache *cache);
 extern bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd);
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 70c7e597f3b0..73a4be339ce1 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3682,7 +3682,7 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULESET:
-		cache_flush(ctx->nft, cmd->op, ctx->msgs);
+		cache_flush(ctx->nft, ctx->msgs);
 		break;
 	case CMD_OBJ_TABLE:
 		/* Flushing a table does not empty the sets in the table nor remove
diff --git a/src/rule.c b/src/rule.c
index 0c0fd07ec70c..4407b0b0ceaa 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -297,7 +297,7 @@ static void __cache_flush(struct list_head *table_list)
 	}
 }
 
-void cache_flush(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
+void cache_flush(struct nft_ctx *nft, struct list_head *msgs)
 {
 	struct netlink_ctx ctx = {
 		.list		= LIST_HEAD_INIT(ctx.list),
-- 
2.11.0

[PATCH nft 2/5] tests: shell: cannot use handle for non-existing rule in kernel

[Pablo Neira Ayuso]

This test invokes the 'replace rule ... handle 2' command. However,
there are no rules in the kernel, therefore it always fails.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/shell/testcases/nft-f/0006action_object_0 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/shell/testcases/nft-f/0006action_object_0 b/tests/shell/testcases/nft-f/0006action_object_0
index ffa6c9bda973..fab3070f493f 100755
--- a/tests/shell/testcases/nft-f/0006action_object_0
+++ b/tests/shell/testcases/nft-f/0006action_object_0
@@ -16,7 +16,7 @@ generate1()
 	add set $family t s {type inet_service;}
 	add element $family t s {8080}
 	insert rule $family t c meta l4proto tcp tcp dport @s accept
-	replace rule $family t c handle 2 meta l4proto tcp tcp dport {9090, 8080}
+	add rule $family t c meta l4proto tcp tcp dport {9090, 8080}
 	add map $family t m {type inet_service:verdict;}
 	add element $family t m {10080:drop}
 	insert rule $family t c meta l4proto tcp tcp dport vmap @m
-- 
2.11.0

Re: [PATCH nft 2/5] tests: shell: cannot use handle for non-existing rule in kernel

[Phil Sutter]

Hi,

On Mon, Jun 17, 2019 at 02:25:15PM +0200, Pablo Neira Ayuso wrote:
> This test invokes the 'replace rule ... handle 2' command. However,
> there are no rules in the kernel, therefore it always fails.

This guesses the previously inserted rule's handle. Does this start
failing with your flags conversion in place? My initial implementation
of intra-transaction rule references made this handle guessing
impossible, but your single point cache fetching still allowed for it
(hence why I dropped my patch with a similar change).

Cheers, Phil

Re: [PATCH nft 2/5] tests: shell: cannot use handle for non-existing rule in kernel

[Pablo Neira Ayuso]

Hi Phil,

On Mon, Jun 17, 2019 at 06:00:30PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Mon, Jun 17, 2019 at 02:25:15PM +0200, Pablo Neira Ayuso wrote:
> > This test invokes the 'replace rule ... handle 2' command. However,
> > there are no rules in the kernel, therefore it always fails.
> 
> This guesses the previously inserted rule's handle. Does this start
> failing with your flags conversion in place?

Yes.

> My initial implementation of intra-transaction rule references made
> this handle guessing impossible, but your single point cache
> fetching still allowed for it (hence why I dropped my patch with a
> similar change).

Hm. I think we should not guess the handle that the kernel assigns.

In a batch, handles do not exist. We could expose the
intra-transaction index if needed to the user. But I don't see a
use-case for this.

I think we should leave the handle as a reference to already existing
rules in the kernel.

Re: [PATCH nft 2/5] tests: shell: cannot use handle for non-existing rule in kernel

[Phil Sutter]

Hi Pablo,

On Mon, Jun 17, 2019 at 06:06:57PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 17, 2019 at 06:00:30PM +0200, Phil Sutter wrote:
[...]
> > My initial implementation of intra-transaction rule references made
> > this handle guessing impossible, but your single point cache
> > fetching still allowed for it (hence why I dropped my patch with a
> > similar change).
> 
> Hm. I think we should not guess the handle that the kernel assigns.
> 
> In a batch, handles do not exist. We could expose the
> intra-transaction index if needed to the user. But I don't see a
> use-case for this.
> 
> I think we should leave the handle as a reference to already existing
> rules in the kernel.

Yes, it's an ugly hack that should never have worked in the first place,
I fully agree. Yet that it stops working indicates user space starts
doing more than it has to - IMHO relying upon the kernel verifier is
desirable. At least it allows for much better handling of large
rulesets.

Cheers, Phil

[PATCH nft 3/5] src: add cache level flags

[Pablo Neira Ayuso]

The score approach based on command type is confusing.

This patch introduces cache level flags, each flag specifies what kind
of object type is needed. These flags are set on/off depending on the
list of commands coming in this batch.

cache_is_complete() now checks if the cache contains the objects that
are needed in the cache through these new flags.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/cache.h    |  22 +++++++++++
 include/nftables.h |   2 +-
 include/rule.h     |   3 +-
 src/cache.c        |  95 +++++++++++++++++++++++++++--------------------
 src/evaluate.c     |   1 +
 src/libnftables.c  |   6 +--
 src/rule.c         | 107 +++++++++++++++++++++++------------------------------
 7 files changed, 129 insertions(+), 107 deletions(-)
 create mode 100644 include/cache.h

diff --git a/include/cache.h b/include/cache.h
new file mode 100644
index 000000000000..9bf4a2a79eeb
--- /dev/null
+++ b/include/cache.h
@@ -0,0 +1,22 @@
+#ifndef _NFT_CACHE_H_
+#define _NFT_CACHE_H_
+
+enum cache_level_flags {
+	NFT_CACHE_EMPTY		= 0,
+	NFT_CACHE_TABLE		= (1 << 0),
+	NFT_CACHE_CHAIN 	= (1 << 1),
+	NFT_CACHE_SET 		= (1 << 2),
+	NFT_CACHE_FLOWTABLE	= (1 << 3),
+	NFT_CACHE_OBJECT	= (1 << 4),
+	NFT_CACHE_SETELEM 	= (1 << 5),
+	NFT_CACHE_RULE		= (1 << 6),
+	NFT_CACHE_FULL		= (NFT_CACHE_TABLE	|
+				   NFT_CACHE_CHAIN	|
+				   NFT_CACHE_SET	|
+				   NFT_CACHE_FLOWTABLE	|
+				   NFT_CACHE_OBJECT	|
+				   NFT_CACHE_SETELEM	|
+				   NFT_CACHE_RULE),
+};
+
+#endif /* _NFT_CACHE_H_ */
diff --git a/include/nftables.h b/include/nftables.h
index b7c78572da77..ed446e2d16cf 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -81,7 +81,7 @@ struct nft_cache {
 	uint32_t		genid;
 	struct list_head	list;
 	uint32_t		seqnum;
-	uint32_t		cmd;
+	uint32_t		flags;
 };
 
 struct mnl_socket;
diff --git a/include/rule.h b/include/rule.h
index 299485ffeeaa..aefb24d95163 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -462,7 +462,6 @@ enum cmd_ops {
 	CMD_EXPORT,
 	CMD_MONITOR,
 	CMD_DESCRIBE,
-	__CMD_FLUSH_RULESET,
 };
 
 /**
@@ -636,7 +635,7 @@ extern struct error_record *rule_postprocess(struct rule *rule);
 struct netlink_ctx;
 extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 
-extern int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds);
+extern unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds);
 extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 			struct list_head *msgs);
 extern void cache_flush(struct nft_ctx *ctx, struct list_head *msgs);
diff --git a/src/cache.c b/src/cache.c
index d7153f6f6b8f..6bd0a9a1442b 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -11,112 +11,125 @@
 #include <rule.h>
 #include <erec.h>
 #include <utils.h>
+#include <cache.h>
 
-static unsigned int evaluate_cache_add(struct cmd *cmd)
+static unsigned int evaluate_cache_add(struct cmd *cmd, unsigned int flags)
 {
-	unsigned int completeness = CMD_INVALID;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
-	case CMD_OBJ_SET:
-	case CMD_OBJ_CHAIN:
-	case CMD_OBJ_FLOWTABLE:
-		completeness = cmd->op;
+		flags |= NFT_CACHE_SETELEM;
 		break;
 	case CMD_OBJ_RULE:
-		if (cmd->handle.index.id)
-			completeness = CMD_LIST;
+		if (cmd->handle.index.id ||
+		    cmd->handle.position.id ||
+		    cmd->handle.handle.id) {
+			flags |= NFT_CACHE_SETELEM |
+				 NFT_CACHE_RULE;
+		}
 		break;
 	default:
 		break;
 	}
 
-	return completeness;
+	return flags;
 }
 
-static unsigned int evaluate_cache_del(struct cmd *cmd)
+static unsigned int evaluate_cache_del(struct cmd *cmd, unsigned int flags)
 {
-	unsigned int completeness = CMD_INVALID;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
-		completeness = cmd->op;
+		flags |= NFT_CACHE_SETELEM;
 		break;
 	default:
 		break;
 	}
 
-	return completeness;
+	return flags;
 }
 
-static unsigned int evaluate_cache_flush(struct cmd *cmd)
+static unsigned int evaluate_cache_flush(struct cmd *cmd, unsigned int flags)
 {
-	unsigned int completeness = CMD_INVALID;
-
 	switch (cmd->obj) {
-	case CMD_OBJ_RULESET:
-		completeness = __CMD_FLUSH_RULESET;
-		break;
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 	case CMD_OBJ_METER:
-		completeness = cmd->op;
+		flags |= NFT_CACHE_TABLE |
+			 NFT_CACHE_CHAIN |
+			 NFT_CACHE_SET |
+			 NFT_CACHE_FLOWTABLE |
+			 NFT_CACHE_OBJECT;
 		break;
+	case CMD_OBJ_RULESET:
 	default:
+		flags = NFT_CACHE_EMPTY;
 		break;
 	}
 
-	return completeness;
+	return flags;
 }
 
-static unsigned int evaluate_cache_rename(struct cmd *cmd)
+static unsigned int evaluate_cache_rename(struct cmd *cmd, unsigned int flags)
 {
-	unsigned int completeness = CMD_INVALID;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_CHAIN:
-		completeness = cmd->op;
+		flags |= NFT_CACHE_TABLE |
+			 NFT_CACHE_CHAIN;
 		break;
 	default:
 		break;
 	}
 
-	return completeness;
+	return flags;
 }
 
-int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
+unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
 {
-	unsigned int echo_completeness = CMD_INVALID;
-	unsigned int completeness = CMD_INVALID;
+	unsigned int flags = NFT_CACHE_EMPTY;
 	struct cmd *cmd;
 
 	list_for_each_entry(cmd, cmds, list) {
 		switch (cmd->op) {
 		case CMD_ADD:
 		case CMD_INSERT:
-		case CMD_REPLACE:
-			if (nft_output_echo(&nft->output))
-				echo_completeness = cmd->op;
-
+			flags |= NFT_CACHE_TABLE |
+				 NFT_CACHE_CHAIN |
+				 NFT_CACHE_SET |
+				 NFT_CACHE_FLOWTABLE |
+				 NFT_CACHE_OBJECT;
+
+			if (nft_output_echo(&nft->output)) {
+				flags |= NFT_CACHE_SETELEM |
+					 NFT_CACHE_RULE;
+				break;
+			}
 			/* Fall through */
 		case CMD_CREATE:
-			completeness = evaluate_cache_add(cmd);
+			flags = evaluate_cache_add(cmd, flags);
+			break;
+		case CMD_REPLACE:
+			flags |= NFT_CACHE_FULL;
 			break;
 		case CMD_DELETE:
-			completeness = evaluate_cache_del(cmd);
+			flags |= NFT_CACHE_TABLE |
+				 NFT_CACHE_CHAIN |
+				 NFT_CACHE_SET |
+				 NFT_CACHE_FLOWTABLE |
+				 NFT_CACHE_OBJECT;
+
+			flags = evaluate_cache_del(cmd, flags);
 			break;
 		case CMD_GET:
 		case CMD_LIST:
 		case CMD_RESET:
 		case CMD_EXPORT:
 		case CMD_MONITOR:
-			completeness = cmd->op;
+			flags |= NFT_CACHE_FULL;
 			break;
 		case CMD_FLUSH:
-			completeness = evaluate_cache_flush(cmd);
+			flags = evaluate_cache_flush(cmd, flags);
 			break;
 		case CMD_RENAME:
-			completeness = evaluate_cache_rename(cmd);
+			flags = evaluate_cache_rename(cmd, flags);
 			break;
 		case CMD_DESCRIBE:
 		case CMD_IMPORT:
@@ -126,5 +139,5 @@ int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
 		}
 	}
 
-	return max(completeness, echo_completeness);
+	return flags;
 }
diff --git a/src/evaluate.c b/src/evaluate.c
index 73a4be339ce1..ff0888d0c784 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -29,6 +29,7 @@
 #include <netlink.h>
 #include <time.h>
 #include <rule.h>
+#include <cache.h>
 #include <erec.h>
 #include <gmputil.h>
 #include <utils.h>
diff --git a/src/libnftables.c b/src/libnftables.c
index abd133bee127..dccb8ab4da42 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -381,11 +381,11 @@ static int nft_parse_bison_filename(struct nft_ctx *nft, const char *filename,
 static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 			struct list_head *cmds)
 {
-	unsigned int completeness;
+	unsigned int flags;
 	struct cmd *cmd;
 
-	completeness = cache_evaluate(nft, cmds);
-	if (cache_update(nft, completeness, msgs) < 0)
+	flags = cache_evaluate(nft, cmds);
+	if (cache_update(nft, flags, msgs) < 0)
 		return -1;
 
 	list_for_each_entry(cmd, cmds, list) {
diff --git a/src/rule.c b/src/rule.c
index 4407b0b0ceaa..782253b81589 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -24,6 +24,7 @@
 #include <mnl.h>
 #include <misspell.h>
 #include <json.h>
+#include <cache.h>
 
 #include <libnftnl/common.h>
 #include <libnftnl/ruleset.h>
@@ -147,97 +148,84 @@ static int cache_init_tables(struct netlink_ctx *ctx, struct handle *h,
 	return 0;
 }
 
-static int cache_init_objects(struct netlink_ctx *ctx, enum cmd_ops cmd)
+static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags)
 {
+	struct rule *rule, *nrule;
 	struct table *table;
 	struct chain *chain;
-	struct rule *rule, *nrule;
 	struct set *set;
 	int ret;
 
 	list_for_each_entry(table, &ctx->nft->cache.list, list) {
-		ret = netlink_list_sets(ctx, &table->handle);
-		list_splice_tail_init(&ctx->list, &table->sets);
-
-		if (ret < 0)
-			return -1;
-
-		list_for_each_entry(set, &table->sets, list) {
-			ret = netlink_list_setelems(ctx, &set->handle, set);
+		if (flags & NFT_CACHE_SET) {
+			ret = netlink_list_sets(ctx, &table->handle);
+			list_splice_tail_init(&ctx->list, &table->sets);
 			if (ret < 0)
 				return -1;
 		}
-
-		ret = netlink_list_chains(ctx, &table->handle);
-		if (ret < 0)
-			return -1;
-		list_splice_tail_init(&ctx->list, &table->chains);
-
-		ret = netlink_list_flowtables(ctx, &table->handle);
-		if (ret < 0)
-			return -1;
-		list_splice_tail_init(&ctx->list, &table->flowtables);
-
-		if (cmd != CMD_RESET) {
+		if (flags & NFT_CACHE_SETELEM) {
+			list_for_each_entry(set, &table->sets, list) {
+				ret = netlink_list_setelems(ctx, &set->handle,
+							    set);
+				if (ret < 0)
+					return -1;
+			}
+		}
+		if (flags & NFT_CACHE_CHAIN) {
+			ret = netlink_list_chains(ctx, &table->handle);
+			if (ret < 0)
+				return -1;
+			list_splice_tail_init(&ctx->list, &table->chains);
+		}
+		if (flags & NFT_CACHE_FLOWTABLE) {
+			ret = netlink_list_flowtables(ctx, &table->handle);
+			if (ret < 0)
+				return -1;
+			list_splice_tail_init(&ctx->list, &table->flowtables);
+		}
+		if (flags & NFT_CACHE_OBJECT) {
 			ret = netlink_list_objs(ctx, &table->handle);
 			if (ret < 0)
 				return -1;
 			list_splice_tail_init(&ctx->list, &table->objs);
 		}
-
-		/* Skip caching other objects to speed up things: We only need
-		 * a full cache when listing the existing ruleset.
-		 */
-		if (cmd != CMD_LIST)
-			continue;
-
-		ret = netlink_list_table(ctx, &table->handle);
-		list_for_each_entry_safe(rule, nrule, &ctx->list, list) {
-			chain = chain_lookup(table, &rule->handle);
-			list_move_tail(&rule->list, &chain->rules);
+		if (flags & NFT_CACHE_RULE) {
+			ret = netlink_list_table(ctx, &table->handle);
+			list_for_each_entry_safe(rule, nrule, &ctx->list, list) {
+				chain = chain_lookup(table, &rule->handle);
+				list_move_tail(&rule->list, &chain->rules);
+			}
+			if (ret < 0)
+				return -1;
 		}
-
-		if (ret < 0)
-			return -1;
 	}
 	return 0;
 }
 
-static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
+static int cache_init(struct netlink_ctx *ctx, unsigned int flags)
 {
 	struct handle handle = {
 		.family = NFPROTO_UNSPEC,
 	};
 	int ret;
 
-	if (cmd == __CMD_FLUSH_RULESET)
+	if (flags == NFT_CACHE_EMPTY)
 		return 0;
 
+	/* assume NFT_CACHE_TABLE is always set. */
 	ret = cache_init_tables(ctx, &handle, &ctx->nft->cache);
 	if (ret < 0)
 		return ret;
-	ret = cache_init_objects(ctx, cmd);
+	ret = cache_init_objects(ctx, flags);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-/* Return a "score" of how complete local cache will be if
- * cache_init_objects() ran for given cmd. Higher value
- * means more complete. */
-static int cache_completeness(enum cmd_ops cmd)
+bool cache_is_complete(struct nft_cache *cache, unsigned int flags)
 {
-	if (cmd == CMD_LIST)
-		return 3;
-	if (cmd != CMD_RESET)
-		return 2;
-	return 1;
-}
-
-bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd)
-{
-	return cache_completeness(cache->cmd) >= cache_completeness(cmd);
+	return (cache->flags & flags) == flags;
 }
 
 static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
@@ -245,7 +233,7 @@ static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
 	return genid && genid == cache->genid;
 }
 
-int cache_update(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
+int cache_update(struct nft_ctx *nft, unsigned int flags, struct list_head *msgs)
 {
 	struct netlink_ctx ctx = {
 		.list		= LIST_HEAD_INIT(ctx.list),
@@ -259,14 +247,14 @@ int cache_update(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 replay:
 	ctx.seqnum = cache->seqnum++;
 	genid = mnl_genid_get(&ctx);
-	if (cache_is_complete(cache, cmd) &&
+	if (cache_is_complete(cache, flags) &&
 	    cache_is_updated(cache, genid))
 		return 0;
 
 	if (cache->genid)
 		cache_release(cache);
 
-	ret = cache_init(&ctx, cmd);
+	ret = cache_init(&ctx, flags);
 	if (ret < 0) {
 		cache_release(cache);
 		if (errno == EINTR) {
@@ -283,7 +271,7 @@ replay:
 	}
 
 	cache->genid = genid;
-	cache->cmd = cmd;
+	cache->flags = flags;
 	return 0;
 }
 
@@ -308,15 +296,14 @@ void cache_flush(struct nft_ctx *nft, struct list_head *msgs)
 
 	__cache_flush(&cache->list);
 	cache->genid = mnl_genid_get(&ctx);
-	cache->cmd = CMD_LIST;
+	cache->flags = NFT_CACHE_FULL;
 }
 
 void cache_release(struct nft_cache *cache)
 {
 	__cache_flush(&cache->list);
 	cache->genid = 0;
-	cache->cmd = CMD_INVALID;
-
+	cache->flags = NFT_CACHE_EMPTY;
 }
 
 /* internal ID to uniquely identify a set in the batch */
-- 
2.11.0

Re: [PATCH nft 3/5] src: add cache level flags

[Phil Sutter]

Hi,

On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> The score approach based on command type is confusing.
> 
> This patch introduces cache level flags, each flag specifies what kind
> of object type is needed. These flags are set on/off depending on the
> list of commands coming in this batch.
> 
> cache_is_complete() now checks if the cache contains the objects that
> are needed in the cache through these new flags.

Thanks for committing to getting the cache stuff right!

[...]
> +enum cache_level_flags {
> +	NFT_CACHE_EMPTY		= 0,
> +	NFT_CACHE_TABLE		= (1 << 0),
> +	NFT_CACHE_CHAIN 	= (1 << 1),
> +	NFT_CACHE_SET 		= (1 << 2),
> +	NFT_CACHE_FLOWTABLE	= (1 << 3),
> +	NFT_CACHE_OBJECT	= (1 << 4),
> +	NFT_CACHE_SETELEM 	= (1 << 5),
> +	NFT_CACHE_RULE		= (1 << 6),
> +	NFT_CACHE_FULL		= (NFT_CACHE_TABLE	|
> +				   NFT_CACHE_CHAIN	|
> +				   NFT_CACHE_SET	|
> +				   NFT_CACHE_FLOWTABLE	|
> +				   NFT_CACHE_OBJECT	|
> +				   NFT_CACHE_SETELEM	|
> +				   NFT_CACHE_RULE),
> +};

I think we can do this in a way which reflects the implicit dependencies
when fetching ruleset elements. I think of something like:

| enum nft_cache_bits {
| 	NFT_CACHE_TABLE_BIT	= (1 << 0),
| 	NFT_CACHE_CHAIN_BIT	= (1 << 1),
| 	NFT_CACHE_SET_BIT	= (1 << 2),
| 	NFT_CACHE_FLOWTABLE_BIT	= (1 << 3),
| 	NFT_CACHE_OBJECT_BIT	= (1 << 4),
| 	NFT_CACHE_SETELEM_BIT	= (1 << 5),
| 	NFT_CACHE_RULE_BIT	= (1 << 6),
| 	__NFT_CACHE_MAX_BIT	= (1 << 7),
| };
| 
| enum cache_level_flags {
| 	NFT_CACHE_EMPTY		= 0,
| 	NFT_CACHE_TABLE		= NFT_CACHE_TABLE_BIT,
| 	NFT_CACHE_CHAIN		= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_CHAIN_BIT,
| 	NFT_CACHE_SET		= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_SET_BIT,
| 	NFT_CACHE_FLOWTABLE	= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_FLOWTABLE_BIT,
| 	NFT_CACHE_OBJECT	= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_OBJECT_BIT,
| 	NFT_CACHE_SETELEM	= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_SET_BIT
| 				| NFT_CACHE_SETELEM_BIT,
| 	NFT_CACHE_RULE		= NFT_CACHE_TABLE_BIT
| 				| NFT_CACHE_CHAIN_BIT
| 				| NFT_CACHE_SETELEM_BIT
| 				| NFT_CACHE_RULE_BIT,
| 	NFT_CACHE_FULL		= __NFT_CACHE_MAX_BIT - 1,
| };

This removes these dependency details from cache_evaluate() functions.
What do you think?

Cheers, Phil

Re: [PATCH nft 3/5] src: add cache level flags

[Phil Sutter]

Hi,

On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
[...]
> -int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> +unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
>  {
> -	unsigned int echo_completeness = CMD_INVALID;
> -	unsigned int completeness = CMD_INVALID;
> +	unsigned int flags = NFT_CACHE_EMPTY;
>  	struct cmd *cmd;
>  
>  	list_for_each_entry(cmd, cmds, list) {
>  		switch (cmd->op) {
>  		case CMD_ADD:
>  		case CMD_INSERT:
> -		case CMD_REPLACE:
> -			if (nft_output_echo(&nft->output))
> -				echo_completeness = cmd->op;
> -
> +			flags |= NFT_CACHE_TABLE |
> +				 NFT_CACHE_CHAIN |
> +				 NFT_CACHE_SET |
> +				 NFT_CACHE_FLOWTABLE |
> +				 NFT_CACHE_OBJECT;

This means we start fetching the cache for simple 'add rule' commands
again, right?

This should be the reason why that test case started failing for you.

> +
> +			if (nft_output_echo(&nft->output)) {
> +				flags |= NFT_CACHE_SETELEM |
> +					 NFT_CACHE_RULE;
> +				break;
> +			}
>  			/* Fall through */
>  		case CMD_CREATE:
> -			completeness = evaluate_cache_add(cmd);
> +			flags = evaluate_cache_add(cmd, flags);
> +			break;
> +		case CMD_REPLACE:
> +			flags |= NFT_CACHE_FULL;
>  			break;
>  		case CMD_DELETE:
> -			completeness = evaluate_cache_del(cmd);
> +			flags |= NFT_CACHE_TABLE |
> +				 NFT_CACHE_CHAIN |
> +				 NFT_CACHE_SET |
> +				 NFT_CACHE_FLOWTABLE |
> +				 NFT_CACHE_OBJECT;

Same here, I guess: Single 'delete rule' command causes fetching of
above ruleset items (unless I miss something).

Cheers, Phil

Re: [PATCH nft 3/5] src: add cache level flags

[Pablo Neira Ayuso]

On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > -int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> > +unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> >  {
> > -	unsigned int echo_completeness = CMD_INVALID;
> > -	unsigned int completeness = CMD_INVALID;
> > +	unsigned int flags = NFT_CACHE_EMPTY;
> >  	struct cmd *cmd;
> >  
> >  	list_for_each_entry(cmd, cmds, list) {
> >  		switch (cmd->op) {
> >  		case CMD_ADD:
> >  		case CMD_INSERT:
> > -		case CMD_REPLACE:
> > -			if (nft_output_echo(&nft->output))
> > -				echo_completeness = cmd->op;
> > -
> > +			flags |= NFT_CACHE_TABLE |
> > +				 NFT_CACHE_CHAIN |
> > +				 NFT_CACHE_SET |
> > +				 NFT_CACHE_FLOWTABLE |
> > +				 NFT_CACHE_OBJECT;
> 
> This means we start fetching the cache for simple 'add rule' commands
> again, right?

We need these for references to sets, eg.

        add rule x y ip saddr @x

same for other flowtable and object.

We should not use NFT_CACHE_RULE in this case, if this is what you
suggest.

Re: [PATCH nft 3/5] src: add cache level flags

[Phil Sutter]

On Mon, Jun 17, 2019 at 06:28:40PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > -int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> > > +unsigned int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> > >  {
> > > -	unsigned int echo_completeness = CMD_INVALID;
> > > -	unsigned int completeness = CMD_INVALID;
> > > +	unsigned int flags = NFT_CACHE_EMPTY;
> > >  	struct cmd *cmd;
> > >  
> > >  	list_for_each_entry(cmd, cmds, list) {
> > >  		switch (cmd->op) {
> > >  		case CMD_ADD:
> > >  		case CMD_INSERT:
> > > -		case CMD_REPLACE:
> > > -			if (nft_output_echo(&nft->output))
> > > -				echo_completeness = cmd->op;
> > > -
> > > +			flags |= NFT_CACHE_TABLE |
> > > +				 NFT_CACHE_CHAIN |
> > > +				 NFT_CACHE_SET |
> > > +				 NFT_CACHE_FLOWTABLE |
> > > +				 NFT_CACHE_OBJECT;
> > 
> > This means we start fetching the cache for simple 'add rule' commands
> > again, right?
> 
> We need these for references to sets, eg.
> 
>         add rule x y ip saddr @x
> 
> same for other flowtable and object.

Oh, right. I got that wrong - old code is always fetching the above
items unless there's no ruleset in kernel (i.e., returned genid is 0).

I confused that with fetching rules which at some point started to
happen by accident with my changes.

> We should not use NFT_CACHE_RULE in this case, if this is what you
> suggest.

No, quite the opposite: I thought we could get by without fetching
anything from kernel at all.

Yet now I wonder why the handle guessing stops working, because the
above can't be the cause of it.

Cheers, Phil

Re: [PATCH nft 3/5] src: add cache level flags

[Pablo Neira Ayuso]

On Mon, Jun 17, 2019 at 06:45:59PM +0200, Phil Sutter wrote:
> On Mon, Jun 17, 2019 at 06:28:40PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
[...]
> > 
> > We need these for references to sets, eg.
> > 
> >         add rule x y ip saddr @x
> > 
> > same for other flowtable and object.
> 
> Oh, right. I got that wrong - old code is always fetching the above
> items unless there's no ruleset in kernel (i.e., returned genid is 0).
> 
> I confused that with fetching rules which at some point started to
> happen by accident with my changes.
> 
> > We should not use NFT_CACHE_RULE in this case, if this is what you
> > suggest.
> 
> No, quite the opposite: I thought we could get by without fetching
> anything from kernel at all.
> 
> Yet now I wonder why the handle guessing stops working, because the
> above can't be the cause of it.

I think we should partial revert the changes that are doing the
handle guessing, would you submit a patch for this?

Thanks!

Re: [PATCH nft 3/5] src: add cache level flags

[Phil Sutter]

On Mon, Jun 17, 2019 at 07:24:33PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 17, 2019 at 06:45:59PM +0200, Phil Sutter wrote:
> > On Mon, Jun 17, 2019 at 06:28:40PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > > 
> > > We need these for references to sets, eg.
> > > 
> > >         add rule x y ip saddr @x
> > > 
> > > same for other flowtable and object.
> > 
> > Oh, right. I got that wrong - old code is always fetching the above
> > items unless there's no ruleset in kernel (i.e., returned genid is 0).
> > 
> > I confused that with fetching rules which at some point started to
> > happen by accident with my changes.
> > 
> > > We should not use NFT_CACHE_RULE in this case, if this is what you
> > > suggest.
> > 
> > No, quite the opposite: I thought we could get by without fetching
> > anything from kernel at all.
> > 
> > Yet now I wonder why the handle guessing stops working, because the
> > above can't be the cause of it.
> 
> I think we should partial revert the changes that are doing the
> handle guessing, would you submit a patch for this?

There are no explicit changes allowing for it. The reason it works is
because user space doesn't care to check the given handle for existence
and in kernel space it is valid already.

Cheers, Phil

Re: [PATCH nft 3/5] src: add cache level flags

[Pablo Neira Ayuso]

On Mon, Jun 17, 2019 at 07:28:31PM +0200, Phil Sutter wrote:
> On Mon, Jun 17, 2019 at 07:24:33PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jun 17, 2019 at 06:45:59PM +0200, Phil Sutter wrote:
> > > On Mon, Jun 17, 2019 at 06:28:40PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > > 
> > > > We need these for references to sets, eg.
> > > > 
> > > >         add rule x y ip saddr @x
> > > > 
> > > > same for other flowtable and object.
> > > 
> > > Oh, right. I got that wrong - old code is always fetching the above
> > > items unless there's no ruleset in kernel (i.e., returned genid is 0).
> > > 
> > > I confused that with fetching rules which at some point started to
> > > happen by accident with my changes.
> > > 
> > > > We should not use NFT_CACHE_RULE in this case, if this is what you
> > > > suggest.
> > > 
> > > No, quite the opposite: I thought we could get by without fetching
> > > anything from kernel at all.
> > > 
> > > Yet now I wonder why the handle guessing stops working, because the
> > > above can't be the cause of it.
> > 
> > I think we should partial revert the changes that are doing the
> > handle guessing, would you submit a patch for this?
> 
> There are no explicit changes allowing for it. The reason it works is
> because user space doesn't care to check the given handle for existence
> and in kernel space it is valid already.

OK, but this test just works because we are being lucky in
guessing, right? I don't think we should not have a test for handles
from the batch, for an unexisting rule in the kernel.

Re: [PATCH nft 3/5] src: add cache level flags

[Phil Sutter]

On Mon, Jun 17, 2019 at 07:33:29PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 17, 2019 at 07:28:31PM +0200, Phil Sutter wrote:
> > On Mon, Jun 17, 2019 at 07:24:33PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Jun 17, 2019 at 06:45:59PM +0200, Phil Sutter wrote:
> > > > On Mon, Jun 17, 2019 at 06:28:40PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Mon, Jun 17, 2019 at 06:11:04PM +0200, Phil Sutter wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Mon, Jun 17, 2019 at 02:25:16PM +0200, Pablo Neira Ayuso wrote:
> > > [...]
> > > > > 
> > > > > We need these for references to sets, eg.
> > > > > 
> > > > >         add rule x y ip saddr @x
> > > > > 
> > > > > same for other flowtable and object.
> > > > 
> > > > Oh, right. I got that wrong - old code is always fetching the above
> > > > items unless there's no ruleset in kernel (i.e., returned genid is 0).
> > > > 
> > > > I confused that with fetching rules which at some point started to
> > > > happen by accident with my changes.
> > > > 
> > > > > We should not use NFT_CACHE_RULE in this case, if this is what you
> > > > > suggest.
> > > > 
> > > > No, quite the opposite: I thought we could get by without fetching
> > > > anything from kernel at all.
> > > > 
> > > > Yet now I wonder why the handle guessing stops working, because the
> > > > above can't be the cause of it.
> > > 
> > > I think we should partial revert the changes that are doing the
> > > handle guessing, would you submit a patch for this?
> > 
> > There are no explicit changes allowing for it. The reason it works is
> > because user space doesn't care to check the given handle for existence
> > and in kernel space it is valid already.
> 
> OK, but this test just works because we are being lucky in
> guessing, right? I don't think we should not have a test for handles
> from the batch, for an unexisting rule in the kernel.

Yes, that's right. The test in question was added back in 2016 to make
sure the separate commands syntax works with 'nft -f'. I guess it was
simply convenient to add all the different commands to the same
transaction.

Cheers, Phil

[PATCH nft 4/5] rule: skip cache population from do_command_monitor()

[Pablo Neira Ayuso]

nft_evaluate() already populates the cache before running the monitor
command. Remove this code.

Fixes: 7df42800cf89 ("src: single cache_update() call to build cache before evaluation")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/rule.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 782253b81589..732de2874877 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2414,8 +2414,6 @@ static bool need_cache(const struct cmd *cmd)
 
 static int do_command_monitor(struct netlink_ctx *ctx, struct cmd *cmd)
 {
-	struct table *t;
-	struct set *s;
 	struct netlink_mon_handler monhandler = {
 		.monitor_flags	= cmd->monitor->flags,
 		.format		= cmd->monitor->format,
@@ -2429,36 +2427,6 @@ static int do_command_monitor(struct netlink_ctx *ctx, struct cmd *cmd)
 		monhandler.format = NFTNL_OUTPUT_JSON;
 
 	monhandler.cache_needed = need_cache(cmd);
-	if (monhandler.cache_needed) {
-		struct rule *rule, *nrule;
-		struct chain *chain;
-		int ret;
-
-		list_for_each_entry(t, &ctx->nft->cache.list, list) {
-			list_for_each_entry(s, &t->sets, list)
-				s->init = set_expr_alloc(&cmd->location, s);
-
-			if (!(cmd->monitor->flags & (1 << NFT_MSG_TRACE)))
-				continue;
-
-			/* When tracing we'd like to translate the rule handle
-			 * we receive in the trace messages to the actual rule
-			 * struct to print that out.  Populate rule cache now.
-			 */
-			ret = netlink_list_table(ctx, &t->handle);
-
-			if (ret != 0)
-				/* Shouldn't happen and doesn't break things
-				 * too badly
-				 */
-				continue;
-
-			list_for_each_entry_safe(rule, nrule, &ctx->list, list) {
-				chain = chain_lookup(t, &rule->handle);
-				list_move_tail(&rule->list, &chain->rules);
-			}
-		}
-	}
 
 	return netlink_monitor(&monhandler, ctx->nft->nf_sock);
 }
-- 
2.11.0

[PATCH nft 5/5] netlink: remove netlink_list_table()

[Pablo Neira Ayuso]

Remove this wrapper, call netlink_list_rules() instead.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/netlink.h | 2 +-
 src/netlink.c     | 7 +------
 src/rule.c        | 2 +-
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index 0c08b1abbf6a..279723f33d31 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -104,6 +104,7 @@ extern struct expr *netlink_alloc_data(const struct location *loc,
 				       const struct nft_data_delinearize *nld,
 				       enum nft_registers dreg);
 
+extern int netlink_list_rules(struct netlink_ctx *ctx, const struct handle *h);
 extern void netlink_linearize_rule(struct netlink_ctx *ctx,
 				   struct nftnl_rule *nlr,
 				   const struct rule *rule);
@@ -115,7 +116,6 @@ extern struct chain *netlink_delinearize_chain(struct netlink_ctx *ctx,
 					       const struct nftnl_chain *nlc);
 
 extern int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h);
-extern int netlink_list_table(struct netlink_ctx *ctx, const struct handle *h);
 extern struct table *netlink_delinearize_table(struct netlink_ctx *ctx,
 					       const struct nftnl_table *nlt);
 
diff --git a/src/netlink.c b/src/netlink.c
index a6d81b4f6424..24d8f03ae4be 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -333,7 +333,7 @@ static int list_rule_cb(struct nftnl_rule *nlr, void *arg)
 	return 0;
 }
 
-static int netlink_list_rules(struct netlink_ctx *ctx, const struct handle *h)
+int netlink_list_rules(struct netlink_ctx *ctx, const struct handle *h)
 {
 	struct nftnl_rule_list *rule_cache;
 
@@ -485,11 +485,6 @@ int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h)
 	return 0;
 }
 
-int netlink_list_table(struct netlink_ctx *ctx, const struct handle *h)
-{
-	return netlink_list_rules(ctx, h);
-}
-
 enum nft_data_types dtype_map_to_kernel(const struct datatype *dtype)
 {
 	switch (dtype->type) {
diff --git a/src/rule.c b/src/rule.c
index 732de2874877..2d42a0328b66 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -190,7 +190,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags)
 			list_splice_tail_init(&ctx->list, &table->objs);
 		}
 		if (flags & NFT_CACHE_RULE) {
-			ret = netlink_list_table(ctx, &table->handle);
+			ret = netlink_list_rules(ctx, &table->handle);
 			list_for_each_entry_safe(rule, nrule, &ctx->list, list) {
 				chain = chain_lookup(table, &rule->handle);
 				list_move_tail(&rule->list, &chain->rules);
-- 
2.11.0