* [PATCH nft 0/3] fix crash bug during rule restore
@ 2019-07-21 0:14 Florian Westphal
2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Florian Westphal @ 2019-07-21 0:14 UTC (permalink / raw)
To: netfilter-devel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH nft 1/3] src: erec: fall back to internal location if its null 2019-07-21 0:14 [PATCH nft 0/3] fix crash bug during rule restore Florian Westphal @ 2019-07-21 0:14 ` Florian Westphal 2019-07-21 18:46 ` Pablo Neira Ayuso 2019-07-23 19:19 ` Pablo Neira Ayuso 2019-07-21 0:14 ` [PATCH nft 2/3] src: evaluate: don't rely on global chain ctx for error reporting Florian Westphal 2019-07-21 0:14 ` [PATCH nft 3/3] src: evaluate: return immediately if no op was requested Florian Westphal 2 siblings, 2 replies; 14+ messages in thread From: Florian Westphal @ 2019-07-21 0:14 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null 2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal @ 2019-07-21 18:46 ` Pablo Neira Ayuso 2019-07-21 18:50 ` Florian Westphal 2019-07-23 19:19 ` Pablo Neira Ayuso 1 sibling, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-21 18:46 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null 2019-07-21 18:46 ` Pablo Neira Ayuso @ 2019-07-21 18:50 ` Florian Westphal 0 siblings, 0 replies; 14+ messages in thread From: Florian Westphal @ 2019-07-21 18:50 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/3] src: erec: fall back to internal location if its null 2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal 2019-07-21 18:46 ` Pablo Neira Ayuso @ 2019-07-23 19:19 ` Pablo Neira Ayuso 1 sibling, 0 replies; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-23 19:19 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH nft 2/3] src: evaluate: don't rely on global chain ctx for error reporting 2019-07-21 0:14 [PATCH nft 0/3] fix crash bug during rule restore Florian Westphal 2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal @ 2019-07-21 0:14 ` Florian Westphal 2019-07-21 0:14 ` [PATCH nft 3/3] src: evaluate: return immediately if no op was requested Florian Westphal 2 siblings, 0 replies; 14+ messages in thread From: Florian Westphal @ 2019-07-21 0:14 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-21 0:14 [PATCH nft 0/3] fix crash bug during rule restore Florian Westphal 2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal 2019-07-21 0:14 ` [PATCH nft 2/3] src: evaluate: don't rely on global chain ctx for error reporting Florian Westphal @ 2019-07-21 0:14 ` Florian Westphal 2019-07-21 18:49 ` Pablo Neira Ayuso 2019-07-23 19:18 ` Pablo Neira Ayuso 2 siblings, 2 replies; 14+ messages in thread From: Florian Westphal @ 2019-07-21 0:14 UTC (permalink / raw) To: netfilter-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-21 0:14 ` [PATCH nft 3/3] src: evaluate: return immediately if no op was requested Florian Westphal @ 2019-07-21 18:49 ` Pablo Neira Ayuso 2019-07-21 18:50 ` Florian Westphal 2019-07-23 19:18 ` Pablo Neira Ayuso 1 sibling, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-21 18:49 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-21 18:49 ` Pablo Neira Ayuso @ 2019-07-21 18:50 ` Florian Westphal 2019-07-22 21:25 ` Pablo Neira Ayuso 0 siblings, 1 reply; 14+ messages in thread From: Florian Westphal @ 2019-07-21 18:50 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-21 18:50 ` Florian Westphal @ 2019-07-22 21:25 ` Pablo Neira Ayuso 2019-07-23 13:11 ` Phil Sutter 0 siblings, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-22 21:25 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel, phil [-- 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); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-22 21:25 ` Pablo Neira Ayuso @ 2019-07-23 13:11 ` Phil Sutter 2019-07-23 13:52 ` Pablo Neira Ayuso 0 siblings, 1 reply; 14+ messages in thread From: Phil Sutter @ 2019-07-23 13:11 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-23 13:11 ` Phil Sutter @ 2019-07-23 13:52 ` Pablo Neira Ayuso 0 siblings, 0 replies; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-23 13:52 UTC (permalink / raw) To: Phil Sutter, Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-21 0:14 ` [PATCH nft 3/3] src: evaluate: return immediately if no op was requested Florian Westphal 2019-07-21 18:49 ` Pablo Neira Ayuso @ 2019-07-23 19:18 ` Pablo Neira Ayuso 2019-07-23 22:44 ` Florian Westphal 1 sibling, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2019-07-23 19:18 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 3/3] src: evaluate: return immediately if no op was requested 2019-07-23 19:18 ` Pablo Neira Ayuso @ 2019-07-23 22:44 ` Florian Westphal 0 siblings, 0 replies; 14+ messages in thread From: Florian Westphal @ 2019-07-23 22:44 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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! ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-07-23 22:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-21 0:14 [PATCH nft 0/3] fix crash bug during rule restore Florian Westphal 2019-07-21 0:14 ` [PATCH nft 1/3] src: erec: fall back to internal location if its null Florian Westphal 2019-07-21 18:46 ` Pablo Neira Ayuso 2019-07-21 18:50 ` Florian Westphal 2019-07-23 19:19 ` Pablo Neira Ayuso 2019-07-21 0:14 ` [PATCH nft 2/3] src: evaluate: don't rely on global chain ctx for error reporting Florian Westphal 2019-07-21 0:14 ` [PATCH nft 3/3] src: evaluate: return immediately if no op was requested Florian Westphal 2019-07-21 18:49 ` Pablo Neira Ayuso 2019-07-21 18:50 ` Florian Westphal 2019-07-22 21:25 ` Pablo Neira Ayuso 2019-07-23 13:11 ` Phil Sutter 2019-07-23 13:52 ` Pablo Neira Ayuso 2019-07-23 19:18 ` Pablo Neira Ayuso 2019-07-23 22:44 ` Florian Westphal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).