From 005c220f08964958eae2ca6e40a070b5bc9d6f79 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Thu, 7 Nov 2024 18:38:45 +0100 Subject: [PATCH] src: collapse set element commands from parser JIRA: https://issues.redhat.com/browse/RHEL-65346 Upstream Status: nftables commit 20f1c60ac8c88be3bdf3096083b24ada06570a77 commit 20f1c60ac8c88be3bdf3096083b24ada06570a77 Author: Pablo Neira Ayuso Date: Wed Oct 23 11:43:58 2024 +0200 src: collapse set element commands from parser 498a5f0c219d ("rule: collapse set element commands") does not help to reduce memory consumption in the case of large sets defined by one element per line: add element ip x y { 1.1.1.1 } add element ip x y { 1.1.1.2 } ... This patch reduces memory consumption by ~75%, set elements are collapsed into an existing cmd object wherever possible to reduce the number of cmd objects. This patch also adds a special case for variables for sets similar to: be055af5c58d ("cmd: skip variable set elements when collapsing commands") This patch requires this small kernel fix: commit b53c116642502b0c85ecef78bff4f826a7dd4145 Author: Pablo Neira Ayuso Date: Fri May 20 00:02:06 2022 +0200 netfilter: nf_tables: set element extended ACK reporting support which is already included in recent -stable kernels: # cat ruleset.nft add table ip x add chain ip x y add set ip x y { type ipv4_addr; } create element ip x y { 1.1.1.1 } create element ip x y { 1.1.1.1 } # nft -f ruleset.nft ruleset.nft:5:25-31: Error: Could not process rule: File exists create element ip x y { 1.1.1.1 } ^^^^^^^ since there is no need to relate commands via sequence number anymore, this allows also removes the uncollapse step. Fixes: 498a5f0c219d ("rule: collapse set element commands") Signed-off-by: Pablo Neira Ayuso Signed-off-by: Phil Sutter --- include/cmd.h | 7 +-- include/expression.h | 1 - include/list.h | 11 +++++ include/rule.h | 1 - src/cmd.c | 105 +++++++++++-------------------------------- src/libnftables.c | 7 --- src/parser_bison.y | 13 ++++++ src/rule.c | 1 - 8 files changed, 54 insertions(+), 92 deletions(-) diff --git a/include/cmd.h b/include/cmd.h index 92a4152..0a8779b 100644 --- a/include/cmd.h +++ b/include/cmd.h @@ -2,12 +2,13 @@ #define _NFT_CMD_H_ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc); +struct mnl_err; void nft_cmd_error(struct netlink_ctx *ctx, struct cmd *cmd, struct mnl_err *err); +bool nft_cmd_collapse_elems(enum cmd_ops op, struct list_head *cmds, + struct handle *handle, struct expr *init); + void nft_cmd_expand(struct cmd *cmd); -void nft_cmd_post_expand(struct cmd *cmd); -bool nft_cmd_collapse(struct list_head *cmds); -void nft_cmd_uncollapse(struct list_head *cmds); #endif diff --git a/include/expression.h b/include/expression.h index 8982110..da2f693 100644 --- a/include/expression.h +++ b/include/expression.h @@ -255,7 +255,6 @@ struct expr { enum expr_types etype:8; enum ops op:8; unsigned int len; - struct cmd *cmd; union { struct { diff --git a/include/list.h b/include/list.h index 857921e..37fbe3e 100644 --- a/include/list.h +++ b/include/list.h @@ -348,6 +348,17 @@ static inline void list_splice_tail_init(struct list_head *list, #define list_first_entry(ptr, type, member) \ list_entry((ptr)->next, type, member) +/** + * list_last_entry - get the last element from a list + * @ptr: the list head to take the element from. + * @type: the type of the struct this is embedded in. + * @member: the name of the list_head within the struct. + * + * Note, that list is expected to be not empty. + */ +#define list_last_entry(ptr, type, member) \ + list_entry((ptr)->prev, type, member) + /** * list_next_entry - get the next element in list * @pos: the type * to cursor diff --git a/include/rule.h b/include/rule.h index 5b3e12b..a1628d8 100644 --- a/include/rule.h +++ b/include/rule.h @@ -718,7 +718,6 @@ struct cmd { enum cmd_obj obj; struct handle handle; uint32_t seqnum; - struct list_head collapse_list; union { void *data; struct expr *expr; diff --git a/src/cmd.c b/src/cmd.c index 9a572b5..e010dcb 100644 --- a/src/cmd.c +++ b/src/cmd.c @@ -378,6 +378,32 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds } } +bool nft_cmd_collapse_elems(enum cmd_ops op, struct list_head *cmds, + struct handle *handle, struct expr *init) +{ + struct cmd *last_cmd; + + if (list_empty(cmds)) + return false; + + if (init->etype == EXPR_VARIABLE) + return false; + + last_cmd = list_last_entry(cmds, struct cmd, list); + if (last_cmd->op != op || + last_cmd->obj != CMD_OBJ_ELEMENTS || + last_cmd->expr->etype == EXPR_VARIABLE || + last_cmd->handle.family != handle->family || + strcmp(last_cmd->handle.table.name, handle->table.name) || + strcmp(last_cmd->handle.set.name, handle->set.name)) + return false; + + list_splice_tail_init(&init->expressions, &last_cmd->expr->expressions); + last_cmd->expr->size += init->size; + + return true; +} + void nft_cmd_expand(struct cmd *cmd) { struct list_head new_cmds; @@ -459,82 +485,3 @@ void nft_cmd_expand(struct cmd *cmd) break; } } - -bool nft_cmd_collapse(struct list_head *cmds) -{ - struct cmd *cmd, *next, *elems = NULL; - struct expr *expr, *enext; - bool collapse = false; - - list_for_each_entry_safe(cmd, next, cmds, list) { - if (cmd->op != CMD_ADD && - cmd->op != CMD_CREATE) { - elems = NULL; - continue; - } - - if (cmd->obj != CMD_OBJ_ELEMENTS) { - elems = NULL; - continue; - } - - if (cmd->expr->etype == EXPR_VARIABLE) - continue; - - if (!elems) { - elems = cmd; - continue; - } - - if (cmd->op != elems->op) { - elems = cmd; - continue; - } - - if (elems->handle.family != cmd->handle.family || - strcmp(elems->handle.table.name, cmd->handle.table.name) || - strcmp(elems->handle.set.name, cmd->handle.set.name)) { - elems = cmd; - continue; - } - - collapse = true; - list_for_each_entry_safe(expr, enext, &cmd->expr->expressions, list) { - expr->cmd = cmd; - list_move_tail(&expr->list, &elems->expr->expressions); - } - elems->expr->size += cmd->expr->size; - list_move_tail(&cmd->list, &elems->collapse_list); - } - - return collapse; -} - -void nft_cmd_uncollapse(struct list_head *cmds) -{ - struct cmd *cmd, *cmd_next, *collapse_cmd, *collapse_cmd_next; - struct expr *expr, *next; - - list_for_each_entry_safe(cmd, cmd_next, cmds, list) { - if (list_empty(&cmd->collapse_list)) - continue; - - assert(cmd->obj == CMD_OBJ_ELEMENTS); - - list_for_each_entry_safe(expr, next, &cmd->expr->expressions, list) { - if (!expr->cmd) - continue; - - list_move_tail(&expr->list, &expr->cmd->expr->expressions); - cmd->expr->size--; - expr->cmd = NULL; - } - - list_for_each_entry_safe(collapse_cmd, collapse_cmd_next, &cmd->collapse_list, list) { - if (cmd->elem.set) - collapse_cmd->elem.set = set_get(cmd->elem.set); - - list_add(&collapse_cmd->list, &cmd->list); - } - } -} diff --git a/src/libnftables.c b/src/libnftables.c index 2ae2150..2834c99 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -513,7 +513,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs, { struct nft_cache_filter *filter; struct cmd *cmd, *next; - bool collapsed = false; unsigned int flags; int err = 0; @@ -529,9 +528,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs, nft_cache_filter_fini(filter); - if (nft_cmd_collapse(cmds)) - collapsed = true; - list_for_each_entry(cmd, cmds, list) { if (cmd->op != CMD_ADD && cmd->op != CMD_CREATE) @@ -553,9 +549,6 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs, } } - if (collapsed) - nft_cmd_uncollapse(cmds); - if (err < 0 || nft->state->nerrs) return -1; diff --git a/src/parser_bison.y b/src/parser_bison.y index e2936d1..602fc60 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -35,6 +35,7 @@ #include #include +#include #include #include #include @@ -1219,6 +1220,12 @@ add_cmd : TABLE table_spec } | ELEMENT set_spec set_block_expr { + if (nft_cmd_collapse_elems(CMD_ADD, state->cmds, &$2, $3)) { + handle_free(&$2); + expr_free($3); + $$ = NULL; + break; + } $$ = cmd_alloc(CMD_ADD, CMD_OBJ_ELEMENTS, &$2, &@$, $3); } | FLOWTABLE flowtable_spec flowtable_block_alloc @@ -1336,6 +1343,12 @@ create_cmd : TABLE table_spec } | ELEMENT set_spec set_block_expr { + if (nft_cmd_collapse_elems(CMD_CREATE, state->cmds, &$2, $3)) { + handle_free(&$2); + expr_free($3); + $$ = NULL; + break; + } $$ = cmd_alloc(CMD_CREATE, CMD_OBJ_ELEMENTS, &$2, &@$, $3); } | FLOWTABLE flowtable_spec flowtable_block_alloc diff --git a/src/rule.c b/src/rule.c index 9bc160e..9536e68 100644 --- a/src/rule.c +++ b/src/rule.c @@ -1332,7 +1332,6 @@ struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj, cmd->attr = xzalloc_array(NFT_NLATTR_LOC_MAX, sizeof(struct nlerr_loc)); cmd->attr_array_len = NFT_NLATTR_LOC_MAX; - init_list_head(&cmd->collapse_list); return cmd; }