[iptables PATCH 0/4] Implement a best-effort forward compat solution

[iptables PATCH 0/4] Implement a best-effort forward compat solution

[Phil Sutter]

Instead of adding a second, compatible rule-representation to kernel for
consumption by older user space, follow a much simpler route by
implementing a compat-mode into current *tables-nft which avoids any of
the later internal changes which may prevent an old iptables-nft from
parsing a kernel's rule correctly.

Patch 1 is just prep work, patch 2 adds the core logic, patch 3 exposes
it to CLI and patch 4 finally adds some testing.

This should resolve nfbz#1632[1], albeit requiring adjustments in how
users call iptables.

[1] https://bugzilla.netfilter.org/show_bug.cgi?id=1632
Phil Sutter (4):
  nft: Pass nft_handle to add_{target,action}()
  nft: Introduce and use bool nft_handle::compat
  Add --compat option to *tables-nft and *-nft-restore commands
  tests: Test compat mode

 iptables-test.py                              | 19 ++++--
 iptables/nft-arp.c                            |  2 +-
 iptables/nft-bridge.c                         |  9 +--
 iptables/nft-ipv4.c                           |  2 +-
 iptables/nft-ipv6.c                           |  2 +-
 iptables/nft-shared.c                         |  2 +-
 iptables/nft.c                                | 15 +++--
 iptables/nft.h                                |  5 +-
 .../testcases/nft-only/0011-compat-mode_0     | 61 +++++++++++++++++++
 iptables/xshared.c                            |  7 ++-
 iptables/xshared.h                            |  1 +
 iptables/xtables-arp.c                        |  1 +
 iptables/xtables-eb.c                         |  7 ++-
 iptables/xtables-restore.c                    | 17 +++++-
 iptables/xtables.c                            |  2 +
 15 files changed, 129 insertions(+), 23 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/nft-only/0011-compat-mode_0

-- 
2.40.0

[iptables PATCH 1/4] nft: Pass nft_handle to add_{target,action}()

[Phil Sutter]

Prepare for varying rule content based on a global flag.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c    | 2 +-
 iptables/nft-bridge.c | 9 +++++----
 iptables/nft-ipv4.c   | 2 +-
 iptables/nft-ipv6.c   | 2 +-
 iptables/nft.c        | 9 +++++----
 iptables/nft.h        | 4 ++--
 6 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 265de5f88cea0..192819276b692 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -151,7 +151,7 @@ static int nft_arp_add(struct nft_handle *h, struct nftnl_rule *r,
 		else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
 			ret = add_verdict(r, NFT_RETURN);
 		else
-			ret = add_target(r, cs->target->t);
+			ret = add_target(h, r, cs->target->t);
 	} else if (strlen(cs->jumpto) > 0) {
 		/* No goto in arptables */
 		ret = add_jumpto(r, cs->jumpto, NFT_JUMP);
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index f3dfa488c6202..f5b376c0dfe6b 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -117,7 +117,8 @@ static int add_meta_broute(struct nftnl_rule *r)
 	return 0;
 }
 
-static int _add_action(struct nftnl_rule *r, struct iptables_command_state *cs)
+static int _add_action(struct nft_handle *h, struct nftnl_rule *r,
+		       struct iptables_command_state *cs)
 {
 	const char *table = nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
 
@@ -133,7 +134,7 @@ static int _add_action(struct nftnl_rule *r, struct iptables_command_state *cs)
 		}
 	}
 
-	return add_action(r, cs, false);
+	return add_action(h, r, cs, false);
 }
 
 static int
@@ -220,7 +221,7 @@ static int nft_bridge_add(struct nft_handle *h,
 			if (nft_bridge_add_match(h, fw, r, iter->u.match->m))
 				break;
 		} else {
-			if (add_target(r, iter->u.watcher->t))
+			if (add_target(h, r, iter->u.watcher->t))
 				break;
 		}
 	}
@@ -228,7 +229,7 @@ static int nft_bridge_add(struct nft_handle *h,
 	if (add_counters(r, cs->counters.pcnt, cs->counters.bcnt) < 0)
 		return -1;
 
-	return _add_action(r, cs);
+	return _add_action(h, r, cs);
 }
 
 static bool nft_rule_to_ebtables_command_state(struct nft_handle *h,
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 6df4e46bc3773..92fe263769695 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -92,7 +92,7 @@ static int nft_ipv4_add(struct nft_handle *h, struct nftnl_rule *r,
 	if (add_counters(r, cs->counters.pcnt, cs->counters.bcnt) < 0)
 		return -1;
 
-	return add_action(r, cs, !!(cs->fw.ip.flags & IPT_F_GOTO));
+	return add_action(h, r, cs, !!(cs->fw.ip.flags & IPT_F_GOTO));
 }
 
 static bool nft_ipv4_is_same(const struct iptables_command_state *a,
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 693a1c87b997d..cfada13ce5ee8 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -79,7 +79,7 @@ static int nft_ipv6_add(struct nft_handle *h, struct nftnl_rule *r,
 	if (add_counters(r, cs->counters.pcnt, cs->counters.bcnt) < 0)
 		return -1;
 
-	return add_action(r, cs, !!(cs->fw6.ipv6.flags & IP6T_F_GOTO));
+	return add_action(h, r, cs, !!(cs->fw6.ipv6.flags & IP6T_F_GOTO));
 }
 
 static bool nft_ipv6_is_same(const struct iptables_command_state *a,
diff --git a/iptables/nft.c b/iptables/nft.c
index 1cb104e75ccc5..55f98c164846e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1526,7 +1526,8 @@ static int add_meta_nftrace(struct nftnl_rule *r)
 	return 0;
 }
 
-int add_target(struct nftnl_rule *r, struct xt_entry_target *t)
+int add_target(struct nft_handle *h, struct nftnl_rule *r,
+	       struct xt_entry_target *t)
 {
 	struct nftnl_expr *expr;
 	int ret;
@@ -1575,8 +1576,8 @@ int add_verdict(struct nftnl_rule *r, int verdict)
 	return 0;
 }
 
-int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
-	       bool goto_set)
+int add_action(struct nft_handle *h, struct nftnl_rule *r,
+	       struct iptables_command_state *cs, bool goto_set)
 {
 	int ret = 0;
 
@@ -1592,7 +1593,7 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 		else if (strcmp(cs->jumpto, "NFLOG") == 0)
 			ret = add_log(r, cs);
 		else
-			ret = add_target(r, cs->target->t);
+			ret = add_target(h, r, cs->target->t);
 	} else if (strlen(cs->jumpto) > 0) {
 		/* Not standard, then it's a go / jump to chain */
 		if (goto_set)
diff --git a/iptables/nft.h b/iptables/nft.h
index 1d18982dc8cf7..c8d5bfdc50871 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -189,9 +189,9 @@ int nft_rule_zero_counters(struct nft_handle *h, const char *chain, const char *
 int add_counters(struct nftnl_rule *r, uint64_t packets, uint64_t bytes);
 int add_verdict(struct nftnl_rule *r, int verdict);
 int add_match(struct nft_handle *h, struct nftnl_rule *r, struct xt_entry_match *m);
-int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
+int add_target(struct nft_handle *h, 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);
+int add_action(struct nft_handle *h, struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
 int add_log(struct nftnl_rule *r, struct iptables_command_state *cs);
 char *get_comment(const void *data, uint32_t data_len);
 
-- 
2.40.0

[iptables PATCH 2/4] nft: Introduce and use bool nft_handle::compat

[Phil Sutter]

If set, create rules using compat expressions where possible and disable
the bitwise expression avoidance introduced in 323259001d617 ("nft:
Optimize class-based IP prefix matches").

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c | 2 +-
 iptables/nft.c        | 6 +++++-
 iptables/nft.h        | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 12860fbf6d575..8e7a706f8765d 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -198,7 +198,7 @@ void add_addr(struct nft_handle *h, struct nftnl_rule *r,
 
 	for (i = 0; i < len; i++) {
 		if (m[i] != 0xff) {
-			bitwise = m[i] != 0;
+			bitwise = h->compat || m[i] != 0;
 			break;
 		}
 	}
diff --git a/iptables/nft.c b/iptables/nft.c
index 55f98c164846e..786e4a12cf720 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1468,6 +1468,9 @@ int add_match(struct nft_handle *h,
 	struct nftnl_expr *expr;
 	int ret;
 
+	if (h->compat && strcmp(m->u.user.name, "among"))
+		goto add_compat_expr;
+
 	if (!strcmp(m->u.user.name, "limit"))
 		return add_nft_limit(r, m);
 	else if (!strcmp(m->u.user.name, "among"))
@@ -1479,6 +1482,7 @@ int add_match(struct nft_handle *h,
 	else if (!strcmp(m->u.user.name, "mark"))
 		return add_nft_mark(h, r, m);
 
+add_compat_expr:
 	expr = nftnl_expr_alloc("match");
 	if (expr == NULL)
 		return -ENOMEM;
@@ -1532,7 +1536,7 @@ int add_target(struct nft_handle *h, struct nftnl_rule *r,
 	struct nftnl_expr *expr;
 	int ret;
 
-	if (strcmp(t->u.user.name, "TRACE") == 0)
+	if (!h->compat && strcmp(t->u.user.name, "TRACE") == 0)
 		return add_meta_nftrace(r);
 
 	expr = nftnl_expr_alloc("target");
diff --git a/iptables/nft.h b/iptables/nft.h
index c8d5bfdc50871..6f56f5b46e775 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -111,6 +111,7 @@ struct nft_handle {
 	struct list_head	cmd_list;
 	bool			cache_init;
 	int			verbose;
+	bool			compat;
 
 	/* meta data, for error reporting */
 	struct {
-- 
2.40.0

[iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Phil Sutter]

The flag sets nft_handle::compat boolean, indicating a compatible rule
implementation is wanted. Users expecting their created rules to be
fetched from kernel by an older version of *tables-nft may use this to
avoid potential compatibility issues.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.c         |  7 ++++++-
 iptables/xshared.h         |  1 +
 iptables/xtables-arp.c     |  1 +
 iptables/xtables-eb.c      |  7 ++++++-
 iptables/xtables-restore.c | 17 +++++++++++++++--
 iptables/xtables.c         |  2 ++
 6 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 17aed04e02b09..502d0a9bda4c6 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1273,7 +1273,8 @@ xtables_printhelp(const struct xtables_rule_match *matches)
 	printf(
 "  --modprobe=<command>		try to insert modules using this command\n"
 "  --set-counters -c PKTS BYTES	set the counter during insert/append\n"
-"[!] --version	-V		print package version.\n");
+"[!] --version	-V		print package version\n"
+"  --compat			create rules compatible for parsing with old binaries\n");
 
 	if (afinfo->family == NFPROTO_ARP) {
 		int i;
@@ -1796,6 +1797,10 @@ void do_parse(int argc, char *argv[],
 
 			exit_tryhelp(2, p->line);
 
+		case 15: /* --compat */
+			p->compat = true;
+			break;
+
 		case 1: /* non option */
 			if (optarg[0] == '!' && optarg[1] == '\0') {
 				if (invert)
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 0ed9f3c29c600..d8c56cf38790d 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -276,6 +276,7 @@ struct xt_cmd_parse {
 	int				line;
 	int				verbose;
 	bool				xlate;
+	bool				compat;
 	struct xt_cmd_parse_ops		*ops;
 };
 
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 71518a9cbdb6a..c6a9c6d68cb10 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -78,6 +78,7 @@ static struct option original_opts[] = {
 	{ "line-numbers", 0, 0, '0' },
 	{ "modprobe", 1, 0, 'M' },
 	{ "set-counters", 1, 0, 'c' },
+	{ "compat", 0, 0, 15 },
 	{ 0 }
 };
 
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index bf35f52b7585d..857a3c6f19d82 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -199,6 +199,7 @@ struct option ebt_original_options[] =
 	{ "init-table"     , no_argument      , 0, 11  },
 	{ "concurrent"     , no_argument      , 0, 13  },
 	{ "check"          , required_argument, 0, 14  },
+	{ "compat"         , no_argument      , 0, 15  },
 	{ 0 }
 };
 
@@ -311,7 +312,8 @@ static void print_help(const struct xtables_target *t,
 "--modprobe -M program         : try to insert modules using this program\n"
 "--concurrent                  : use a file lock to support concurrent scripts\n"
 "--verbose -v                  : verbose mode\n"
-"--version -V                  : print package version\n\n"
+"--version -V                  : print package version\n"
+"--compat                      : create rules compatible for parsing with old binaries\n\n"
 "Environment variable:\n"
 /*ATOMIC_ENV_VARIABLE "          : if set <FILE> (see above) will equal its value"*/
 "\n\n");
@@ -1074,6 +1076,9 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 			return 1;
 		case 13 :
 			break;
+		case 15:
+			h->compat = true;
+			break;
 		case 1 :
 			if (!strcmp(optarg, "!"))
 				ebt_check_inverse2(optarg, argc, argv);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index abe56374289f4..14699a514f5ce 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -26,6 +26,7 @@ static int counters, verbose;
 /* Keeping track of external matches and targets.  */
 static const struct option options[] = {
 	{.name = "counters", .has_arg = false, .val = 'c'},
+	{.name = "compat",   .has_arg = false, .val = 'C'},
 	{.name = "verbose",  .has_arg = false, .val = 'v'},
 	{.name = "version",       .has_arg = 0, .val = 'V'},
 	{.name = "test",     .has_arg = false, .val = 't'},
@@ -45,8 +46,9 @@ static const struct option options[] = {
 
 static void print_usage(const char *name, const char *version)
 {
-	fprintf(stderr, "Usage: %s [-c] [-v] [-V] [-t] [-h] [-n] [-T table] [-M command] [-4] [-6] [file]\n"
+	fprintf(stderr, "Usage: %s [-c] [-C] [-v] [-V] [-t] [-h] [-n] [-T table] [-M command] [-4] [-6] [file]\n"
 			"	   [ --counters ]\n"
+			"	   [ --compat ]\n"
 			"	   [ --verbose ]\n"
 			"	   [ --version]\n"
 			"	   [ --test ]\n"
@@ -291,6 +293,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 		.cb = &restore_cb,
 	};
 	bool noflush = false;
+	bool compat = false;
 	struct nft_handle h;
 	int c;
 
@@ -313,6 +316,9 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 			case 'c':
 				counters = 1;
 				break;
+			case 'C':
+				compat = true;
+				break;
 			case 'v':
 				verbose++;
 				break;
@@ -389,6 +395,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 	}
 	h.noflush = noflush;
 	h.restore = true;
+	h.compat = compat;
 
 	xtables_restore_parse(&h, &p);
 
@@ -419,6 +426,7 @@ static const struct nft_xt_restore_cb ebt_restore_cb = {
 };
 
 static const struct option ebt_restore_options[] = {
+	{.name = "compat",  .has_arg = 0, .val = 'C'},
 	{.name = "noflush", .has_arg = 0, .val = 'n'},
 	{.name = "verbose", .has_arg = 0, .val = 'v'},
 	{ 0 }
@@ -431,12 +439,16 @@ int xtables_eb_restore_main(int argc, char *argv[])
 		.cb = &ebt_restore_cb,
 	};
 	bool noflush = false;
+	bool compat = false;
 	struct nft_handle h;
 	int c;
 
 	while ((c = getopt_long(argc, argv, "nv",
 				ebt_restore_options, NULL)) != -1) {
 		switch(c) {
+		case 'C':
+			compat = true;
+			break;
 		case 'n':
 			noflush = 1;
 			break;
@@ -445,7 +457,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 			break;
 		default:
 			fprintf(stderr,
-				"Usage: ebtables-restore [ --verbose ] [ --noflush ]\n");
+				"Usage: ebtables-restore [ --compat ] [ --verbose ] [ --noflush ]\n");
 			exit(1);
 			break;
 		}
@@ -453,6 +465,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 
 	nft_init_eb(&h, "ebtables-restore");
 	h.noflush = noflush;
+	h.compat = compat;
 	xtables_restore_parse(&h, &p);
 	nft_fini_eb(&h);
 
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 22d6ea58376fc..25b4dbc6b8475 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -82,6 +82,7 @@ static struct option original_opts[] = {
 	{.name = "goto",	  .has_arg = 1, .val = 'g'},
 	{.name = "ipv4",	  .has_arg = 0, .val = '4'},
 	{.name = "ipv6",	  .has_arg = 0, .val = '6'},
+	{.name = "compat",        .has_arg = 0, .val = 15 },
 	{NULL},
 };
 
@@ -161,6 +162,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
 
 	do_parse(argc, argv, &p, &cs, &args);
 	h->verbose = p.verbose;
+	h->compat = p.compat;
 
 	if (!nft_table_builtin_find(h, p.table))
 		xtables_error(VERSION_PROBLEM,
-- 
2.40.0

[iptables PATCH 4/4] tests: Test compat mode

[Phil Sutter]

Extend iptables-test.py by a third mode, which is using
xtables-nft-multi and passing --compat to all calls creating rules.

Also add a shell testcase asserting the effectiveness of --compat by
comparing debug (-vv) output.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables-test.py                              | 19 ++++--
 .../testcases/nft-only/0011-compat-mode_0     | 61 +++++++++++++++++++
 2 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/nft-only/0011-compat-mode_0

diff --git a/iptables-test.py b/iptables-test.py
index ef0a35d3daa22..d41778ba4e653 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -28,6 +28,8 @@ EBTABLES_SAVE = "ebtables-save"
 #IPTABLES_SAVE = ['xtables-save','-4']
 #IP6TABLES_SAVE = ['xtables-save','-6']
 
+COMPAT_ARG = ""
+
 EXTENSIONS_PATH = "extensions"
 LOGFILE="/tmp/iptables-test.log"
 log_file = None
@@ -83,7 +85,7 @@ STDERR_IS_TTY = sys.stderr.isatty()
     '''
     ret = 0
 
-    cmd = iptables + " -A " + rule
+    cmd = iptables + COMPAT_ARG + " -A " + rule
     ret = execute_cmd(cmd, filename, lineno, netns)
 
     #
@@ -318,7 +320,7 @@ STDERR_IS_TTY = sys.stderr.isatty()
 
     # load all rules via iptables_restore
 
-    command = EXECUTABLE + " " + iptables + "-restore"
+    command = EXECUTABLE + " " + iptables + "-restore" + COMPAT_ARG
     if netns:
         command = "ip netns exec " + netns + " " + command
 
@@ -555,6 +557,8 @@ STDERR_IS_TTY = sys.stderr.isatty()
                         help='Check for missing tests')
     parser.add_argument('-n', '--nftables', action='store_true',
                         help='Test iptables-over-nftables')
+    parser.add_argument('-c', '--nft-compat', action='store_true',
+                        help='Test iptables-over-nftables in compat mode')
     parser.add_argument('-N', '--netns', action='store_const',
                         const='____iptables-container-test',
                         help='Test netnamespace path')
@@ -574,8 +578,10 @@ STDERR_IS_TTY = sys.stderr.isatty()
         variants.append("legacy")
     if args.nftables:
         variants.append("nft")
+    if args.nft_compat:
+        variants.append("nft_compat")
     if len(variants) == 0:
-        variants = [ "legacy", "nft" ]
+        variants = [ "legacy", "nft", "nft_compat" ]
 
     if os.getuid() != 0:
         print("You need to be root to run this, sorry", file=sys.stderr)
@@ -595,7 +601,12 @@ STDERR_IS_TTY = sys.stderr.isatty()
     total_tests = 0
     for variant in variants:
         global EXECUTABLE
-        EXECUTABLE = "xtables-" + variant + "-multi"
+        global COMPAT_ARG
+        if variant == "nft_compat":
+            EXECUTABLE = "xtables-nft-multi"
+            COMPAT_ARG = " --compat"
+        else:
+            EXECUTABLE = "xtables-" + variant + "-multi"
 
         test_files = 0
         tests = 0
diff --git a/iptables/tests/shell/testcases/nft-only/0011-compat-mode_0 b/iptables/tests/shell/testcases/nft-only/0011-compat-mode_0
new file mode 100755
index 0000000000000..b339dd9ec870b
--- /dev/null
+++ b/iptables/tests/shell/testcases/nft-only/0011-compat-mode_0
@@ -0,0 +1,61 @@
+#!/bin/bash
+
+[[ $XT_MULTI == *xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+
+set -e
+
+# reduce noise in debug output
+$XT_MULTI iptables -t raw -A OUTPUT
+$XT_MULTI iptables -t raw -F
+
+# add all the things which were "optimized" here
+RULE='-t raw -A OUTPUT'
+
+# prefix matches on class (actually: byte) boundaries no longer need a bitwise
+RULE+=' -s 10.0.0.0/8 -d 192.168.0.0/16'
+
+# these were turned into native matches meanwhile
+# (plus -m tcp, but it conflicts with -m udp)
+RULE+=' -m limit --limit 1/min'
+RULE+=' -p udp -m udp --sport 1024:65535'
+RULE+=' -m mark --mark 0xfeedcafe/0xfeedcafe'
+RULE+=' -j TRACE'
+
+EXPECT_COMMON='TRACE  udp opt -- in * out *  10.0.0.0/8  -> 192.168.0.0/16   limit: avg 1/min burst 5 udp spts:1024:65535 mark match 0xfeedcafe/0xfeedcafe
+ip raw OUTPUT
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000011 ]'
+
+EXPECT="$EXPECT_COMMON
+  [ payload load 1b @ network header + 12 => reg 1 ]
+  [ cmp eq reg 1 0x0000000a ]
+  [ payload load 2b @ network header + 16 => reg 1 ]
+  [ cmp eq reg 1 0x0000a8c0 ]
+  [ limit rate 1/minute burst 5 type packets flags 0x0 ]
+  [ payload load 2b @ transport header + 0 => reg 1 ]
+  [ range eq reg 1 0x00000004 0x0000ffff ]
+  [ meta load mark => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0xfeedcafe ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0xfeedcafe ]
+  [ counter pkts 0 bytes 0 ]
+  [ immediate reg 9 0x00000001 ]
+  [ meta set nftrace with reg 9 ]
+"
+
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables -vv $RULE)
+
+EXPECT="$EXPECT_COMMON
+  [ payload load 4b @ network header + 12 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x000000ff ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x0000000a ]
+  [ payload load 4b @ network header + 16 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x0000ffff ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x0000a8c0 ]
+  [ match name limit rev 0 ]
+  [ match name udp rev 0 ]
+  [ match name mark rev 1 ]
+  [ counter pkts 0 bytes 0 ]
+  [ target name TRACE rev 0 ]
+"
+
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables --compat -vv $RULE)
-- 
2.40.0

Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Pablo Neira Ayuso]

Hi Phil,

On Fri, May 05, 2023 at 08:34:45PM +0200, Phil Sutter wrote:
> The flag sets nft_handle::compat boolean, indicating a compatible rule
> implementation is wanted. Users expecting their created rules to be
> fetched from kernel by an older version of *tables-nft may use this to
> avoid potential compatibility issues.

This would require containers to be updated to use this new option or
maybe there is a transparent way to invoke this new --compat option?

I still think using userdata for this is the way to address I call it
"forward compatibility" issue, that is: old iptables binaries can
interpret what new iptables binary generates.

I am afraid this new option does not handle these two scenarios?

- new match/target that is not supported by older iptables version
  could not be printed.
- match/target from xtables-addons that is not supported by different
  iptables without these extensions.

I read the notes we collected during NFWS and we seem to agree at that
time. Maybe some of the requirements have changed since NFWS?

Apologies in advance if you feel we are going a bit into circles with
this.

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/xshared.c         |  7 ++++++-
>  iptables/xshared.h         |  1 +
>  iptables/xtables-arp.c     |  1 +
>  iptables/xtables-eb.c      |  7 ++++++-
>  iptables/xtables-restore.c | 17 +++++++++++++++--
>  iptables/xtables.c         |  2 ++
>  6 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/iptables/xshared.c b/iptables/xshared.c
> index 17aed04e02b09..502d0a9bda4c6 100644
> --- a/iptables/xshared.c
> +++ b/iptables/xshared.c
> @@ -1273,7 +1273,8 @@ xtables_printhelp(const struct xtables_rule_match *matches)
>  	printf(
>  "  --modprobe=<command>		try to insert modules using this command\n"
>  "  --set-counters -c PKTS BYTES	set the counter during insert/append\n"
> -"[!] --version	-V		print package version.\n");
> +"[!] --version	-V		print package version\n"
> +"  --compat			create rules compatible for parsing with old binaries\n");
>  
>  	if (afinfo->family == NFPROTO_ARP) {
>  		int i;
> @@ -1796,6 +1797,10 @@ void do_parse(int argc, char *argv[],
>  
>  			exit_tryhelp(2, p->line);
>  
> +		case 15: /* --compat */
> +			p->compat = true;
> +			break;
> +
>  		case 1: /* non option */
>  			if (optarg[0] == '!' && optarg[1] == '\0') {
>  				if (invert)
> diff --git a/iptables/xshared.h b/iptables/xshared.h
> index 0ed9f3c29c600..d8c56cf38790d 100644
> --- a/iptables/xshared.h
> +++ b/iptables/xshared.h
> @@ -276,6 +276,7 @@ struct xt_cmd_parse {
>  	int				line;
>  	int				verbose;
>  	bool				xlate;
> +	bool				compat;
>  	struct xt_cmd_parse_ops		*ops;
>  };
>  
> diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
> index 71518a9cbdb6a..c6a9c6d68cb10 100644
> --- a/iptables/xtables-arp.c
> +++ b/iptables/xtables-arp.c
> @@ -78,6 +78,7 @@ static struct option original_opts[] = {
>  	{ "line-numbers", 0, 0, '0' },
>  	{ "modprobe", 1, 0, 'M' },
>  	{ "set-counters", 1, 0, 'c' },
> +	{ "compat", 0, 0, 15 },
>  	{ 0 }
>  };
>  
> diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
> index bf35f52b7585d..857a3c6f19d82 100644
> --- a/iptables/xtables-eb.c
> +++ b/iptables/xtables-eb.c
> @@ -199,6 +199,7 @@ struct option ebt_original_options[] =
>  	{ "init-table"     , no_argument      , 0, 11  },
>  	{ "concurrent"     , no_argument      , 0, 13  },
>  	{ "check"          , required_argument, 0, 14  },
> +	{ "compat"         , no_argument      , 0, 15  },
>  	{ 0 }
>  };
>  
> @@ -311,7 +312,8 @@ static void print_help(const struct xtables_target *t,
>  "--modprobe -M program         : try to insert modules using this program\n"
>  "--concurrent                  : use a file lock to support concurrent scripts\n"
>  "--verbose -v                  : verbose mode\n"
> -"--version -V                  : print package version\n\n"
> +"--version -V                  : print package version\n"
> +"--compat                      : create rules compatible for parsing with old binaries\n\n"
>  "Environment variable:\n"
>  /*ATOMIC_ENV_VARIABLE "          : if set <FILE> (see above) will equal its value"*/
>  "\n\n");
> @@ -1074,6 +1076,9 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
>  			return 1;
>  		case 13 :
>  			break;
> +		case 15:
> +			h->compat = true;
> +			break;
>  		case 1 :
>  			if (!strcmp(optarg, "!"))
>  				ebt_check_inverse2(optarg, argc, argv);
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> index abe56374289f4..14699a514f5ce 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -26,6 +26,7 @@ static int counters, verbose;
>  /* Keeping track of external matches and targets.  */
>  static const struct option options[] = {
>  	{.name = "counters", .has_arg = false, .val = 'c'},
> +	{.name = "compat",   .has_arg = false, .val = 'C'},
>  	{.name = "verbose",  .has_arg = false, .val = 'v'},
>  	{.name = "version",       .has_arg = 0, .val = 'V'},
>  	{.name = "test",     .has_arg = false, .val = 't'},
> @@ -45,8 +46,9 @@ static const struct option options[] = {
>  
>  static void print_usage(const char *name, const char *version)
>  {
> -	fprintf(stderr, "Usage: %s [-c] [-v] [-V] [-t] [-h] [-n] [-T table] [-M command] [-4] [-6] [file]\n"
> +	fprintf(stderr, "Usage: %s [-c] [-C] [-v] [-V] [-t] [-h] [-n] [-T table] [-M command] [-4] [-6] [file]\n"
>  			"	   [ --counters ]\n"
> +			"	   [ --compat ]\n"
>  			"	   [ --verbose ]\n"
>  			"	   [ --version]\n"
>  			"	   [ --test ]\n"
> @@ -291,6 +293,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
>  		.cb = &restore_cb,
>  	};
>  	bool noflush = false;
> +	bool compat = false;
>  	struct nft_handle h;
>  	int c;
>  
> @@ -313,6 +316,9 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
>  			case 'c':
>  				counters = 1;
>  				break;
> +			case 'C':
> +				compat = true;
> +				break;
>  			case 'v':
>  				verbose++;
>  				break;
> @@ -389,6 +395,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
>  	}
>  	h.noflush = noflush;
>  	h.restore = true;
> +	h.compat = compat;
>  
>  	xtables_restore_parse(&h, &p);
>  
> @@ -419,6 +426,7 @@ static const struct nft_xt_restore_cb ebt_restore_cb = {
>  };
>  
>  static const struct option ebt_restore_options[] = {
> +	{.name = "compat",  .has_arg = 0, .val = 'C'},
>  	{.name = "noflush", .has_arg = 0, .val = 'n'},
>  	{.name = "verbose", .has_arg = 0, .val = 'v'},
>  	{ 0 }
> @@ -431,12 +439,16 @@ int xtables_eb_restore_main(int argc, char *argv[])
>  		.cb = &ebt_restore_cb,
>  	};
>  	bool noflush = false;
> +	bool compat = false;
>  	struct nft_handle h;
>  	int c;
>  
>  	while ((c = getopt_long(argc, argv, "nv",
>  				ebt_restore_options, NULL)) != -1) {
>  		switch(c) {
> +		case 'C':
> +			compat = true;
> +			break;
>  		case 'n':
>  			noflush = 1;
>  			break;
> @@ -445,7 +457,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
>  			break;
>  		default:
>  			fprintf(stderr,
> -				"Usage: ebtables-restore [ --verbose ] [ --noflush ]\n");
> +				"Usage: ebtables-restore [ --compat ] [ --verbose ] [ --noflush ]\n");
>  			exit(1);
>  			break;
>  		}
> @@ -453,6 +465,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
>  
>  	nft_init_eb(&h, "ebtables-restore");
>  	h.noflush = noflush;
> +	h.compat = compat;
>  	xtables_restore_parse(&h, &p);
>  	nft_fini_eb(&h);
>  
> diff --git a/iptables/xtables.c b/iptables/xtables.c
> index 22d6ea58376fc..25b4dbc6b8475 100644
> --- a/iptables/xtables.c
> +++ b/iptables/xtables.c
> @@ -82,6 +82,7 @@ static struct option original_opts[] = {
>  	{.name = "goto",	  .has_arg = 1, .val = 'g'},
>  	{.name = "ipv4",	  .has_arg = 0, .val = '4'},
>  	{.name = "ipv6",	  .has_arg = 0, .val = '6'},
> +	{.name = "compat",        .has_arg = 0, .val = 15 },
>  	{NULL},
>  };
>  
> @@ -161,6 +162,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
>  
>  	do_parse(argc, argv, &p, &cs, &args);
>  	h->verbose = p.verbose;
> +	h->compat = p.compat;
>  
>  	if (!nft_table_builtin_find(h, p.table))
>  		xtables_error(VERSION_PROBLEM,
> -- 
> 2.40.0
> 

Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Phil Sutter]

Hi Pablo,

On Wed, May 31, 2023 at 02:16:20AM +0200, Pablo Neira Ayuso wrote:
> On Fri, May 05, 2023 at 08:34:45PM +0200, Phil Sutter wrote:
> > The flag sets nft_handle::compat boolean, indicating a compatible rule
> > implementation is wanted. Users expecting their created rules to be
> > fetched from kernel by an older version of *tables-nft may use this to
> > avoid potential compatibility issues.
> 
> This would require containers to be updated to use this new option or
> maybe there is a transparent way to invoke this new --compat option?

It does not require a container update, merely the host (or whatever
runs the newer iptables-nft binary) must be adjusted. The idea is to
provide the ruleset in a compatible way so old userspace will parse it
correctly.

> I still think using userdata for this is the way to address I call it
> "forward compatibility" issue, that is: old iptables binaries can
> interpret what new iptables binary generates.

But it requires to update the "old iptables binaries", no? If so, are
they still "old" then? ;)

> I am afraid this new option does not handle these two scenarios?
> 
> - new match/target that is not supported by older iptables version
>   could not be printed.
> - match/target from xtables-addons that is not supported by different
>   iptables without these extensions.

That's correct. Both these limitations apply to legacy iptables as well,
and there's a third one, namely new match/target revision.

I could introduce a flag for extensions to disqualify them for use with
--compat if required. On the other hand, new extensions (or revisions)
are not as common anymore since we instruct people to add new features
to nftables instead.

> I read the notes we collected during NFWS and we seem to agree at that
> time. Maybe some of the requirements have changed since NFWS?

During NFWS, Florian suggested to just add the rule in textual
representation to userdata. Though this is ugly as there are two
different formats (save and print) so user space has to parse and
reprint the rule.

Then I revived my "rule bytecode for output" approach and got it working
apart from lookup expression. But finally you axed it since it requires
kernel adjustments.

From my perspective though, all solutions divide into good and bad ones:
The bad ones are those requiring to touch the old binaries. So if
acceptable, I'd much rather go with any of the "good" ones even though
it has obvious drawbacks.

> Apologies in advance if you feel we are going a bit into circles with
> this.

No worries! All this could be clarified over the course of two beers in
a pub. This medium is less lossy, though. :D

Cheers, Phil

Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Then I revived my "rule bytecode for output" approach and got it working
> apart from lookup expression. But finally you axed it since it requires
> kernel adjustments.

Can you remind me what the problem with userdata is/was?
Brief summary will hopefully be enough ...

I agree text representation sucks due to two different formats, but what
about storing binary blob (xt format) of the rule in userdata?

Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Phil Sutter]

On Wed, May 31, 2023 at 01:28:16PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Then I revived my "rule bytecode for output" approach and got it working
> > apart from lookup expression. But finally you axed it since it requires
> > kernel adjustments.
> 
> Can you remind me what the problem with userdata is/was?
> Brief summary will hopefully be enough ...
> 
> I agree text representation sucks due to two different formats, but what
> about storing binary blob (xt format) of the rule in userdata?

It requires updated binaries to support it on the receiver side. Or are
you suggesting the kernel to put the blob from userdata into
NFTA_RULE_EXPRESSIONS in dumps?

Cheers, Phil

Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands

[Phil Sutter]

On Wed, May 31, 2023 at 02:10:42PM +0200, Phil Sutter wrote:
> On Wed, May 31, 2023 at 01:28:16PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > Then I revived my "rule bytecode for output" approach and got it working
> > > apart from lookup expression. But finally you axed it since it requires
> > > kernel adjustments.
> > 
> > Can you remind me what the problem with userdata is/was?
> > Brief summary will hopefully be enough ...
> > 
> > I agree text representation sucks due to two different formats, but what
> > about storing binary blob (xt format) of the rule in userdata?
> 
> It requires updated binaries to support it on the receiver side. Or are
> you suggesting the kernel to put the blob from userdata into
> NFTA_RULE_EXPRESSIONS in dumps?

Which would also not work if it contained lookup expressions as they
won't get initialized.

Any further feedback? I know it's not a perfect solution, but given the
constraints I see no good alternative.

Cheers, Phil