rule: rework CMD_OBJ_SETELEMS logic

Do not clone the set and zap the elements during the set and map
expansion to the CMD_OBJ_SETELEMS command.

Instead, update the CMD_OBJ_SET command to add the set to the kernel
(without elements) and let CMD_OBJ_SETELEMS add the elements. The
CMD_OBJ_SET command calls set_to_intervals() to update set->init->size
(NFTNL_SET_DESC_SIZE) before adding the set to the kernel. Updating the
set size from do_add_setelems() comes too late, it might result in
spurious ENFILE errors for interval sets.

Moreover, skip CMD_OBJ_SETELEMS if the set definition specifies no
elements.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1500
Fixes: c9eae091983a ("src: add CMD_OBJ_SETELEMS")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

2 files changed, 40 insertions, 19 deletions

diff --git a/src/rule.c b/src/rule.c
index dcf1646a..8e426c5f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1286,11 +1286,11 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const 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;
@@ -1356,14 +1356,13 @@ void nft_cmd_expand(struct cmd *cmd)
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 		set = cmd->set;
+		if (!set->init)
+			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_SETELEMS, &h,
-				&set->location, newset);
+				&set->location, set_get(set));
 		list_add(&new->list, &cmd->list);
 		break;
 	default:
@@ -1461,7 +1460,7 @@ void cmd_free(struct cmd *cmd)
 #include <netlink.h>
 #include <mnl.h>
 
-static int __do_add_setelems(struct netlink_ctx *ctx, struct set *set,
+static int __do_add_elements(struct netlink_ctx *ctx, struct set *set,
 			     struct expr *expr, uint32_t flags)
 {
 	expr->set_flags |= set->flags;
@@ -1481,7 +1480,7 @@ static int __do_add_setelems(struct netlink_ctx *ctx, struct set *set,
 	return 0;
 }
 
-static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
+static int do_add_elements(struct netlink_ctx *ctx, struct cmd *cmd,
 			   uint32_t flags)
 {
 	struct expr *init = cmd->expr;
@@ -1493,27 +1492,35 @@ static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
 			     &ctx->nft->output) < 0)
 		return -1;
 
-	return __do_add_setelems(ctx, set, init, flags);
+	return __do_add_elements(ctx, set, init, flags);
+}
+
+static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
+			   uint32_t flags)
+{
+	struct set *set = cmd->set;
+
+	return __do_add_elements(ctx, set, set->init, flags);
 }
 
 static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
-		      uint32_t flags, bool add)
+		      uint32_t flags)
 {
 	struct set *set = cmd->set;
 
 	if (set->init != NULL) {
+		/* Update set->init->size (NFTNL_SET_DESC_SIZE) before adding
+		 * the set to the kernel. Calling this from do_add_setelems()
+		 * comes too late which might result in spurious ENFILE errors.
+		 */
 		if (set_is_non_concat_range(set) &&
 		    set_to_intervals(ctx->msgs, set, set->init, true,
 				     ctx->nft->debug_mask, set->automerge,
 				     &ctx->nft->output) < 0)
 			return -1;
 	}
-	if (add && mnl_nft_set_add(ctx, cmd, flags) < 0)
-		return -1;
-	if (set->init != NULL) {
-		return __do_add_setelems(ctx, set, set->init, flags);
-	}
-	return 0;
+
+	return mnl_nft_set_add(ctx, cmd, flags);
 }
 
 static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
@@ -1531,11 +1538,11 @@ 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, true);
+		return do_add_set(ctx, cmd, flags);
 	case CMD_OBJ_SETELEMS:
-		return do_add_set(ctx, cmd, flags, false);
-	case CMD_OBJ_ELEMENTS:
 		return do_add_setelems(ctx, cmd, flags);
+	case CMD_OBJ_ELEMENTS:
+		return do_add_elements(ctx, cmd, flags);
 	case CMD_OBJ_COUNTER:
 	case CMD_OBJ_QUOTA:
 	case CMD_OBJ_CT_HELPER:
diff --git a/tests/shell/testcases/nft-f/0026listing_0 b/tests/shell/testcases/nft-f/0026listing_0
new file mode 100755
index 00000000..0f2f27c6
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0026listing_0
@@ -0,0 +1,14 @@
+#!/bin/bash
+
+# This is like "flush ruleset" except only flushes THIS ruleset, not ALL rulesets.
+# In particular, it leaves the dynamic sshguard/fail2ban deny lists untouched.
+RULESET="add table A
+delete table A
+table A {
+    chain B {
+        tcp dport {1,2} accept
+    }
+}
+list ruleset"
+
+exec $NFT -f - <<< "$RULESET"