On Thu, May 07, 2020 at 04:11:48PM +0200, Florian Westphal wrote:
[...]
> Yes, but there is something else going on.
>
> The command above works without this patch if you use a shorter table name.
> There is another bug that causes nft to pull the wrong error object
> from the queue.
>
> The kernel doesn't generate an error for NFTA_SET_NAME in the above
> rule, so we should not crash even without this (correct) fix, because
> nft should not find this particular error object.
>
> Seems the generated error is for NFTA_SET_ELEM_LIST_TABLE when handling
> nf_tables_newsetelem() in kernel (which makes sense, the table doesn't
> exist).
>
> With the above command (traffic-filter) NFTA_SET_NAMEs start offset
> matches the offset of NFTA_SET_ELEM_LIST_TABLE error message in the
> other netlink message (the one adding the element to the set), it will
> erronously find the cmd_add_loc() of NFTA_SET_NAME and then barf because
> of the bug fixed here.
>
> Not sure how to fix nft_cmd_error(), it looks like the error queueing assumes
> 1:1 mapping of cmd struct and netlink message header...?
Thanks for explaining.
I forgot anonymous sets are missing the expansion to achieve the 1:1
mapping between commands and netlink messages.
Please, have a look at attached sketch patch.
diff --git a/include/rule.h b/include/rule.h
index 1a4ec3d8bc37..e11740d548ca 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -587,6 +587,7 @@ enum cmd_ops {
enum cmd_obj {
CMD_OBJ_INVALID,
CMD_OBJ_SETELEM,
+ CMD_OBJ_SETELEM_ANONYMOUS,
CMD_OBJ_SET,
CMD_OBJ_SETS,
CMD_OBJ_RULE,
diff --git a/src/libnftables.c b/src/libnftables.c
index 32da0a29ee21..668e3fc43031 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -419,8 +419,12 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
if (nft->state->nerrs)
return -1;
- list_for_each_entry(cmd, cmds, list)
+ list_for_each_entry(cmd, cmds, list) {
+ if (cmd->op != CMD_ADD)
+ continue;
+
nft_cmd_expand(cmd);
+ }
return 0;
}
diff --git a/src/rule.c b/src/rule.c
index c58aa359259e..166c6c2befaf 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1417,11 +1417,11 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, struct location *loc)
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 rule *rule;
- struct set *set;
struct obj *obj;
struct cmd *new;
struct handle h;
@@ -1477,6 +1477,22 @@ void nft_cmd_expand(struct cmd *cmd)
}
list_splice(&new_cmds, &cmd->list);
break;
+ case CMD_OBJ_SET:
+ set = cmd->set;
+ if (!set_is_anonymous(set->flags))
+ break;
+
+ memset(&h, 0, sizeof(h));
+ handle_merge(&h, &set->handle);
+ newset = set_clone(set);
+ newset->handle.set_id = set->handle.set_id;
+ newset->init = set->init;
+ set->init = NULL;
+ new = cmd_alloc(CMD_ADD, CMD_OBJ_SETELEM_ANONYMOUS, &h,
+ &set->location, newset);
+ list_add(&new->list, &cmd->list);
+ set->init = NULL;
+ break;
default:
break;
}
@@ -1525,6 +1541,7 @@ void cmd_free(struct cmd *cmd)
expr_free(cmd->expr);
break;
case CMD_OBJ_SET:
+ case CMD_OBJ_SETELEM_ANONYMOUS:
set_free(cmd->set);
break;
case CMD_OBJ_RULE:
@@ -1610,7 +1627,7 @@ static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
}
static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
- uint32_t flags)
+ uint32_t flags, bool anonymous)
{
struct set *set = cmd->set;
@@ -1621,7 +1638,7 @@ static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
&ctx->nft->output) < 0)
return -1;
}
- if (mnl_nft_set_add(ctx, cmd, flags) < 0)
+ if (!anonymous && mnl_nft_set_add(ctx, cmd, flags) < 0)
return -1;
if (set->init != NULL) {
return __do_add_setelems(ctx, set, set->init, flags);
@@ -1644,7 +1661,9 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
case CMD_OBJ_RULE:
return mnl_nft_rule_add(ctx, cmd, flags | NLM_F_APPEND);
case CMD_OBJ_SET:
- return do_add_set(ctx, cmd, flags);
+ return do_add_set(ctx, cmd, flags, false);
+ case CMD_OBJ_SETELEM_ANONYMOUS:
+ return do_add_set(ctx, cmd, flags, true);
case CMD_OBJ_SETELEM:
return do_add_setelems(ctx, cmd, flags);
case CMD_OBJ_COUNTER:
