[nft PATCH v2 0/3] Resolve cache update woes

[nft PATCH v2 0/3] Resolve cache update woes

[Phil Sutter]

This series implements a fix for situations where a cache update removes
local (still uncommitted) items from cache leading to spurious errors
afterwards.

Changes since v1:
- As suggested by Eric, I took his patch and folded my enhancement
  (former patch 1) into his one.
- Changed patch 3 to include Eric's fix.

Eric Garver (1):
  src: update cache if cmd is more specific

Phil Sutter (2):
  libnftables: Keep list of commands in nft context
  src: Restore local entries after cache update

 include/nftables.h                            |  2 +
 src/libnftables.c                             | 21 ++---
 src/rule.c                                    | 93 +++++++++++++++++++
 .../shell/testcases/cache/0003_cache_update_0 | 14 +++
 4 files changed, 119 insertions(+), 11 deletions(-)

-- 
2.21.0

[nft PATCH v2 1/3] src: update cache if cmd is more specific

[Phil Sutter]

From: Eric Garver <eric@garver.life>

If we've done a partial fetch of the cache and the genid is the same the
cache update will be skipped without fetching the needed items. This
change flushes the cache if the new request is more specific than the
current cache - forcing a cache update which includes the needed items.

Introduces a simple scoring system which reflects how
cache_init_objects() looks at the current command to decide if it is
finished already or not. Then use that in cache_needs_more(): If current
command's score is higher than old command's, cache needs an update.

Fixes: 816d8c7659c1 ("Support 'add/insert rule index <IDX>'")
Signed-off-by: Eric Garver <eric@garver.life>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Folded former "src: Improve cache_needs_more() algorithm" patch into
  this one.
---
 include/nftables.h                            |  1 +
 src/rule.c                                    | 20 +++++++++++++++++++
 .../shell/testcases/cache/0003_cache_update_0 | 14 +++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/include/nftables.h b/include/nftables.h
index b17a16a4adef7..bb9bb2091716d 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -81,6 +81,7 @@ struct nft_cache {
 	uint16_t		genid;
 	struct list_head	list;
 	uint32_t		seqnum;
+	uint32_t		cmd;
 };
 
 struct mnl_socket;
diff --git a/src/rule.c b/src/rule.c
index dc75c7cd5fb08..17bf5bbbe680c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -220,6 +220,23 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
 	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)
+{
+	if (cmd == CMD_LIST)
+		return 3;
+	if (cmd != CMD_RESET)
+		return 2;
+	return 1;
+}
+
+static bool cache_needs_more(enum cmd_ops old_cmd, enum cmd_ops cmd)
+{
+	return cache_completeness(old_cmd) < cache_completeness(cmd);
+}
+
 int cache_update(struct nft_ctx *nft, enum cmd_ops cmd, struct list_head *msgs)
 {
 	uint16_t genid;
@@ -235,6 +252,8 @@ 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->genid && cache_needs_more(cache->cmd, cmd))
+		cache_release(cache);
 	if (genid && genid == cache->genid)
 		return 0;
 	if (cache->genid)
@@ -250,6 +269,7 @@ replay:
 		return -1;
 	}
 	cache->genid = genid;
+	cache->cmd = cmd;
 	return 0;
 }
 
diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
index deb45db2c43be..fa9b5df380a41 100755
--- a/tests/shell/testcases/cache/0003_cache_update_0
+++ b/tests/shell/testcases/cache/0003_cache_update_0
@@ -27,3 +27,17 @@ EOF
 $NFT -i >/dev/null <<EOF
 add table ip t3; add chain ip t c
 EOF
+
+# The following test exposes a problem with incremental cache update when
+# reading commands from a file that add a rule using the "index" keyword.
+#
+# 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
+$NFT -i >/dev/null <<EOF
+add table ip t4; add chain ip t4 c
+add rule ip t4 c meta l4proto icmp accept
+EOF
+$NFT -f - >/dev/null <<EOF
+add rule ip t4 c index 0 drop
+EOF
-- 
2.21.0

Re: [nft PATCH v2 1/3] src: update cache if cmd is more specific

[Pablo Neira Ayuso]

On Wed, May 22, 2019 at 09:44:04PM +0200, Phil Sutter wrote:
> From: Eric Garver <eric@garver.life>
> 
> If we've done a partial fetch of the cache and the genid is the same the
> cache update will be skipped without fetching the needed items. This
> change flushes the cache if the new request is more specific than the
> current cache - forcing a cache update which includes the needed items.
> 
> Introduces a simple scoring system which reflects how
> cache_init_objects() looks at the current command to decide if it is
> finished already or not. Then use that in cache_needs_more(): If current
> command's score is higher than old command's, cache needs an update.

Applied this one, thanks Phil and Eric.

[nft PATCH v2 2/3] 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 199dbc97b801c..f6ea668f6770a 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -143,6 +143,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;
@@ -342,7 +343,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);
 
@@ -381,7 +382,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;
 
@@ -389,17 +389,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);
 	}
@@ -421,7 +421,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);
@@ -433,17 +432,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

Re: [nft PATCH v2 2/3] libnftables: Keep list of commands in nft context

[Eric Garver]

On Wed, May 22, 2019 at 09:44:05PM +0200, Phil Sutter wrote:
> 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>
> ---

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

[nft PATCH v2 3/3] 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 v1:
- Don't add anonymous sets to cache when restoring, as suggested by Eric
  Garver.
---
 src/rule.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/src/rule.c b/src/rule.c
index 17bf5bbbe680c..d1a86ea9fadd6 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -220,6 +220,78 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
 	return 0;
 }
 
+static void cache_add_set_cmd(struct nft_ctx *nft, struct cmd *cmd)
+{
+	struct table *table;
+
+	table = table_lookup(&cmd->handle, &nft->cache);
+	if (table == NULL)
+		return;
+
+	if (set_lookup(table, cmd->set->handle.set.name) == NULL)
+		set_add_hash(set_get(cmd->set), table);
+}
+
+static void cache_add_chain_cmd(struct nft_ctx *nft, struct cmd *cmd)
+{
+	struct table *table;
+	struct chain *chain;
+
+	table = table_lookup(&cmd->handle, &nft->cache);
+	if (table == NULL)
+		return;
+
+	if (cmd->chain == NULL) {
+		if (chain_lookup(table, &cmd->handle) == NULL) {
+			chain = chain_alloc(NULL);
+			handle_merge(&chain->handle, &cmd->handle);
+			chain_add_hash(chain, table);
+		}
+		return;
+	}
+	if (chain_lookup(table, &cmd->chain->handle) == NULL)
+		chain_add_hash(chain_get(cmd->chain), table);
+}
+
+static void cache_add_table_cmd(struct nft_ctx *nft, struct cmd *cmd)
+{
+	struct table *table;
+
+	if (table_lookup(&cmd->handle, &nft->cache))
+		return;
+
+	if (cmd->table == NULL) {
+		table = table_alloc();
+		handle_merge(&table->handle, &cmd->handle);
+		table_add_hash(table, &nft->cache);
+	} else {
+		table_add_hash(table_get(cmd->table), &nft->cache);
+	}
+}
+
+static void cache_add_commands(struct nft_ctx *nft)
+{
+	struct cmd *cmd;
+
+	list_for_each_entry(cmd, &nft->cmds, list) {
+		switch (cmd->obj) {
+		case CMD_OBJ_SET:
+			if (cmd->set->flags & NFT_SET_ANONYMOUS)
+				continue;
+			cache_add_set_cmd(nft, cmd);
+			break;
+		case CMD_OBJ_CHAIN:
+			cache_add_chain_cmd(nft, cmd);
+			break;
+		case CMD_OBJ_TABLE:
+			cache_add_table_cmd(nft, cmd);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 /* Return a "score" of how complete local cache will be if
  * cache_init_objects() ran for given cmd. Higher value
  * means more complete. */
@@ -270,6 +342,7 @@ replay:
 	}
 	cache->genid = genid;
 	cache->cmd = cmd;
+	cache_add_commands(nft);
 	return 0;
 }
 
-- 
2.21.0

Re: [nft PATCH v2 3/3] src: Restore local entries after cache update

[Eric Garver]

On Wed, May 22, 2019 at 09:44:06PM +0200, Phil Sutter wrote:
> 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>
> ---

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