| Message ID | 20181230190612.29413-6-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Separate rule cache per chain et al. | expand |
On Sun, Dec 30, 2018 at 08:06:12PM +0100, Phil Sutter wrote: > diff --git a/iptables/xtables.c b/iptables/xtables.c > index da11e8cc159a0..28223e8edc799 100644 > --- a/iptables/xtables.c > +++ b/iptables/xtables.c > @@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table, > cs.options & OPT_NUMERIC, > cs.options & OPT_EXPANDED, > cs.options & OPT_LINENUMBERS); > + if (p.command == CMD_LIST) > + nft_abort(h); Goal of this patch is to reset the batch of any pending object to be added before -L call? Let me know, thanks!
On Thu, Jan 10, 2019 at 01:28:22AM +0100, Pablo Neira Ayuso wrote: > On Sun, Dec 30, 2018 at 08:06:12PM +0100, Phil Sutter wrote: > > diff --git a/iptables/xtables.c b/iptables/xtables.c > > index da11e8cc159a0..28223e8edc799 100644 > > --- a/iptables/xtables.c > > +++ b/iptables/xtables.c > > @@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table, > > cs.options & OPT_NUMERIC, > > cs.options & OPT_EXPANDED, > > cs.options & OPT_LINENUMBERS); > > + if (p.command == CMD_LIST) > > + nft_abort(h); > > Goal of this patch is to reset the batch of any pending object to be > added before -L call? The goal is to prevent the unconditional commit (e.g. in xtables_main()) from adding builtin tables/chains when listing only. In the past, we had to commit early after creating builtin tables/chains to make them appear in ruleset listing. A problem we faced with multiple times was that 'nft flush ruleset ; iptables-nft -L' did not print anything. Fix was to commit early (commit a4e78370af849) and flush cache so it's refreshed (commit 206033ede9461). Since builtin tables/chains are manually added to cache, the early commit and cache flush is no longer required. But after listing the empty ruleset, the final commit still adds the builtin tables/chains although not required for list command. This patch eliminates those pointless batch jobs. Cheers, Phil
On Thu, Jan 10, 2019 at 11:32:27AM +0100, Phil Sutter wrote: > On Thu, Jan 10, 2019 at 01:28:22AM +0100, Pablo Neira Ayuso wrote: > > On Sun, Dec 30, 2018 at 08:06:12PM +0100, Phil Sutter wrote: > > > diff --git a/iptables/xtables.c b/iptables/xtables.c > > > index da11e8cc159a0..28223e8edc799 100644 > > > --- a/iptables/xtables.c > > > +++ b/iptables/xtables.c > > > @@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table, > > > cs.options & OPT_NUMERIC, > > > cs.options & OPT_EXPANDED, > > > cs.options & OPT_LINENUMBERS); > > > + if (p.command == CMD_LIST) > > > + nft_abort(h); > > > > Goal of this patch is to reset the batch of any pending object to be > > added before -L call? > > The goal is to prevent the unconditional commit (e.g. in xtables_main()) > from adding builtin tables/chains when listing only. Ah I see, thanks for explaining. > In the past, we had to commit early after creating builtin tables/chains > to make them appear in ruleset listing. A problem we faced with multiple > times was that 'nft flush ruleset ; iptables-nft -L' did not print > anything. Fix was to commit early (commit a4e78370af849) and flush cache > so it's refreshed (commit 206033ede9461). > > Since builtin tables/chains are manually added to cache, the early > commit and cache flush is no longer required. But after listing the > empty ruleset, the final commit still adds the builtin tables/chains > although not required for list command. This patch eliminates those > pointless batch jobs. I think we need to add the built-in chains when listing if we want to emulate the iptables-legacy behaviour. Listing via -L implies table autoload, ie. # iptables-legacy -L -t raw pulls in the raw table and its chains. For iptables-save, behaviour is different since we do _not_ pull in tables and built-in chains. Anyway, if we need disable the nft_commit() call, I would prefer if we pass a parameter to do_commandx(..., &commit); so we just set this commit to false from within do_commandx() depending on the command type, rather than calling nft_abort(). Thanks.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > I think we need to add the built-in chains when listing if we want to > emulate the iptables-legacy behaviour. Listing via -L implies table > autoload, ie. > > # iptables-legacy -L -t raw > > pulls in the raw table and its chains. Yes, but I think thats a bug :-) I would prefer if iptables-nft would NOT do that, and instead list nothing, with exit value of 0.
On Fri, Jan 11, 2019 at 12:16:33AM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > I think we need to add the built-in chains when listing if we want to > > emulate the iptables-legacy behaviour. Listing via -L implies table > > autoload, ie. > > > > # iptables-legacy -L -t raw > > > > pulls in the raw table and its chains. > > Yes, but I think thats a bug :-) OK, but that buggy behaviour has been there since the beginning IIRC :-) > I would prefer if iptables-nft would NOT do that, and instead > list nothing, with exit value of 0. People should be using iptables-save instead for listing anyway, so I don't mind if this is changed.
Hi, On Fri, Jan 11, 2019 at 12:33:10AM +0100, Pablo Neira Ayuso wrote: > On Fri, Jan 11, 2019 at 12:16:33AM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > I think we need to add the built-in chains when listing if we want to > > > emulate the iptables-legacy behaviour. Listing via -L implies table > > > autoload, ie. > > > > > > # iptables-legacy -L -t raw > > > > > > pulls in the raw table and its chains. > > > > Yes, but I think thats a bug :-) > > OK, but that buggy behaviour has been there since the beginning IIRC :-) Hmm. Can you imagine a use-case where this would matter? I mean, behaviour would only divert as long as the user didn't add a rule yet. Is there a (user-relevant) functional difference between no table/chains and table with empty chains? The only use-case I could imagine is to check for given table type support by checking the list command's return code. > > I would prefer if iptables-nft would NOT do that, and instead > > list nothing, with exit value of 0. > > People should be using iptables-save instead for listing anyway, so I > don't mind if this is changed. We explicitly fixed for no output in list command on empty ruleset, so that is worth keeping IMO. Regarding the abort or avoid commit change, I don't have any good reasons for pushing it other than that it's not needed. So if you don't think it's a good idea or not worth the risk, no big deal for me. Cheers, Phil
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c index 2f369d9aadb01..10cc4c9fbc875 100644 --- a/iptables/xtables-arp.c +++ b/iptables/xtables-arp.c @@ -1366,6 +1366,7 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table, options&OPT_NUMERIC, /*options&OPT_EXPANDED*/0, options&OPT_LINENUMBERS); + nft_abort(h); break; case CMD_FLUSH: ret = nft_rule_flush(h, chain, *table, options & OPT_VERBOSE); diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c index 16d874120c0bb..a9a6fccb53c6a 100644 --- a/iptables/xtables-eb.c +++ b/iptables/xtables-eb.c @@ -1288,6 +1288,7 @@ print_zero: /*flags&OPT_EXPANDED*/0, flags&LIST_N, flags&LIST_C); + nft_abort(h); } if (flags & OPT_ZERO) { ret = nft_chain_zero_counters(h, chain, *table, 0); diff --git a/iptables/xtables.c b/iptables/xtables.c index da11e8cc159a0..28223e8edc799 100644 --- a/iptables/xtables.c +++ b/iptables/xtables.c @@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table, cs.options & OPT_NUMERIC, cs.options & OPT_EXPANDED, cs.options & OPT_LINENUMBERS); + if (p.command == CMD_LIST) + nft_abort(h); if (ret && (p.command & CMD_ZERO)) { ret = nft_chain_zero_counters(h, p.chain, p.table, cs.options & OPT_VERBOSE); @@ -1154,6 +1156,8 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table, case CMD_LIST_RULES|CMD_ZERO_NUM: ret = list_rules(h, p.chain, p.table, p.rulenum, cs.options & OPT_VERBOSE); + if (p.command == CMD_LIST_RULES) + nft_abort(h); if (ret && (p.command & CMD_ZERO)) { ret = nft_chain_zero_counters(h, p.chain, p.table, cs.options & OPT_VERBOSE);
When only listing rules, avoid to create the basic ruleset. Initializing the latter is still needed so that a completely empty ruleset does not lead to no output. But with builtin chains being added to cache immediately, there is no need to push the changes to the kernel anymore. Avoid this by calling nft_abort() in the right spots. Signed-off-by: Phil Sutter <phil@nwl.cc> --- iptables/xtables-arp.c | 1 + iptables/xtables-eb.c | 1 + iptables/xtables.c | 4 ++++ 3 files changed, 6 insertions(+)