netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [nft PATCH v3 0/3] Fix printing of range elements in named sets
@ 2017-07-18 15:40 Phil Sutter
  2017-07-18 15:40 ` [nft PATCH 1/3] segtree: Introduce flag for half-open range elements Phil Sutter
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Phil Sutter @ 2017-07-18 15:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Arturo Borrero Gonzalez

This is the third attempt at fixing 'nft monitor' for range elements.
This time introduce a flag which allows to identify half-open ranges,
then reuse Arturo's patch since it works nicely after a few adjustments.
Finally resubmit my 'nft monitor' test framework without any changes to
it's first version.

Arturo Borrero Gonzalez (1):
  monitor: Fix printing of range elements in named sets

Phil Sutter (2):
  segtree: Introduce flag for half-open range elements
  tests: Add basic monitor testing framework

 include/expression.h                   |   1 +
 include/rule.h                         |   9 +++
 src/netlink.c                          | 121 ++++++++++++++++++++++++++++-----
 src/segtree.c                          |   5 ++
 tests/monitor/run-tests.sh             |  78 +++++++++++++++++++++
 tests/monitor/testcases/set-mixed.t    |  21 ++++++
 tests/monitor/testcases/set-multiple.t |  15 ++++
 tests/monitor/testcases/set-simple.t   |  49 +++++++++++++
 8 files changed, 282 insertions(+), 17 deletions(-)
 create mode 100755 tests/monitor/run-tests.sh
 create mode 100644 tests/monitor/testcases/set-mixed.t
 create mode 100644 tests/monitor/testcases/set-multiple.t
 create mode 100644 tests/monitor/testcases/set-simple.t

-- 
2.13.1


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

* [nft PATCH 1/3] segtree: Introduce flag for half-open range elements
  2017-07-18 15:40 [nft PATCH v3 0/3] Fix printing of range elements in named sets Phil Sutter
@ 2017-07-18 15:40 ` Phil Sutter
  2017-07-18 16:44   ` Pablo Neira Ayuso
  2017-07-18 15:40 ` [nft PATCH v2 2/3] monitor: Fix printing of range elements in named sets Phil Sutter
  2017-07-18 15:40 ` [nft PATCH 3/3] tests: Add basic monitor testing framework Phil Sutter
  2 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2017-07-18 15:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Arturo Borrero Gonzalez

This flag is required by userspace only, so can live within userdata.
It's sole purpose is for 'nft monitor' to detect half-open ranges (which
are comprised of a single element only).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/expression.h |  1 +
 include/rule.h       |  7 ++++++
 src/netlink.c        | 66 ++++++++++++++++++++++++++++++++++++++--------------
 src/segtree.c        |  5 ++++
 4 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index 68a36e8af792a..202eb4c140eda 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -180,6 +180,7 @@ enum expr_flags {
 	EXPR_F_PROTOCOL		= 0x4,
 	EXPR_F_INTERVAL_END	= 0x8,
 	EXPR_F_BOOLEAN		= 0x10,
+	EXPR_F_INTERVAL_OPEN	= 0x20,
 };
 
 #include <payload.h>
diff --git a/include/rule.h b/include/rule.h
index 7424b21c6e019..592c93ddd0e2f 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -503,4 +503,11 @@ enum udata_set_type {
 };
 #define UDATA_SET_MAX (__UDATA_SET_MAX - 1)
 
+enum udata_set_elem_type {
+	UDATA_SET_ELEM_COMMENT,
+	UDATA_SET_ELEM_FLAGS,
+	__UDATA_SET_ELEM_MAX,
+};
+#define UDATA_SET_ELEM_MAX (__UDATA_SET_ELEM_MAX - 1)
+
 #endif /* NFTABLES_RULE_H */
diff --git a/src/netlink.c b/src/netlink.c
index 2e30622de4bb1..be26188e097aa 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -211,7 +211,7 @@ static struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
 	const struct expr *elem, *key, *data;
 	struct nftnl_set_elem *nlse;
 	struct nft_data_linearize nld;
-	struct nftnl_udata_buf *udbuf;
+	struct nftnl_udata_buf *udbuf = NULL;
 
 	nlse = nftnl_set_elem_alloc();
 	if (nlse == NULL)
@@ -232,13 +232,22 @@ static struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
 	if (elem->timeout)
 		nftnl_set_elem_set_u64(nlse, NFTNL_SET_ELEM_TIMEOUT,
 				       elem->timeout);
-	if (elem->comment) {
+	if (elem->comment || expr->flags & EXPR_F_INTERVAL_OPEN) {
 		udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
 		if (!udbuf)
 			memory_allocation_error();
-		if (!nftnl_udata_put_strz(udbuf, UDATA_TYPE_COMMENT,
+	}
+	if (elem->comment) {
+		if (!nftnl_udata_put_strz(udbuf, UDATA_SET_ELEM_COMMENT,
 					  elem->comment))
 			memory_allocation_error();
+	}
+	if (expr->flags & EXPR_F_INTERVAL_OPEN) {
+		if (!nftnl_udata_put_u32(udbuf, UDATA_SET_ELEM_FLAGS,
+					 EXPR_F_INTERVAL_OPEN))
+			memory_allocation_error();
+	}
+	if (udbuf) {
 		nftnl_set_elem_set(nlse, NFTNL_SET_ELEM_USERDATA,
 				   nftnl_udata_buf_data(udbuf),
 				   nftnl_udata_buf_len(udbuf));
@@ -1603,17 +1612,45 @@ static int parse_udata_cb(const struct nftnl_udata *attr, void *data)
 	return 0;
 }
 
-static char *udata_get_comment(const void *data, uint32_t data_len)
+static int set_elem_parse_udata_cb(const struct nftnl_udata *attr, void *data)
 {
-	const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
+	const struct nftnl_udata **tb = data;
+	unsigned char *value = nftnl_udata_get(attr);
+	uint8_t type = nftnl_udata_type(attr);
+	uint8_t len = nftnl_udata_len(attr);
 
-	if (nftnl_udata_parse(data, data_len, parse_udata_cb, tb) < 0)
-		return NULL;
+	switch (type) {
+	case UDATA_SET_ELEM_COMMENT:
+		if (value[len - 1] != '\0')
+			return -1;
+		break;
+	case UDATA_SET_ELEM_FLAGS:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	default:
+		return 0;
+	}
+	tb[type] = attr;
+	return 0;
+}
 
-	if (!tb[UDATA_TYPE_COMMENT])
-		return NULL;
+static void set_elem_parse_udata(struct nftnl_set_elem *nlse,
+				 struct expr *expr)
+{
+	const struct nftnl_udata *ud[UDATA_SET_ELEM_MAX + 1] = {};
+	const void *data;
+	uint32_t len;
 
-	return xstrdup(nftnl_udata_get(tb[UDATA_TYPE_COMMENT]));
+	data = nftnl_set_elem_get(nlse, NFTNL_SET_ELEM_USERDATA, &len);
+	if (nftnl_udata_parse(data, len, set_elem_parse_udata_cb, ud))
+		return;
+
+	if (ud[UDATA_SET_ELEM_COMMENT])
+		expr->comment =
+			xstrdup(nftnl_udata_get(ud[UDATA_SET_ELEM_COMMENT]));
+	if (ud[UDATA_SET_ELEM_FLAGS])
+		expr->flags |= nftnl_udata_get_u32(ud[UDATA_SET_ELEM_FLAGS]);
 }
 
 static int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
@@ -1647,13 +1684,8 @@ static int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 		expr->timeout	 = nftnl_set_elem_get_u64(nlse, NFTNL_SET_ELEM_TIMEOUT);
 	if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_EXPIRATION))
 		expr->expiration = nftnl_set_elem_get_u64(nlse, NFTNL_SET_ELEM_EXPIRATION);
-	if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_USERDATA)) {
-		const void *data;
-		uint32_t len;
-
-		data = nftnl_set_elem_get(nlse, NFTNL_SET_ELEM_USERDATA, &len);
-		expr->comment = udata_get_comment(data, len);
-	}
+	if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_USERDATA))
+		set_elem_parse_udata(nlse, expr);
 	if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_EXPR)) {
 		const struct nftnl_expr *nle;
 
diff --git a/src/segtree.c b/src/segtree.c
index f53536210018d..de98600bf1bcc 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -37,6 +37,7 @@ struct seg_tree {
 
 enum elementary_interval_flags {
 	EI_F_INTERVAL_END	= 0x1,
+	EI_F_INTERVAL_OPEN	= 0x2,
 };
 
 /**
@@ -512,6 +513,8 @@ static void segtree_linearize(struct list_head *list, const struct set *set,
 		mpz_bitmask(q, tree->keylen);
 		nei = ei_alloc(p, q, NULL, EI_F_INTERVAL_END);
 		list_add_tail(&nei->list, list);
+	} else {
+		prev->flags |= EI_F_INTERVAL_OPEN;
 	}
 
 	mpz_clear(p);
@@ -538,6 +541,8 @@ static void set_insert_interval(struct expr *set, struct seg_tree *tree,
 
 	if (ei->flags & EI_F_INTERVAL_END)
 		expr->flags |= EXPR_F_INTERVAL_END;
+	if (ei->flags & EI_F_INTERVAL_OPEN)
+		expr->flags |= EXPR_F_INTERVAL_OPEN;
 
 	compound_expr_add(set, expr);
 }
-- 
2.13.1


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

* [nft PATCH v2 2/3] monitor: Fix printing of range elements in named sets
  2017-07-18 15:40 [nft PATCH v3 0/3] Fix printing of range elements in named sets Phil Sutter
  2017-07-18 15:40 ` [nft PATCH 1/3] segtree: Introduce flag for half-open range elements Phil Sutter
@ 2017-07-18 15:40 ` Phil Sutter
  2017-07-18 15:40 ` [nft PATCH 3/3] tests: Add basic monitor testing framework Phil Sutter
  2 siblings, 0 replies; 5+ messages in thread
From: Phil Sutter @ 2017-07-18 15:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Arturo Borrero Gonzalez

From: Arturo Borrero Gonzalez <arturo@netfilter.org>

If you add set elements to interval sets, the output is wrong.
Fix this by caching first element of the range (first event),
then wait for the second element of the range (second event) to
print them both at the same time.

We also avoid printing the first null element required in the RB tree.

Before this patch:

% nft add element t s {10-20, 30-40}
add element ip t s { 0 }
add element ip t s { 10 }
add element ip t s { ftp }
add element ip t s { 30 }
add element ip t s { 41 }

After this patch:

% nft add element t s {10-20, 30-40}
add element ip t s { 10-20 }
add element ip t s { 30-40 }

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Rewrite netlink_event_range_cache() so it correctly handles half-open
  ranges.
- Enable elem caching for NFT_MSG_DELSETELEM events also.
- Drop call to expr_free() for set->rg_cache since that will be done
  when freeing dummyset.
---
 include/rule.h |  2 ++
 src/netlink.c  | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/include/rule.h b/include/rule.h
index 592c93ddd0e2f..ab376bdd5bcfd 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -217,6 +217,7 @@ extern struct rule *rule_lookup(const struct chain *chain, uint64_t handle);
  * @datalen:	mapping data len
  * @objtype:	mapping object type
  * @init:	initializer
+ * @rg_cache:	cached range element (left)
  * @policy:	set mechanism policy
  * @desc:	set mechanism desc
  */
@@ -234,6 +235,7 @@ struct set {
 	unsigned int		datalen;
 	uint32_t		objtype;
 	struct expr		*init;
+	struct expr		*rg_cache;
 	uint32_t		policy;
 	struct {
 		uint32_t	size;
diff --git a/src/netlink.c b/src/netlink.c
index be26188e097aa..e1db8e7be923c 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -2228,6 +2228,51 @@ out:
 	return MNL_CB_OK;
 }
 
+/* returns true if the event should be ignored (i.e. null element) */
+static bool netlink_event_ignore_range_event(struct nftnl_set_elem *nlse)
+{
+        uint32_t flags = 0;
+
+	if (nftnl_set_elem_is_set(nlse, NFTNL_SET_ELEM_FLAGS))
+		flags = nftnl_set_elem_get_u32(nlse, NFTNL_SET_ELEM_FLAGS);
+	if (!(flags & NFT_SET_ELEM_INTERVAL_END))
+		return false;
+
+	if (nftnl_set_elem_get_u32(nlse, NFTNL_SET_ELEM_KEY) != 0)
+		return false;
+
+	return true;
+}
+
+/* returns true if the we cached the range element */
+static bool netlink_event_range_cache(struct set *cached_set,
+				      struct set *dummyset)
+{
+	struct expr *elem;
+
+	/* not an interval ? */
+	if (!(cached_set->flags & NFT_SET_INTERVAL))
+		return false;
+
+	/* if cache exists, dummyset must contain the other end of the range */
+	if (cached_set->rg_cache) {
+		compound_expr_add(dummyset->init, cached_set->rg_cache);
+		cached_set->rg_cache = NULL;
+		goto out_decompose;
+	}
+
+	/* don't cache half-open range elements */
+	elem = list_entry(dummyset->init->expressions.prev, struct expr, list);
+	if (!(elem->flags & EXPR_F_INTERVAL_OPEN)) {
+		cached_set->rg_cache = expr_clone(elem);
+		return true;
+	}
+
+out_decompose:
+	interval_map_decompose(dummyset->init);
+	return false;
+}
+
 static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 				     struct netlink_mon_handler *monh)
 {
@@ -2270,6 +2315,11 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 
 		nlse = nftnl_set_elems_iter_next(nlsei);
 		while (nlse != NULL) {
+			if (netlink_event_ignore_range_event(nlse)) {
+				set_free(dummyset);
+				nftnl_set_elems_iter_destroy(nlsei);
+				goto out;
+			}
 			if (netlink_delinearize_setelem(nlse, dummyset) < 0) {
 				set_free(dummyset);
 				nftnl_set_elems_iter_destroy(nlsei);
@@ -2279,6 +2329,11 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		}
 		nftnl_set_elems_iter_destroy(nlsei);
 
+		if (netlink_event_range_cache(set, dummyset)) {
+			set_free(dummyset);
+			goto out;
+		}
+
 		switch (type) {
 		case NFT_MSG_NEWSETELEM:
 			printf("add ");
-- 
2.13.1


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

* [nft PATCH 3/3] tests: Add basic monitor testing framework
  2017-07-18 15:40 [nft PATCH v3 0/3] Fix printing of range elements in named sets Phil Sutter
  2017-07-18 15:40 ` [nft PATCH 1/3] segtree: Introduce flag for half-open range elements Phil Sutter
  2017-07-18 15:40 ` [nft PATCH v2 2/3] monitor: Fix printing of range elements in named sets Phil Sutter
@ 2017-07-18 15:40 ` Phil Sutter
  2 siblings, 0 replies; 5+ messages in thread
From: Phil Sutter @ 2017-07-18 15:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Arturo Borrero Gonzalez

This implements testing of 'nft monitor' output correctness and adds a
number of testcases for named sets.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/monitor/run-tests.sh             | 78 ++++++++++++++++++++++++++++++++++
 tests/monitor/testcases/set-mixed.t    | 21 +++++++++
 tests/monitor/testcases/set-multiple.t | 15 +++++++
 tests/monitor/testcases/set-simple.t   | 49 +++++++++++++++++++++
 4 files changed, 163 insertions(+)
 create mode 100755 tests/monitor/run-tests.sh
 create mode 100644 tests/monitor/testcases/set-mixed.t
 create mode 100644 tests/monitor/testcases/set-multiple.t
 create mode 100644 tests/monitor/testcases/set-simple.t

diff --git a/tests/monitor/run-tests.sh b/tests/monitor/run-tests.sh
new file mode 100755
index 0000000000000..7447adf1febd6
--- /dev/null
+++ b/tests/monitor/run-tests.sh
@@ -0,0 +1,78 @@
+#!/bin/bash
+
+cd $(dirname $0)
+
+testdir=$(mktemp -d)
+if [ ! -d $testdir ]; then
+	echo "Failed to create test directory" >&2
+	exit 0
+fi
+trap "rm -rf $testdir" EXIT
+
+nft=../../src/nft
+command_file=$(mktemp -p $testdir)
+output_file=$(mktemp -p $testdir)
+
+cmd_append() {
+	echo "$*" >>$command_file
+}
+output_append() {
+	echo "$*" >>$output_file
+}
+run_test() {
+	monitor_output=$(mktemp -p $testdir)
+	$nft monitor >$monitor_output &
+	monitor_pid=$!
+
+	sleep 0.5
+
+	$nft -f $command_file || {
+		echo "nft command failed!"
+		kill $monitor_pid
+		wait >/dev/null 2>&1
+		exit 1
+	}
+	sleep 0.5
+	kill $monitor_pid
+	wait >/dev/null 2>&1
+	if ! diff -Z -q $monitor_output $output_file >/dev/null 2>&1; then
+		echo "monitor output differs!"
+		diff -Z -u $output_file $monitor_output
+		exit 1
+	fi
+	rm $command_file
+	rm $output_file
+	touch $command_file
+	touch $output_file
+}
+
+for testcase in testcases/*.t; do
+	echo "running tests from file $(basename $testcase)"
+	# files are like this:
+	#
+	# I add table ip t
+	# O add table ip t
+	# I add chain ip t c
+	# O add chain ip t c
+
+	$nft flush ruleset
+
+	input_complete=false
+	while read dir line; do
+		case $dir in
+		I)
+			$input_complete && run_test
+			input_complete=false
+			cmd_append "$line"
+			;;
+		O)
+			input_complete=true
+			output_append "$line"
+			;;
+		'#'|'')
+			# ignore comments and empty lines
+			;;
+		esac
+	done <$testcase
+	$input_complete && run_test
+done
diff --git a/tests/monitor/testcases/set-mixed.t b/tests/monitor/testcases/set-mixed.t
new file mode 100644
index 0000000000000..afdfd32deab66
--- /dev/null
+++ b/tests/monitor/testcases/set-mixed.t
@@ -0,0 +1,21 @@
+# first the setup
+I add table ip t
+O add table ip t
+I add chain ip t c
+O add chain ip t c
+I add set ip t portrange { type inet_service; flags interval; }
+O add set ip t portrange { type inet_service;flags interval }
+I add set ip t ports { type inet_service; }
+O add set ip t ports { type inet_service;}
+
+# make sure concurrent adds work
+I add element ip t portrange { 1024-65535 }
+I add element ip t ports { 10 }
+O add element ip t portrange { 1024-65535 }
+O add element ip t ports { 10 }
+
+# delete items again
+I delete element ip t portrange { 1024-65535 }
+I delete element ip t ports { 10 }
+O delete element ip t portrange { 1024-65535 }
+O delete element ip t ports { 10 }
diff --git a/tests/monitor/testcases/set-multiple.t b/tests/monitor/testcases/set-multiple.t
new file mode 100644
index 0000000000000..c017678d9d074
--- /dev/null
+++ b/tests/monitor/testcases/set-multiple.t
@@ -0,0 +1,15 @@
+# first the setup
+I add table ip t
+O add table ip t
+I add chain ip t c
+O add chain ip t c
+I add set ip t portrange { type inet_service; flags interval; }
+O add set ip t portrange { type inet_service;flags interval }
+I add set ip t portrange2 { type inet_service; flags interval; }
+O add set ip t portrange2 { type inet_service;flags interval }
+
+# make sure concurrent adds work
+I add element ip t portrange { 1024-65535 }
+I add element ip t portrange2 { 10-20 }
+O add element ip t portrange { 1024-65535 }
+O add element ip t portrange2 { 10-20 }
diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
new file mode 100644
index 0000000000000..64b6e3456bf4e
--- /dev/null
+++ b/tests/monitor/testcases/set-simple.t
@@ -0,0 +1,49 @@
+# first the setup
+I add table ip t
+O add table ip t
+I add chain ip t c
+O add chain ip t c
+I add set ip t portrange { type inet_service; flags interval; }
+O add set ip t portrange { type inet_service;flags interval }
+
+# adding some ranges
+I add element ip t portrange { 1-10 }
+O add element ip t portrange { 1-10 }
+I add element ip t portrange { 1024-65535 }
+O add element ip t portrange { 1024-65535 }
+I add element ip t portrange { 20-30, 40-50 }
+O add element ip t portrange { 20-30 }
+O add element ip t portrange { 40-50 }
+
+# test flushing -> elements are removed in reverse
+I flush set ip t portrange
+O delete element ip t portrange { 1024-65535 }
+O delete element ip t portrange { 40-50 }
+O delete element ip t portrange { 20-30 }
+O delete element ip t portrange { 1-10 }
+
+# make sure lower scope boundary works
+I add element ip t portrange { 0-10 }
+O add element ip t portrange { 0-10 }
+
+# make sure half open before other element works
+I add element ip t portrange { 1024-65535 }
+I add element ip t portrange { 100-200 }
+O add element ip t portrange { 1024-65535 }
+O add element ip t portrange { 100-200 }
+
+# make sure deletion of elements works
+I delete element ip t portrange { 0-10 }
+O delete element ip t portrange { 0-10 }
+I delete element ip t portrange { 100-200 }
+I delete element ip t portrange { 1024-65535 }
+O delete element ip t portrange { 100-200 }
+O delete element ip t portrange { 1024-65535 }
+
+# make sure mixed add/delete works
+I add element ip t portrange { 10-20 }
+I add element ip t portrange { 1024-65535 }
+I delete element ip t portrange { 10-20 }
+O add element ip t portrange { 10-20 }
+O add element ip t portrange { 1024-65535 }
+O delete element ip t portrange { 10-20 }
-- 
2.13.1


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

* Re: [nft PATCH 1/3] segtree: Introduce flag for half-open range elements
  2017-07-18 15:40 ` [nft PATCH 1/3] segtree: Introduce flag for half-open range elements Phil Sutter
@ 2017-07-18 16:44   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2017-07-18 16:44 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel, Arturo Borrero Gonzalez

Hi Phil,

This series looks to good to me. Only one comment below.

On Tue, Jul 18, 2017 at 05:40:27PM +0200, Phil Sutter wrote:
> This flag is required by userspace only, so can live within userdata.
> It's sole purpose is for 'nft monitor' to detect half-open ranges (which
> are comprised of a single element only).
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/expression.h |  1 +
>  include/rule.h       |  7 ++++++
>  src/netlink.c        | 66 ++++++++++++++++++++++++++++++++++++++--------------
>  src/segtree.c        |  5 ++++
>  4 files changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/include/expression.h b/include/expression.h
> index 68a36e8af792a..202eb4c140eda 100644
> --- a/include/expression.h
> +++ b/include/expression.h
> @@ -180,6 +180,7 @@ enum expr_flags {
>  	EXPR_F_PROTOCOL		= 0x4,
>  	EXPR_F_INTERVAL_END	= 0x8,
>  	EXPR_F_BOOLEAN		= 0x10,
> +	EXPR_F_INTERVAL_OPEN	= 0x20,

Could you place this here?

diff --git a/include/expression.h b/include/expression.h
index 7ddcc3226267..12434b335b71 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -256,6 +256,7 @@ struct expr {
                        uint64_t                expiration;
                        const char              *comment;
                        struct stmt             *stmt;
+                       unsigned int            elem_flags;
                };
                struct {
                        /* EXPR_UNARY */

We can probably move EXPR_F_INTERVAL_END there too in a follow up
patch.

>  };
>  
>  #include <payload.h>
> diff --git a/include/rule.h b/include/rule.h
> index 7424b21c6e019..592c93ddd0e2f 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -503,4 +503,11 @@ enum udata_set_type {
>  };
>  #define UDATA_SET_MAX (__UDATA_SET_MAX - 1)
>  
> +enum udata_set_elem_type {
> +	UDATA_SET_ELEM_COMMENT,
> +	UDATA_SET_ELEM_FLAGS,
> +	__UDATA_SET_ELEM_MAX,
> +};
> +#define UDATA_SET_ELEM_MAX (__UDATA_SET_ELEM_MAX - 1)
> +
>  #endif /* NFTABLES_RULE_H */
> diff --git a/src/netlink.c b/src/netlink.c
> index 2e30622de4bb1..be26188e097aa 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -211,7 +211,7 @@ static struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
>  	const struct expr *elem, *key, *data;
>  	struct nftnl_set_elem *nlse;
>  	struct nft_data_linearize nld;
> -	struct nftnl_udata_buf *udbuf;
> +	struct nftnl_udata_buf *udbuf = NULL;
>  
>  	nlse = nftnl_set_elem_alloc();
>  	if (nlse == NULL)
> @@ -232,13 +232,22 @@ static struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
>  	if (elem->timeout)
>  		nftnl_set_elem_set_u64(nlse, NFTNL_SET_ELEM_TIMEOUT,
>  				       elem->timeout);
> -	if (elem->comment) {
> +	if (elem->comment || expr->flags & EXPR_F_INTERVAL_OPEN) {
>  		udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
>  		if (!udbuf)
>  			memory_allocation_error();
> -		if (!nftnl_udata_put_strz(udbuf, UDATA_TYPE_COMMENT,
> +	}
> +	if (elem->comment) {
> +		if (!nftnl_udata_put_strz(udbuf, UDATA_SET_ELEM_COMMENT,
>  					  elem->comment))
>  			memory_allocation_error();
> +	}
> +	if (expr->flags & EXPR_F_INTERVAL_OPEN) {
> +		if (!nftnl_udata_put_u32(udbuf, UDATA_SET_ELEM_FLAGS,
> +					 EXPR_F_INTERVAL_OPEN))

Better make a new definition for this? Instead of using
EXPR_F_INTERVAL_OPEN which is 0x20.

Once we expose this value, we cannot change it otherwise we may break
nft between software updates, we should avoid this.

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

end of thread, other threads:[~2017-07-18 16:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-18 15:40 [nft PATCH v3 0/3] Fix printing of range elements in named sets Phil Sutter
2017-07-18 15:40 ` [nft PATCH 1/3] segtree: Introduce flag for half-open range elements Phil Sutter
2017-07-18 16:44   ` Pablo Neira Ayuso
2017-07-18 15:40 ` [nft PATCH v2 2/3] monitor: Fix printing of range elements in named sets Phil Sutter
2017-07-18 15:40 ` [nft PATCH 3/3] tests: Add basic monitor testing framework Phil Sutter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).