[nft PATCH 0/3] Resolve cache update woes

[nft PATCH 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.

The series is based on Eric's "src: update cache if cmd is more
specific" patch which is still under review but resolves a distinct
problem from the one addressed in this series.

The first patch improves Eric's patch a bit. If he's OK with my change,
it may very well be just folded into his.

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

 include/nftables.h |  1 +
 src/libnftables.c  | 21 +++++------
 src/rule.c         | 91 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 96 insertions(+), 17 deletions(-)

-- 
2.21.0

[nft PATCH 1/3] src: Improve cache_needs_more() algorithm

[Phil Sutter]

The old logic wasn't optimal: If e.g. current command was CMD_RESET and
old command was CMD_LIST, cache was already fully populated but still
refreshed.

Introduce 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
commands score is higher than old command's, cache needs an update.

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

diff --git a/src/rule.c b/src/rule.c
index afe37cd90b1da..17bf5bbbe680c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -220,13 +220,21 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
 	return 0;
 }
 
-static int cache_needs_more(enum cmd_ops old_cmd, enum cmd_ops cmd)
+/* 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 && old_cmd != CMD_LIST)
-		return 1;
-	if (cmd == CMD_RESET && old_cmd != CMD_RESET)
-		return 1;
-	return 0;
+	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)
-- 
2.21.0

[nft PATCH 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

[nft PATCH 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>
---
 src/rule.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/src/rule.c b/src/rule.c
index 17bf5bbbe680c..4f015fc5354b7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -220,6 +220,76 @@ 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:
+			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 +340,7 @@ replay:
 	}
 	cache->genid = genid;
 	cache->cmd = cmd;
+	cache_add_commands(nft);
 	return 0;
 }
 
-- 
2.21.0

Re: [nft PATCH 1/3] src: Improve cache_needs_more() algorithm

[Eric Garver]

On Sat, May 18, 2019 at 01:00:31AM +0200, Phil Sutter wrote:
> The old logic wasn't optimal: If e.g. current command was CMD_RESET and
> old command was CMD_LIST, cache was already fully populated but still
> refreshed.
> 
> Introduce 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
> commands score is higher than old command's, cache needs an update.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/rule.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/rule.c b/src/rule.c
> index afe37cd90b1da..17bf5bbbe680c 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -220,13 +220,21 @@ static int cache_init(struct netlink_ctx *ctx, enum cmd_ops cmd)
>  	return 0;
>  }
>  
> -static int cache_needs_more(enum cmd_ops old_cmd, enum cmd_ops cmd)
> +/* 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 && old_cmd != CMD_LIST)
> -		return 1;
> -	if (cmd == CMD_RESET && old_cmd != CMD_RESET)
> -		return 1;
> -	return 0;
> +	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)
> -- 
> 2.21.0

LGTM. Feel free to take my patch and fold it into this series.

Re: [nft PATCH 0/3] Resolve cache update woes

[Pablo Neira Ayuso]

On Sat, May 18, 2019 at 01:00:30AM +0200, Phil Sutter wrote:
> This series implements a fix for situations where a cache update removes
> local (still uncommitted) items from cache leading to spurious errors
> afterwards.
> 
> The series is based on Eric's "src: update cache if cmd is more
> specific" patch which is still under review but resolves a distinct
> problem from the one addressed in this series.
> 
> The first patch improves Eric's patch a bit. If he's OK with my change,
> it may very well be just folded into his.

Thanks for your patchset. This fixing up any of the existing tests
that is broken, right?

Re: [nft PATCH 0/3] Resolve cache update woes

[Eric Garver]

Hi Phil,

On Sat, May 18, 2019 at 01:00:30AM +0200, Phil Sutter wrote:
> This series implements a fix for situations where a cache update removes
> local (still uncommitted) items from cache leading to spurious errors
> afterwards.
>
> The series is based on Eric's "src: update cache if cmd is more
> specific" patch which is still under review but resolves a distinct
> problem from the one addressed in this series.
>
> The first patch improves Eric's patch a bit. If he's OK with my change,
> it may very well be just folded into his.
>
> Phil Sutter (3):
>   src: Improve cache_needs_more() algorithm
>   libnftables: Keep list of commands in nft context
>   src: Restore local entries after cache update
>
>  include/nftables.h |  1 +
>  src/libnftables.c  | 21 +++++------
>  src/rule.c         | 91 +++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 96 insertions(+), 17 deletions(-)
>
> --
> 2.21.0

I've been testing this series. I found anonymous sets are mistakenly
free()d if a cache_release occurs.

I think this occurs because the table objects are added to the cache.
Sets (named and anonymous) hang off the table object. But only named set
objects are added to the cache. As such cache_release() causes a
table_free() and set_free() which frees the anonymous sets because
refcount == 1.

To illustrate this, I tried this HACK and no longer see issues.

diff --git a/src/rule.c b/src/rule.c
index 4f015fc5354b..3604bcbcfa7f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -350,6 +350,14 @@ static void __cache_flush(struct list_head *table_list)

        list_for_each_entry_safe(table, next, table_list, list) {
                list_del(&table->list);
+               if (table->refcnt == 1) {
+                       struct set *set, *nset;
+                       list_for_each_entry_safe(set, nset, &table->sets, list) {
+                               if (set->flags & NFT_SET_ANONYMOUS) {
+                                       set_get(set);
+                               }
+                       }
+               }
                table_free(table);
        }
 }

--->8---

Below is an example of the double free of the anonymous set.

(gdb) bt
#0  0x00007f894cf92491 in free () at /lib64/libc.so.6
#1  0x00007f893736a73b in expr_destroy (e=0x55be74bf28c0) at expression.c:79
#2  0x00007f893736a73b in expr_free (expr=0x55be74bf28c0) at expression.c:93
#3  0x00007f89373622b3 in set_free (set=0x55be74bc8970) at rule.c:424
#4  0x00007f8937363565 in table_free (table=0x55be74b22220) at rule.c:1232
#5  0x00007f8937363608 in __cache_flush (table_list=table_list@entry=0x55be74abc1e8) at rule.c:353
#6  0x00007f893736368d in cache_release (cache=cache@entry=0x55be74abc1e0) at rule.c:372
#7  0x00007f893736376e in cache_update (nft=0x55be74abc160, cmd=CMD_REPLACE, msgs=<optimized out>) at rule.c:330
#8  0x00007f8937363ce1 in do_command_add (ctx=0x7fff52b44c10, cmd=0x55be74b7fed0, excl=excl@entry=false) at rule.c:1573
#9  0x00007f8937365a27 in do_command (ctx=ctx@entry=0x7fff52b44c10, cmd=cmd@entry=0x55be74b7fed0) at rule.c:2566
#10 0x00007f89373871ad in nft_netlink (nft=nft@entry=0x55be74abc160, cmds=cmds@entry=0x55be74abc220, msgs=msgs@entry=0x7fff52b44c90, nf_sock=<optimized out>) at libnftables.c:45
#11 0x00007f8937387721 in nft_run_cmd_from_buffer (nft=0x55be74abc160, buf=<optimized out>) at libnftables.c:398
[...]


--->8---

It can also manifest as assertion failures in the expression due to
garbage data in the expr.

{"nftables": [
    {"metainfo": {"json_schema_version": 1}},
    {"add" : {"rule": {"family": "inet", "table": "firewalld", "chain": "filter_IN_public_allow", "expr": [{"match": {"left": {"payload": {"protocol": "dccp", "field": "dport"}}, "op": "==", "right": 222}}, {"match": {"left": {"ct": {"key": "state"}}, "op": "in", "right": {"set": ["new", "untracked"]}}}, {"accept": null}]}}}
]}

BUG: Unknown expression type 44
expression.c:1208: expr_ops: Assertion `0' failed.

Re: [nft PATCH 0/3] Resolve cache update woes

[Eric Garver]

On Tue, May 21, 2019 at 01:06:14PM -0400, Eric Garver wrote:
> Hi Phil,
> 
> On Sat, May 18, 2019 at 01:00:30AM +0200, Phil Sutter wrote:
> > This series implements a fix for situations where a cache update removes
> > local (still uncommitted) items from cache leading to spurious errors
> > afterwards.
> >
> > The series is based on Eric's "src: update cache if cmd is more
> > specific" patch which is still under review but resolves a distinct
> > problem from the one addressed in this series.
> >
> > The first patch improves Eric's patch a bit. If he's OK with my change,
> > it may very well be just folded into his.
> >
> > Phil Sutter (3):
> >   src: Improve cache_needs_more() algorithm
> >   libnftables: Keep list of commands in nft context
> >   src: Restore local entries after cache update
> >
> >  include/nftables.h |  1 +
> >  src/libnftables.c  | 21 +++++------
> >  src/rule.c         | 91 +++++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 96 insertions(+), 17 deletions(-)
> >
> > --
> > 2.21.0
> 
> I've been testing this series. I found anonymous sets are mistakenly
> free()d if a cache_release occurs.

Below is a real fix for this issue. After a cache update we need to skip adding
anonymous sets from the cmd list into the cache.

Phil, if you agree please fold this into your series.

diff --git a/src/rule.c b/src/rule.c
index 4f015fc5354b..94830b651925 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -224,6 +224,9 @@ static void cache_add_set_cmd(struct nft_ctx *nft, struct cmd *cmd)
 {
        struct table *table;
 
+       if (cmd->set->flags & NFT_SET_ANONYMOUS)
+               return;
+
        table = table_lookup(&cmd->handle, &nft->cache);
        if (table == NULL)
                return;