* [iptables PATCH 1/4] nft: Pass nft_handle to add_{target,action}()
2023-05-05 18:34 [iptables PATCH 0/4] Implement a best-effort forward compat solution Phil Sutter
@ 2023-05-05 18:34 ` Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 2/4] nft: Introduce and use bool nft_handle::compat Phil Sutter
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2023-05-05 18:34 UTC (permalink / raw)
To: netfilter-devel
Cc: Florian Westphal, Pablo Neira Ayuso, Eric Garver, danw, aauren
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [iptables PATCH 2/4] nft: Introduce and use bool nft_handle::compat
2023-05-05 18:34 [iptables PATCH 0/4] Implement a best-effort forward compat solution Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 1/4] nft: Pass nft_handle to add_{target,action}() Phil Sutter
@ 2023-05-05 18:34 ` Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 4/4] tests: Test compat mode Phil Sutter
3 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2023-05-05 18:34 UTC (permalink / raw)
To: netfilter-devel
Cc: Florian Westphal, Pablo Neira Ayuso, Eric Garver, danw, aauren
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-05 18:34 [iptables PATCH 0/4] Implement a best-effort forward compat solution Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 1/4] nft: Pass nft_handle to add_{target,action}() Phil Sutter
2023-05-05 18:34 ` [iptables PATCH 2/4] nft: Introduce and use bool nft_handle::compat Phil Sutter
@ 2023-05-05 18:34 ` Phil Sutter
2023-05-31 0:16 ` Pablo Neira Ayuso
2023-05-05 18:34 ` [iptables PATCH 4/4] tests: Test compat mode Phil Sutter
3 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2023-05-05 18:34 UTC (permalink / raw)
To: netfilter-devel
Cc: Florian Westphal, Pablo Neira Ayuso, Eric Garver, danw, aauren
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-05 18:34 ` [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands Phil Sutter
@ 2023-05-31 0:16 ` Pablo Neira Ayuso
2023-05-31 9:02 ` Phil Sutter
0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-31 0:16 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel, Florian Westphal, Eric Garver, danw, aauren
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-31 0:16 ` Pablo Neira Ayuso
@ 2023-05-31 9:02 ` Phil Sutter
2023-05-31 11:28 ` Florian Westphal
0 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2023-05-31 9:02 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netfilter-devel, Florian Westphal, Eric Garver, danw, aauren
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-31 9:02 ` Phil Sutter
@ 2023-05-31 11:28 ` Florian Westphal
2023-05-31 12:10 ` Phil Sutter
0 siblings, 1 reply; 10+ messages in thread
From: Florian Westphal @ 2023-05-31 11:28 UTC (permalink / raw)
To: Phil Sutter, Pablo Neira Ayuso, netfilter-devel, Florian Westphal,
Eric Garver, danw, aauren
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-31 11:28 ` Florian Westphal
@ 2023-05-31 12:10 ` Phil Sutter
2023-06-23 16:52 ` Phil Sutter
0 siblings, 1 reply; 10+ messages in thread
From: Phil Sutter @ 2023-05-31 12:10 UTC (permalink / raw)
To: Florian Westphal
Cc: Pablo Neira Ayuso, netfilter-devel, Eric Garver, danw, aauren
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands
2023-05-31 12:10 ` Phil Sutter
@ 2023-06-23 16:52 ` Phil Sutter
0 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2023-06-23 16:52 UTC (permalink / raw)
To: Florian Westphal, Pablo Neira Ayuso, netfilter-devel, Eric Garver,
danw, aauren
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [iptables PATCH 4/4] tests: Test compat mode
2023-05-05 18:34 [iptables PATCH 0/4] Implement a best-effort forward compat solution Phil Sutter
` (2 preceding siblings ...)
2023-05-05 18:34 ` [iptables PATCH 3/4] Add --compat option to *tables-nft and *-nft-restore commands Phil Sutter
@ 2023-05-05 18:34 ` Phil Sutter
3 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2023-05-05 18:34 UTC (permalink / raw)
To: netfilter-devel
Cc: Florian Westphal, Pablo Neira Ayuso, Eric Garver, danw, aauren
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
^ permalink raw reply related [flat|nested] 10+ messages in thread