| Message ID | 20230505183446.28822-4-phil@nwl.cc |
|---|---|
| State | Superseded, archived |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Implement a best-effort forward compat solution | expand |
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 >
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
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?
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
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
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,
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(-)