[iptables PATCH 0/8] nft: Implement forward compat for future binaries

[iptables PATCH 0/8] nft: Implement forward compat for future binaries

[Phil Sutter]

Time to abandon earlier attempts at providing compatibility for old
binaries, choose the next best option which is not relying upon any
kernel changes.

Basically, all extensions replaced by native bytecode are appended to
rule userdata so when nftnl rule parsing code fails, it may retry
omitting all these expressions and restoring an extension from userdata
instead.

The idea behind this is that extensions are stable which relieves native
bytecode from being the same. With this series in place, one may
(re-)start converting extensions into native nftables bytecode again.

For now, appending compat extensions is always active. Keeping it
disabled by default and enabling via commandline flag or (simpler) env
variable might make sense (I haven't tested performance yet). The
parsing component will take action only if standard rule parsing fails,
so no need to manually enable this IMO.

The actual implementation sits in patch 8, the preceeding ones are
(mostly) preparation.

To forcibly exercise the fallback rule parsing code, compile with
CFLAGS='-DDEBUG_COMPAT_EXT=1'.

Phil Sutter (8):
  ebtables: Zero freed pointers in ebt_cs_clean()
  ebtables: Introduce nft_bridge_init_cs()
  nft: Reduce overhead in nft_rule_find()
  nft: ruleparse: Drop 'iter' variable in
    nft_rule_to_iptables_command_state
  nft: ruleparse: Introduce nft_parse_rule_expr()
  nft: __add_{match,target}() can't fail
  nft: Introduce UDATA_TYPE_COMPAT_EXT
  nft: Support compat extensions in rule userdata

 configure.ac             |   9 ++
 iptables/Makefile.am     |   1 +
 iptables/nft-bridge.c    |  12 +--
 iptables/nft-compat.c    | 217 +++++++++++++++++++++++++++++++++++++++
 iptables/nft-compat.h    |  54 ++++++++++
 iptables/nft-ruleparse.c | 106 +++++++++++--------
 iptables/nft-ruleparse.h |   4 +
 iptables/nft.c           | 112 +++++++++++---------
 iptables/nft.h           |  14 +++
 iptables/xtables-eb.c    |   4 +-
 10 files changed, 438 insertions(+), 95 deletions(-)
 create mode 100644 iptables/nft-compat.c
 create mode 100644 iptables/nft-compat.h

-- 
2.43.0

[iptables PATCH 1/8] ebtables: Zero freed pointers in ebt_cs_clean()

[Phil Sutter]

Trying to recycle an iptables_command_state object by calling first
clear_cs then init_cs callbacks causes invalid data accesses with
ebtables otherwise.

Fixes: fe97f60e5d2a9 ("ebtables-compat: add watchers support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-bridge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 0f85e21861cde..f75a13fbf1120 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -46,6 +46,7 @@ void ebt_cs_clean(struct iptables_command_state *cs)
 		free(m);
 		m = nm;
 	}
+	cs->match_list = NULL;
 
 	if (cs->target) {
 		free(cs->target->t);
-- 
2.43.0

[iptables PATCH 2/8] ebtables: Introduce nft_bridge_init_cs()

[Phil Sutter]

The custom init done by nft_rule_to_ebtables_command_state() (which is
also the reason for its existence in the first place) should better go
into an ebtables-specific init_cs callback. Properly calling it from
do_commandeb() then removes the need for that custom rule_to_cs
callback.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-bridge.c | 11 +++++------
 iptables/xtables-eb.c |  4 +++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index f75a13fbf1120..1623acbac0ba6 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -203,12 +203,9 @@ static int nft_bridge_add(struct nft_handle *h, struct nft_rule_ctx *ctx,
 	return _add_action(r, cs);
 }
 
-static bool nft_rule_to_ebtables_command_state(struct nft_handle *h,
-					       const struct nftnl_rule *r,
-					       struct iptables_command_state *cs)
+static void nft_bridge_init_cs(struct iptables_command_state *cs)
 {
 	cs->eb.bitmask = EBT_NOPROTO;
-	return nft_rule_to_iptables_command_state(h, r, cs);
 }
 
 static void print_iface(const char *option, const char *name, bool invert)
@@ -353,7 +350,8 @@ static void nft_bridge_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	if (format & FMT_LINENUMBERS)
 		printf("%d. ", num);
 
-	nft_rule_to_ebtables_command_state(h, r, &cs);
+	nft_bridge_init_cs(&cs);
+	nft_rule_to_iptables_command_state(h, r, &cs);
 	__nft_bridge_save_rule(&cs, format);
 	ebt_cs_clean(&cs);
 }
@@ -699,7 +697,8 @@ struct nft_family_ops nft_family_ops_bridge = {
 	.print_rule		= nft_bridge_print_rule,
 	.save_rule		= nft_bridge_save_rule,
 	.save_chain		= nft_bridge_save_chain,
-	.rule_to_cs		= nft_rule_to_ebtables_command_state,
+	.rule_to_cs		= nft_rule_to_iptables_command_state,
+	.init_cs		= nft_bridge_init_cs,
 	.clear_cs		= ebt_cs_clean,
 	.xlate			= nft_bridge_xlate,
 };
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 51c699defb047..45663a3ad0ee0 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -557,7 +557,6 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 		.argc = argc,
 		.argv = argv,
 		.jumpto	= "",
-		.eb.bitmask = EBT_NOPROTO,
 	};
 	const struct builtin_table *t;
 	struct xtables_args args = {
@@ -572,6 +571,9 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 	};
 	int ret = 0;
 
+	if (h->ops->init_cs)
+		h->ops->init_cs(&cs);
+
 	do_parse(argc, argv, &p, &cs, &args);
 
 	h->verbose	= p.verbose;
-- 
2.43.0

[iptables PATCH 3/8] nft: Reduce overhead in nft_rule_find()

[Phil Sutter]

When iterating through the list of rules in a chain comparing against a
sample, there is no point in carrying that sample as nftnl_rule object
and converting into iptables_command_state object prior to each
comparison. Just do it up front and adjust the callback accordingly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 8b1803181b207..88be5ede5171d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2392,25 +2392,22 @@ static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule *r)
 }
 
 static bool nft_rule_cmp(struct nft_handle *h, struct nftnl_rule *r,
-			 struct nftnl_rule *rule)
+			 struct iptables_command_state *cs)
 {
-	struct iptables_command_state _cs = {}, this = {}, *cs = &_cs;
-	bool ret = false, ret_this, ret_that;
+	struct iptables_command_state this = {};
+	bool ret = false, ret_this;
 
-	if (h->ops->init_cs) {
+	if (h->ops->init_cs)
 		h->ops->init_cs(&this);
-		h->ops->init_cs(cs);
-	}
 
 	ret_this = h->ops->rule_to_cs(h, r, &this);
-	ret_that = h->ops->rule_to_cs(h, rule, cs);
 
-	DEBUGP("comparing with... ");
+	DEBUGP("with ... ");
 #ifdef DEBUG_DEL
 	nft_rule_print_save(h, r, NFT_RULE_APPEND, 0);
 #endif
-	if (!ret_this || !ret_that)
-		DEBUGP("Cannot convert rules: %d %d\n", ret_this, ret_that);
+	if (!ret_this)
+		DEBUGP("Cannot convert rule: %d\n", ret_this);
 
 	if (!h->ops->is_same(cs, &this))
 		goto out;
@@ -2434,7 +2431,6 @@ static bool nft_rule_cmp(struct nft_handle *h, struct nftnl_rule *r,
 	ret = true;
 out:
 	h->ops->clear_cs(&this);
-	h->ops->clear_cs(cs);
 	return ret;
 }
 
@@ -2442,6 +2438,7 @@ static struct nftnl_rule *
 nft_rule_find(struct nft_handle *h, struct nft_chain *nc,
 	      struct nftnl_rule *rule, int rulenum)
 {
+	struct iptables_command_state cs = {};
 	struct nftnl_chain *c = nc->nftnl;
 	struct nftnl_rule *r;
 	struct nftnl_rule_iter *iter;
@@ -2455,9 +2452,20 @@ nft_rule_find(struct nft_handle *h, struct nft_chain *nc,
 	if (iter == NULL)
 		return 0;
 
+	if (h->ops->init_cs)
+		h->ops->init_cs(&cs);
+
+	if (!h->ops->rule_to_cs(h, rule, &cs))
+		return NULL;
+
+	DEBUGP("comparing ... ");
+#ifdef DEBUG_DEL
+	nft_rule_print_save(h, rule, NFT_RULE_APPEND, 0);
+#endif
+
 	r = nftnl_rule_iter_next(iter);
 	while (r != NULL) {
-		found = nft_rule_cmp(h, r, rule);
+		found = nft_rule_cmp(h, r, &cs);
 		if (found)
 			break;
 		r = nftnl_rule_iter_next(iter);
@@ -2465,6 +2473,8 @@ nft_rule_find(struct nft_handle *h, struct nft_chain *nc,
 
 	nftnl_rule_iter_destroy(iter);
 
+	h->ops->clear_cs(&cs);
+
 	return found ? r : NULL;
 }
 
-- 
2.43.0

[iptables PATCH 4/8] nft: ruleparse: Drop 'iter' variable in nft_rule_to_iptables_command_state

[Phil Sutter]

Use the same named field in 'ctx' instead, it has to carry the value
anyway.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-ruleparse.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/iptables/nft-ruleparse.c b/iptables/nft-ruleparse.c
index 3b1cbe4fa1499..1ee7a94db59de 100644
--- a/iptables/nft-ruleparse.c
+++ b/iptables/nft-ruleparse.c
@@ -891,7 +891,6 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 					const struct nftnl_rule *r,
 					struct iptables_command_state *cs)
 {
-	struct nftnl_expr_iter *iter;
 	struct nftnl_expr *expr;
 	struct nft_xt_ctx ctx = {
 		.cs = cs,
@@ -900,12 +899,11 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 	};
 	bool ret = true;
 
-	iter = nftnl_expr_iter_create(r);
-	if (iter == NULL)
+	ctx.iter = nftnl_expr_iter_create(r);
+	if (ctx.iter == NULL)
 		return false;
 
-	ctx.iter = iter;
-	expr = nftnl_expr_iter_next(iter);
+	expr = nftnl_expr_iter_next(ctx.iter);
 	while (expr != NULL) {
 		const char *name =
 			nftnl_expr_get_str(expr, NFTNL_EXPR_NAME);
@@ -941,10 +939,10 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 			ret = false;
 		}
 
-		expr = nftnl_expr_iter_next(iter);
+		expr = nftnl_expr_iter_next(ctx.iter);
 	}
 
-	nftnl_expr_iter_destroy(iter);
+	nftnl_expr_iter_destroy(ctx.iter);
 
 	if (nftnl_rule_is_set(r, NFTNL_RULE_USERDATA)) {
 		const void *data;
-- 
2.43.0

[iptables PATCH 5/8] nft: ruleparse: Introduce nft_parse_rule_expr()

[Phil Sutter]

Extract the parsing of one expression into a separate function and
export it, preparing for following code changes.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-ruleparse.c | 73 ++++++++++++++++++++++------------------
 iptables/nft-ruleparse.h |  4 +++
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/iptables/nft-ruleparse.c b/iptables/nft-ruleparse.c
index 1ee7a94db59de..757d3c29fc816 100644
--- a/iptables/nft-ruleparse.c
+++ b/iptables/nft-ruleparse.c
@@ -887,6 +887,45 @@ static void nft_parse_range(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	}
 }
 
+bool nft_parse_rule_expr(struct nft_handle *h,
+			 struct nftnl_expr *expr,
+			 struct nft_xt_ctx *ctx)
+{
+	const char *name = nftnl_expr_get_str(expr, NFTNL_EXPR_NAME);
+
+	if (strcmp(name, "counter") == 0)
+		nft_parse_counter(expr, &ctx->cs->counters);
+	else if (strcmp(name, "payload") == 0)
+		nft_parse_payload(ctx, expr);
+	else if (strcmp(name, "meta") == 0)
+		nft_parse_meta(ctx, expr);
+	else if (strcmp(name, "bitwise") == 0)
+		nft_parse_bitwise(ctx, expr);
+	else if (strcmp(name, "cmp") == 0)
+		nft_parse_cmp(ctx, expr);
+	else if (strcmp(name, "immediate") == 0)
+		nft_parse_immediate(ctx, expr);
+	else if (strcmp(name, "match") == 0)
+		nft_parse_match(ctx, expr);
+	else if (strcmp(name, "target") == 0)
+		nft_parse_target(ctx, expr);
+	else if (strcmp(name, "limit") == 0)
+		nft_parse_limit(ctx, expr);
+	else if (strcmp(name, "lookup") == 0)
+		nft_parse_lookup(ctx, h, expr);
+	else if (strcmp(name, "log") == 0)
+		nft_parse_log(ctx, expr);
+	else if (strcmp(name, "range") == 0)
+		nft_parse_range(ctx, expr);
+
+	if (ctx->errmsg) {
+		fprintf(stderr, "Error: %s\n", ctx->errmsg);
+		ctx->errmsg = NULL;
+		return false;
+	}
+	return true;
+}
+
 bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 					const struct nftnl_rule *r,
 					struct iptables_command_state *cs)
@@ -905,40 +944,8 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 
 	expr = nftnl_expr_iter_next(ctx.iter);
 	while (expr != NULL) {
-		const char *name =
-			nftnl_expr_get_str(expr, NFTNL_EXPR_NAME);
-
-		if (strcmp(name, "counter") == 0)
-			nft_parse_counter(expr, &ctx.cs->counters);
-		else if (strcmp(name, "payload") == 0)
-			nft_parse_payload(&ctx, expr);
-		else if (strcmp(name, "meta") == 0)
-			nft_parse_meta(&ctx, expr);
-		else if (strcmp(name, "bitwise") == 0)
-			nft_parse_bitwise(&ctx, expr);
-		else if (strcmp(name, "cmp") == 0)
-			nft_parse_cmp(&ctx, expr);
-		else if (strcmp(name, "immediate") == 0)
-			nft_parse_immediate(&ctx, expr);
-		else if (strcmp(name, "match") == 0)
-			nft_parse_match(&ctx, expr);
-		else if (strcmp(name, "target") == 0)
-			nft_parse_target(&ctx, expr);
-		else if (strcmp(name, "limit") == 0)
-			nft_parse_limit(&ctx, expr);
-		else if (strcmp(name, "lookup") == 0)
-			nft_parse_lookup(&ctx, h, expr);
-		else if (strcmp(name, "log") == 0)
-			nft_parse_log(&ctx, expr);
-		else if (strcmp(name, "range") == 0)
-			nft_parse_range(&ctx, expr);
-
-		if (ctx.errmsg) {
-			fprintf(stderr, "Error: %s\n", ctx.errmsg);
-			ctx.errmsg = NULL;
+		if (!nft_parse_rule_expr(h, expr, &ctx))
 			ret = false;
-		}
-
 		expr = nftnl_expr_iter_next(ctx.iter);
 	}
 
diff --git a/iptables/nft-ruleparse.h b/iptables/nft-ruleparse.h
index 62c9160d77711..0377e4ae17a6e 100644
--- a/iptables/nft-ruleparse.h
+++ b/iptables/nft-ruleparse.h
@@ -133,4 +133,8 @@ int parse_meta(struct nft_xt_ctx *ctx, struct nftnl_expr *e, uint8_t key,
 int nft_parse_hl(struct nft_xt_ctx *ctx, struct nftnl_expr *e,
 		 struct iptables_command_state *cs);
 
+bool nft_parse_rule_expr(struct nft_handle *h,
+			 struct nftnl_expr *expr,
+			 struct nft_xt_ctx *ctx);
+
 #endif /* _NFT_RULEPARSE_H_ */
-- 
2.43.0

[iptables PATCH 6/8] nft: __add_{match,target}() can't fail

[Phil Sutter]

These functions either call xtables_error() which terminates the process
or succeed - make them return void. While at it, export them as rule
parsing code will call them in future. Also make input parameter const,
they're not supposed to alter extension data.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 28 ++++++++++------------------
 iptables/nft.h |  2 ++
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 88be5ede5171d..cabcc884b4069 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1034,7 +1034,7 @@ int nft_chain_set(struct nft_handle *h, const char *table,
 	return 1;
 }
 
-static int __add_match(struct nftnl_expr *e, struct xt_entry_match *m)
+void __add_match(struct nftnl_expr *e, const struct xt_entry_match *m)
 {
 	void *info;
 
@@ -1044,8 +1044,6 @@ static int __add_match(struct nftnl_expr *e, struct xt_entry_match *m)
 	info = xtables_calloc(1, m->u.match_size);
 	memcpy(info, m->data, m->u.match_size - sizeof(*m));
 	nftnl_expr_set(e, NFTNL_EXPR_MT_INFO, info, m->u.match_size - sizeof(*m));
-
-	return 0;
 }
 
 static int add_nft_limit(struct nftnl_rule *r, struct xt_entry_match *m)
@@ -1378,11 +1376,10 @@ static int add_nft_udp(struct nft_handle *h, struct nftnl_rule *r,
 	if (udp->invflags > XT_UDP_INV_MASK ||
 	    udp_all_zero(udp)) {
 		struct nftnl_expr *expr = nftnl_expr_alloc("match");
-		int ret;
 
-		ret = __add_match(expr, m);
+		__add_match(expr, m);
 		nftnl_rule_add_expr(r, expr);
-		return ret;
+		return 0;
 	}
 
 	if (nftnl_rule_get_u32(r, NFTNL_RULE_COMPAT_PROTO) != IPPROTO_UDP)
@@ -1431,11 +1428,10 @@ static int add_nft_tcp(struct nft_handle *h, struct nftnl_rule *r,
 	if (tcp->invflags & ~supported || tcp->option ||
 	    tcp_all_zero(tcp)) {
 		struct nftnl_expr *expr = nftnl_expr_alloc("match");
-		int ret;
 
-		ret = __add_match(expr, m);
+		__add_match(expr, m);
 		nftnl_rule_add_expr(r, expr);
-		return ret;
+		return 0;
 	}
 
 	if (nftnl_rule_get_u32(r, NFTNL_RULE_COMPAT_PROTO) != IPPROTO_TCP)
@@ -1478,7 +1474,6 @@ int add_match(struct nft_handle *h, struct nft_rule_ctx *ctx,
 	      struct nftnl_rule *r, struct xt_entry_match *m)
 {
 	struct nftnl_expr *expr;
-	int ret;
 
 	switch (ctx->command) {
 	case NFT_COMPAT_RULE_APPEND:
@@ -1503,13 +1498,13 @@ int add_match(struct nft_handle *h, struct nft_rule_ctx *ctx,
 	if (expr == NULL)
 		return -ENOMEM;
 
-	ret = __add_match(expr, m);
+	__add_match(expr, m);
 	nftnl_rule_add_expr(r, expr);
 
-	return ret;
+	return 0;
 }
 
-static int __add_target(struct nftnl_expr *e, struct xt_entry_target *t)
+void __add_target(struct nftnl_expr *e, const struct xt_entry_target *t)
 {
 	void *info;
 
@@ -1520,8 +1515,6 @@ static int __add_target(struct nftnl_expr *e, struct xt_entry_target *t)
 	info = xtables_calloc(1, t->u.target_size);
 	memcpy(info, t->data, t->u.target_size - sizeof(*t));
 	nftnl_expr_set(e, NFTNL_EXPR_TG_INFO, info, t->u.target_size - sizeof(*t));
-
-	return 0;
 }
 
 static int add_meta_nftrace(struct nftnl_rule *r)
@@ -1549,7 +1542,6 @@ static int add_meta_nftrace(struct nftnl_rule *r)
 int add_target(struct nftnl_rule *r, struct xt_entry_target *t)
 {
 	struct nftnl_expr *expr;
-	int ret;
 
 	if (strcmp(t->u.user.name, "TRACE") == 0)
 		return add_meta_nftrace(r);
@@ -1558,10 +1550,10 @@ int add_target(struct nftnl_rule *r, struct xt_entry_target *t)
 	if (expr == NULL)
 		return -ENOMEM;
 
-	ret = __add_target(expr, t);
+	__add_target(expr, t);
 	nftnl_rule_add_expr(r, expr);
 
-	return ret;
+	return 0;
 }
 
 int add_jumpto(struct nftnl_rule *r, const char *name, int verdict)
diff --git a/iptables/nft.h b/iptables/nft.h
index 8f17f3100a190..54fe5210ad1ac 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -192,8 +192,10 @@ bool nft_rule_is_policy_rule(struct nftnl_rule *r);
  */
 int add_counters(struct nftnl_rule *r, uint64_t packets, uint64_t bytes);
 int add_verdict(struct nftnl_rule *r, int verdict);
+void __add_match(struct nftnl_expr *e, const struct xt_entry_match *m);
 int add_match(struct nft_handle *h, struct nft_rule_ctx *ctx,
 	      struct nftnl_rule *r, struct xt_entry_match *m);
+void __add_target(struct nftnl_expr *e, const struct xt_entry_target *t);
 int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
 int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);
 int add_action(struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
-- 
2.43.0

[iptables PATCH 7/8] nft: Introduce UDATA_TYPE_COMPAT_EXT

[Phil Sutter]

This new rule udata attribute will contain extensions which have been
converted to native nftables expressions for rule parsers to fall back
to.

While at it, export parse_udata_cb() as rule parsing code will call it
in future.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 11 +++--------
 iptables/nft.h | 12 ++++++++++++
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index cabcc884b4069..64ac35f2edcf3 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1668,14 +1668,7 @@ int add_counters(struct nftnl_rule *r, uint64_t packets, uint64_t bytes)
 	return 0;
 }
 
-enum udata_type {
-	UDATA_TYPE_COMMENT,
-	UDATA_TYPE_EBTABLES_POLICY,
-	__UDATA_TYPE_MAX,
-};
-#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
-
-static int parse_udata_cb(const struct nftnl_udata *attr, void *data)
+int parse_udata_cb(const struct nftnl_udata *attr, void *data)
 {
 	unsigned char *value = nftnl_udata_get(attr);
 	uint8_t type = nftnl_udata_type(attr);
@@ -1689,6 +1682,8 @@ static int parse_udata_cb(const struct nftnl_udata *attr, void *data)
 		break;
 	case UDATA_TYPE_EBTABLES_POLICY:
 		break;
+	case UDATA_TYPE_COMPAT_EXT:
+		break;
 	default:
 		return 0;
 	}
diff --git a/iptables/nft.h b/iptables/nft.h
index 54fe5210ad1ac..d6424f499cfcf 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -276,4 +276,16 @@ void nft_assert_table_compatible(struct nft_handle *h,
 int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 			      const char *chain, const char *policy);
 
+struct nftnl_udata;
+
+enum udata_type {
+	UDATA_TYPE_COMMENT,
+	UDATA_TYPE_EBTABLES_POLICY,
+	UDATA_TYPE_COMPAT_EXT,
+	__UDATA_TYPE_MAX,
+};
+#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
+
+int parse_udata_cb(const struct nftnl_udata *attr, void *data);
+
 #endif
-- 
2.43.0

[iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Phil Sutter]

Add a mechanism providing forward compatibility for the current and
future versions of iptables-nft (and all other nft-variants) by
annotating nftnl rules with the extensions they were created for.

Upon nftnl rule parsing failure, warn about the situation and perform a
second attempt loading the respective compat extensions instead of the
native expressions which replace them. The foundational assumption is
that libxtables extensions are stable and thus the VM code created on
their behalf does not need to be.

Since nftnl rule userdata attributes are restricted to 255 bytes, the
implementation focusses on low memory consumption. Therefore, extensions
which remain in the rule as compat expressions are not also added to
userdata. In turn, extensions in userdata are annotated by start and end
expression number they are replacing. Also, the actual payload is
zipped using zlib.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 configure.ac             |   9 ++
 iptables/Makefile.am     |   1 +
 iptables/nft-compat.c    | 217 +++++++++++++++++++++++++++++++++++++++
 iptables/nft-compat.h    |  54 ++++++++++
 iptables/nft-ruleparse.c |  21 ++++
 iptables/nft.c           |  39 +++++--
 6 files changed, 331 insertions(+), 10 deletions(-)
 create mode 100644 iptables/nft-compat.c
 create mode 100644 iptables/nft-compat.h

diff --git a/configure.ac b/configure.ac
index 2293702b17a47..a18df531953d6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,6 +77,14 @@ AC_ARG_WITH([xt-lock-name], AS_HELP_STRING([--with-xt-lock-name=PATH],
 AC_ARG_ENABLE([profiling],
 	AS_HELP_STRING([--enable-profiling], [build for use of gcov/gprof]),
 	[enable_profiling="$enableval"], [enable_profiling="no"])
+AC_ARG_WITH([zlib], [AS_HELP_STRING([--without-zlib],
+	    [Disable payload compression of rule compat expressions])],
+           [], [with_zlib=yes])
+AS_IF([test "x$with_zlib" != xno], [
+       AC_CHECK_LIB([z], [compress], ,
+		    AC_MSG_ERROR([No suitable version of zlib found]))
+       AC_DEFINE([HAVE_ZLIB], [1], [Define if you have zlib])
+])
 
 AC_MSG_CHECKING([whether $LD knows -Wl,--no-undefined])
 saved_LDFLAGS="$LDFLAGS";
@@ -270,6 +278,7 @@ echo "
   nftables support:			${enable_nftables}
   connlabel support:			${enable_connlabel}
   profiling support:			${enable_profiling}
+  compress rule compat expressions:	${with_zlib}
 
 Build parameters:
   Put plugins into executable (static):	${enable_static}
diff --git a/iptables/Makefile.am b/iptables/Makefile.am
index 2007cd10260bd..4855c9a7c2911 100644
--- a/iptables/Makefile.am
+++ b/iptables/Makefile.am
@@ -57,6 +57,7 @@ xtables_nft_multi_SOURCES += nft.c nft.h \
 			     nft-ruleparse-arp.c nft-ruleparse-bridge.c \
 			     nft-ruleparse-ipv4.c nft-ruleparse-ipv6.c \
 			     nft-shared.c nft-shared.h \
+			     nft-compat.c nft-compat.h \
 			     xtables-monitor.c \
 			     xtables.c xtables-arp.c xtables-eb.c \
 			     xtables-standalone.c xtables-eb-standalone.c \
diff --git a/iptables/nft-compat.c b/iptables/nft-compat.c
new file mode 100644
index 0000000000000..2e37dee6cdc43
--- /dev/null
+++ b/iptables/nft-compat.c
@@ -0,0 +1,217 @@
+#include "config.h"
+#include "nft-compat.h"
+#include "nft-ruleparse.h"
+#include "nft.h"
+
+#include <stdlib.h>
+#include <string.h>
+#include <xtables.h>
+
+#ifdef HAVE_ZLIB
+#include <zlib.h>
+#endif
+
+#include <libnftnl/udata.h>
+
+int nftnl_rule_expr_count(const struct nftnl_rule *r)
+{
+	struct nftnl_expr_iter *iter = nftnl_expr_iter_create(r);
+	int cnt = 0;
+
+	if (!iter)
+		return -1;
+
+	while (nftnl_expr_iter_next(iter))
+		cnt++;
+
+	nftnl_expr_iter_destroy(iter);
+	return cnt;
+}
+
+static struct rule_udata_ext *
+__rule_get_udata_ext(const void *data, uint32_t data_len, uint32_t *outlen)
+{
+	const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
+
+	if (nftnl_udata_parse(data, data_len, parse_udata_cb, tb) < 0)
+		return NULL;
+
+	if (!tb[UDATA_TYPE_COMPAT_EXT])
+		return NULL;
+
+	if (outlen)
+		*outlen = nftnl_udata_len(tb[UDATA_TYPE_COMPAT_EXT]);
+	return nftnl_udata_get(tb[UDATA_TYPE_COMPAT_EXT]);
+}
+
+struct rule_udata_ext *
+rule_get_udata_ext(const struct nftnl_rule *r, uint32_t *outlen)
+{
+	struct nftnl_udata_buf *udata;
+	uint32_t udatalen;
+
+	udata = (void *)nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, &udatalen);
+	if (!udata)
+		return NULL;
+
+	return __rule_get_udata_ext(udata, udatalen, outlen);
+}
+
+static void
+pack_rule_udata_ext_data(struct rule_udata_ext *rue,
+			 const void *data, size_t datalen)
+{
+	size_t datalen_out = datalen;
+#ifdef HAVE_ZLIB
+	compress(rue->data, &datalen_out, data, datalen);
+	rue->zip = true;
+#else
+	memcpy(rue->data, data, datalen);
+#endif
+	rue->size = datalen_out;
+}
+
+void rule_add_udata_ext(struct nftnl_rule *r,
+			uint16_t start_idx, uint16_t end_idx,
+			uint8_t type, uint16_t size, const void *data)
+{
+	struct rule_udata_ext *ext = NULL;
+	uint32_t extlen = 0, newextlen;
+	char *newext;
+	void *udata;
+
+	ext = rule_get_udata_ext(r, &extlen);
+	if (!ext)
+		extlen = 0;
+
+	udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
+	if (!udata)
+		xtables_error(OTHER_PROBLEM, "can't alloc memory!");
+
+	newextlen = sizeof(*ext) + size;
+	newext = xtables_malloc(extlen + newextlen);
+	if (extlen)
+		memcpy(newext, ext, extlen);
+	memset(newext + extlen, 0, newextlen);
+
+	ext = (struct rule_udata_ext *)(newext + extlen);
+	ext->start_idx = start_idx;
+	ext->end_idx = end_idx;
+	ext->type = type;
+	ext->orig_size = size;
+	pack_rule_udata_ext_data(ext, data, size);
+	newextlen = sizeof(*ext) + ext->size;
+
+	if (!nftnl_udata_put(udata, UDATA_TYPE_COMPAT_EXT,
+			     extlen + newextlen, newext) ||
+	    nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
+				nftnl_udata_buf_data(udata),
+				nftnl_udata_buf_len(udata)))
+		xtables_error(OTHER_PROBLEM, "can't alloc memory!");
+
+	free(newext);
+	nftnl_udata_buf_free(udata);
+}
+
+static struct nftnl_expr *
+__nftnl_expr_from_udata_ext(struct rule_udata_ext *rue, const void *data)
+{
+	struct nftnl_expr *expr = NULL;
+
+	switch (rue->type) {
+	case RUE_TYPE_MATCH:
+		expr = nftnl_expr_alloc("match");
+		__add_match(expr, data);
+		break;
+	case RUE_TYPE_TARGET:
+		expr = nftnl_expr_alloc("target");
+		__add_target(expr, data);
+		break;
+	default:
+		fprintf(stderr,
+			"Warning: Unexpected udata extension type %d\n",
+			rue->type);
+	}
+
+	return expr;
+}
+
+static struct nftnl_expr *
+nftnl_expr_from_zipped_udata_ext(struct rule_udata_ext *rue)
+{
+#ifdef HAVE_ZLIB
+	uLongf datalen = rue->orig_size;
+	struct nftnl_expr *expr = NULL;
+	void *data;
+
+	data = xtables_malloc(datalen);
+	if (uncompress(data, &datalen, rue->data, rue->size) != Z_OK) {
+		fprintf(stderr, "Warning: Failed to uncompress rule udata extension\n");
+		goto out;
+	}
+
+	expr = __nftnl_expr_from_udata_ext(rue, data);
+out:
+	free(data);
+	return expr;
+#else
+	fprintf(stderr, "Warning: Zipped udata extensions are not supported.\n");
+	return NULL;
+#endif
+}
+
+static struct nftnl_expr *nftnl_expr_from_udata_ext(struct rule_udata_ext *rue)
+{
+	if (rue->zip)
+		return nftnl_expr_from_zipped_udata_ext(rue);
+	else
+		return __nftnl_expr_from_udata_ext(rue, rue->data);
+}
+
+bool rule_has_udata_ext(const struct nftnl_rule *r)
+{
+	return rule_get_udata_ext(r, NULL) != NULL;
+}
+
+#define rule_udata_ext_foreach(rue, ext, extlen)			\
+	for (rue = (void *)(ext);					\
+	     (char *)rue < (char *)(ext) + extlen;			\
+	     rue = (void *)((char *)rue + sizeof(*rue) + rue->size))
+
+bool rule_parse_udata_ext(struct nft_xt_ctx *ctx, const struct nftnl_rule *r)
+{
+	struct rule_udata_ext *rue;
+	struct nftnl_expr *expr;
+	uint32_t extlen;
+	bool ret = true;
+	int eidx = 0;
+	void *ext;
+
+	ext = rule_get_udata_ext(r, &extlen);
+	if (!ext)
+		return false;
+
+	rule_udata_ext_foreach(rue, ext, extlen) {
+		for (; eidx < rue->start_idx; eidx++) {
+			expr = nftnl_expr_iter_next(ctx->iter);
+			if (!nft_parse_rule_expr(ctx->h, expr, ctx))
+				ret = false;
+		}
+
+		expr = nftnl_expr_from_udata_ext(rue);
+		if (!nft_parse_rule_expr(ctx->h, expr, ctx))
+			ret = false;
+		nftnl_expr_free(expr);
+
+		for (; eidx < rue->end_idx; eidx++)
+			nftnl_expr_iter_next(ctx->iter);
+	}
+	expr = nftnl_expr_iter_next(ctx->iter);
+	while (expr != NULL) {
+		if (!nft_parse_rule_expr(ctx->h, expr, ctx))
+			ret = false;
+		expr = nftnl_expr_iter_next(ctx->iter);
+	}
+	return ret;
+}
+
diff --git a/iptables/nft-compat.h b/iptables/nft-compat.h
new file mode 100644
index 0000000000000..e91e2299bd2ae
--- /dev/null
+++ b/iptables/nft-compat.h
@@ -0,0 +1,54 @@
+#ifndef _NFT_COMPAT_H_
+#define _NFT_COMPAT_H_
+
+#include <libnftnl/rule.h>
+
+#include <linux/netfilter/x_tables.h>
+
+int nftnl_rule_expr_count(const struct nftnl_rule *r);
+
+enum rule_udata_ext_type {
+	RUE_TYPE_MATCH = 0,
+	RUE_TYPE_TARGET = 1,
+};
+
+struct rule_udata_ext {
+	uint8_t start_idx;
+	uint8_t end_idx;
+	uint8_t type;
+	uint8_t zip:1;
+	uint16_t orig_size;
+	uint16_t size;
+	unsigned char data[];
+};
+
+struct rule_udata_ext *
+rule_get_udata_ext(const struct nftnl_rule *r, uint32_t *outlen);
+
+void rule_add_udata_ext(struct nftnl_rule *r,
+			uint16_t start_idx, uint16_t end_idx,
+			uint8_t type, uint16_t size, const void *data);
+static inline void
+rule_add_udata_match(struct nftnl_rule *r,
+		     uint16_t start_idx, uint16_t end_idx,
+		     const struct xt_entry_match *m)
+{
+	rule_add_udata_ext(r, start_idx, end_idx,
+			   RUE_TYPE_MATCH, m->u.match_size, m);
+}
+
+static inline void
+rule_add_udata_target(struct nftnl_rule *r,
+		      uint16_t start_idx, uint16_t end_idx,
+		      const struct xt_entry_target *t)
+{
+	rule_add_udata_ext(r, start_idx, end_idx,
+			   RUE_TYPE_TARGET, t->u.target_size, t);
+}
+
+struct nft_xt_ctx;
+
+bool rule_has_udata_ext(const struct nftnl_rule *r);
+bool rule_parse_udata_ext(struct nft_xt_ctx *ctx, const struct nftnl_rule *r);
+
+#endif /* _NFT_COMPAT_H_ */
diff --git a/iptables/nft-ruleparse.c b/iptables/nft-ruleparse.c
index 757d3c29fc816..b58e16fff45cd 100644
--- a/iptables/nft-ruleparse.c
+++ b/iptables/nft-ruleparse.c
@@ -10,6 +10,7 @@
  * This code has been sponsored by Sophos Astaro <http://www.sophos.com>
  */
 
+#include "config.h"
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
@@ -27,6 +28,7 @@
 
 #include <xtables.h>
 
+#include "nft-compat.h"
 #include "nft-ruleparse.h"
 #include "nft.h"
 
@@ -948,6 +950,25 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 			ret = false;
 		expr = nftnl_expr_iter_next(ctx.iter);
 	}
+#ifdef DEBUG_COMPAT_EXT
+	if (rule_has_udata_ext(r))
+		ret = false;
+#endif
+	if (!ret && rule_has_udata_ext(r)) {
+		fprintf(stderr,
+			"Warning: Rule parser failed, trying compat fallback\n");
+
+		h->ops->clear_cs(cs);
+		if (h->ops->init_cs)
+			h->ops->init_cs(cs);
+
+		nftnl_expr_iter_destroy(ctx.iter);
+		ctx.iter = nftnl_expr_iter_create(r);
+		if (!ctx.iter)
+			return false;
+
+		ret = rule_parse_udata_ext(&ctx, r);
+	}
 
 	nftnl_expr_iter_destroy(ctx.iter);
 
diff --git a/iptables/nft.c b/iptables/nft.c
index 64ac35f2edcf3..de20d9714695f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -9,6 +9,7 @@
  * This code has been sponsored by Sophos Astaro <http://www.sophos.com>
  */
 
+#include "config.h"
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
@@ -60,6 +61,7 @@
 #include "nft-cache.h"
 #include "nft-shared.h"
 #include "nft-bridge.h" /* EBT_NOPROTO */
+#include "nft-compat.h"
 
 static void *nft_fn;
 
@@ -1049,6 +1051,7 @@ void __add_match(struct nftnl_expr *e, const struct xt_entry_match *m)
 static int add_nft_limit(struct nftnl_rule *r, struct xt_entry_match *m)
 {
 	struct xt_rateinfo *rinfo = (void *)m->data;
+	int i, ecnt = nftnl_rule_expr_count(r);
 	static const uint32_t mult[] = {
 		XT_LIMIT_SCALE*24*60*60,	/* day */
 		XT_LIMIT_SCALE*60*60,		/* hour */
@@ -1056,7 +1059,8 @@ static int add_nft_limit(struct nftnl_rule *r, struct xt_entry_match *m)
 		XT_LIMIT_SCALE,			/* sec */
 	};
 	struct nftnl_expr *expr;
-	int i;
+
+	rule_add_udata_match(r, ecnt, ecnt + 1, m);
 
 	expr = nftnl_expr_alloc("limit");
 	if (!expr)
@@ -1371,6 +1375,7 @@ static bool udp_all_zero(const struct xt_udp *u)
 static int add_nft_udp(struct nft_handle *h, struct nftnl_rule *r,
 		       struct xt_entry_match *m)
 {
+	int ret, ecnt = nftnl_rule_expr_count(r);
 	struct xt_udp *udp = (void *)m->data;
 
 	if (udp->invflags > XT_UDP_INV_MASK ||
@@ -1385,8 +1390,12 @@ static int add_nft_udp(struct nft_handle *h, struct nftnl_rule *r,
 	if (nftnl_rule_get_u32(r, NFTNL_RULE_COMPAT_PROTO) != IPPROTO_UDP)
 		xtables_error(PARAMETER_PROBLEM, "UDP match requires '-p udp'");
 
-	return add_nft_tcpudp(h, r, udp->spts, udp->invflags & XT_UDP_INV_SRCPT,
-			      udp->dpts, udp->invflags & XT_UDP_INV_DSTPT);
+	ret = add_nft_tcpudp(h, r, udp->spts, udp->invflags & XT_UDP_INV_SRCPT,
+			     udp->dpts, udp->invflags & XT_UDP_INV_DSTPT);
+
+	rule_add_udata_match(r, ecnt, nftnl_rule_expr_count(r), m);
+
+	return ret;
 }
 
 static int add_nft_tcpflags(struct nft_handle *h, struct nftnl_rule *r,
@@ -1423,6 +1432,7 @@ static int add_nft_tcp(struct nft_handle *h, struct nftnl_rule *r,
 		       struct xt_entry_match *m)
 {
 	static const uint8_t supported = XT_TCP_INV_SRCPT | XT_TCP_INV_DSTPT | XT_TCP_INV_FLAGS;
+	int ret, ecnt = nftnl_rule_expr_count(r);
 	struct xt_tcp *tcp = (void *)m->data;
 
 	if (tcp->invflags & ~supported || tcp->option ||
@@ -1438,23 +1448,27 @@ static int add_nft_tcp(struct nft_handle *h, struct nftnl_rule *r,
 		xtables_error(PARAMETER_PROBLEM, "TCP match requires '-p tcp'");
 
 	if (tcp->flg_mask) {
-		int ret = add_nft_tcpflags(h, r, tcp->flg_cmp, tcp->flg_mask,
-					   tcp->invflags & XT_TCP_INV_FLAGS);
+		ret = add_nft_tcpflags(h, r, tcp->flg_cmp, tcp->flg_mask,
+				       tcp->invflags & XT_TCP_INV_FLAGS);
 
 		if (ret < 0)
 			return ret;
 	}
 
-	return add_nft_tcpudp(h, r, tcp->spts, tcp->invflags & XT_TCP_INV_SRCPT,
-			      tcp->dpts, tcp->invflags & XT_TCP_INV_DSTPT);
+	ret = add_nft_tcpudp(h, r, tcp->spts, tcp->invflags & XT_TCP_INV_SRCPT,
+			     tcp->dpts, tcp->invflags & XT_TCP_INV_DSTPT);
+
+	rule_add_udata_match(r, ecnt, nftnl_rule_expr_count(r), m);
+
+	return ret;
 }
 
 static int add_nft_mark(struct nft_handle *h, struct nftnl_rule *r,
 			struct xt_entry_match *m)
 {
 	struct xt_mark_mtinfo1 *mark = (void *)m->data;
+	int op, ecnt = nftnl_rule_expr_count(r);
 	uint8_t reg;
-	int op;
 
 	add_meta(h, r, NFT_META_MARK, &reg);
 	if (mark->mask != 0xffffffff)
@@ -1467,6 +1481,8 @@ static int add_nft_mark(struct nft_handle *h, struct nftnl_rule *r,
 
 	add_cmp_u32(r, mark->mark, op, reg);
 
+	rule_add_udata_match(r, ecnt, nftnl_rule_expr_count(r), m);
+
 	return 0;
 }
 
@@ -1517,10 +1533,13 @@ void __add_target(struct nftnl_expr *e, const struct xt_entry_target *t)
 	nftnl_expr_set(e, NFTNL_EXPR_TG_INFO, info, t->u.target_size - sizeof(*t));
 }
 
-static int add_meta_nftrace(struct nftnl_rule *r)
+static int add_meta_nftrace(struct nftnl_rule *r, struct xt_entry_target *t)
 {
+	int ecnt = nftnl_rule_expr_count(r);
 	struct nftnl_expr *expr;
 
+	rule_add_udata_target(r, ecnt, ecnt + 2, t);
+
 	expr = nftnl_expr_alloc("immediate");
 	if (expr == NULL)
 		return -ENOMEM;
@@ -1544,7 +1563,7 @@ int add_target(struct nftnl_rule *r, struct xt_entry_target *t)
 	struct nftnl_expr *expr;
 
 	if (strcmp(t->u.user.name, "TRACE") == 0)
-		return add_meta_nftrace(r);
+		return add_meta_nftrace(r, t);
 
 	expr = nftnl_expr_alloc("target");
 	if (expr == NULL)
-- 
2.43.0

Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Pablo Neira Ayuso]

Hi Phil,

On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> Add a mechanism providing forward compatibility for the current and
> future versions of iptables-nft (and all other nft-variants) by
> annotating nftnl rules with the extensions they were created for.
> 
> Upon nftnl rule parsing failure, warn about the situation and perform a
> second attempt loading the respective compat extensions instead of the
> native expressions which replace them. The foundational assumption is
> that libxtables extensions are stable and thus the VM code created on
> their behalf does not need to be.
> 
> Since nftnl rule userdata attributes are restricted to 255 bytes, the
> implementation focusses on low memory consumption. Therefore, extensions
> which remain in the rule as compat expressions are not also added to
> userdata. In turn, extensions in userdata are annotated by start and end
> expression number they are replacing. Also, the actual payload is
> zipped using zlib.

What is store in the userdata extension? Is this a textual
representation of the match/target?

What is in your opinion the upside/downside of this approach?

Thanks.

Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Phil Sutter]

Hi Pablo,

On Wed, Aug 07, 2024 at 07:56:53PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> > Add a mechanism providing forward compatibility for the current and
> > future versions of iptables-nft (and all other nft-variants) by
> > annotating nftnl rules with the extensions they were created for.
> > 
> > Upon nftnl rule parsing failure, warn about the situation and perform a
> > second attempt loading the respective compat extensions instead of the
> > native expressions which replace them. The foundational assumption is
> > that libxtables extensions are stable and thus the VM code created on
> > their behalf does not need to be.
> > 
> > Since nftnl rule userdata attributes are restricted to 255 bytes, the
> > implementation focusses on low memory consumption. Therefore, extensions
> > which remain in the rule as compat expressions are not also added to
> > userdata. In turn, extensions in userdata are annotated by start and end
> > expression number they are replacing. Also, the actual payload is
> > zipped using zlib.
> 
> What is store in the userdata extension? Is this a textual
> representation of the match/target?

The patch introduces a new attribute UDATA_TYPE_COMPAT_EXT which holds
an "array" of this data structure:

| struct rule_udata_ext {
|         uint8_t start_idx;
|         uint8_t end_idx;
|         uint8_t type;
|         uint8_t zip:1;
|         uint16_t orig_size;
|         uint16_t size;
|         unsigned char data[];
| };

start_idx/end_idx are those of expressions in the rule which are to be
replaced by this extension in fallback case. The 'type' field
distinguishes matches from targets (could be single-bit as well), the
'zip' field indicates 'data' is zlib-compressed. The remaining fields
are self-explanatory, whereat 'data' holds a (compressed) object of
either struct xt_entry_match or struct xt_entry_target.

> What is in your opinion the upside/downside of this approach?

You may recall, I tried to build a mechanism which works with old
binaries. This one does not, it requires user space support.
Distributions might backport it though, maybe even just the parser part.

The upside to this is that no kernel modifications are needed, the whole
thing is transparent to the kernel (apart from the increased rule size).

I had implemented a first approach embedding the rule in textual
representation into userdata, but it was ugly for different reasons.
Also I refrained from generating the string rep. ad-hoc from a given
iptables_command_state object because that would require refactoring of
the whole printing code to use a buffer or defined fp instead of stdout
directly. Apart from ugliness caused by reusing "whatever" the user put
into argv[], I had to overcome some obstacles:

- Space restrictions in userdata, breaking for "long" rules (e.g. having
  long comments).
- Parsing a rule from string ad-hoc (e.g. to compare user input with
  rules in cache) triggered some "funny" bugs.
- No way to omit redundant data (i.e., extensions which remain as compat
  expressions in the rule).

Vice-versa, this implementation has the following benefits:

- Rule parsing in fallback case is relatively simple, userdata bits
  parse similar to compat expression payload.
- Provide just the minimum parts of the rule in userdata. Comments will
  always remain in an extension, so will never be carried in userdata.
- Extensions compress relatively well (due to zero bytes in data
  structures).

One may assess better readability of netlink debug output when using a
string rep. This got somewhat reduced by me using NUL-chars to separate
arguments, but neither nft nor libnftnl will be able to convert the
binary payload of this approach to something user-friendly. Using
libxtables though, one could print the individual extensions into
iptables "command line parts".

I'll happily answer further questions, just shoot!

Cheers, Phil

Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Pablo Neira Ayuso]

Hi Phil,

I have no better idea to cope with this forward compatibility
requirements.

On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> Add a mechanism providing forward compatibility for the current and
> future versions of iptables-nft (and all other nft-variants) by
> annotating nftnl rules with the extensions they were created for.
>
> Upon nftnl rule parsing failure, warn about the situation and perform a
> second attempt loading the respective compat extensions instead of the
> native expressions which replace them.

OK, so this is last resort to interpret the rule.

> The foundational assumption is that libxtables extensions are stable
> and thus the VM code created on their behalf does not need to be.

OK, this requires xtables API becomes frozen forever.

> Since nftnl rule userdata attributes are restricted to 255 bytes, the
> implementation focusses on low memory consumption. Therefore, extensions
> which remain in the rule as compat expressions are not also added to
> userdata. In turn, extensions in userdata are annotated by start and end
> expression number they are replacing. Also, the actual payload is
> zipped using zlib.

Binary layout is better than storing text in the userdata area.

Is this zlib approach sufficient to cope with ebtables among
extension? Maybe that one is excluded because it is using the set
infrastructure since the beginning.

I guess you already checked for worst case to make sure compression
always allows to make things fit into 255 bytes?

Thanks.

Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Phil Sutter]

Hi Pablo,

On Mon, Sep 16, 2024 at 12:13:07AM +0200, Pablo Neira Ayuso wrote:
> I have no better idea to cope with this forward compatibility
> requirements.

From my point of view, it's the best approach among the bad ones (i.e.,
those not providing compatibility for old binaries out of the box).

> On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> > Add a mechanism providing forward compatibility for the current and
> > future versions of iptables-nft (and all other nft-variants) by
> > annotating nftnl rules with the extensions they were created for.
> >
> > Upon nftnl rule parsing failure, warn about the situation and perform a
> > second attempt loading the respective compat extensions instead of the
> > native expressions which replace them.
> 
> OK, so this is last resort to interpret the rule.

It is. I had your concerns regarding crafted compat payload in rules in
mind with this. The downside is that we may make subtle changes to VM
code which the old binary won't detect and ignore. Maybe I could feature
it via flag or env var to prefer the userdata extensions. WDYT?

> > The foundational assumption is that libxtables extensions are stable
> > and thus the VM code created on their behalf does not need to be.
> 
> OK, this requires xtables API becomes frozen forever.

Well, not more than it has been: Take iptables-legacy for instance: An
old version may see a newer extension revision than it has support for,
so will fail just like iptables-nft with native code it can't parse. So
effectively iptables-nft becomes *as compatible as* the same version of
iptables-legacy.
Another perspective to this: Extension development gradually slows down
which we'll leverage while at the same time support increased
development in nftables itself and conversion of extensions to VM code.

> > Since nftnl rule userdata attributes are restricted to 255 bytes, the
> > implementation focusses on low memory consumption. Therefore, extensions
> > which remain in the rule as compat expressions are not also added to
> > userdata. In turn, extensions in userdata are annotated by start and end
> > expression number they are replacing. Also, the actual payload is
> > zipped using zlib.
> 
> Binary layout is better than storing text in the userdata area.
> 
> Is this zlib approach sufficient to cope with ebtables among
> extension? Maybe that one is excluded because it is using the set
> infrastructure since the beginning.

Yes, among is not an issue because ebtables-nft never implemented this
as extension.

> I guess you already checked for worst case to make sure compression
> always allows to make things fit into 255 bytes?

Well, we don't convert too many extensions to nftables anymore since
Florian reverted a bunch as a quick countermeasure against the compat
complaints. Things will certainly get worse, but the question is mostly
how many different extensions will "the largest rule" have. I added some
debug output and in shell testsuite for instance the largest compat_ext
userdata I see is 68 bytes. I was able to craft a rule which uses 159
bytes though:

iptables-nft -A FORWARD \
	-m limit --limit 1000/hour \
	-p udp -m udp --sport 1024:65535 --dport 4:65535 \
	-m mark --mark 0xfeedcafe/0xfeedcafe \
	-j NFLOG --nflog-group 23 \
	         --nflog-prefix "this is a damn long log prefix"

The current implementation calls xtables_error() if nftnl_udata_put()
fails. Maybe a better error path would be to only warn the user and not
add compat_ext to userdata. Guess it depends on whether this should be
enabled by default or only upon request - if user asks for compat_ext,
failing to do so should be fatal.

Cheers, Phil

Re: [iptables PATCH 0/8] nft: Implement forward compat for future binaries

[Phil Sutter]

On Thu, Aug 01, 2024 at 12:26:55AM +0200, Phil Sutter wrote:
> Time to abandon earlier attempts at providing compatibility for old
> binaries, choose the next best option which is not relying upon any
> kernel changes.
> 
> Basically, all extensions replaced by native bytecode are appended to
> rule userdata so when nftnl rule parsing code fails, it may retry
> omitting all these expressions and restoring an extension from userdata
> instead.
> 
> The idea behind this is that extensions are stable which relieves native
> bytecode from being the same. With this series in place, one may
> (re-)start converting extensions into native nftables bytecode again.
> 
> For now, appending compat extensions is always active. Keeping it
> disabled by default and enabling via commandline flag or (simpler) env
> variable might make sense (I haven't tested performance yet). The
> parsing component will take action only if standard rule parsing fails,
> so no need to manually enable this IMO.
> 
> The actual implementation sits in patch 8, the preceeding ones are
> (mostly) preparation.
> 
> To forcibly exercise the fallback rule parsing code, compile with
> CFLAGS='-DDEBUG_COMPAT_EXT=1'.
> 
> Phil Sutter (8):
>   ebtables: Zero freed pointers in ebt_cs_clean()
>   ebtables: Introduce nft_bridge_init_cs()
>   nft: Reduce overhead in nft_rule_find()
>   nft: ruleparse: Drop 'iter' variable in
>     nft_rule_to_iptables_command_state
>   nft: ruleparse: Introduce nft_parse_rule_expr()
>   nft: __add_{match,target}() can't fail
>   nft: Introduce UDATA_TYPE_COMPAT_EXT
>   nft: Support compat extensions in rule userdata

Applied patches 1-4 as they are independent from the actual compat
extensions feature.