[nft PATCH v2 0/2] monitor: Fix printing of range elements in named sets

[nft PATCH v2 0/2] monitor: Fix printing of range elements in named sets

[Phil Sutter]

This is supposed to be v2 of my patch series sent earlier, but it turned
into a complete rewrite of the whole SETELEM callback and so there is no
reuse of former changes.

The second patch adds a simple testbench for 'nft monitor' output which
helped me quite a lot to get things right.

Phil Sutter (2):
  monitor: Rewrite SETELEM callback
  tests: Add basic monitor testing framework

 src/netlink.c                          | 272 ++++++++++++++++++++++++++-------
 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 ++++++
 5 files changed, 382 insertions(+), 53 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

[nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Phil Sutter]

Printing of SETELEM events for interval sets was broken as the
callback did not reassemble the two elements that (mostly) make up a
range. But since half-open ranges are represented by a single element
only and set flushes return the elements in reverse order, a fix was not
quite straightforward.

This patch implements an element cache needed for reassembly and makes
element printing dependent upon a number of conditions so that all cases
are treated correctly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 272 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 219 insertions(+), 53 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 2e30622de4bb1..18ccf7e4f4f65 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -2196,84 +2196,248 @@ out:
 	return MNL_CB_OK;
 }
 
-static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
-				     struct netlink_mon_handler *monh)
+static void setelem_print_default(struct set *set, int type,
+				  struct netlink_mon_handler *monh)
+{
+	switch (type) {
+		case NFT_MSG_NEWSETELEM:
+			printf("add ");
+			break;
+		case NFT_MSG_DELSETELEM:
+			printf("delete ");
+			break;
+		default:
+			return;
+	}
+	printf("element %s %s %s ", family2str(set->handle.family),
+	       set->handle.table, set->handle.set);
+	expr_print(set->init, monh->ctx->octx);
+	printf("\n");
+}
+
+static struct {
+	struct set *set;
+	int event_type;
+} setelem_cache;
+
+static unsigned int setelem_cache_size(void)
+{
+	if (!setelem_cache.set)
+		return 0;
+
+	return setelem_cache.set->init->size;
+}
+
+static void setelem_cache_print_default(struct netlink_mon_handler *monh)
+{
+	if (!setelem_cache.set)
+		return;
+
+	interval_map_decompose(setelem_cache.set->init);
+	setelem_print_default(setelem_cache.set,
+			      setelem_cache.event_type, monh);
+	set_free(setelem_cache.set);
+	setelem_cache.set = NULL;
+}
+
+static struct expr *setelem_cache_get_first(void)
+{
+	struct expr *expr;
+
+	if (!setelem_cache.set)
+		return NULL;
+
+	list_for_each_entry(expr, &setelem_cache.set->init->expressions, list)
+		return expr;
+
+	return NULL;
+}
+
+static bool setelem_cache_should_print(struct expr *new, int new_type)
+{
+	unsigned int cache_size = setelem_cache_size();
+	unsigned long new_val = mpz_get_ui(new->key->value);
+	struct expr *cache_elem = setelem_cache_get_first();
+
+	/* Check elem count:
+	 * - nothing to do if cache is empty,
+	 * - always print if cache is full.
+	 */
+	if (cache_size == 0)
+		return false;
+	if (cache_size >= 2)
+		return true;
+
+	/* Cache contains a single element, which may be part of a regular
+	 * range, or the single element identifying a half-open range. To find
+	 * that out, compare the new element with the existing one:
+	 * - If event type changed, elements are unrelated.
+	 * - If cached element is range end, wait for second one.
+	 * - If new element is range start as well, both are unrelated.
+	 * - If new element is range end but value is not bigger than cached
+	 *   one's, both are unrelated.
+	 */
+
+	if (setelem_cache.event_type != new_type)
+		return true;
+
+	if (cache_elem->flags & EXPR_F_INTERVAL_END)
+		return false;
+
+	if (!(new->flags & EXPR_F_INTERVAL_END))
+		return true;
+
+	if (new_val <= mpz_get_ui(cache_elem->key->value))
+		return true;
+
+	return false;
+}
+
+static void setelem_cache_init(struct netlink_mon_handler *monh,
+			       const struct set *set)
+{
+	struct set *cset;
+
+	if (setelem_cache.set)
+		return;
+
+	cset = set_alloc(monh->loc);
+	cset->keytype	= set->keytype;
+	cset->datatype	= set->datatype;
+	cset->flags	= set->flags;
+	cset->init		= set_expr_alloc(monh->loc, set);
+	cset->handle.family	= set->handle.family;
+	cset->handle.table	= xstrdup(set->handle.table);
+	cset->handle.set	= xstrdup(set->handle.set);
+
+	setelem_cache.set = cset;
+}
+
+static void setelem_cache_add(const struct expr *elem, int type)
+{
+	compound_expr_add(setelem_cache.set->init, expr_clone(elem));
+	setelem_cache.event_type = type;
+}
+
+static struct set *parse_nftnl_set(struct netlink_mon_handler *monh,
+				   const struct nftnl_set *nls)
 {
 	struct nftnl_set_elems_iter *nlsei;
 	struct nftnl_set_elem *nlse;
-	struct nftnl_set *nls;
-	struct set *dummyset;
-	struct set *set;
-	const char *setname, *table;
+	const char *table, *setname;
 	uint32_t family;
+	struct set *orig, *set;
 
-	nls = netlink_setelem_alloc(nlh);
 	table = nftnl_set_get_str(nls, NFTNL_SET_TABLE);
 	setname = nftnl_set_get_str(nls, NFTNL_SET_NAME);
 	family = nftnl_set_get_u32(nls, NFTNL_SET_FAMILY);
 
-	set = set_lookup_global(family, table, setname);
-	if (set == NULL) {
-		fprintf(stderr, "W: Received event for an unknown set.");
-		goto out;
+	orig = set_lookup_global(family, table, setname);
+
+	if (orig == NULL) {
+		fprintf(stderr, "W: Received event for an unknown set.\n");
+		return NULL;
 	}
 
-	switch (monh->format) {
-	case NFTNL_OUTPUT_DEFAULT:
-		if (set->flags & NFT_SET_ANONYMOUS)
-			goto out;
+	set = set_alloc(monh->loc);
+	set->keytype	= orig->keytype;
+	set->datatype	= orig->datatype;
+	set->flags	= orig->flags;
+	set->init	= set_expr_alloc(monh->loc, orig);
+	set->handle.family	= family;
+	set->handle.table	= xstrdup(table);
+	set->handle.set		= xstrdup(setname);
 
-		/* we want to 'delinearize' the set_elem, but don't
-		 * modify the original cached set. This path is only
-		 * used by named sets, so use a dummy set.
-		 */
-		dummyset = set_alloc(monh->loc);
-		dummyset->keytype = set->keytype;
-		dummyset->datatype = set->datatype;
-		dummyset->init = set_expr_alloc(monh->loc, set);
-
-		nlsei = nftnl_set_elems_iter_create(nls);
-		if (nlsei == NULL)
-			memory_allocation_error();
+	nlsei = nftnl_set_elems_iter_create(nls);
+	if (nlsei == NULL)
+		memory_allocation_error();
 
-		nlse = nftnl_set_elems_iter_next(nlsei);
-		while (nlse != NULL) {
-			if (netlink_delinearize_setelem(nlse, dummyset) < 0) {
-				set_free(dummyset);
-				nftnl_set_elems_iter_destroy(nlsei);
-				goto out;
-			}
-			nlse = nftnl_set_elems_iter_next(nlsei);
+	nlse = nftnl_set_elems_iter_next(nlsei);
+	while (nlse != NULL) {
+		if (netlink_delinearize_setelem(nlse, set) < 0) {
+			fprintf(stderr, "W: Parsing nftnl set failed.\n");
+			set_free(set);
+			nftnl_set_elems_iter_destroy(nlsei);
+			return NULL;
 		}
-		nftnl_set_elems_iter_destroy(nlsei);
+		nlse = nftnl_set_elems_iter_next(nlsei);
+	}
+	nftnl_set_elems_iter_destroy(nlsei);
 
-		switch (type) {
-		case NFT_MSG_NEWSETELEM:
-			printf("add ");
-			break;
-		case NFT_MSG_DELSETELEM:
-			printf("delete ");
-			break;
-		default:
-			set_free(dummyset);
-			goto out;
-		}
-		printf("element %s %s %s ", family2str(family), table, setname);
-		expr_print(dummyset->init, monh->ctx->octx);
-		printf("\n");
+	return set;
+}
 
-		set_free(dummyset);
-		break;
+static bool expr_is_null_elem(const struct expr *expr)
+{
+	if (!(expr->flags & EXPR_F_INTERVAL_END))
+		return false;
+
+	if (mpz_cmp_ui(expr->key->value, 0))
+		return false;
+
+	return true;
+}
+
+static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
+				     struct netlink_mon_handler *monh)
+{
+	struct nftnl_set *nls;
+	struct set *dummyset;
+	struct expr *expr;
+
+	nls = netlink_setelem_alloc(nlh);
+
+	switch (monh->format) {
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
+		/* XXX: this still prints the null element */
 		nftnl_set_fprintf(stdout, nls, monh->format,
 				  netlink_msg2nftnl_of(type));
 		fprintf(stdout, "\n");
+		goto out_free_nftnl_set;
+	case NFTNL_OUTPUT_DEFAULT:
 		break;
 	}
-out:
+
+	dummyset = parse_nftnl_set(monh, nls);
+	if (!dummyset)
+		goto out_free_nftnl_set;
+
+	if (dummyset->flags & NFT_SET_ANONYMOUS)
+		goto out_free_dummyset;
+
+	if (!(dummyset->flags & NFT_SET_INTERVAL)) {
+		setelem_cache_print_default(monh);
+		setelem_print_default(dummyset, type, monh);
+		goto out_free_dummyset;
+	}
+
+	list_for_each_entry(expr, &dummyset->init->expressions, list) {
+		if (expr_is_null_elem(expr))
+			continue;
+
+		if (setelem_cache_should_print(expr, type))
+			setelem_cache_print_default(monh);
+
+		setelem_cache_init(monh, dummyset);
+		setelem_cache_add(expr, type);
+	}
+
+out_free_dummyset:
+	set_free(dummyset);
+out_free_nftnl_set:
 	nftnl_set_free(nls);
 	return MNL_CB_OK;
+
+}
+
+static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
+					    int type,
+					    struct netlink_mon_handler *monh)
+{
+	setelem_cache_print_default(monh);
+
+	return MNL_CB_OK;
 }
 
 static int netlink_events_obj_cb(const struct nlmsghdr *nlh, int type,
@@ -2914,6 +3078,8 @@ static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 	case NFT_MSG_DELOBJ:
 		ret = netlink_events_obj_cb(nlh, type, monh);
 		break;
+	case NFT_MSG_NEWGEN:
+		ret = netlink_events_setelem_newgen_cb(nlh, type, monh);
 	}
 	fflush(stdout);
 
-- 
2.13.1

[nft PATCH 2/2] tests: Add basic monitor testing framework

[Phil Sutter]

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

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Pablo Neira Ayuso]

On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
[...]
> +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> +					    int type,
> +					    struct netlink_mon_handler *monh)
> +{
> +	setelem_cache_print_default(monh);
> +
> +	return MNL_CB_OK;
>  }

I would really like we don't rely on newgen for this. If there is no
way to catch a case with the existing way we represent this, then we
probably need to fix things from the kernel.

Before we follow that patch, I would like to understand what corner
case is pushing us to use the newgen event.

Thanks!

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Phil Sutter]

On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> [...]
> > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > +					    int type,
> > +					    struct netlink_mon_handler *monh)
> > +{
> > +	setelem_cache_print_default(monh);
> > +
> > +	return MNL_CB_OK;
> >  }
> 
> I would really like we don't rely on newgen for this. If there is no
> way to catch a case with the existing way we represent this, then we
> probably need to fix things from the kernel.
> 
> Before we follow that patch, I would like to understand what corner
> case is pushing us to use the newgen event.

It is required for half-open ranges occurring at the end of the
transaction: For those, we only get a single element without
EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
a regular range, monitor has to wait for what's next - which is in doubt
only the NEWGEN message.

Maybe we could introduce a new flag to mark these?

Cheers, Phil

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Pablo Neira Ayuso]

On Mon, Jul 17, 2017 at 06:41:14PM +0200, Phil Sutter wrote:
> On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> > [...]
> > > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > > +					    int type,
> > > +					    struct netlink_mon_handler *monh)
> > > +{
> > > +	setelem_cache_print_default(monh);
> > > +
> > > +	return MNL_CB_OK;
> > >  }
> > 
> > I would really like we don't rely on newgen for this. If there is no
> > way to catch a case with the existing way we represent this, then we
> > probably need to fix things from the kernel.
> > 
> > Before we follow that patch, I would like to understand what corner
> > case is pushing us to use the newgen event.
> 
> It is required for half-open ranges occurring at the end of the
> transaction: For those, we only get a single element without
> EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
> a regular range, monitor has to wait for what's next - which is in doubt
> only the NEWGEN message.
> 
> Maybe we could introduce a new flag to mark these?

Right, I think we need the new flag indeed, only for userspace.

Would you propose one and the specific semantics for it?

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Phil Sutter]

On Mon, Jul 17, 2017 at 07:16:29PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 17, 2017 at 06:41:14PM +0200, Phil Sutter wrote:
> > On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> > > [...]
> > > > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > > > +					    int type,
> > > > +					    struct netlink_mon_handler *monh)
> > > > +{
> > > > +	setelem_cache_print_default(monh);
> > > > +
> > > > +	return MNL_CB_OK;
> > > >  }
> > > 
> > > I would really like we don't rely on newgen for this. If there is no
> > > way to catch a case with the existing way we represent this, then we
> > > probably need to fix things from the kernel.
> > > 
> > > Before we follow that patch, I would like to understand what corner
> > > case is pushing us to use the newgen event.
> > 
> > It is required for half-open ranges occurring at the end of the
> > transaction: For those, we only get a single element without
> > EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
> > a regular range, monitor has to wait for what's next - which is in doubt
> > only the NEWGEN message.
> > 
> > Maybe we could introduce a new flag to mark these?
> 
> Right, I think we need the new flag indeed, only for userspace.
> 
> Would you propose one and the specific semantics for it?

My current PoC passes the additional flag as userdata attribute so the
kernel won't reject the element due to unknown flag. Is that fine with
you? I'm trying to avoid changing the kernel so the solution is
backwards compatible.

Cheers, Phil

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Pablo Neira Ayuso]

On Tue, Jul 18, 2017 at 11:05:16AM +0200, Phil Sutter wrote:
> On Mon, Jul 17, 2017 at 07:16:29PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jul 17, 2017 at 06:41:14PM +0200, Phil Sutter wrote:
> > > On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> > > > [...]
> > > > > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > > > > +					    int type,
> > > > > +					    struct netlink_mon_handler *monh)
> > > > > +{
> > > > > +	setelem_cache_print_default(monh);
> > > > > +
> > > > > +	return MNL_CB_OK;
> > > > >  }
> > > > 
> > > > I would really like we don't rely on newgen for this. If there is no
> > > > way to catch a case with the existing way we represent this, then we
> > > > probably need to fix things from the kernel.
> > > > 
> > > > Before we follow that patch, I would like to understand what corner
> > > > case is pushing us to use the newgen event.
> > > 
> > > It is required for half-open ranges occurring at the end of the
> > > transaction: For those, we only get a single element without
> > > EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
> > > a regular range, monitor has to wait for what's next - which is in doubt
> > > only the NEWGEN message.
> > > 
> > > Maybe we could introduce a new flag to mark these?
> > 
> > Right, I think we need the new flag indeed, only for userspace.
> > 
> > Would you propose one and the specific semantics for it?
> 
> My current PoC passes the additional flag as userdata attribute so the
> kernel won't reject the element due to unknown flag. Is that fine with
> you? I'm trying to avoid changing the kernel so the solution is
> backwards compatible.

I suggest you add a new flag to SET_ELEM instead. Userdata area usage
is exclusive to userspace.

Thanks!

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Phil Sutter]

On Tue, Jul 18, 2017 at 11:09:37AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 18, 2017 at 11:05:16AM +0200, Phil Sutter wrote:
> > On Mon, Jul 17, 2017 at 07:16:29PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Jul 17, 2017 at 06:41:14PM +0200, Phil Sutter wrote:
> > > > On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> > > > > [...]
> > > > > > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > > > > > +					    int type,
> > > > > > +					    struct netlink_mon_handler *monh)
> > > > > > +{
> > > > > > +	setelem_cache_print_default(monh);
> > > > > > +
> > > > > > +	return MNL_CB_OK;
> > > > > >  }
> > > > > 
> > > > > I would really like we don't rely on newgen for this. If there is no
> > > > > way to catch a case with the existing way we represent this, then we
> > > > > probably need to fix things from the kernel.
> > > > > 
> > > > > Before we follow that patch, I would like to understand what corner
> > > > > case is pushing us to use the newgen event.
> > > > 
> > > > It is required for half-open ranges occurring at the end of the
> > > > transaction: For those, we only get a single element without
> > > > EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
> > > > a regular range, monitor has to wait for what's next - which is in doubt
> > > > only the NEWGEN message.
> > > > 
> > > > Maybe we could introduce a new flag to mark these?
> > > 
> > > Right, I think we need the new flag indeed, only for userspace.
> > > 
> > > Would you propose one and the specific semantics for it?
> > 
> > My current PoC passes the additional flag as userdata attribute so the
> > kernel won't reject the element due to unknown flag. Is that fine with
> > you? I'm trying to avoid changing the kernel so the solution is
> > backwards compatible.
> 
> I suggest you add a new flag to SET_ELEM instead. Userdata area usage
> is exclusive to userspace.

You mean nft_set_elem_flags? The new flag will indeed be used by
userspace only: It is set when creating a half-open range and not used
by the kernel at all.

Thanks, Phil

Re: [nft PATCH 1/2] monitor: Rewrite SETELEM callback

[Pablo Neira Ayuso]

On Tue, Jul 18, 2017 at 11:17:26AM +0200, Phil Sutter wrote:
> On Tue, Jul 18, 2017 at 11:09:37AM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 18, 2017 at 11:05:16AM +0200, Phil Sutter wrote:
> > > On Mon, Jul 17, 2017 at 07:16:29PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, Jul 17, 2017 at 06:41:14PM +0200, Phil Sutter wrote:
> > > > > On Mon, Jul 17, 2017 at 06:30:18PM +0200, Pablo Neira Ayuso wrote:
> > > > > > On Mon, Jul 17, 2017 at 05:06:05PM +0200, Phil Sutter wrote:
> > > > > > [...]
> > > > > > > +static int netlink_events_setelem_newgen_cb(const struct nlmsghdr *nlh,
> > > > > > > +					    int type,
> > > > > > > +					    struct netlink_mon_handler *monh)
> > > > > > > +{
> > > > > > > +	setelem_cache_print_default(monh);
> > > > > > > +
> > > > > > > +	return MNL_CB_OK;
> > > > > > >  }
> > > > > > 
> > > > > > I would really like we don't rely on newgen for this. If there is no
> > > > > > way to catch a case with the existing way we represent this, then we
> > > > > > probably need to fix things from the kernel.
> > > > > > 
> > > > > > Before we follow that patch, I would like to understand what corner
> > > > > > case is pushing us to use the newgen event.
> > > > > 
> > > > > It is required for half-open ranges occurring at the end of the
> > > > > transaction: For those, we only get a single element without
> > > > > EXPR_F_INTERVAL_END flag set. Since this could also be the first part of
> > > > > a regular range, monitor has to wait for what's next - which is in doubt
> > > > > only the NEWGEN message.
> > > > > 
> > > > > Maybe we could introduce a new flag to mark these?
> > > > 
> > > > Right, I think we need the new flag indeed, only for userspace.
> > > > 
> > > > Would you propose one and the specific semantics for it?
> > > 
> > > My current PoC passes the additional flag as userdata attribute so the
> > > kernel won't reject the element due to unknown flag. Is that fine with
> > > you? I'm trying to avoid changing the kernel so the solution is
> > > backwards compatible.
> > 
> > I suggest you add a new flag to SET_ELEM instead. Userdata area usage
> > is exclusive to userspace.
> 
> You mean nft_set_elem_flags? The new flag will indeed be used by
> userspace only: It is set when creating a half-open range and not used
> by the kernel at all.

That's fine indeed.