[nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Phil Sutter]

Next round of combined cache update fix and intra-transaction rule
reference support.

Patch 2 is new, it avoids accidential cache updates when committing a
transaction containing flush ruleset command and kernel ruleset has
changed meanwhile.

Patch 3 is also new: If a transaction fails in kernel, local cache is
incorrect - drop it.

Patch 9 is a new requirement for patch 10 due to relocation of new
functions.

Patch 10 was changed, changelog included.

Phil Sutter (10):
  src: Fix cache_flush() in cache_needs_more() logic
  src: Utilize CMD_FLUSH for cache->cmd
  libnftables: Drop cache in error case
  libnftables: Keep list of commands in nft context
  src: Make {table,chain}_not_found() public
  src: Restore local entries after cache update
  rule: Introduce rule_lookup_by_index()
  src: Make cache_is_complete() public
  include: Collect __stmt_binary_error() wrapper macros
  src: Support intra-transaction rule references

 include/erec.h                                |   6 +
 include/nftables.h                            |   1 +
 include/rule.h                                |  10 +
 src/evaluate.c                                |  71 ++----
 src/libnftables.c                             |  25 ++-
 src/mnl.c                                     |   4 +
 src/rule.c                                    | 202 +++++++++++++++++-
 tests/json_echo/run-test.py                   |   6 +-
 .../shell/testcases/cache/0003_cache_update_0 |   7 +
 tests/shell/testcases/transactions/0024rule_0 |  17 ++
 tests/shell/testcases/transactions/0025rule_0 |  21 ++
 .../transactions/dumps/0024rule_0.nft         |   8 +
 .../transactions/dumps/0025rule_0.nft         |   6 +
 13 files changed, 314 insertions(+), 70 deletions(-)
 create mode 100755 tests/shell/testcases/transactions/0024rule_0
 create mode 100755 tests/shell/testcases/transactions/0025rule_0
 create mode 100644 tests/shell/testcases/transactions/dumps/0024rule_0.nft
 create mode 100644 tests/shell/testcases/transactions/dumps/0025rule_0.nft

-- 
2.21.0

[nft PATCH v5 01/10] src: Fix cache_flush() in cache_needs_more() logic

[Phil Sutter]

Commit 34a20645d54fa enabled cache updates depending on command causing
it. As a side-effect, this disabled measures in cache_flush() preventing
a later cache update. Re-establish this by setting cache->cmd in
addition to cache->genid after dropping cache entries.

While being at it, set cache->cmd in cache_release() as well. This
shouldn't be necessary since zeroing cache->genid should suffice for
cache_update(), but better be consistent (and future-proof) here.

Fixes: 34a20645d54fa ("src: update cache if cmd is more specific")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/rule.c b/src/rule.c
index 326edb5dd459a..966948cd7ae90 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -299,12 +299,15 @@ void cache_flush(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 
 	__cache_flush(&cache->list);
 	cache->genid = mnl_genid_get(&ctx);
+	cache->cmd = CMD_LIST;
 }
 
 void cache_release(struct nft_cache *cache)
 {
 	__cache_flush(&cache->list);
 	cache->genid = 0;
+	cache->cmd = CMD_INVALID;
+
 }
 
 /* internal ID to uniquely identify a set in the batch */
-- 
2.21.0

Re: [nft PATCH v5 01/10] src: Fix cache_flush() in cache_needs_more() logic

[Pablo Neira Ayuso]

On Tue, Jun 04, 2019 at 07:31:49PM +0200, Phil Sutter wrote:
> Commit 34a20645d54fa enabled cache updates depending on command causing
> it. As a side-effect, this disabled measures in cache_flush() preventing
> a later cache update. Re-establish this by setting cache->cmd in
> addition to cache->genid after dropping cache entries.
> 
> While being at it, set cache->cmd in cache_release() as well. This
> shouldn't be necessary since zeroing cache->genid should suffice for
> cache_update(), but better be consistent (and future-proof) here.

Applied, thanks Phil.

[nft PATCH v5 02/10] src: Utilize CMD_FLUSH for cache->cmd

[Phil Sutter]

Make cache_flush() set cache->cmd to CMD_FLUSH and treat that as a new
highest cache completeness level. Prevent cache_update() from doing its
thing if it's set even if kernel's ruleset is newer.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 966948cd7ae90..f6ef1f6b0addd 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -225,6 +225,8 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
  * means more complete. */
 static int cache_completeness(enum cmd_ops cmd)
 {
+	if (cmd == CMD_FLUSH)
+		return 4;
 	if (cmd == CMD_LIST)
 		return 3;
 	if (cmd != CMD_RESET)
@@ -258,7 +260,8 @@ replay:
 	ctx.seqnum = cache->seqnum++;
 	genid = mnl_genid_get(&ctx);
 	if (cache_is_complete(cache, cmd) &&
-	    cache_is_updated(cache, genid))
+	    (cache_is_updated(cache, genid) ||
+	     cache_is_complete(cache, CMD_FLUSH)))
 		return 0;
 
 	if (cache->genid)
@@ -299,7 +302,7 @@ void cache_flush(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 
 	__cache_flush(&cache->list);
 	cache->genid = mnl_genid_get(&ctx);
-	cache->cmd = CMD_LIST;
+	cache->cmd = CMD_FLUSH;
 }
 
 void cache_release(struct nft_cache *cache)
-- 
2.21.0

[nft PATCH v5 03/10] libnftables: Drop cache in error case

[Phil Sutter]

If a transaction is rejected by the kernel (for instance due to a
semantic error), cache contents are potentially invalid. Release the
cache in that case to avoid the inconsistency.

The problem is easy to reproduce in an interactive session:

| nft> list ruleset
| table ip t {
| 	chain c {
| 	}
| }
| nft> flush ruleset; add rule ip t c accept
| Error: No such file or directory
| flush ruleset; add rule ip t c accept
|                            ^
| nft> list ruleset
| nft>

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/libnftables.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/libnftables.c b/src/libnftables.c
index d8de89ca509cd..e928ce476a90f 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -423,6 +423,8 @@ err:
 	    nft_output_json(&nft->output) &&
 	    nft_output_echo(&nft->output))
 		json_print_echo(nft);
+	if (rc)
+		cache_release(&nft->cache);
 	return rc;
 }
 
@@ -466,6 +468,8 @@ err:
 	    nft_output_json(&nft->output) &&
 	    nft_output_echo(&nft->output))
 		json_print_echo(nft);
+	if (rc)
+		cache_release(&nft->cache);
 	return rc;
 }
 
-- 
2.21.0

Re: [nft PATCH v5 03/10] libnftables: Drop cache in error case

[Pablo Neira Ayuso]

On Tue, Jun 04, 2019 at 07:31:51PM +0200, Phil Sutter wrote:
> If a transaction is rejected by the kernel (for instance due to a
> semantic error), cache contents are potentially invalid. Release the
> cache in that case to avoid the inconsistency.
> 
> The problem is easy to reproduce in an interactive session:
> 
> | nft> list ruleset
> | table ip t {
> | 	chain c {
> | 	}
> | }
> | nft> flush ruleset; add rule ip t c accept
> | Error: No such file or directory
> | flush ruleset; add rule ip t c accept
> |                            ^
> | nft> list ruleset
> | nft>

Also applied, thanks Phil.

[nft PATCH v5 04/10] libnftables: Keep list of commands in nft context

[Phil Sutter]

To fix the pending issues with cache updates, the list of commands needs
to be accessible from within cache_update(). In theory, there is a path
via nft->state->cmds but that struct parser_state is used (and
initialized) by bison parser only so that does not work with JSON
parser.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h |  1 +
 src/libnftables.c  | 21 ++++++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index bb9bb2091716d..faacf26509104 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -102,6 +102,7 @@ struct nft_ctx {
 	struct parser_state	*state;
 	void			*scanner;
 	void			*json_root;
+	struct list_head	cmds;
 	FILE			*f[MAX_INCLUDE_DEPTH];
 };
 
diff --git a/src/libnftables.c b/src/libnftables.c
index e928ce476a90f..b53b3f2ecfece 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -152,6 +152,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	nft_ctx_add_include_path(ctx, DEFAULT_INCLUDE_PATH);
 	ctx->parser_max_errors	= 10;
 	init_list_head(&ctx->cache.list);
+	init_list_head(&ctx->cmds);
 	ctx->flags = flags;
 	ctx->output.output_fp = stdout;
 	ctx->output.error_fp = stderr;
@@ -351,7 +352,7 @@ static int nft_parse_bison_buffer(struct nft_ctx *nft, const char *buf,
 	struct cmd *cmd;
 	int ret;
 
-	parser_init(nft, nft->state, msgs, cmds);
+	parser_init(nft, nft->state, msgs, &nft->cmds);
 	nft->scanner = scanner_init(nft->state);
 	scanner_push_buffer(nft->scanner, &indesc_cmdline, buf);
 
@@ -390,7 +391,6 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 {
 	struct cmd *cmd, *next;
 	LIST_HEAD(msgs);
-	LIST_HEAD(cmds);
 	char *nlbuf;
 	int rc = -EINVAL;
 
@@ -398,17 +398,17 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 	sprintf(nlbuf, "%s\n", buf);
 
 	if (nft_output_json(&nft->output))
-		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
+		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &nft->cmds);
 	if (rc == -EINVAL)
-		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds);
+		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &nft->cmds);
 	if (rc)
 		goto err;
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &nft->cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	list_for_each_entry_safe(cmd, next, &cmds, list) {
+	list_for_each_entry_safe(cmd, next, &nft->cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
@@ -432,7 +432,6 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 {
 	struct cmd *cmd, *next;
 	LIST_HEAD(msgs);
-	LIST_HEAD(cmds);
 	int rc;
 
 	rc = cache_update(nft, CMD_INVALID, &msgs);
@@ -444,17 +443,17 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 
 	rc = -EINVAL;
 	if (nft_output_json(&nft->output))
-		rc = nft_parse_json_filename(nft, filename, &msgs, &cmds);
+		rc = nft_parse_json_filename(nft, filename, &msgs, &nft->cmds);
 	if (rc == -EINVAL)
-		rc = nft_parse_bison_filename(nft, filename, &msgs, &cmds);
+		rc = nft_parse_bison_filename(nft, filename, &msgs, &nft->cmds);
 	if (rc)
 		goto err;
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &nft->cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	list_for_each_entry_safe(cmd, next, &cmds, list) {
+	list_for_each_entry_safe(cmd, next, &nft->cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
-- 
2.21.0

[nft PATCH v5 05/10] src: Make {table,chain}_not_found() public

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h | 3 +++
 src/evaluate.c | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index 8e70c129fcce0..61aa040a2e891 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -646,4 +646,7 @@ struct timeout_protocol {
 extern struct timeout_protocol timeout_protocol[IPPROTO_MAX];
 extern int timeout_str2num(uint16_t l4proto, struct timeout_state *ts);
 
+int table_not_found(struct eval_ctx *ctx);
+int chain_not_found(struct eval_ctx *ctx);
+
 #endif /* NFTABLES_RULE_H */
diff --git a/src/evaluate.c b/src/evaluate.c
index 55fb3b6131e04..09bb1fd37a301 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -166,7 +166,7 @@ static struct table *table_lookup_global(struct eval_ctx *ctx)
 	return table;
 }
 
-static int table_not_found(struct eval_ctx *ctx)
+int table_not_found(struct eval_ctx *ctx)
 {
 	struct table *table;
 
@@ -181,7 +181,7 @@ static int table_not_found(struct eval_ctx *ctx)
 			 family2str(table->handle.family));
 }
 
-static int chain_not_found(struct eval_ctx *ctx)
+int chain_not_found(struct eval_ctx *ctx)
 {
 	const struct table *table;
 	struct chain *chain;
-- 
2.21.0

[nft PATCH v5 06/10] src: Restore local entries after cache update

[Phil Sutter]

When batching up multiple commands, one may run into a situation where
the current command requires a cache update while the previous ones
didn't and that causes objects added by previous commands to be removed
from cache. If the current or any following command references any of
these objects, the command is rejected.

Resolve this by copying Florian's solution from iptables-nft: After
droping the whole cache and populating it again with entries fetched
from kernel, use the current list of commands to restore local entries
again.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v2:
- Move cache_add_commands() and called helper functions to a better
  place, it calls cache_is_complete().
- Add error handling to cache_add_commands() and helper functions. This
  should cover the case where required ruleset parts have disappeared
  from kernel. Error reporting makes use of {table,chain}_not_found(),
  so prepare a struct eval_ctx which also serves fine for passing
  pointers around.
- Drop cache bug workaround in tests/json_echo.

Changes since v1:
- Don't add anonymous sets to cache when restoring, as suggested by Eric
  Garver.
---
 src/rule.c                  | 83 ++++++++++++++++++++++++++++++++++++-
 tests/json_echo/run-test.py |  6 +--
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index f6ef1f6b0addd..a734ec6097b16 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -244,6 +244,87 @@ static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
 	return genid && genid == cache->genid;
 }
 
+static int cache_add_table_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+
+	if (table_lookup(&ectx->cmd->handle, &ectx->nft->cache))
+		return 0;
+
+	if (!ectx->cmd->table) {
+		table = table_alloc();
+		handle_merge(&table->handle, &ectx->cmd->handle);
+		table_add_hash(table, &ectx->nft->cache);
+	} else {
+		table_add_hash(table_get(ectx->cmd->table), &ectx->nft->cache);
+	}
+	return 0;
+}
+
+static int cache_add_chain_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+	struct chain *chain;
+
+	table = table_lookup(&ectx->cmd->handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	if (!ectx->cmd->chain) {
+		if (!chain_lookup(table, &ectx->cmd->handle)) {
+			chain = chain_alloc(NULL);
+			handle_merge(&chain->handle, &ectx->cmd->handle);
+			chain_add_hash(chain, table);
+		}
+	} else if (!chain_lookup(table, &ectx->cmd->chain->handle)) {
+		chain_add_hash(chain_get(ectx->cmd->chain), table);
+	}
+	return 0;
+}
+
+static int cache_add_set_cmd(struct eval_ctx *ectx)
+{
+	struct table *table;
+
+	table = table_lookup(&ectx->cmd->handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	if (!set_lookup(table, ectx->cmd->set->handle.set.name))
+		set_add_hash(set_get(ectx->cmd->set), table);
+	return 0;
+}
+
+static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
+{
+	struct eval_ctx ectx = {
+		.nft = nft,
+		.msgs = msgs,
+	};
+	int ret = 0;
+
+	list_for_each_entry(ectx.cmd, &nft->cmds, list) {
+		switch (ectx.cmd->obj) {
+		case CMD_OBJ_TABLE:
+			ret = cache_add_table_cmd(&ectx);
+			break;
+		case CMD_OBJ_CHAIN:
+			ret = cache_add_chain_cmd(&ectx);
+			break;
+		case CMD_OBJ_SET:
+			if (ectx.cmd->set->flags & NFT_SET_ANONYMOUS)
+				continue;
+			ret = cache_add_set_cmd(&ectx);
+			break;
+		default:
+			break;
+		}
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 int cache_update(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 {
 	uint16_t genid;
@@ -278,7 +359,7 @@ replay:
 	}
 	cache->genid = genid;
 	cache->cmd = cmd;
-	return 0;
+	return cache_add_commands(nft, msgs);
 }
 
 static void __cache_flush(struct list_head *table_list)
diff --git a/tests/json_echo/run-test.py b/tests/json_echo/run-test.py
index dd7797fb6f041..a636d5f247702 100755
--- a/tests/json_echo/run-test.py
+++ b/tests/json_echo/run-test.py
@@ -271,12 +271,10 @@ add_quota["add"]["quota"]["name"] = "q"
 do_flush()
 
 print("doing multi add")
-# XXX: Add table separately, otherwise this triggers cache bug
-out = do_command(add_table)
-thandle = get_handle(out, add_table["add"])
-add_multi = [ add_chain, add_set, add_rule ]
+add_multi = [ add_table, add_chain, add_set, add_rule ]
 out = do_command(add_multi)
 
+thandle = get_handle(out, add_table["add"])
 chandle = get_handle(out, add_chain["add"])
 shandle = get_handle(out, add_set["add"])
 rhandle = get_handle(out, add_rule["add"])
-- 
2.21.0

[nft PATCH v5 07/10] rule: Introduce rule_lookup_by_index()

[Phil Sutter]

In contrast to rule_lookup(), this function returns a chain's rule at a
given index instead of by handle.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h |  2 ++
 src/rule.c     | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/rule.h b/include/rule.h
index 61aa040a2e891..a7dd042d60e3f 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -260,6 +260,8 @@ extern struct rule *rule_get(struct rule *rule);
 extern void rule_free(struct rule *rule);
 extern void rule_print(const struct rule *rule, struct output_ctx *octx);
 extern struct rule *rule_lookup(const struct chain *chain, uint64_t handle);
+extern struct rule *rule_lookup_by_index(const struct chain *chain,
+					 uint64_t index);
 
 /**
  * struct set - nftables set
diff --git a/src/rule.c b/src/rule.c
index a734ec6097b16..4270fc7a9cd92 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -719,6 +719,17 @@ struct rule *rule_lookup(const struct chain *chain, uint64_t handle)
 	return NULL;
 }
 
+struct rule *rule_lookup_by_index(const struct chain *chain, uint64_t index)
+{
+	struct rule *rule;
+
+	list_for_each_entry(rule, &chain->rules, list) {
+		if (!--index)
+			return rule;
+	}
+	return NULL;
+}
+
 struct scope *scope_init(struct scope *scope, const struct scope *parent)
 {
 	scope->parent = parent;
-- 
2.21.0

[nft PATCH v5 08/10] src: Make cache_is_complete() public

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/rule.h | 1 +
 src/rule.c     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/rule.h b/include/rule.h
index a7dd042d60e3f..aa8881d375b96 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -638,6 +638,7 @@ extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 extern void cache_flush(struct nft_ctx *ctx, enum cmd_ops cmd,
 			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);
 
 struct timeout_protocol {
 	uint32_t array_size;
diff --git a/src/rule.c b/src/rule.c
index 4270fc7a9cd92..8343614a9d9c5 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -234,7 +234,7 @@ static int cache_completeness(enum cmd_ops cmd)
 	return 1;
 }
 
-static bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd)
+bool cache_is_complete(struct nft_cache *cache, enum cmd_ops cmd)
 {
 	return cache_completeness(cache->cmd) >= cache_completeness(cmd);
 }
-- 
2.21.0

[nft PATCH v5 09/10] include: Collect __stmt_binary_error() wrapper macros

[Phil Sutter]

At least cmd_error() is useful outside of evaluate.c, so collect all
these macros into erec.h.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/erec.h | 6 ++++++
 src/evaluate.c | 7 -------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/erec.h b/include/erec.h
index 79a162902304b..fc512a622947f 100644
--- a/include/erec.h
+++ b/include/erec.h
@@ -75,5 +75,11 @@ extern int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
 	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
 #define stmt_binary_error(ctx, s1, s2, fmt, args...) \
 	__stmt_binary_error(ctx, &(s1)->location, &(s2)->location, fmt, ## args)
+#define chain_error(ctx, s1, fmt, args...) \
+	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
+#define monitor_error(ctx, s1, fmt, args...) \
+	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
+#define cmd_error(ctx, loc, fmt, args...) \
+	__stmt_binary_error(ctx, loc, NULL, fmt, ## args)
 
 #endif /* NFTABLES_EREC_H */
diff --git a/src/evaluate.c b/src/evaluate.c
index 09bb1fd37a301..358f5b7152634 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -42,13 +42,6 @@ static const char * const byteorder_names[] = {
 	[BYTEORDER_BIG_ENDIAN]		= "big endian",
 };
 
-#define chain_error(ctx, s1, fmt, args...) \
-	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
-#define monitor_error(ctx, s1, fmt, args...) \
-	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
-#define cmd_error(ctx, loc, fmt, args...) \
-	__stmt_binary_error(ctx, loc, NULL, fmt, ## args)
-
 static int __fmtstring(3, 4) set_error(struct eval_ctx *ctx,
 				       const struct set *set,
 				       const char *fmt, ...)
-- 
2.21.0

[nft PATCH v5 10/10] src: Support intra-transaction rule references

[Phil Sutter]

A rule may be added before or after another one using index keyword. To
support for the other rule being added within the same batch, one has to
make use of NFTNL_RULE_ID and NFTNL_RULE_POSITION_ID attributes. This
patch does just that among a few more crucial things:

* Fetch full kernel ruleset upon encountering a rule which references
  another one. Any earlier rule add/insert commands are then restored by
  cache_add_commands().

* Avoid cache updates for rules not referencing another one by index,
  but add them immediately to cache if cache is already complete -
  otherwise they are not inserted if a later one with index reference is
  added.

* Reduce rule_translate_index() to its core code which is the actual
  linking of rules and consequently rename the function. The removed
  bits are pulled into the calling rule_evaluate() to reduce code
  duplication in between cache inserts with and without rule reference.

* Pass the current command op to rule_evaluate() as indicator whether to
  insert before or after a referenced rule or at beginning or end of
  chain in cache. Exploit this from chain_evaluate() to avoid adding
  the chain's rules a second time.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v4:
- Move whole rule reference finding and linking code into
  rule_cache_update() to simplify callers.
- Move rule_cache_update() to right before cache_add_rule_cmd().
- Fix the code regarding which handle and which rule pointer to use,
  resolves The segfault Eric mentioned.
- Skip cache updates for rule delete and replace commands, this restores
  old behaviour regarding "guessed" rule handles.
- Also simplify skp logic itself: If cache is complete, add all rules
  while evaluating them. Previous cache completeness level lowering was
  problematic after cache flush command.
- Add additional test case for delete/replace commands.

Changes since v1:
- Move rule list manipulation into a dedicated function
  rule_cache_update().
- Restore old performance for simple commands by fetching a full rule
  cache only if the currently evaluated rule references another one.
- Extend 0024rule_0 test a bit to make sure things work with interactive
  nft also.
---
 include/rule.h                                |  4 +
 src/evaluate.c                                | 60 ++++--------
 src/mnl.c                                     |  4 +
 src/rule.c                                    | 98 +++++++++++++++++++
 .../shell/testcases/cache/0003_cache_update_0 |  7 ++
 tests/shell/testcases/transactions/0024rule_0 | 17 ++++
 tests/shell/testcases/transactions/0025rule_0 | 21 ++++
 .../transactions/dumps/0024rule_0.nft         |  8 ++
 .../transactions/dumps/0025rule_0.nft         |  6 ++
 9 files changed, 182 insertions(+), 43 deletions(-)
 create mode 100755 tests/shell/testcases/transactions/0024rule_0
 create mode 100755 tests/shell/testcases/transactions/0025rule_0
 create mode 100644 tests/shell/testcases/transactions/dumps/0024rule_0.nft
 create mode 100644 tests/shell/testcases/transactions/dumps/0025rule_0.nft

diff --git a/include/rule.h b/include/rule.h
index aa8881d375b96..87ea8b8a25b96 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -73,6 +73,8 @@ struct handle {
 	struct position_spec	position;
 	struct position_spec	index;
 	uint32_t		set_id;
+	uint32_t		rule_id;
+	uint32_t		position_id;
 };
 
 extern void handle_merge(struct handle *dst, const struct handle *src);
@@ -639,6 +641,8 @@ extern void cache_flush(struct nft_ctx *ctx, enum cmd_ops cmd,
 			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);
+extern int rule_cache_update(struct eval_ctx *ectx, struct rule *rule,
+			     enum cmd_ops op);
 
 struct timeout_protocol {
 	uint32_t array_size;
diff --git a/src/evaluate.c b/src/evaluate.c
index 358f5b7152634..4fa4a0c73e9ca 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3177,46 +3177,11 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
 	return 0;
 }
 
-/* Convert rule's handle.index into handle.position. */
-static int rule_translate_index(struct eval_ctx *ctx, struct rule *rule)
-{
-	struct table *table;
-	struct chain *chain;
-	uint64_t index = 0;
-	struct rule *r;
-	int ret;
-
-	/* update cache with CMD_LIST so that rules are fetched, too */
-	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
-	if (ret < 0)
-		return ret;
-
-	table = table_lookup(&rule->handle, &ctx->nft->cache);
-	if (!table)
-		return table_not_found(ctx);
-
-	chain = chain_lookup(table, &rule->handle);
-	if (!chain)
-		return chain_not_found(ctx);
-
-	list_for_each_entry(r, &chain->rules, list) {
-		if (++index < rule->handle.index.id)
-			continue;
-		rule->handle.position.id = r->handle.handle.id;
-		rule->handle.position.location = rule->handle.index.location;
-		break;
-	}
-	if (!rule->handle.position.id)
-		return cmd_error(ctx, &rule->handle.index.location,
-				"Could not process rule: %s",
-				strerror(ENOENT));
-	return 0;
-}
-
-static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
+static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule, enum cmd_ops op)
 {
 	struct stmt *stmt, *tstmt = NULL;
 	struct error_record *erec;
+	int ret;
 
 	proto_ctx_init(&ctx->pctx, rule->handle.family, ctx->nft->debug_mask);
 	memset(&ctx->ectx, 0, sizeof(ctx->ectx));
@@ -3241,11 +3206,20 @@ static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
 		return -1;
 	}
 
-	if (rule->handle.index.id &&
-	    rule_translate_index(ctx, rule))
-		return -1;
+	/* Cache update may be avoided if the rule doesn't reference another
+	 * one by index: If later rules containing a reference by index will
+	 * cause a cache update, the current one will be inserted from command
+	 * list. */
+	if (!rule->handle.index.id &&
+	    !cache_is_complete(&ctx->nft->cache, CMD_LIST))
+			return 0;
 
-	return 0;
+	/* update cache with CMD_LIST so that rules are fetched, too */
+	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
+	if (ret < 0)
+		return ret;
+
+	return rule_cache_update(ctx, rule, op);
 }
 
 static uint32_t str2hooknum(uint32_t family, const char *hook)
@@ -3326,7 +3300,7 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 
 	list_for_each_entry(rule, &chain->rules, list) {
 		handle_merge(&rule->handle, &chain->handle);
-		if (rule_evaluate(ctx, rule) < 0)
+		if (rule_evaluate(ctx, rule, CMD_INVALID) < 0)
 			return -1;
 	}
 	return 0;
@@ -3423,7 +3397,7 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 		return set_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
 		handle_merge(&cmd->rule->handle, &cmd->handle);
-		return rule_evaluate(ctx, cmd->rule);
+		return rule_evaluate(ctx, cmd->rule, cmd->op);
 	case CMD_OBJ_CHAIN:
 		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
 		if (ret < 0)
diff --git a/src/mnl.c b/src/mnl.c
index 579210e4736a4..a38a4b4d3dd51 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -373,6 +373,10 @@ int mnl_nft_rule_add(struct netlink_ctx *ctx, const struct cmd *cmd,
 	nftnl_rule_set_str(nlr, NFTNL_RULE_CHAIN, h->chain.name);
 	if (h->position.id)
 		nftnl_rule_set_u64(nlr, NFTNL_RULE_POSITION, h->position.id);
+	if (h->rule_id)
+		nftnl_rule_set_u32(nlr, NFTNL_RULE_ID, h->rule_id);
+	if (h->position_id)
+		nftnl_rule_set_u32(nlr, NFTNL_RULE_POSITION_ID, h->position_id);
 
 	netlink_linearize_rule(ctx, nlr, rule);
 	nlh = nftnl_nlmsg_build_hdr(nftnl_batch_buffer(ctx->batch),
diff --git a/src/rule.c b/src/rule.c
index 8343614a9d9c5..699efca2a6e11 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -24,6 +24,7 @@
 #include <mnl.h>
 #include <misspell.h>
 #include <json.h>
+#include <erec.h>
 
 #include <libnftnl/common.h>
 #include <libnftnl/ruleset.h>
@@ -295,6 +296,98 @@ static int cache_add_set_cmd(struct eval_ctx *ectx)
 	return 0;
 }
 
+/* make src point at dst, either via handle.position or handle.position_id */
+static void link_rules(struct rule *src, struct rule *dst)
+{
+	static uint32_t ref_id = 0;
+
+	if (dst->handle.handle.id) {
+		/* dst is in kernel, make src reference it by handle */
+		src->handle.position.id = dst->handle.handle.id;
+		src->handle.position.location = src->handle.index.location;
+		return;
+	}
+
+	/* dst is not in kernel, make src reference it by per-transaction ID */
+	if (!dst->handle.rule_id)
+		dst->handle.rule_id = ++ref_id;
+	src->handle.position_id = dst->handle.rule_id;
+}
+
+int rule_cache_update(struct eval_ctx *ectx, struct rule *rule, enum cmd_ops op)
+{
+	struct handle *handle = rule ? &rule->handle : &ectx->cmd->handle;
+	struct rule *ref = NULL;
+	struct table *table;
+	struct chain *chain;
+
+	if (!rule)
+		rule = ectx->cmd->rule;
+
+	table = table_lookup(handle, &ectx->nft->cache);
+	if (!table)
+		return table_not_found(ectx);
+
+	chain = chain_lookup(table, handle);
+	if (!chain)
+		return chain_not_found(ectx);
+
+	if (handle->index.id) {
+		ref = rule_lookup_by_index(chain, handle->index.id);
+		if (!ref)
+			return cmd_error(ectx, &handle->index.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+
+		link_rules(rule, ref);
+	} else if (handle->handle.id) {
+		ref = rule_lookup(chain, handle->handle.id);
+		if (!ref)
+			return cmd_error(ectx, &handle->handle.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+	} else if (handle->position.id) {
+		ref = rule_lookup(chain, handle->position.id);
+		if (!ref)
+			return cmd_error(ectx, &handle->position.location,
+					 "Could not process rule: %s",
+					 strerror(ENOENT));
+	}
+
+	switch (op) {
+	case CMD_INSERT:
+		rule_get(rule);
+		if (ref)
+			list_add_tail(&rule->list, &ref->list);
+		else
+			list_add(&rule->list, &chain->rules);
+		break;
+	case CMD_ADD:
+		rule_get(rule);
+		if (ref)
+			list_add(&rule->list, &ref->list);
+		else
+			list_add_tail(&rule->list, &chain->rules);
+		break;
+	case CMD_REPLACE:
+		rule_get(rule);
+		list_add(&rule->list, &ref->list);
+		/* fall through */
+	case CMD_DELETE:
+		list_del(&ref->list);
+		rule_free(ref);
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static int cache_add_rule_cmd(struct eval_ctx *ectx)
+{
+	return rule_cache_update(ectx, ectx->cmd->rule, ectx->cmd->op);
+}
+
 static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
 {
 	struct eval_ctx ectx = {
@@ -316,6 +409,11 @@ static int cache_add_commands(struct nft_ctx *nft, struct list_head *msgs)
 				continue;
 			ret = cache_add_set_cmd(&ectx);
 			break;
+		case CMD_OBJ_RULE:
+			if (!cache_is_complete(&nft->cache, CMD_LIST))
+				continue;
+			ret = cache_add_rule_cmd(&ectx);
+			break;
 		default:
 			break;
 		}
diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
index fa9b5df380a41..05edc9c7c33eb 100755
--- a/tests/shell/testcases/cache/0003_cache_update_0
+++ b/tests/shell/testcases/cache/0003_cache_update_0
@@ -34,6 +34,9 @@ EOF
 # add rule ip t4 c meta l4proto icmp accept -> rule to reference in next step
 # add rule ip t4 c index 0 drop -> index 0 is not found due to rule cache not
 #                                  being updated
+# add rule ip t4 c index 2 drop -> index 2 is not found due to igmp rule being
+#                                  in same transaction and therefore not having
+#                                  an allocated handle
 $NFT -i >/dev/null <<EOF
 add table ip t4; add chain ip t4 c
 add rule ip t4 c meta l4proto icmp accept
@@ -41,3 +44,7 @@ EOF
 $NFT -f - >/dev/null <<EOF
 add rule ip t4 c index 0 drop
 EOF
+$NFT -f - >/dev/null <<EOF
+add rule ip t4 c meta l4proto igmp accept
+add rule ip t4 c index 2 drop
+EOF
diff --git a/tests/shell/testcases/transactions/0024rule_0 b/tests/shell/testcases/transactions/0024rule_0
new file mode 100755
index 0000000000000..4c1ac41db3b47
--- /dev/null
+++ b/tests/shell/testcases/transactions/0024rule_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+RULESET="flush ruleset
+add table x
+add chain x y
+add rule x y accept comment rule1
+add rule x y accept comment rule4
+add rule x y index 0 accept comment rule2
+insert rule x y index 2 accept comment rule3"
+
+$NFT -f - <<< "$RULESET" && \
+	$NFT -f - <<< "$RULESET" && \
+	echo "$RULESET" | tr '\n' ';' | $NFT -i >/dev/null && \
+	exit 0
+echo "E: intra-transaction rule reference failed"
+exit 1
+
diff --git a/tests/shell/testcases/transactions/0025rule_0 b/tests/shell/testcases/transactions/0025rule_0
new file mode 100755
index 0000000000000..d72d5cfcc75d4
--- /dev/null
+++ b/tests/shell/testcases/transactions/0025rule_0
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+# make sure stored delete/replace rule commands are correctly applied
+
+set -e
+
+$NFT -f - <<EOF
+flush ruleset
+table x {
+	chain y {
+		accept
+		log
+	}
+}
+EOF
+
+$NFT -f - <<EOF
+replace rule x y handle 2 log
+delete rule x y handle 3
+add rule x y index 0 drop
+EOF
diff --git a/tests/shell/testcases/transactions/dumps/0024rule_0.nft b/tests/shell/testcases/transactions/dumps/0024rule_0.nft
new file mode 100644
index 0000000000000..7860ff654c5e2
--- /dev/null
+++ b/tests/shell/testcases/transactions/dumps/0024rule_0.nft
@@ -0,0 +1,8 @@
+table ip x {
+	chain y {
+		accept comment "rule1"
+		accept comment "rule2"
+		accept comment "rule3"
+		accept comment "rule4"
+	}
+}
diff --git a/tests/shell/testcases/transactions/dumps/0025rule_0.nft b/tests/shell/testcases/transactions/dumps/0025rule_0.nft
new file mode 100644
index 0000000000000..dcb61ae65fbde
--- /dev/null
+++ b/tests/shell/testcases/transactions/dumps/0025rule_0.nft
@@ -0,0 +1,6 @@
+table ip x {
+	chain y {
+		log
+		drop
+	}
+}
-- 
2.21.0

Re: [nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Pablo Neira Ayuso]

Hi Phil,

Thanks a lot for working on this, I have a few comments.

On Tue, Jun 04, 2019 at 07:31:48PM +0200, Phil Sutter wrote:
> Next round of combined cache update fix and intra-transaction rule
> reference support.

Patch 1 looks good.

> Patch 2 is new, it avoids accidential cache updates when committing a
> transaction containing flush ruleset command and kernel ruleset has
> changed meanwhile.

Patch 2: Could you provide an example scenario for this new patch?

> Patch 3 is also new: If a transaction fails in kernel, local cache is
> incorrect - drop it.

Patch 3 looks good!

Regarding patches 4, 5 and 6. I think we can skip them if we follow
the approach described by [1], given there is only one single
cache_update() call after that patchset, we don't need to do the
"Restore local entries after cache update" logic.

[1] https://marc.info/?l=netfilter-devel&m=155975322308042&w=2

> Patch 9 is a new requirement for patch 10 due to relocation of new
> functions.
> 
> Patch 10 was changed, changelog included.

Patch 10 looks fine. However, as said, I would like to avoid the patch
dependencies 4, 5 and 6, they are adding more cache_update() calls and
I think we should go in the opposite direction to end up with a more
simple approach.

Thanks!

Re: [nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Phil Sutter]

Hi Pablo,

On Wed, Jun 05, 2019 at 07:05:41PM +0200, Pablo Neira Ayuso wrote:
> Thanks a lot for working on this, I have a few comments.
> 
> On Tue, Jun 04, 2019 at 07:31:48PM +0200, Phil Sutter wrote:
> > Next round of combined cache update fix and intra-transaction rule
> > reference support.
> 
> Patch 1 looks good.
> 
> > Patch 2 is new, it avoids accidential cache updates when committing a
> > transaction containing flush ruleset command and kernel ruleset has
> > changed meanwhile.
> 
> Patch 2: Could you provide an example scenario for this new patch?

Given the sample nft input:

| flush ruleset
| add table t
| add chain t c
| ...

First command causes call of cache_flush(), which updates cache->genid
from kernel. Third command causes call to cache_update(). If in between
these two another process has committed a change to kernel, kernel's
genid has bumped and cache_update() will populate the cache because
cache_is_updated() returns false.

From the top of my head I don't see where these stray cache entries
would lead to spurious errors but it is clearly false behaviour in cache
update logic.

> > Patch 3 is also new: If a transaction fails in kernel, local cache is
> > incorrect - drop it.
> 
> Patch 3 looks good!
> 
> Regarding patches 4, 5 and 6. I think we can skip them if we follow
> the approach described by [1], given there is only one single
> cache_update() call after that patchset, we don't need to do the
> "Restore local entries after cache update" logic.
> 
> [1] https://marc.info/?l=netfilter-devel&m=155975322308042&w=2

The question is how we want to treat rule reference by index if cache is
outdated. We could be nice and recalculate the rule reference which
would require to rebuild the cache including own additions. We could
also just refer to the warning in nft.8 and do nothing about it. :)

> > Patch 9 is a new requirement for patch 10 due to relocation of new
> > functions.
> > 
> > Patch 10 was changed, changelog included.
> 
> Patch 10 looks fine. However, as said, I would like to avoid the patch
> dependencies 4, 5 and 6, they are adding more cache_update() calls and
> I think we should go in the opposite direction to end up with a more
> simple approach.

OK, so how to proceed from here? I see you've based your patch series
upon an earlier version of mine. If you provide me with a clean version,
I could apply the index reference stuff on top. Or something else?

Cheers, Phil

Re: [nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Pablo Neira Ayuso]

On Wed, Jun 05, 2019 at 09:24:29PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, Jun 05, 2019 at 07:05:41PM +0200, Pablo Neira Ayuso wrote:
> > Thanks a lot for working on this, I have a few comments.
> > 
> > On Tue, Jun 04, 2019 at 07:31:48PM +0200, Phil Sutter wrote:
> > > Next round of combined cache update fix and intra-transaction rule
> > > reference support.
> > 
> > Patch 1 looks good.
> > 
> > > Patch 2 is new, it avoids accidential cache updates when committing a
> > > transaction containing flush ruleset command and kernel ruleset has
> > > changed meanwhile.
> > 
> > Patch 2: Could you provide an example scenario for this new patch?
> 
> Given the sample nft input:
> 
> | flush ruleset
> | add table t
> | add chain t c
> | ...
> 
> First command causes call of cache_flush(), which updates cache->genid
> from kernel. Third command causes call to cache_update(). If in between
> these two another process has committed a change to kernel, kernel's
> genid has bumped and cache_update() will populate the cache because
> cache_is_updated() returns false.
>
> From the top of my head I don't see where these stray cache entries
> would lead to spurious errors but it is clearly false behaviour in cache
> update logic.

Thanks for explaining, so this is resulting from lack of consistency
in the cache handling.

> > > Patch 3 is also new: If a transaction fails in kernel, local cache is
> > > incorrect - drop it.
> > 
> > Patch 3 looks good!
> > 
> > Regarding patches 4, 5 and 6. I think we can skip them if we follow
> > the approach described by [1], given there is only one single
> > cache_update() call after that patchset, we don't need to do the
> > "Restore local entries after cache update" logic.
> > 
> > [1] https://marc.info/?l=netfilter-devel&m=155975322308042&w=2
> 
> The question is how we want to treat rule reference by index if cache is
> outdated. We could be nice and recalculate the rule reference which
> would require to rebuild the cache including own additions. We could
> also just refer to the warning in nft.8 and do nothing about it. :)

We can set the generation ID of reference that is used for this batch,
the kernel then bails out with ERESTART so we can rebuild the cache.

However, I think the problem with the index is that it is unreliable
itself for concurrent updates. In that case the rule handle should be
used.

> > > Patch 9 is a new requirement for patch 10 due to relocation of new
> > > functions.
> > > 
> > > Patch 10 was changed, changelog included.
> > 
> > Patch 10 looks fine. However, as said, I would like to avoid the patch
> > dependencies 4, 5 and 6, they are adding more cache_update() calls and
> > I think we should go in the opposite direction to end up with a more
> > simple approach.
> 
> OK, so how to proceed from here? I see you've based your patch series
> upon an earlier version of mine. If you provide me with a clean version,
> I could apply the index reference stuff on top. Or something else?

That would be great.

I'm going to send a v2 for:

http://patchwork.ozlabs.org/patch/1110586/

addressing your feedback, then you can rebase on top.

Thanks!

Re: [nft PATCH v5 00/10] Cache update fix && intra-transaction rule references

[Pablo Neira Ayuso]

On Thu, Jun 06, 2019 at 10:36:12AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 05, 2019 at 09:24:29PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, Jun 05, 2019 at 07:05:41PM +0200, Pablo Neira Ayuso wrote:
> > > Thanks a lot for working on this, I have a few comments.
> > > 
> > > On Tue, Jun 04, 2019 at 07:31:48PM +0200, Phil Sutter wrote:
> > > > Next round of combined cache update fix and intra-transaction rule
> > > > reference support.
> > > 
> > > Patch 1 looks good.
> > > 
> > > > Patch 2 is new, it avoids accidential cache updates when committing a
> > > > transaction containing flush ruleset command and kernel ruleset has
> > > > changed meanwhile.
> > > 
> > > Patch 2: Could you provide an example scenario for this new patch?
> > 
> > Given the sample nft input:
> > 
> > | flush ruleset
> > | add table t
> > | add chain t c
> > | ...
> > 
> > First command causes call of cache_flush(), which updates cache->genid
> > from kernel. Third command causes call to cache_update(). If in between
> > these two another process has committed a change to kernel, kernel's
> > genid has bumped and cache_update() will populate the cache because
> > cache_is_updated() returns false.
> >
> > From the top of my head I don't see where these stray cache entries
> > would lead to spurious errors but it is clearly false behaviour in cache
> > update logic.
> 
> Thanks for explaining, so this is resulting from lack of consistency
> in the cache handling.

Now that there is a single cache_update() call, fetching a consistent
cache like iptables-nft is doing should not too hard, I think this
will be sufficient to fix this scenario.