On 2023-10-09, at 13:36:23 +0200, Pablo Neira Ayuso wrote:
> Hi Arturo, Jeremy,
>
> This is a small batch offering fixes for nftables 0.9.8. It only
> includes the fixes for the implicit chain regression in recent
> kernels.
>
> This is a few dependency patches that are missing in 0.9.8 are
> required:
>
> 3542e49cf539 ("evaluate: init cmd pointer for new on-stack context")
> a3ac2527724d ("src: split chain list in table")
> 4e718641397c ("cache: rename chain_htable to cache_chain_ht")
>
> a3ac2527724d is fixing an issue with the cache that is required by the
> fixes. Then, the backport fixes for the implicit chain regression with
> Linux -stable:
>
> 3975430b12d9 ("src: expand table command before evaluation")
> 27c753e4a8d4 ("rule: expand standalone chain that contains rules")
> 784597a4ed63 ("rule: add helper function to expand chain rules into commands")
>
> I tested with tests/shell at the time of the nftables 0.9.8 release
> (*I did not use git HEAD tests/shell as I did for 1.0.6*).
>
> I have kept back the backport of this patch intentionally:
>
> 56c90a2dd2eb ("evaluate: expand sets and maps before evaluation")
>
> this depends on the new src/interval.c code, in 0.9.8 overlap and
> automerge come a later stage and cache is not updated incrementally,
> I tried the tests coming in this patch and it works fine.
>
> I did run a few more tests with rulesets that I have been collecting
> from people that occasionally send them to me for my personal ruleset
> repo.
>
> I: results: [OK] 266 [FAILED] 0 [TOTAL] 266
>
> This has been tested with latest Linux kernel 5.10 -stable.
>
> I can still run a few more tests, I will get back to you if I find any
> issue.
>
> Let me know, thanks.
Thanks for this. I started looking into it, but had not yet identified
the preliminaries needed to get the regression fixes working with 0.9.8,
and I was going to ask you for help. I'll take a proper look this
evening.
J.
> From 0a39091a75b6255422832126df4cbf73c86845cd Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Thu, 1 Apr 2021 22:18:29 +0200
> Subject: [PATCH nft 0.9.8 1/7] cache: rename chain_htable to cache_chain_ht
>
> upstream 3542e49cf539ecfcef6ef7c2d4befb7896ade2cd commit.
>
> Rename the hashtable chain that is used for fast cache lookups.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> include/rule.h | 4 ++--
> src/cache.c | 6 +++---
> src/rule.c | 6 +++---
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/rule.h b/include/rule.h
> index 330a09aa77fa..43872db8947a 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -154,7 +154,7 @@ struct table {
> struct handle handle;
> struct location location;
> struct scope scope;
> - struct list_head *chain_htable;
> + struct list_head *cache_chain_ht;
> struct list_head chains;
> struct list_head sets;
> struct list_head objs;
> @@ -220,7 +220,7 @@ struct hook_spec {
> */
> struct chain {
> struct list_head list;
> - struct list_head hlist;
> + struct list_head cache_hlist;
> struct handle handle;
> struct location location;
> unsigned int refcnt;
> diff --git a/src/cache.c b/src/cache.c
> index ed2609008e22..7101b74160be 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -194,7 +194,7 @@ static int chain_cache_cb(struct nftnl_chain *nlc, void *arg)
> if (chain->flags & CHAIN_F_BINDING) {
> list_add_tail(&chain->list, &ctx->table->chain_bindings);
> } else {
> - list_add_tail(&chain->hlist, &ctx->table->chain_htable[hash]);
> + list_add_tail(&chain->cache_hlist, &ctx->table->cache_chain_ht[hash]);
> list_add_tail(&chain->list, &ctx->table->chains);
> }
>
> @@ -238,7 +238,7 @@ void chain_cache_add(struct chain *chain, struct table *table)
> uint32_t hash;
>
> hash = djb_hash(chain->handle.chain.name) % NFT_CACHE_HSIZE;
> - list_add_tail(&chain->hlist, &table->chain_htable[hash]);
> + list_add_tail(&chain->cache_hlist, &table->cache_chain_ht[hash]);
> list_add_tail(&chain->list, &table->chains);
> }
>
> @@ -249,7 +249,7 @@ struct chain *chain_cache_find(const struct table *table,
> uint32_t hash;
>
> hash = djb_hash(handle->chain.name) % NFT_CACHE_HSIZE;
> - list_for_each_entry(chain, &table->chain_htable[hash], hlist) {
> + list_for_each_entry(chain, &table->cache_chain_ht[hash], cache_hlist) {
> if (!strcmp(chain->handle.chain.name, handle->chain.name))
> return chain;
> }
> diff --git a/src/rule.c b/src/rule.c
> index e4bb6bae276a..3b445851f657 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1328,10 +1328,10 @@ struct table *table_alloc(void)
> init_list_head(&table->scope.symbols);
> table->refcnt = 1;
>
> - table->chain_htable =
> + table->cache_chain_ht =
> xmalloc(sizeof(struct list_head) * NFT_CACHE_HSIZE);
> for (i = 0; i < NFT_CACHE_HSIZE; i++)
> - init_list_head(&table->chain_htable[i]);
> + init_list_head(&table->cache_chain_ht[i]);
>
> return table;
> }
> @@ -1359,7 +1359,7 @@ void table_free(struct table *table)
> obj_free(obj);
> handle_free(&table->handle);
> scope_release(&table->scope);
> - xfree(table->chain_htable);
> + xfree(table->cache_chain_ht);
> xfree(table);
> }
>
> --
> 2.30.2
>
> From f37e4261130b021edf068e4d5c6ca062ce4e2ac1 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Thu, 1 Apr 2021 22:19:30 +0200
> Subject: [PATCH nft 0.9.8 2/7] src: split chain list in table
>
> upstream a3ac2527724dd27628e12caaa55f731b109e4586 commit.
>
> This patch splits table->lists in two:
>
> - Chains that reside in the cache are stored in the new
> tables->cache_chain and tables->cache_chain_ht. The hashtable chain
> cache allows for fast chain lookups.
>
> - Chains that defined via command line / ruleset file reside in
> tables->chains.
>
> Note that chains in the cache (already in the kernel) are not placed in
> the table->chains.
>
> By keeping separated lists, chains defined via command line / ruleset
> file can be added to cache.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> include/rule.h | 2 ++
> src/cache.c | 6 +++---
> src/json.c | 6 +++---
> src/rule.c | 18 +++++++++++-------
> 4 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/include/rule.h b/include/rule.h
> index 43872db8947a..dde32367f48f 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -155,6 +155,7 @@ struct table {
> struct location location;
> struct scope scope;
> struct list_head *cache_chain_ht;
> + struct list_head cache_chain;
> struct list_head chains;
> struct list_head sets;
> struct list_head objs;
> @@ -221,6 +222,7 @@ struct hook_spec {
> struct chain {
> struct list_head list;
> struct list_head cache_hlist;
> + struct list_head cache_list;
> struct handle handle;
> struct location location;
> unsigned int refcnt;
> diff --git a/src/cache.c b/src/cache.c
> index 7101b74160be..32e6eea4ac5c 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -192,10 +192,10 @@ static int chain_cache_cb(struct nftnl_chain *nlc, void *arg)
> chain = netlink_delinearize_chain(ctx->nlctx, nlc);
>
> if (chain->flags & CHAIN_F_BINDING) {
> - list_add_tail(&chain->list, &ctx->table->chain_bindings);
> + list_add_tail(&chain->cache_list, &ctx->table->chain_bindings);
> } else {
> list_add_tail(&chain->cache_hlist, &ctx->table->cache_chain_ht[hash]);
> - list_add_tail(&chain->list, &ctx->table->chains);
> + list_add_tail(&chain->cache_list, &ctx->table->cache_chain);
> }
>
> nftnl_chain_list_del(nlc);
> @@ -239,7 +239,7 @@ void chain_cache_add(struct chain *chain, struct table *table)
>
> hash = djb_hash(chain->handle.chain.name) % NFT_CACHE_HSIZE;
> list_add_tail(&chain->cache_hlist, &table->cache_chain_ht[hash]);
> - list_add_tail(&chain->list, &table->chains);
> + list_add_tail(&chain->cache_list, &table->cache_chain);
> }
>
> struct chain *chain_cache_find(const struct table *table,
> diff --git a/src/json.c b/src/json.c
> index 585d35326ac0..737e73b08b5a 100644
> --- a/src/json.c
> +++ b/src/json.c
> @@ -1594,7 +1594,7 @@ static json_t *table_print_json_full(struct netlink_ctx *ctx,
> tmp = flowtable_print_json(flowtable);
> json_array_append_new(root, tmp);
> }
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> tmp = chain_print_json(chain);
> json_array_append_new(root, tmp);
>
> @@ -1656,7 +1656,7 @@ static json_t *do_list_chain_json(struct netlink_ctx *ctx,
> struct chain *chain;
> struct rule *rule;
>
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> if (chain->handle.family != cmd->handle.family ||
> strcmp(cmd->handle.chain.name, chain->handle.chain.name))
> continue;
> @@ -1684,7 +1684,7 @@ static json_t *do_list_chains_json(struct netlink_ctx *ctx, struct cmd *cmd)
> cmd->handle.family != table->handle.family)
> continue;
>
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> json_t *tmp = chain_print_json(chain);
>
> json_array_append_new(root, tmp);
> diff --git a/src/rule.c b/src/rule.c
> index 3b445851f657..f76c27c9d091 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -953,7 +953,7 @@ struct chain *chain_lookup(const struct table *table, const struct handle *h)
> {
> struct chain *chain;
>
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> if (!strcmp(chain->handle.chain.name, h->chain.name))
> return chain;
> }
> @@ -965,7 +965,7 @@ struct chain *chain_binding_lookup(const struct table *table,
> {
> struct chain *chain;
>
> - list_for_each_entry(chain, &table->chain_bindings, list) {
> + list_for_each_entry(chain, &table->chain_bindings, cache_list) {
> if (!strcmp(chain->handle.chain.name, chain_name))
> return chain;
> }
> @@ -983,7 +983,7 @@ struct chain *chain_lookup_fuzzy(const struct handle *h,
> string_misspell_init(&st);
>
> list_for_each_entry(table, &cache->list, list) {
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> if (!strcmp(chain->handle.chain.name, h->chain.name)) {
> *t = table;
> return chain;
> @@ -1321,6 +1321,7 @@ struct table *table_alloc(void)
>
> table = xzalloc(sizeof(*table));
> init_list_head(&table->chains);
> + init_list_head(&table->cache_chain);
> init_list_head(&table->sets);
> init_list_head(&table->objs);
> init_list_head(&table->flowtables);
> @@ -1349,7 +1350,10 @@ void table_free(struct table *table)
> xfree(table->comment);
> list_for_each_entry_safe(chain, next, &table->chains, list)
> chain_free(chain);
> - list_for_each_entry_safe(chain, next, &table->chain_bindings, list)
> + list_for_each_entry_safe(chain, next, &table->chain_bindings, cache_list)
> + chain_free(chain);
> + /* this is implicitly releasing chains in the hashtable cache */
> + list_for_each_entry_safe(chain, next, &table->cache_chain, cache_list)
> chain_free(chain);
> list_for_each_entry_safe(set, nset, &table->sets, list)
> set_free(set);
> @@ -1465,7 +1469,7 @@ static void table_print(const struct table *table, struct output_ctx *octx)
> flowtable_print(flowtable, octx);
> delim = "\n";
> }
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> nft_print(octx, "%s", delim);
> chain_print(chain, octx);
> delim = "\n";
> @@ -2555,7 +2559,7 @@ static int do_list_chain(struct netlink_ctx *ctx, struct cmd *cmd,
>
> table_print_declaration(table, &ctx->nft->output);
>
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> if (chain->handle.family != cmd->handle.family ||
> strcmp(cmd->handle.chain.name, chain->handle.chain.name) != 0)
> continue;
> @@ -2580,7 +2584,7 @@ static int do_list_chains(struct netlink_ctx *ctx, struct cmd *cmd)
>
> table_print_declaration(table, &ctx->nft->output);
>
> - list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry(chain, &table->cache_chain, cache_list) {
> chain_print_declaration(chain, &ctx->nft->output);
> nft_print(&ctx->nft->output, "\t}\n");
> }
> --
> 2.30.2
>
> From 5af65a30a12396281c751e635509ab1d9363f4bc Mon Sep 17 00:00:00 2001
> From: Florian Westphal <fw@xxxxxxxxx>
> Date: Fri, 4 Mar 2022 11:30:55 +0100
> Subject: [PATCH nft 0.9.8 3/7] evaluate: init cmd pointer for new on-stack
> context
>
> upstream 4e718641397c876315a87db441afc53139863122 commit
>
> else, this will segfault when trying to print the
> "table 'x' doesn't exist" error message.
>
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> ---
> src/evaluate.c | 1 +
> tests/shell/testcases/chains/0041chain_binding_0 | 6 ++++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index c830dcdbd965..f546667131e1 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3211,6 +3211,7 @@ static int stmt_evaluate_chain(struct eval_ctx *ctx, struct stmt *stmt)
> struct eval_ctx rule_ctx = {
> .nft = ctx->nft,
> .msgs = ctx->msgs,
> + .cmd = ctx->cmd,
> };
> struct handle h2 = {};
>
> diff --git a/tests/shell/testcases/chains/0041chain_binding_0 b/tests/shell/testcases/chains/0041chain_binding_0
> index 59bdbe9f0ba9..4b541bb55c30 100755
> --- a/tests/shell/testcases/chains/0041chain_binding_0
> +++ b/tests/shell/testcases/chains/0041chain_binding_0
> @@ -1,5 +1,11 @@
> #!/bin/bash
>
> +# no table x, caused segfault in earlier nft releases
> +$NFT insert rule inet x y handle 107 'goto { log prefix "MOO! "; }'
> +if [ $? -ne 1 ]; then
> + exit 1
> +fi
> +
> set -e
>
> EXPECTED="table inet x {
> --
> 2.30.2
>
> From 0f559011ee7e805df883be635b88396639fbb87e Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Tue, 12 Sep 2023 23:22:46 +0200
> Subject: [PATCH nft 0.9.8 4/7] rule: add helper function to expand chain rules
> into commands
>
> upstream 784597a4ed63b9decb10d74fdb49a1b021e22728 commit.
>
> This patch adds a helper function to expand chain rules into commands.
> This comes in preparation for the follow up patch.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> src/rule.c | 39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/src/rule.c b/src/rule.c
> index f76c27c9d091..3fbf4271accd 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1503,6 +1503,25 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
> cmd->num_attrs++;
> }
>
> +static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds)
> +{
> + struct rule *rule;
> + struct handle h;
> + struct cmd *new;
> +
> + list_for_each_entry(rule, &chain->rules, list) {
> + memset(&h, 0, sizeof(h));
> + handle_merge(&h, &rule->handle);
> + if (chain->flags & CHAIN_F_BINDING) {
> + rule->handle.chain_id = chain->handle.chain_id;
> + rule->handle.chain.location = chain->location;
> + }
> + new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
> + &rule->location, rule_get(rule));
> + list_add_tail(&new->list, new_cmds);
> + }
> +}
> +
> void nft_cmd_expand(struct cmd *cmd)
> {
> struct list_head new_cmds;
> @@ -1510,7 +1529,6 @@ void nft_cmd_expand(struct cmd *cmd)
> struct flowtable *ft;
> struct table *table;
> struct chain *chain;
> - struct rule *rule;
> struct obj *obj;
> struct cmd *new;
> struct handle h;
> @@ -1555,22 +1573,9 @@ void nft_cmd_expand(struct cmd *cmd)
> &ft->location, flowtable_get(ft));
> list_add_tail(&new->list, &new_cmds);
> }
> - list_for_each_entry(chain, &table->chains, list) {
> - list_for_each_entry(rule, &chain->rules, list) {
> - memset(&h, 0, sizeof(h));
> - handle_merge(&h, &rule->handle);
> - if (chain->flags & CHAIN_F_BINDING) {
> - rule->handle.chain_id =
> - chain->handle.chain_id;
> - rule->handle.chain.location =
> - chain->location;
> - }
> - new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
> - &rule->location,
> - rule_get(rule));
> - list_add_tail(&new->list, &new_cmds);
> - }
> - }
> + list_for_each_entry(chain, &table->chains, list)
> + nft_cmd_expand_chain(chain, &new_cmds);
> +
> list_splice(&new_cmds, &cmd->list);
> break;
> case CMD_OBJ_SET:
> --
> 2.30.2
>
> From f03bc399d75ef724fcbed184f74fc306ca8ef324 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Mon, 6 Feb 2023 15:28:41 +0100
> Subject: [PATCH nft 0.9.8 5/7] rule: expand standalone chain that contains
> rules
>
> upstream 27c753e4a8d4744f479345e3f5e34cafef751602 commit.
>
> Otherwise rules that this chain contains are ignored when expressed
> using the following syntax:
>
> chain inet filter input2 {
> type filter hook input priority filter; policy accept;
> ip saddr 1.2.3.4 tcp dport { 22, 443, 123 } drop
> }
>
> When expanding the chain, remove the rule so the new CMD_OBJ_CHAIN
> case does not expand it again.
>
> Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1655
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> src/rule.c | 15 +++++++++---
> .../testcases/include/0020include_chain_0 | 23 +++++++++++++++++++
> .../include/dumps/0020include_chain_0.nft | 6 +++++
> 3 files changed, 41 insertions(+), 3 deletions(-)
> create mode 100755 tests/shell/testcases/include/0020include_chain_0
> create mode 100644 tests/shell/testcases/include/dumps/0020include_chain_0.nft
>
> diff --git a/src/rule.c b/src/rule.c
> index 3fbf4271accd..9139418e1bf8 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1505,11 +1505,12 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
>
> static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds)
> {
> - struct rule *rule;
> + struct rule *rule, *next;
> struct handle h;
> struct cmd *new;
>
> - list_for_each_entry(rule, &chain->rules, list) {
> + list_for_each_entry_safe(rule, next, &chain->rules, list) {
> + list_del(&rule->list);
> memset(&h, 0, sizeof(h));
> handle_merge(&h, &rule->handle);
> if (chain->flags & CHAIN_F_BINDING) {
> @@ -1517,7 +1518,7 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
> rule->handle.chain.location = chain->location;
> }
> new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
> - &rule->location, rule_get(rule));
> + &rule->location, rule);
> list_add_tail(&new->list, new_cmds);
> }
> }
> @@ -1578,6 +1579,14 @@ void nft_cmd_expand(struct cmd *cmd)
>
> list_splice(&new_cmds, &cmd->list);
> break;
> + case CMD_OBJ_CHAIN:
> + chain = cmd->chain;
> + if (!chain || list_empty(&chain->rules))
> + break;
> +
> + nft_cmd_expand_chain(chain, &new_cmds);
> + list_splice(&new_cmds, &cmd->list);
> + break;
> case CMD_OBJ_SET:
> case CMD_OBJ_MAP:
> set = cmd->set;
> diff --git a/tests/shell/testcases/include/0020include_chain_0 b/tests/shell/testcases/include/0020include_chain_0
> new file mode 100755
> index 000000000000..2ff83c92fda8
> --- /dev/null
> +++ b/tests/shell/testcases/include/0020include_chain_0
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +
> +set -e
> +
> +tmpfile1=$(mktemp -p .)
> +if [ ! -w $tmpfile1 ] ; then
> + echo "Failed to create tmp file" >&2
> + exit 0
> +fi
> +
> +trap "rm -rf $tmpfile1" EXIT # cleanup if aborted
> +
> +RULESET="table inet filter { }
> +include \"$tmpfile1\""
> +
> +RULESET2="chain inet filter input2 {
> + type filter hook input priority filter; policy accept;
> + ip saddr 1.2.3.4 tcp dport { 22, 443, 123 } drop
> +}"
> +
> +echo "$RULESET2" > $tmpfile1
> +
> +$NFT -f - <<< $RULESET
> diff --git a/tests/shell/testcases/include/dumps/0020include_chain_0.nft b/tests/shell/testcases/include/dumps/0020include_chain_0.nft
> new file mode 100644
> index 000000000000..3ad6db14d2f5
> --- /dev/null
> +++ b/tests/shell/testcases/include/dumps/0020include_chain_0.nft
> @@ -0,0 +1,6 @@
> +table inet filter {
> + chain input2 {
> + type filter hook input priority filter; policy accept;
> + ip saddr 1.2.3.4 tcp dport { 22, 123, 443 } drop
> + }
> +}
> --
> 2.30.2
>
> From 050e0b7a85016b733e1a59285df501d1c05eec0b Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Tue, 12 Sep 2023 23:25:27 +0200
> Subject: [PATCH nft 0.9.8 6/7] src: expand table command before evaluation
>
> upstream 3975430b12d97c92cdf03753342f2269153d5624 commit.
>
> The nested syntax notation results in one single table command which
> includes all other objects. This differs from the flat notation where
> there is usually one command per object.
>
> This patch adds a previous step to the evaluation phase to expand the
> objects that are contained in the table into independent commands, so
> both notations have similar representations.
>
> Remove the code to evaluate the nested representation in the evaluation
> phase since commands are independently evaluated after the expansion.
>
> The commands are expanded after the set element collapse step, in case
> that there is a long list of singleton element commands to be added to
> the set, to shorten the command list iteration.
>
> This approach also avoids interference with the object cache that is
> populated in the evaluation, which might refer to objects coming in the
> existing command list that is being processed.
>
> There is still a post_expand phase to detach the elements from the set
> which could be consolidated by updating the evaluation step to handle
> the CMD_OBJ_SETELEMS command type.
>
> This patch fixes 27c753e4a8d4 ("rule: expand standalone chain that
> contains rules") which broke rule addition/insertion by index because
> the expansion code after the evaluation messes up the cache.
>
> Fixes: 27c753e4a8d4 ("rule: expand standalone chain that contains rules")
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> include/rule.h | 1 +
> src/evaluate.c | 39 ---------------------------------------
> src/libnftables.c | 9 ++++++++-
> src/rule.c | 21 +++++++++++++++++++--
> 4 files changed, 28 insertions(+), 42 deletions(-)
>
> diff --git a/include/rule.h b/include/rule.h
> index dde32367f48f..f880cfd32bd8 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -717,6 +717,7 @@ extern struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj,
> const struct handle *h, const struct location *loc,
> void *data);
> extern void nft_cmd_expand(struct cmd *cmd);
> +extern void nft_cmd_post_expand(struct cmd *cmd);
> extern struct cmd *cmd_alloc_obj_ct(enum cmd_ops op, int type,
> const struct handle *h,
> const struct location *loc, struct obj *obj);
> diff --git a/src/evaluate.c b/src/evaluate.c
> index f546667131e1..232ae39020cc 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -4068,7 +4068,6 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
> static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> {
> struct table *table;
> - struct rule *rule;
>
> table = table_lookup_global(ctx);
> if (table == NULL)
> @@ -4122,11 +4121,6 @@ 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, CMD_INVALID) < 0)
> - return -1;
> - }
> return 0;
> }
>
> @@ -4183,11 +4177,6 @@ static int obj_evaluate(struct eval_ctx *ctx, struct obj *obj)
>
> static int table_evaluate(struct eval_ctx *ctx, struct table *table)
> {
> - struct flowtable *ft;
> - struct chain *chain;
> - struct set *set;
> - struct obj *obj;
> -
> if (table_lookup(&ctx->cmd->handle, &ctx->nft->cache) == NULL) {
> if (table == NULL) {
> table = table_alloc();
> @@ -4198,34 +4187,6 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
> }
> }
>
> - if (ctx->cmd->table == NULL)
> - return 0;
> -
> - ctx->table = table;
> - list_for_each_entry(set, &table->sets, list) {
> - expr_set_context(&ctx->ectx, NULL, 0);
> - handle_merge(&set->handle, &table->handle);
> - if (set_evaluate(ctx, set) < 0)
> - return -1;
> - }
> - list_for_each_entry(chain, &table->chains, list) {
> - handle_merge(&chain->handle, &table->handle);
> - ctx->cmd->handle.chain.location = chain->location;
> - if (chain_evaluate(ctx, chain) < 0)
> - return -1;
> - }
> - list_for_each_entry(ft, &table->flowtables, list) {
> - handle_merge(&ft->handle, &table->handle);
> - if (flowtable_evaluate(ctx, ft) < 0)
> - return -1;
> - }
> - list_for_each_entry(obj, &table->objs, list) {
> - handle_merge(&obj->handle, &table->handle);
> - if (obj_evaluate(ctx, obj) < 0)
> - return -1;
> - }
> -
> - ctx->table = NULL;
> return 0;
> }
>
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 044365914747..b1f57802b90e 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -420,6 +420,13 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
> if (cache_update(nft, flags, msgs) < 0)
> return -1;
>
> + list_for_each_entry(cmd, cmds, list) {
> + if (cmd->op != CMD_ADD)
> + continue;
> +
> + nft_cmd_expand(cmd);
> + }
> +
> list_for_each_entry(cmd, cmds, list) {
> struct eval_ctx ectx = {
> .nft = nft,
> @@ -437,7 +444,7 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
> if (cmd->op != CMD_ADD)
> continue;
>
> - nft_cmd_expand(cmd);
> + nft_cmd_post_expand(cmd);
> }
>
> return 0;
> diff --git a/src/rule.c b/src/rule.c
> index 9139418e1bf8..9c74b89c1fb1 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1511,8 +1511,9 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
>
> list_for_each_entry_safe(rule, next, &chain->rules, list) {
> list_del(&rule->list);
> + handle_merge(&rule->handle, &chain->handle);
> memset(&h, 0, sizeof(h));
> - handle_merge(&h, &rule->handle);
> + handle_merge(&h, &chain->handle);
> if (chain->flags & CHAIN_F_BINDING) {
> rule->handle.chain_id = chain->handle.chain_id;
> rule->handle.chain.location = chain->location;
> @@ -1526,10 +1527,10 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
> void nft_cmd_expand(struct cmd *cmd)
> {
> struct list_head new_cmds;
> - struct set *set, *newset;
> struct flowtable *ft;
> struct table *table;
> struct chain *chain;
> + struct set *set;
> struct obj *obj;
> struct cmd *new;
> struct handle h;
> @@ -1543,6 +1544,7 @@ void nft_cmd_expand(struct cmd *cmd)
> return;
>
> list_for_each_entry(chain, &table->chains, list) {
> + handle_merge(&chain->handle, &table->handle);
> memset(&h, 0, sizeof(h));
> handle_merge(&h, &chain->handle);
> h.chain_id = chain->handle.chain_id;
> @@ -1587,6 +1589,21 @@ void nft_cmd_expand(struct cmd *cmd)
> nft_cmd_expand_chain(chain, &new_cmds);
> list_splice(&new_cmds, &cmd->list);
> break;
> + default:
> + break;
> + }
> +}
> +
> +void nft_cmd_post_expand(struct cmd *cmd)
> +{
> + struct list_head new_cmds;
> + struct set *set, *newset;
> + struct cmd *new;
> + struct handle h;
> +
> + init_list_head(&new_cmds);
> +
> + switch (cmd->obj) {
> case CMD_OBJ_SET:
> case CMD_OBJ_MAP:
> set = cmd->set;
> --
> 2.30.2
>
> From 69d1ab7c50a6a1dd369b50a5edad769b98779e12 Mon Sep 17 00:00:00 2001
> From: Phil Sutter <phil@xxxxxx>
> Date: Wed, 23 Aug 2023 18:18:49 +0200
> Subject: [PATCH nft 0.9.8 7/7] tests: shell: Stabilize
> sets/0043concatenated_ranges_0 test
>
> upstream c791765cb0d62ba261f0b495e07315437b3ae914 commit.
>
> On a slow system, one of the 'delete element' commands would
> occasionally fail. Assuming it can only happen if the 2s timeout passes
> "too quickly", work around it by adding elements with a 2m timeout
> instead and when wanting to test the element expiry just drop and add
> the element again with a short timeout.
>
> Fixes: 6231d3fa4af1e ("tests: shell: Fix for unstable sets/0043concatenated_ranges_0")
> Signed-off-by: Phil Sutter <phil@xxxxxx>
> ---
> tests/shell/testcases/sets/0043concatenated_ranges_0 | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/shell/testcases/sets/0043concatenated_ranges_0 b/tests/shell/testcases/sets/0043concatenated_ranges_0
> index 11767373bcd2..8d3dacf6e38a 100755
> --- a/tests/shell/testcases/sets/0043concatenated_ranges_0
> +++ b/tests/shell/testcases/sets/0043concatenated_ranges_0
> @@ -147,7 +147,7 @@ for ta in ${TYPES}; do
> eval add_b=\$ADD_${tb}
> eval add_c=\$ADD_${tc}
> ${NFT} add element inet filter test \
> - "{ ${add_a} . ${add_b} . ${add_c} timeout 1s${mapv}}"
> + "{ ${add_a} . ${add_b} . ${add_c} timeout 2m${mapv}}"
> [ $(${NFT} list ${setmap} inet filter test | \
> grep -c "${add_a} . ${add_b} . ${add_c}") -eq 1 ]
>
> @@ -180,6 +180,10 @@ for ta in ${TYPES}; do
> continue
> fi
>
> + ${NFT} delete element inet filter test \
> + "{ ${add_a} . ${add_b} . ${add_c} ${mapv}}"
> + ${NFT} add element inet filter test \
> + "{ ${add_a} . ${add_b} . ${add_c} timeout 1s${mapv}}"
> sleep 1
> [ $(${NFT} list ${setmap} inet filter test | \
> grep -c "${add_a} . ${add_b} . ${add_c} ${mapv}") -eq 0 ]
> --
> 2.30.2
>
Attachment:
signature.asc
Description: PGP signature
