[PATCH nft 1/3] parser_bison: fix error location for set elements

[PATCH nft 1/3] parser_bison: fix error location for set elements

[Pablo Neira Ayuso]

opt_newline causes interfere since it points to the previous line.
Refer to set element key for error reporting.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/parser_bison.y | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index ca5c488cd5ff..e03fb35d96c7 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2915,7 +2915,7 @@ verdict_map_list_expr	:	verdict_map_list_member_expr
 
 verdict_map_list_member_expr:	opt_newline	set_elem_expr	COLON	verdict_expr	opt_newline
 			{
-				$$ = mapping_expr_alloc(&@$, $2, $4);
+				$$ = mapping_expr_alloc(&@2, $2, $4);
 			}
 			;
 
@@ -4263,7 +4263,7 @@ set_list_member_expr	:	opt_newline	set_expr	opt_newline
 			}
 			|	opt_newline	set_elem_expr	COLON	set_rhs_expr	opt_newline
 			{
-				$$ = mapping_expr_alloc(&@$, $2, $4);
+				$$ = mapping_expr_alloc(&@2, $2, $4);
 			}
 			;
 
-- 
2.30.2

[PATCH nft 2/3] src: remove NFT_NLATTR_LOC_MAX limit for netlink location error reporting

[Pablo Neira Ayuso]

Set might have more than 16 elements, use a runtime array to store
netlink error location.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h | 13 ++++++++-----
 src/cmd.c      |  2 +-
 src/rule.c     | 10 ++++++++--
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index e232b97afed7..44e51847b70a 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -681,6 +681,11 @@ void monitor_free(struct monitor *m);
 
 #define NFT_NLATTR_LOC_MAX 32
 
+struct nlerr_loc {
+	uint16_t		offset;
+	const struct location	*location;
+};
+
 /**
  * struct cmd - command statement
  *
@@ -716,11 +721,9 @@ struct cmd {
 		struct markup	*markup;
 		struct obj	*object;
 	};
-	struct {
-		uint16_t		offset;
-		const struct location	*location;
-	} attr[NFT_NLATTR_LOC_MAX];
-	int			num_attrs;
+	struct nlerr_loc	*attr;
+	uint32_t		attr_size;
+	uint32_t		num_attrs;
 	const void		*arg;
 };
 
diff --git a/src/cmd.c b/src/cmd.c
index f6a8aa114768..63692422e765 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -237,7 +237,7 @@ void nft_cmd_error(struct netlink_ctx *ctx, struct cmd *cmd,
 		   struct mnl_err *err)
 {
 	const struct location *loc = NULL;
-	int i;
+	uint32_t i;
 
 	for (i = 0; i < cmd->num_attrs; i++) {
 		if (!cmd->attr[i].offset)
diff --git a/src/rule.c b/src/rule.c
index 799092eb15c5..78f47300d0fc 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1279,13 +1279,18 @@ struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj,
 	cmd->handle   = *h;
 	cmd->location = *loc;
 	cmd->data     = data;
+	cmd->attr     = calloc(NFT_NLATTR_LOC_MAX, sizeof(struct nlerr_loc));
+	cmd->attr_size = NFT_NLATTR_LOC_MAX;
+
 	return cmd;
 }
 
 void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
 {
-	if (cmd->num_attrs >= NFT_NLATTR_LOC_MAX)
-		return;
+	if (cmd->num_attrs >= cmd->attr_size) {
+		cmd->attr_size *= 2;
+		cmd->attr = reallocarray(cmd->attr, sizeof(struct nlerr_loc), cmd->attr_size);
+	}
 
 	cmd->attr[cmd->num_attrs].offset = offset;
 	cmd->attr[cmd->num_attrs].location = loc;
@@ -1462,6 +1467,7 @@ void cmd_free(struct cmd *cmd)
 			BUG("invalid command object type %u\n", cmd->obj);
 		}
 	}
+	xfree(cmd->attr);
 	xfree(cmd->arg);
 	xfree(cmd);
 }
-- 
2.30.2

[PATCH nft 3/3] mnl: store netlink error location for set elements

[Pablo Neira Ayuso]

Store set element location in the per-command netlink error location
array.  This allows for fine grain error reporting when adding and
deleting elements.

 # nft -f test.nft
 test.nft:5:4-20: Error: Could not process rule: File exists
                        00:01:45:09:0b:26 : drop,
                        ^^^^^^^^^^^^^^^^^

test.nft contains a large map with one redundant entry.

Thus, users do not have to find the needle in the stack.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/mnl.h |  9 +++++----
 src/mnl.c     | 28 +++++++++++++++++-----------
 src/rule.c    | 12 ++++++------
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 4c701d4ee6dc..8e0a7e3fccab 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -60,10 +60,11 @@ int mnl_nft_set_del(struct netlink_ctx *ctx, struct cmd *cmd);
 struct nftnl_set_list *mnl_nft_set_dump(struct netlink_ctx *ctx, int family,
 					const char *table, const char *set);
 
-int mnl_nft_setelem_add(struct netlink_ctx *ctx, const struct set *set,
-			const struct expr *expr, unsigned int flags);
-int mnl_nft_setelem_del(struct netlink_ctx *ctx, const struct handle *h,
-			const struct expr *init);
+int mnl_nft_setelem_add(struct netlink_ctx *ctx, struct cmd *cmd,
+			const struct set *set, const struct expr *expr,
+			unsigned int flags);
+int mnl_nft_setelem_del(struct netlink_ctx *ctx, struct cmd *cmd,
+			const struct handle *h, const struct expr *init);
 int mnl_nft_setelem_flush(struct netlink_ctx *ctx, const struct cmd *cmd);
 int mnl_nft_setelem_get(struct netlink_ctx *ctx, struct nftnl_set *nls);
 struct nftnl_set *mnl_nft_setelem_get_one(struct netlink_ctx *ctx,
diff --git a/src/mnl.c b/src/mnl.c
index 7dd77be1bec0..e87b033870b0 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1609,9 +1609,9 @@ static void netlink_dump_setelem_done(struct netlink_ctx *ctx)
 	fprintf(fp, "\n");
 }
 
-static int mnl_nft_setelem_batch(const struct nftnl_set *nls,
+static int mnl_nft_setelem_batch(const struct nftnl_set *nls, struct cmd *cmd,
 				 struct nftnl_batch *batch,
-				 enum nf_tables_msg_types cmd,
+				 enum nf_tables_msg_types msg_type,
 				 unsigned int flags, uint32_t seqnum,
 				 const struct expr *set,
 				 struct netlink_ctx *ctx)
@@ -1622,14 +1622,14 @@ static int mnl_nft_setelem_batch(const struct nftnl_set *nls,
 	struct expr *expr = NULL;
 	int i = 0;
 
-	if (cmd == NFT_MSG_NEWSETELEM)
+	if (msg_type == NFT_MSG_NEWSETELEM)
 		flags |= NLM_F_CREATE;
 
 	if (set)
 		expr = list_first_entry(&set->expressions, struct expr, list);
 
 next:
-	nlh = nftnl_nlmsg_build_hdr(nftnl_batch_buffer(batch), cmd,
+	nlh = nftnl_nlmsg_build_hdr(nftnl_batch_buffer(batch), msg_type,
 				    nftnl_set_get_u32(nls, NFTNL_SET_FAMILY),
 				    flags, seqnum);
 
@@ -1653,7 +1653,12 @@ next:
 	nest1 = mnl_attr_nest_start(nlh, NFTA_SET_ELEM_LIST_ELEMENTS);
 	list_for_each_entry_from(expr, &set->expressions, list) {
 		nlse = alloc_nftnl_setelem(set, expr);
-		nest2 = nftnl_set_elem_nlmsg_build(nlh, nlse, ++i);
+
+		cmd_add_loc(cmd, nlh->nlmsg_len, &expr->location);
+		nest2 = mnl_attr_nest_start(nlh, ++i);
+		nftnl_set_elem_nlmsg_build_payload(nlh, nlse);
+		mnl_attr_nest_end(nlh, nest2);
+
 		netlink_dump_setelem(nlse, ctx);
 		nftnl_set_elem_free(nlse);
 		if (mnl_nft_attr_nest_overflow(nlh, nest1, nest2)) {
@@ -1669,8 +1674,9 @@ next:
 	return 0;
 }
 
-int mnl_nft_setelem_add(struct netlink_ctx *ctx, const struct set *set,
-			const struct expr *expr, unsigned int flags)
+int mnl_nft_setelem_add(struct netlink_ctx *ctx, struct cmd *cmd,
+			const struct set *set, const struct expr *expr,
+			unsigned int flags)
 {
 	const struct handle *h = &set->handle;
 	struct nftnl_set *nls;
@@ -1691,7 +1697,7 @@ int mnl_nft_setelem_add(struct netlink_ctx *ctx, const struct set *set,
 
 	netlink_dump_set(nls, ctx);
 
-	err = mnl_nft_setelem_batch(nls, ctx->batch, NFT_MSG_NEWSETELEM,
+	err = mnl_nft_setelem_batch(nls, cmd, ctx->batch, NFT_MSG_NEWSETELEM,
 				    flags, ctx->seqnum, expr, ctx);
 	nftnl_set_free(nls);
 
@@ -1728,8 +1734,8 @@ int mnl_nft_setelem_flush(struct netlink_ctx *ctx, const struct cmd *cmd)
 	return 0;
 }
 
-int mnl_nft_setelem_del(struct netlink_ctx *ctx, const struct handle *h,
-			const struct expr *init)
+int mnl_nft_setelem_del(struct netlink_ctx *ctx, struct cmd *cmd,
+			const struct handle *h, const struct expr *init)
 {
 	struct nftnl_set *nls;
 	int err;
@@ -1747,7 +1753,7 @@ int mnl_nft_setelem_del(struct netlink_ctx *ctx, const struct handle *h,
 
 	netlink_dump_set(nls, ctx);
 
-	err = mnl_nft_setelem_batch(nls, ctx->batch, NFT_MSG_DELSETELEM, 0,
+	err = mnl_nft_setelem_batch(nls, cmd, ctx->batch, NFT_MSG_DELSETELEM, 0,
 				    ctx->seqnum, init, ctx);
 	nftnl_set_free(nls);
 
diff --git a/src/rule.c b/src/rule.c
index 78f47300d0fc..d809225cdf7f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1475,11 +1475,11 @@ void cmd_free(struct cmd *cmd)
 #include <netlink.h>
 #include <mnl.h>
 
-static int __do_add_elements(struct netlink_ctx *ctx, struct set *set,
-			     struct expr *expr, uint32_t flags)
+static int __do_add_elements(struct netlink_ctx *ctx, struct cmd *cmd,
+			     struct set *set, struct expr *expr, uint32_t flags)
 {
 	expr->set_flags |= set->flags;
-	if (mnl_nft_setelem_add(ctx, set, expr, flags) < 0)
+	if (mnl_nft_setelem_add(ctx, cmd, set, expr, flags) < 0)
 		return -1;
 
 	return 0;
@@ -1495,7 +1495,7 @@ static int do_add_elements(struct netlink_ctx *ctx, struct cmd *cmd,
 	    set_to_intervals(set, init, true) < 0)
 		return -1;
 
-	return __do_add_elements(ctx, set, init, flags);
+	return __do_add_elements(ctx, cmd, set, init, flags);
 }
 
 static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
@@ -1503,7 +1503,7 @@ static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
 {
 	struct set *set = cmd->set;
 
-	return __do_add_elements(ctx, set, set->init, flags);
+	return __do_add_elements(ctx, cmd, set, set->init, flags);
 }
 
 static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
@@ -1597,7 +1597,7 @@ static int do_delete_setelems(struct netlink_ctx *ctx, struct cmd *cmd)
 	    set_to_intervals(set, expr, false) < 0)
 		return -1;
 
-	if (mnl_nft_setelem_del(ctx, &cmd->handle, cmd->elem.expr) < 0)
+	if (mnl_nft_setelem_del(ctx, cmd, &cmd->handle, cmd->elem.expr) < 0)
 		return -1;
 
 	return 0;
-- 
2.30.2

Re: [PATCH nft 1/3] parser_bison: fix error location for set elements

[Pablo Neira Ayuso]

On Wed, May 18, 2022 at 08:04:33PM +0200, Pablo Neira Ayuso wrote:
> opt_newline causes interfere since it points to the previous line.
> Refer to set element key for error reporting.

For the record, pushed out this series with minor comestic updates.