[PATCH nft 0/3] fix crash bug during rule restore

[PATCH nft 0/3] fix crash bug during rule restore

[Florian Westphal]

https://bugzilla.netfilter.org/show_bug.cgi?id=1351 

nft -f <<EOF
flush ruleset       

table inet filter {
}
table inet filter {
      chain test {
        counter
    }
}
EOF

segfaults during error reporting.
First patch makes error handling more robust.
Second patch passes the right handle -- with above ruleset this
highlights "chain test" in the resulting error message.

Last patch skips rule cache updates for invalid op to restore
the 0.9.0 behaviour.

[PATCH nft 1/3] src: erec: fall back to internal location if its null

[Florian Westphal]

This should never happen (we should pass valid locations to the error
reporting functions), but in case we screw up we will segfault during
error reporting.

cat crash
table inet filter {
}
table inet filter {
      chain test {
        counter
    }
}
"nft -f crash" Now reports:
internal:0:0-0: Error: No such file or directory

... which is both bogus and useless, but better than crashing.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/erec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/erec.c b/src/erec.c
index c550a596b38c..28197924a82c 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -92,6 +92,9 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
 	FILE *f;
 	int l;
 
+	if (!indesc)
+		indesc = &internal_indesc;
+
 	switch (indesc->type) {
 	case INDESC_BUFFER:
 	case INDESC_CLI:
-- 
2.21.0

[PATCH nft 2/3] src: evaluate: don't rely on global chain ctx for error reporting

[Florian Westphal]

table inet filter {
}
table inet filter {
      chain test {
        counter
    }
}

will now report:
crash:7:13-16: Error: No such file or directory
      chain test {
            ^^^^
... which is still bogus, but we won't fallback to the 'internal'
location anymore and at least somehwat hint that there is a problem
with 'test' chain.

The error occurs because we're looking up the chain in the first
'table inet filter' instance, not the second.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 69b853f58722..b56932ccabcc 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -182,17 +182,17 @@ static int table_not_found(struct eval_ctx *ctx)
 			 family2str(table->handle.family));
 }
 
-static int chain_not_found(struct eval_ctx *ctx)
+static int chain_not_found(struct eval_ctx *ctx, struct handle *h)
 {
 	const struct table *table;
 	struct chain *chain;
 
-	chain = chain_lookup_fuzzy(&ctx->cmd->handle, &ctx->nft->cache, &table);
+	chain = chain_lookup_fuzzy(h, &ctx->nft->cache, &table);
 	if (chain == NULL)
-		return cmd_error(ctx, &ctx->cmd->handle.chain.location,
+		return cmd_error(ctx, &h->chain.location,
 				 "%s", strerror(ENOENT));
 
-	return cmd_error(ctx, &ctx->cmd->handle.chain.location,
+	return cmd_error(ctx, &h->chain.location,
 			 "%s; did you mean chain ‘%s’ in table %s ‘%s’?",
 			 strerror(ENOENT), chain->handle.chain.name,
 			 family2str(chain->handle.family),
@@ -3264,7 +3264,7 @@ static int rule_cache_update(struct eval_ctx *ctx, enum cmd_ops op)
 
 	chain = chain_lookup(table, &rule->handle);
 	if (!chain)
-		return chain_not_found(ctx);
+		return chain_not_found(ctx, &rule->handle);
 
 	if (rule->handle.index.id) {
 		ref = rule_lookup_by_index(chain, rule->handle.index.id);
@@ -3710,7 +3710,7 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 			return table_not_found(ctx);
 
 		if (chain_lookup(table, &cmd->handle) == NULL)
-			return chain_not_found(ctx);
+			return chain_not_found(ctx, &cmd->handle);
 
 		return 0;
 	case CMD_OBJ_QUOTA:
@@ -3843,7 +3843,7 @@ static int cmd_evaluate_rename(struct eval_ctx *ctx, struct cmd *cmd)
 			return table_not_found(ctx);
 
 		if (chain_lookup(table, &ctx->cmd->handle) == NULL)
-			return chain_not_found(ctx);
+			return chain_not_found(ctx, &ctx->cmd->handle);
 
 		break;
 	default:
-- 
2.21.0

[PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Florian Westphal]

This makes nft behave like 0.9.0 -- the ruleset

flush ruleset
table inet filter {
}
table inet filter {
      chain test {
        counter
    }
}

loads again without generating an error message.
I've added a test case for this, without this it will create an error,
and with a checkout of the 'fixes' tag we get crash.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                  |  3 +++
 tests/shell/testcases/cache/0003_cache_update_0 | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index b56932ccabcc..8c1c82abed4e 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3258,6 +3258,9 @@ static int rule_cache_update(struct eval_ctx *ctx, enum cmd_ops op)
 	struct table *table;
 	struct chain *chain;
 
+	if (op == CMD_INVALID)
+		return 0;
+
 	table = table_lookup(&rule->handle, &ctx->nft->cache);
 	if (!table)
 		return table_not_found(ctx);
diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
index 05edc9c7c33e..fb4b0e24c790 100755
--- a/tests/shell/testcases/cache/0003_cache_update_0
+++ b/tests/shell/testcases/cache/0003_cache_update_0
@@ -48,3 +48,15 @@ $NFT -f - >/dev/null <<EOF
 add rule ip t4 c meta l4proto igmp accept
 add rule ip t4 c index 2 drop
 EOF
+
+# Trigger a crash or rule restore error with nft 0.9.1
+$NFT -f - >/dev/null <<EOF
+flush ruleset
+table inet testfilter {
+}
+table inet testfilter {
+      chain test {
+        counter
+    }
+}
+EOF
-- 
2.21.0

Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 02:14:05AM +0200, Florian Westphal wrote:
> This should never happen (we should pass valid locations to the error
> reporting functions), but in case we screw up we will segfault during
> error reporting.
> 
> cat crash
> table inet filter {
> }
> table inet filter {
>       chain test {
>         counter
>     }
> }
> "nft -f crash" Now reports:
> internal:0:0-0: Error: No such file or directory
> 
> ... which is both bogus and useless, but better than crashing.

This should not ever happen, right?

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> This makes nft behave like 0.9.0 -- the ruleset
> 
> flush ruleset
> table inet filter {
> }
> table inet filter {
>       chain test {
>         counter
>     }
> }
> 
> loads again without generating an error message.
> I've added a test case for this, without this it will create an error,
> and with a checkout of the 'fixes' tag we get crash.
> 
> Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")

This one is causing the cache corruption, right?

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/evaluate.c                                  |  3 +++
>  tests/shell/testcases/cache/0003_cache_update_0 | 12 ++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index b56932ccabcc..8c1c82abed4e 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3258,6 +3258,9 @@ static int rule_cache_update(struct eval_ctx *ctx, enum cmd_ops op)
>  	struct table *table;
>  	struct chain *chain;
>  
> +	if (op == CMD_INVALID)
> +		return 0;
> +
>  	table = table_lookup(&rule->handle, &ctx->nft->cache);
>  	if (!table)
>  		return table_not_found(ctx);
> diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
> index 05edc9c7c33e..fb4b0e24c790 100755
> --- a/tests/shell/testcases/cache/0003_cache_update_0
> +++ b/tests/shell/testcases/cache/0003_cache_update_0
> @@ -48,3 +48,15 @@ $NFT -f - >/dev/null <<EOF
>  add rule ip t4 c meta l4proto igmp accept
>  add rule ip t4 c index 2 drop
>  EOF
> +
> +# Trigger a crash or rule restore error with nft 0.9.1
> +$NFT -f - >/dev/null <<EOF
> +flush ruleset
> +table inet testfilter {
> +}
> +table inet testfilter {
> +      chain test {
> +        counter
> +    }
> +}
> +EOF
> -- 
> 2.21.0
> 

Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jul 21, 2019 at 02:14:05AM +0200, Florian Westphal wrote:
> > This should never happen (we should pass valid locations to the error
> > reporting functions), but in case we screw up we will segfault during
> > error reporting.
> > 
> > cat crash
> > table inet filter {
> > }
> > table inet filter {
> >       chain test {
> >         counter
> >     }
> > }
> > "nft -f crash" Now reports:
> > internal:0:0-0: Error: No such file or directory
> > 
> > ... which is both bogus and useless, but better than crashing.
> 
> This should not ever happen, right?

It happens with current master plus above file.

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > This makes nft behave like 0.9.0 -- the ruleset
> > 
> > flush ruleset
> > table inet filter {
> > }
> > table inet filter {
> >       chain test {
> >         counter
> >     }
> > }
> > 
> > loads again without generating an error message.
> > I've added a test case for this, without this it will create an error,
> > and with a checkout of the 'fixes' tag we get crash.
> > 
> > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> 
> This one is causing the cache corruption, right?

There is no cache corruption.  This patch makes us enter a code
path that we did not take before.

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Sun, Jul 21, 2019 at 08:50:40PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > > This makes nft behave like 0.9.0 -- the ruleset
> > > 
> > > flush ruleset
> > > table inet filter {
> > > }
> > > table inet filter {
> > >       chain test {
> > >         counter
> > >     }
> > > }
> > > 
> > > loads again without generating an error message.
> > > I've added a test case for this, without this it will create an error,
> > > and with a checkout of the 'fixes' tag we get crash.
> > > 
> > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> > 
> > This one is causing the cache corruption, right?
> 
> There is no cache corruption.  This patch makes us enter a code
> path that we did not take before.

Sorry, I mean, this is a cache bug :-)

cache_flush() is cheating, it sets flags to CACHE_FULL, hence this
enters this codepath we dit not take before. This propagates from the
previous logic, as a workaround.

I made this patch, to skip the cache in case "flush ruleset" is
requested.

This breaks testcases/transactions/0024rule_0, which is a recent test
from Phil to check for intra-transaction references, I don't know yet
what makes this code unhappy with my changes.

Phil, would you help me have a look at what assumption breaks? Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2993 bytes --]

diff --git a/include/cache.h b/include/cache.h
index d3502a8a6039..526f6ca57f74 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -30,6 +30,7 @@ enum cache_level_flags {
 				  NFT_CACHE_CHAIN_BIT |
 				  NFT_CACHE_RULE_BIT,
 	NFT_CACHE_FULL		= __NFT_CACHE_MAX_BIT - 1,
+	NFT_CACHE_FLUSHED	= (1 << 31),
 };
 
 #endif /* _NFT_CACHE_H_ */
diff --git a/include/rule.h b/include/rule.h
index 67c3d3314953..d66e03456ad2 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -679,7 +679,6 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 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);
 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/cache.c b/src/cache.c
index e04ead85c830..2f16eee17780 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -72,6 +72,8 @@ static unsigned int evaluate_cache_flush(struct cmd *cmd, unsigned int flags)
 		flags |= NFT_CACHE_SET;
 		break;
 	case CMD_OBJ_RULESET:
+		flags |= NFT_CACHE_FLUSHED;
+		break;
 	default:
 		flags = NFT_CACHE_EMPTY;
 		break;
diff --git a/src/evaluate.c b/src/evaluate.c
index c6cc6ccad75d..b83c77ae4991 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3776,7 +3776,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULESET:
-		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 b957b4571249..d8a243342434 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -234,6 +234,11 @@ static bool cache_is_updated(struct nft_cache *cache, uint16_t genid)
 	return genid && genid == cache->genid;
 }
 
+static bool cache_is_flushed(struct nft_cache *cache)
+{
+	return cache->flags & NFT_CACHE_FLUSHED;
+}
+
 int cache_update(struct nft_ctx *nft, unsigned int flags, struct list_head *msgs)
 {
 	struct netlink_ctx ctx = {
@@ -255,6 +260,9 @@ replay:
 	if (cache->genid)
 		cache_release(cache);
 
+	if (cache_is_flushed(cache))
+		goto skip;
+
 	ret = cache_init(&ctx, flags);
 	if (ret < 0) {
 		cache_release(cache);
@@ -270,7 +278,7 @@ replay:
 		cache_release(cache);
 		goto replay;
 	}
-
+skip:
 	cache->genid = genid;
 	cache->flags = flags;
 	return 0;
@@ -286,20 +294,6 @@ static void __cache_flush(struct list_head *table_list)
 	}
 }
 
-void cache_flush(struct nft_ctx *nft, struct list_head *msgs)
-{
-	struct netlink_ctx ctx = {
-		.list		= LIST_HEAD_INIT(ctx.list),
-		.nft		= nft,
-		.msgs		= msgs,
-	};
-	struct nft_cache *cache = &nft->cache;
-
-	__cache_flush(&cache->list);
-	cache->genid = mnl_genid_get(&ctx);
-	cache->flags = NFT_CACHE_FULL;
-}
-
 void cache_release(struct nft_cache *cache)
 {
 	__cache_flush(&cache->list);

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Phil Sutter]

On Mon, Jul 22, 2019 at 11:25:56PM +0200, Pablo Neira Ayuso wrote:
> On Sun, Jul 21, 2019 at 08:50:40PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > > > This makes nft behave like 0.9.0 -- the ruleset
> > > > 
> > > > flush ruleset
> > > > table inet filter {
> > > > }
> > > > table inet filter {
> > > >       chain test {
> > > >         counter
> > > >     }
> > > > }
> > > > 
> > > > loads again without generating an error message.
> > > > I've added a test case for this, without this it will create an error,
> > > > and with a checkout of the 'fixes' tag we get crash.
> > > > 
> > > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > > > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> > > 
> > > This one is causing the cache corruption, right?
> > 
> > There is no cache corruption.  This patch makes us enter a code
> > path that we did not take before.
> 
> Sorry, I mean, this is a cache bug :-)
> 
> cache_flush() is cheating, it sets flags to CACHE_FULL, hence this
> enters this codepath we dit not take before. This propagates from the
> previous logic, as a workaround.
> 
> I made this patch, to skip the cache in case "flush ruleset" is
> requested.
> 
> This breaks testcases/transactions/0024rule_0, which is a recent test
> from Phil to check for intra-transaction references, I don't know yet
> what makes this code unhappy with my changes.
> 
> Phil, would you help me have a look at what assumption breaks? Thanks.

Sorry, I don't get it. What is happening in the first place? Florian
writes, a lookup happens in the wrong table and it seems
chain_evaluate() doesn't add the chain to cache. Yet I don't understand
why given patch fixes the problem.

Cheers, Phil

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Pablo Neira Ayuso]

On Tue, Jul 23, 2019 at 03:11:42PM +0200, Phil Sutter wrote:
> On Mon, Jul 22, 2019 at 11:25:56PM +0200, Pablo Neira Ayuso wrote:
> > On Sun, Jul 21, 2019 at 08:50:40PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> > > > > This makes nft behave like 0.9.0 -- the ruleset
> > > > > 
> > > > > flush ruleset
> > > > > table inet filter {
> > > > > }
> > > > > table inet filter {
> > > > >       chain test {
> > > > >         counter
> > > > >     }
> > > > > }
> > > > > 
> > > > > loads again without generating an error message.
> > > > > I've added a test case for this, without this it will create an error,
> > > > > and with a checkout of the 'fixes' tag we get crash.
> > > > > 
> > > > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351
> > > > > Fixes: e5382c0d08e3c ("src: Support intra-transaction rule references")
> > > > 
> > > > This one is causing the cache corruption, right?
> > > 
> > > There is no cache corruption.  This patch makes us enter a code
> > > path that we did not take before.
> > 
> > Sorry, I mean, this is a cache bug :-)
> > 
> > cache_flush() is cheating, it sets flags to CACHE_FULL, hence this
> > enters this codepath we dit not take before. This propagates from the
> > previous logic, as a workaround.
> > 
> > I made this patch, to skip the cache in case "flush ruleset" is
> > requested.
> > 
> > This breaks testcases/transactions/0024rule_0, which is a recent test
> > from Phil to check for intra-transaction references, I don't know yet
> > what makes this code unhappy with my changes.
> > 
> > Phil, would you help me have a look at what assumption breaks? Thanks.
> 
> Sorry, I don't get it. What is happening in the first place? Florian
> writes, a lookup happens in the wrong table and it seems
> chain_evaluate() doesn't add the chain to cache. Yet I don't understand
> why given patch fixes the problem.

Just sent a patch for this.

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 02:14:07AM +0200, Florian Westphal wrote:
> This makes nft behave like 0.9.0 -- the ruleset
> 
> flush ruleset
> table inet filter {
> }
> table inet filter {
>       chain test {
>         counter
>     }
> }
> 
> loads again without generating an error message.
> I've added a test case for this, without this it will create an error,
> and with a checkout of the 'fixes' tag we get crash.
> 
> Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1351

This one should fix this bugzilla:

http://git.netfilter.org/nftables/commit/?id=3ab02db5f836ae0cf9fe7fba616d7eb52139d537

more comments below.

[...]
> diff --git a/tests/shell/testcases/cache/0003_cache_update_0 b/tests/shell/testcases/cache/0003_cache_update_0
> index 05edc9c7c33e..fb4b0e24c790 100755
> --- a/tests/shell/testcases/cache/0003_cache_update_0
> +++ b/tests/shell/testcases/cache/0003_cache_update_0
> @@ -48,3 +48,15 @@ $NFT -f - >/dev/null <<EOF
>  add rule ip t4 c meta l4proto igmp accept
>  add rule ip t4 c index 2 drop
>  EOF
> +
> +# Trigger a crash or rule restore error with nft 0.9.1
> +$NFT -f - >/dev/null <<EOF
> +flush ruleset
> +table inet testfilter {
> +}
> +table inet testfilter {
> +      chain test {
> +        counter
> +    }
> +}
> +EOF

I have applied this test as an separated patch, thanks Florian.

Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null

[Pablo Neira Ayuso]

On Sun, Jul 21, 2019 at 02:14:05AM +0200, Florian Westphal wrote:
> This should never happen (we should pass valid locations to the error
> reporting functions), but in case we screw up we will segfault during
> error reporting.
> 
> cat crash
> table inet filter {
> }
> table inet filter {
>       chain test {
>         counter
>     }
> }
> "nft -f crash" Now reports:
> internal:0:0-0: Error: No such file or directory
> 
> ... which is both bogus and useless, but better than crashing.

I'd suggest we add BUG() here, so we catch missing location
information via indesc == NULL. So we can fix the lack of it,
otherwise users will rely on internal, which is very limited.

Thanks.

Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This one should fix this bugzilla:
> 
> http://git.netfilter.org/nftables/commit/?id=3ab02db5f836ae0cf9fe7fba616d7eb52139d537

much better fix, thanks Pablo!