Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] src: add `set_is_meter` helper.
@ 2019-11-05 21:31 Jeremy Sowden
  2019-11-05 21:40 ` [PATCH nft] " Jeremy Sowden
  2019-11-06 14:37 ` [PATCH] " Florian Westphal
  0 siblings, 2 replies; 3+ messages in thread
From: Jeremy Sowden @ 2019-11-05 21:31 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

The sets constructed for meters are flagged as anonymous and dynamic.
However, in some places there are only checks that they are dynamic,
which can lead to normal sets being classified as meters.

For example:

  # nft add table t
  # nft add set t s { type ipv4_addr; size 256; flags dynamic,timeout; }
  # nft add chain t c
  # nft add rule t c tcp dport 80 meter m size 128 { ip saddr limit rate 10/second }
  # nft list meters
  table ip t {
          set s {
                  type ipv4_addr
                  size 256
                  flags dynamic,timeout
          }
          meter m {
                  type ipv4_addr
                  size 128
                  flags dynamic
          }
  }
  # nft list meter t m
  table ip t {
          meter m {
                  type ipv4_addr
                  size 128
                  flags dynamic
          }
  }
  # nft list meter t s
  Error: No such file or directory
  list meter t s
               ^

Add a new helper `set_is_meter` and use it wherever there are checks for
meters.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/rule.h                              |  5 ++++
 src/evaluate.c                              |  6 ++---
 src/expression.c                            | 12 ++++-----
 src/json.c                                  |  4 +--
 src/rule.c                                  |  9 +++----
 tests/shell/testcases/sets/0038meter_list_0 | 29 +++++++++++++++++++++
 6 files changed, 47 insertions(+), 18 deletions(-)
 create mode 100755 tests/shell/testcases/sets/0038meter_list_0

diff --git a/include/rule.h b/include/rule.h
index 2708cbebc9f8..627741f8e82b 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -349,6 +349,11 @@ static inline bool map_is_literal(uint32_t set_flags)
 	return !(set_is_anonymous(set_flags) || !set_is_map(set_flags));
 }
 
+static inline bool set_is_meter(uint32_t set_flags)
+{
+	return set_is_anonymous(set_flags) && (set_flags & NFT_SET_EVAL);
+}
+
 #include <statement.h>
 
 struct counter {
diff --git a/src/evaluate.c b/src/evaluate.c
index a56cd2a56e99..85a218107392 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3870,8 +3870,7 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 		if (set == NULL)
 			return set_not_found(ctx, &ctx->cmd->handle.set.location,
 					     ctx->cmd->handle.set.name);
-		else if (!(set->flags & NFT_SET_EVAL) ||
-			 !(set->flags & NFT_SET_ANONYMOUS))
+		else if (!set_is_meter(set->flags))
 			return cmd_error(ctx, &ctx->cmd->handle.set.location,
 					 "%s", strerror(ENOENT));
 
@@ -4008,8 +4007,7 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 		if (set == NULL)
 			return set_not_found(ctx, &ctx->cmd->handle.set.location,
 					     ctx->cmd->handle.set.name);
-		else if (!(set->flags & NFT_SET_EVAL) ||
-			 !(set->flags & NFT_SET_ANONYMOUS))
+		else if (!set_is_meter(set->flags))
 			return cmd_error(ctx, &ctx->cmd->handle.set.location,
 					 "%s", strerror(ENOENT));
 
diff --git a/src/expression.c b/src/expression.c
index e456010ff8b0..5070b1014392 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1051,14 +1051,12 @@ struct expr *map_expr_alloc(const struct location *loc, struct expr *arg,
 
 static void set_ref_expr_print(const struct expr *expr, struct output_ctx *octx)
 {
-	if (set_is_anonymous(expr->set->flags)) {
-		if (expr->set->flags & NFT_SET_EVAL)
-			nft_print(octx, "%s", expr->set->handle.set.name);
-		else
-			expr_print(expr->set->init, octx);
-	} else {
+	if (set_is_meter(expr->set->flags))
+		nft_print(octx, "%s", expr->set->handle.set.name);
+	else if (set_is_anonymous(expr->set->flags))
+		expr_print(expr->set->init, octx);
+	else
 		nft_print(octx, "@%s", expr->set->handle.set.name);
-	}
 }
 
 static void set_ref_expr_clone(struct expr *new, const struct expr *expr)
diff --git a/src/json.c b/src/json.c
index 13a064249d90..50435adce55a 100644
--- a/src/json.c
+++ b/src/json.c
@@ -86,7 +86,7 @@ static json_t *set_print_json(struct output_ctx *octx, const struct set *set)
 	} else if (set_is_objmap(set->flags)) {
 		type = "map";
 		datatype_ext = obj_type_name(set->objtype);
-	} else if (set->flags & NFT_SET_EVAL) {
+	} else if (set_is_meter(set->flags)) {
 		type = "meter";
 	} else {
 		type = "set";
@@ -1674,7 +1674,7 @@ static json_t *do_list_sets_json(struct netlink_ctx *ctx, struct cmd *cmd)
 			    !set_is_literal(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_METERS &&
-			    !(set->flags & NFT_SET_EVAL))
+			    !set_is_meter(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_MAPS &&
 			    !map_is_literal(set->flags))
diff --git a/src/rule.c b/src/rule.c
index 64756bcee6b8..f7e19735c3b3 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -446,8 +446,7 @@ static void set_print_declaration(const struct set *set,
 	const char *type;
 	uint32_t flags;
 
-	if ((set->flags & (NFT_SET_EVAL | NFT_SET_ANONYMOUS)) ==
-				(NFT_SET_EVAL | NFT_SET_ANONYMOUS))
+	if (set_is_meter(set->flags))
 		type = "meter";
 	else if (set_is_map(set->flags))
 		type = "map";
@@ -534,11 +533,11 @@ static void set_print_declaration(const struct set *set,
 }
 
 static void do_set_print(const struct set *set, struct print_fmt_options *opts,
-			  struct output_ctx *octx)
+			 struct output_ctx *octx)
 {
 	set_print_declaration(set, opts, octx);
 
-	if ((set->flags & NFT_SET_EVAL && nft_output_stateless(octx)) ||
+	if ((set_is_meter(set->flags) && nft_output_stateless(octx)) ||
 	    nft_output_terse(octx)) {
 		nft_print(octx, "%s}%s", opts->tab, opts->nl);
 		return;
@@ -1679,7 +1678,7 @@ static int do_list_sets(struct netlink_ctx *ctx, struct cmd *cmd)
 			    !set_is_literal(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_METERS &&
-			    !(set->flags & NFT_SET_EVAL))
+			    !set_is_meter(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_MAPS &&
 			    !map_is_literal(set->flags))
diff --git a/tests/shell/testcases/sets/0038meter_list_0 b/tests/shell/testcases/sets/0038meter_list_0
new file mode 100755
index 000000000000..e9e0f6fb02b1
--- /dev/null
+++ b/tests/shell/testcases/sets/0038meter_list_0
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+#
+# Listing meters should not include dynamic sets in the output
+#
+
+set -e
+
+RULESET="
+  add table t
+  add set t s { type ipv4_addr; size 256; flags dynamic,timeout; }
+  add chain t c
+  add rule t c tcp dport 80 meter m size 128 { ip saddr limit rate 10/second }
+"
+
+expected_output="table ip t {
+	meter m {
+		type ipv4_addr
+		size 128
+		flags dynamic
+	}
+}"
+
+$NFT -f - <<< "$RULESET"
+
+test_output=$($NFT list meters)
+
+test "$test_output" = "$expected_output"
+
-- 
2.24.0.rc1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH nft] src: add `set_is_meter` helper.
  2019-11-05 21:31 [PATCH] src: add `set_is_meter` helper Jeremy Sowden
@ 2019-11-05 21:40 ` " Jeremy Sowden
  2019-11-06 14:37 ` [PATCH] " Florian Westphal
  1 sibling, 0 replies; 3+ messages in thread
From: Jeremy Sowden @ 2019-11-05 21:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 45 bytes --]

Resending with the right subject prefix.

J.

[-- Attachment #2: 0001-src-add-set_is_meter-helper.patch --]
[-- Type: text/x-diff, Size: 6854 bytes --]

From 4c3519ba46b4b9cfd24dd94246a40f4857be0e32 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Mon, 4 Nov 2019 09:18:07 +0000
Subject: [PATCH nft] src: add `set_is_meter` helper.

The sets constructed for meters are flagged as anonymous and dynamic.
However, in some places there are only checks that they are dynamic,
which can lead to normal sets being classified as meters.

For example:

  # nft add table t
  # nft add set t s { type ipv4_addr; size 256; flags dynamic,timeout; }
  # nft add chain t c
  # nft add rule t c tcp dport 80 meter m size 128 { ip saddr limit rate 10/second }
  # nft list meters
  table ip t {
          set s {
                  type ipv4_addr
                  size 256
                  flags dynamic,timeout
          }
          meter m {
                  type ipv4_addr
                  size 128
                  flags dynamic
          }
  }
  # nft list meter t m
  table ip t {
          meter m {
                  type ipv4_addr
                  size 128
                  flags dynamic
          }
  }
  # nft list meter t s
  Error: No such file or directory
  list meter t s
               ^

Add a new helper `set_is_meter` and use it wherever there are checks for
meters.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/rule.h                              |  5 ++++
 src/evaluate.c                              |  6 ++---
 src/expression.c                            | 12 ++++-----
 src/json.c                                  |  4 +--
 src/rule.c                                  |  9 +++----
 tests/shell/testcases/sets/0038meter_list_0 | 29 +++++++++++++++++++++
 6 files changed, 47 insertions(+), 18 deletions(-)
 create mode 100755 tests/shell/testcases/sets/0038meter_list_0

diff --git a/include/rule.h b/include/rule.h
index 2708cbebc9f8..627741f8e82b 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -349,6 +349,11 @@ static inline bool map_is_literal(uint32_t set_flags)
 	return !(set_is_anonymous(set_flags) || !set_is_map(set_flags));
 }
 
+static inline bool set_is_meter(uint32_t set_flags)
+{
+	return set_is_anonymous(set_flags) && (set_flags & NFT_SET_EVAL);
+}
+
 #include <statement.h>
 
 struct counter {
diff --git a/src/evaluate.c b/src/evaluate.c
index a56cd2a56e99..85a218107392 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3870,8 +3870,7 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 		if (set == NULL)
 			return set_not_found(ctx, &ctx->cmd->handle.set.location,
 					     ctx->cmd->handle.set.name);
-		else if (!(set->flags & NFT_SET_EVAL) ||
-			 !(set->flags & NFT_SET_ANONYMOUS))
+		else if (!set_is_meter(set->flags))
 			return cmd_error(ctx, &ctx->cmd->handle.set.location,
 					 "%s", strerror(ENOENT));
 
@@ -4008,8 +4007,7 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 		if (set == NULL)
 			return set_not_found(ctx, &ctx->cmd->handle.set.location,
 					     ctx->cmd->handle.set.name);
-		else if (!(set->flags & NFT_SET_EVAL) ||
-			 !(set->flags & NFT_SET_ANONYMOUS))
+		else if (!set_is_meter(set->flags))
 			return cmd_error(ctx, &ctx->cmd->handle.set.location,
 					 "%s", strerror(ENOENT));
 
diff --git a/src/expression.c b/src/expression.c
index e456010ff8b0..5070b1014392 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1051,14 +1051,12 @@ struct expr *map_expr_alloc(const struct location *loc, struct expr *arg,
 
 static void set_ref_expr_print(const struct expr *expr, struct output_ctx *octx)
 {
-	if (set_is_anonymous(expr->set->flags)) {
-		if (expr->set->flags & NFT_SET_EVAL)
-			nft_print(octx, "%s", expr->set->handle.set.name);
-		else
-			expr_print(expr->set->init, octx);
-	} else {
+	if (set_is_meter(expr->set->flags))
+		nft_print(octx, "%s", expr->set->handle.set.name);
+	else if (set_is_anonymous(expr->set->flags))
+		expr_print(expr->set->init, octx);
+	else
 		nft_print(octx, "@%s", expr->set->handle.set.name);
-	}
 }
 
 static void set_ref_expr_clone(struct expr *new, const struct expr *expr)
diff --git a/src/json.c b/src/json.c
index 13a064249d90..50435adce55a 100644
--- a/src/json.c
+++ b/src/json.c
@@ -86,7 +86,7 @@ static json_t *set_print_json(struct output_ctx *octx, const struct set *set)
 	} else if (set_is_objmap(set->flags)) {
 		type = "map";
 		datatype_ext = obj_type_name(set->objtype);
-	} else if (set->flags & NFT_SET_EVAL) {
+	} else if (set_is_meter(set->flags)) {
 		type = "meter";
 	} else {
 		type = "set";
@@ -1674,7 +1674,7 @@ static json_t *do_list_sets_json(struct netlink_ctx *ctx, struct cmd *cmd)
 			    !set_is_literal(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_METERS &&
-			    !(set->flags & NFT_SET_EVAL))
+			    !set_is_meter(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_MAPS &&
 			    !map_is_literal(set->flags))
diff --git a/src/rule.c b/src/rule.c
index 64756bcee6b8..f7e19735c3b3 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -446,8 +446,7 @@ static void set_print_declaration(const struct set *set,
 	const char *type;
 	uint32_t flags;
 
-	if ((set->flags & (NFT_SET_EVAL | NFT_SET_ANONYMOUS)) ==
-				(NFT_SET_EVAL | NFT_SET_ANONYMOUS))
+	if (set_is_meter(set->flags))
 		type = "meter";
 	else if (set_is_map(set->flags))
 		type = "map";
@@ -534,11 +533,11 @@ static void set_print_declaration(const struct set *set,
 }
 
 static void do_set_print(const struct set *set, struct print_fmt_options *opts,
-			  struct output_ctx *octx)
+			 struct output_ctx *octx)
 {
 	set_print_declaration(set, opts, octx);
 
-	if ((set->flags & NFT_SET_EVAL && nft_output_stateless(octx)) ||
+	if ((set_is_meter(set->flags) && nft_output_stateless(octx)) ||
 	    nft_output_terse(octx)) {
 		nft_print(octx, "%s}%s", opts->tab, opts->nl);
 		return;
@@ -1679,7 +1678,7 @@ static int do_list_sets(struct netlink_ctx *ctx, struct cmd *cmd)
 			    !set_is_literal(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_METERS &&
-			    !(set->flags & NFT_SET_EVAL))
+			    !set_is_meter(set->flags))
 				continue;
 			if (cmd->obj == CMD_OBJ_MAPS &&
 			    !map_is_literal(set->flags))
diff --git a/tests/shell/testcases/sets/0038meter_list_0 b/tests/shell/testcases/sets/0038meter_list_0
new file mode 100755
index 000000000000..e9e0f6fb02b1
--- /dev/null
+++ b/tests/shell/testcases/sets/0038meter_list_0
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+#
+# Listing meters should not include dynamic sets in the output
+#
+
+set -e
+
+RULESET="
+  add table t
+  add set t s { type ipv4_addr; size 256; flags dynamic,timeout; }
+  add chain t c
+  add rule t c tcp dport 80 meter m size 128 { ip saddr limit rate 10/second }
+"
+
+expected_output="table ip t {
+	meter m {
+		type ipv4_addr
+		size 128
+		flags dynamic
+	}
+}"
+
+$NFT -f - <<< "$RULESET"
+
+test_output=$($NFT list meters)
+
+test "$test_output" = "$expected_output"
+
-- 
2.24.0.rc1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] src: add `set_is_meter` helper.
  2019-11-05 21:31 [PATCH] src: add `set_is_meter` helper Jeremy Sowden
  2019-11-05 21:40 ` [PATCH nft] " Jeremy Sowden
@ 2019-11-06 14:37 ` " Florian Westphal
  1 sibling, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2019-11-06 14:37 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Pablo Neira Ayuso, Netfilter Devel

Jeremy Sowden <jeremy@azazel.net> wrote:
> The sets constructed for meters are flagged as anonymous and dynamic.
> However, in some places there are only checks that they are dynamic,
> which can lead to normal sets being classified as meters.

Applied, thanks for including a test case.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 21:31 [PATCH] src: add `set_is_meter` helper Jeremy Sowden
2019-11-05 21:40 ` [PATCH nft] " Jeremy Sowden
2019-11-06 14:37 ` [PATCH] " Florian Westphal

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git