Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets
@ 2019-09-25 21:25 Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 01/24] xtables_error() does not return Phil Sutter
                   ` (23 more replies)
  0 siblings, 24 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

This is a second approach at improving performance by reduced caching.
In comparison to v1, it is much less complex, at least when having the
general concept in mind:

There are three caching strategies:

1) No cache
2) Minimal cache
3) Full cache

(1) and (3) are relevant only for xtables-restore: Either we flush and
therefore don't need a cache (apart from the list of tables due to the
conditional table deletion logic) or we don't flush and can't know how
much caching is needed in input - the only feasible solution is to just
give in and fetch the full ruleset from kernel.

Assuming xtables-restore input is known, it can be checked for
problematic commands, namely those requiring a rule cache. This is
because otherwise, (2) applies, so cache only what's needed to do the
job. A simple '-A' merely requires knowledge whether table and chain
exist. Other rules in that chain are not interesting. Yet if followed by
e.g. '-L', rule cache needs to have been present before, otherwise the
appended rule might end in the wrong place in cache.

If xtables-restore is called with --noflush and all input is known and
unproblematic, it is treated like repeated calls to xtables, where (2)
applies. Minimal caching means to fetch from kernel only what is needed
in the given situation:

- nftnl_table_list_get() needs just the list of tables,
- nftnl_chain_list_get() needs just the list of chains in given table.

In most cases, nftnl_chain_list_get() is called just to find a specific
chain. By allowing to pass this chain name to the function, code can be
further optimized to just fetch that specific chain from kernel, place
it into the table's chain list and return the list. Calling code needs
no further adjustment, it will just operate on the returned chain list.

In fact, the above is beneficial for iptables: Many commands support
operation on all chains ('-F') or on a specific one ('-F INPUT'). The
corresponding function nft_rule_flush() is called with either a chain
name or NULL as parameter. Passed on to nftnl_chain_list_get() makes it
transparently return just what is needed.

The only thing to be cautious about with these partial cache situations
is to avoid duplicate cache entries:
- fetch_table_cache() is called only if h->cache->tables is not set,
  i.e. no table list exists yet (unless the call comes from a function
  which is called just once anyway).
- In nftnl_chain_list_cb() the retrieved chain from kernel is added to
  cache only if it doesn't exist there yet.
- nft_rule_list_update() does nothing if given chain's rule list is not
  empty. If it is, fetching won't cause duplicates.

Patches 1-4 are more or less fallout and unrelated to caching rework.

Patches 5-15 implement the requirements to minimize caches and change
users accordingly.

Patches 16-23 prepare xtables-restore for input buffering.

Patch 24 implements buffering input in xtables-restore when called with
--noflush for cache requirements prediction. The checker is a bit
sloppy, but it covers a typical use-case of quickly appending/prepending
a bunch of rules to an existing ruleset.

Phil Sutter (24):
  xtables_error() does not return
  tests/shell: Speed up ipt-restore/0004-restore-race_0
  tests: shell: Support running for legacy/nft only
  nft: Fix for add and delete of same rule in single batch
  nft: Make nftnl_table_list_get() fetch only tables
  xtables-restore: Minimize caching when flushing
  nft: Support fetch_rule_cache() per chain
  nft: Fetch only chains in nft_chain_list_get()
  nft: Support fetch_chain_cache() per table
  nft: Support fetch_chain_cache() per chain
  nft: Support nft_chain_list_get() per chain
  nft: Reduce cache overhead of adding a custom chain
  nft: Reduce cache overhead of nft_chain_builtin_init()
  nft: Support nft_is_table_compatible() per chain
  nft: Optimize flushing all chains of a table
  xtables-restore: Introduce rule counter tokenizer function
  xtables-restore: Carry in_table in struct nft_xt_restore_parse
  xtables-restore: Use xt_params->program_name
  xtables-restore: Carry curtable in struct nft_xt_restore_parse
  xtables-restore: Introduce line parsing function
  tests: shell: Add ipt-restore/0007-flush-noflush_0
  xtables-restore: Remove some pointless linebreaks
  xtables-restore: Allow lines without trailing newline character
  xtables-restore: Improve performance of --noflush operation

 iptables/iptables-restore.c                   |  53 +-
 iptables/iptables-xml.c                       |  53 +-
 iptables/nft-shared.h                         |   5 +-
 iptables/nft.c                                | 255 +++++++---
 iptables/nft.h                                |   9 +-
 iptables/tests/shell/run-tests.sh             |  28 +-
 .../ipt-restore/0003-restore-ordering_0       |  18 +-
 .../testcases/ipt-restore/0004-restore-race_0 |   4 +-
 .../ipt-restore/0007-flush-noflush_0          |  42 ++
 .../ipt-restore/0008-restore-counters_0       |  22 +
 iptables/xshared.c                            |  43 +-
 iptables/xshared.h                            |   1 +
 iptables/xtables-restore.c                    | 451 ++++++++++--------
 iptables/xtables-save.c                       |   4 +-
 iptables/xtables-translate.c                  |   2 +-
 15 files changed, 611 insertions(+), 379 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0

-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 01/24] xtables_error() does not return
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:31   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0 Phil Sutter
                   ` (22 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

It's a define which resolves into a callback which in turn is declared
with noreturn attribute. It will never return, therefore drop all
explicit exit() calls or other dead code immediately following it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/iptables-restore.c | 18 ++++++------------
 iptables/iptables-xml.c     | 22 +++++++---------------
 iptables/nft.c              |  8 ++------
 iptables/xshared.c          |  1 -
 iptables/xtables-restore.c  | 13 ++++---------
 5 files changed, 19 insertions(+), 43 deletions(-)

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 430be18b3c300..6bc182bfae4a2 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -82,11 +82,10 @@ create_handle(struct iptables_restore_cb *cb, const char *tablename)
 		handle = cb->ops->init(tablename);
 	}
 
-	if (!handle) {
+	if (!handle)
 		xtables_error(PARAMETER_PROBLEM, "%s: unable to initialize "
 			"table '%s'\n", xt_params->program_name, tablename);
-		exit(1);
-	}
+
 	return handle;
 }
 
@@ -207,12 +206,11 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 
 			table = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table) {
+			if (!table)
 				xtables_error(PARAMETER_PROBLEM,
 					"%s: line %u table name invalid\n",
 					xt_params->program_name, line);
-				exit(1);
-			}
+
 			strncpy(curtable, table, XT_TABLE_MAXNAMELEN);
 			curtable[XT_TABLE_MAXNAMELEN] = '\0';
 
@@ -248,12 +246,10 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 
 			chain = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain) {
+			if (!chain)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u chain name invalid\n",
 					   xt_params->program_name, line);
-				exit(1);
-			}
 
 			if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 				xtables_error(PARAMETER_PROBLEM,
@@ -281,12 +277,10 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 
 			policy = strtok(NULL, " \t\n");
 			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy) {
+			if (!policy)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u policy invalid\n",
 					   xt_params->program_name, line);
-				exit(1);
-			}
 
 			if (strcmp(policy, "-") != 0) {
 				struct xt_counters count = {};
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 9d9ce6d4a13ee..36ad78450b1ef 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -208,12 +208,11 @@ needChain(char *chain)
 static void
 saveChain(char *chain, char *policy, struct xt_counters *ctr)
 {
-	if (nextChain >= maxChains) {
+	if (nextChain >= maxChains)
 		xtables_error(PARAMETER_PROBLEM,
 			   "%s: line %u chain name invalid\n",
 			   prog_name, line);
-		exit(1);
-	};
+
 	chains[nextChain].chain = strdup(chain);
 	chains[nextChain].policy = strdup(policy);
 	chains[nextChain].count = *ctr;
@@ -606,12 +605,11 @@ iptables_xml_main(int argc, char *argv[])
 
 			table = strtok(buffer + 1, " \t\n");
 			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table) {
+			if (!table)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u table name invalid\n",
 					   prog_name, line);
-				exit(1);
-			}
+
 			openTable(table);
 
 			ret = 1;
@@ -623,23 +621,19 @@ iptables_xml_main(int argc, char *argv[])
 
 			chain = strtok(buffer + 1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain) {
+			if (!chain)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u chain name invalid\n",
 					   prog_name, line);
-				exit(1);
-			}
 
 			DEBUGP("Creating new chain '%s'\n", chain);
 
 			policy = strtok(NULL, " \t\n");
 			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy) {
+			if (!policy)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u policy invalid\n",
 					   prog_name, line);
-				exit(1);
-			}
 
 			ctrs = strtok(NULL, " \t\n");
 			parse_counters(ctrs, &count);
@@ -735,13 +729,11 @@ iptables_xml_main(int argc, char *argv[])
 					     param_buffer[1] != '-' &&
 					     strchr(param_buffer, 't')) ||
 					    (!strncmp(param_buffer, "--t", 3) &&
-					     !strncmp(param_buffer, "--table", strlen(param_buffer)))) {
+					     !strncmp(param_buffer, "--table", strlen(param_buffer))))
 						xtables_error(PARAMETER_PROBLEM,
 							   "Line %u seems to have a "
 							   "-t table option.\n",
 							   line);
-						exit(1);
-					}
 
 					add_argv(param_buffer, quoted);
 					if (newargc >= 2
diff --git a/iptables/nft.c b/iptables/nft.c
index 8047a51f00493..90bb0c63c025a 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2517,10 +2517,8 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 
 	ops = nft_family_ops_lookup(h->family);
 
-	if (!nft_is_table_compatible(h, table)) {
+	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
-		return 0;
-	}
 
 	list = nft_chain_list_get(h, table);
 	if (!list)
@@ -2620,10 +2618,8 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 
 	nft_xt_builtin_init(h, table);
 
-	if (!nft_is_table_compatible(h, table)) {
+	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
-		return 0;
-	}
 
 	list = nft_chain_list_get(h, table);
 	if (!list)
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 36a2ec5f193d3..5e6cd4ae7c908 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -181,7 +181,6 @@ int command_default(struct iptables_command_state *cs,
 		xtables_error(PARAMETER_PROBLEM, "unknown option "
 			      "\"%s\"", cs->argv[optind-1]);
 	xtables_error(PARAMETER_PROBLEM, "Unknown arg \"%s\"", optarg);
-	return 0;
 }
 
 static mainfunc_t subcmd_get(const char *cmd, const struct subcommand *cb)
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index f930f5ba2d167..27e65b971727e 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -131,12 +131,11 @@ void xtables_restore_parse(struct nft_handle *h,
 
 			table = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table) {
+			if (!table)
 				xtables_error(PARAMETER_PROBLEM,
 					"%s: line %u table name invalid\n",
 					xt_params->program_name, line);
-				exit(1);
-			}
+
 			curtable = nft_table_builtin_find(h, table);
 			if (!curtable)
 				xtables_error(PARAMETER_PROBLEM,
@@ -168,12 +167,10 @@ void xtables_restore_parse(struct nft_handle *h,
 
 			chain = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain) {
+			if (!chain)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u chain name invalid\n",
 					   xt_params->program_name, line);
-				exit(1);
-			}
 
 			if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 				xtables_error(PARAMETER_PROBLEM,
@@ -183,12 +180,10 @@ void xtables_restore_parse(struct nft_handle *h,
 
 			policy = strtok(NULL, " \t\n");
 			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy) {
+			if (!policy)
 				xtables_error(PARAMETER_PROBLEM,
 					   "%s: line %u policy invalid\n",
 					   xt_params->program_name, line);
-				exit(1);
-			}
 
 			if (nft_chain_builtin_find(curtable, chain)) {
 				if (counters) {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 01/24] xtables_error() does not return Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-26  9:07   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only Phil Sutter
                   ` (21 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

This test tended to cause quite excessive load on my system, sometimes
taking longer than all other tests combined. Even with the reduced
numbers, it still fails reliably after reverting commit 58d7de0181f61
("xtables: handle concurrent ruleset modifications").

Fixes: 4000b4cf2ea38 ("tests: add test script for race-free restore")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../tests/shell/testcases/ipt-restore/0004-restore-race_0     | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0 b/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
index a92d18dcee058..96a5e66d0ab81 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
@@ -24,7 +24,7 @@ clean_tempfile()
 
 trap clean_tempfile EXIT
 
-ENTRY_NUM=$((RANDOM%100))
+ENTRY_NUM=$((RANDOM%10))
 UCHAIN_NUM=$((RANDOM%10))
 
 get_target()
@@ -87,7 +87,7 @@ fi
 
 case "$XT_MULTI" in
 */xtables-nft-multi)
-	attempts=$((RANDOM%200))
+	attempts=$((RANDOM%10))
 	attempts=$((attempts+1))
 	;;
 *)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 01/24] xtables_error() does not return Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0 Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-27 14:19   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch Phil Sutter
                   ` (20 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

After some changes, one might want to test a single variant only. Allow
this by supporting -n/--nft and -l/--legacy parameters, each disabling
the other variant.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/tests/shell/run-tests.sh | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/iptables/tests/shell/run-tests.sh b/iptables/tests/shell/run-tests.sh
index 7bef09f74643d..d71c13729b3ee 100755
--- a/iptables/tests/shell/run-tests.sh
+++ b/iptables/tests/shell/run-tests.sh
@@ -38,6 +38,14 @@ while [ -n "$1" ]; do
 		HOST=y
 		shift
 		;;
+	-l|--legacy)
+		LEGACY_ONLY=y
+		shift
+		;;
+	-n|--nft)
+		NFT_ONLY=y
+		shift
+		;;
 	*${RETURNCODE_SEPARATOR}+([0-9]))
 		SINGLE+=" $1"
 		VERBOSE=y
@@ -98,19 +106,23 @@ do_test() {
 }
 
 echo ""
-for testfile in $(find_tests);do
-	do_test "$testfile" "$XTABLES_LEGACY_MULTI"
-done
-msg_info "legacy results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
+if [ "$NFT_ONLY" != "y" ]; then
+	for testfile in $(find_tests);do
+		do_test "$testfile" "$XTABLES_LEGACY_MULTI"
+	done
+	msg_info "legacy results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
 
+fi
 legacy_ok=$ok
 legacy_fail=$failed
 ok=0
 failed=0
-for testfile in $(find_tests);do
-	do_test "$testfile" "$XTABLES_NFT_MULTI"
-done
-msg_info "nft results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
+if [ "$LEGACY_ONLY" != "y" ]; then
+	for testfile in $(find_tests);do
+		do_test "$testfile" "$XTABLES_NFT_MULTI"
+	done
+	msg_info "nft results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
+fi
 
 ok=$((legacy_ok+ok))
 failed=$((legacy_fail+failed))
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (2 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-27 14:20   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables Phil Sutter
                   ` (19 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Another corner-case found when extending restore ordering test: If a
delete command in a dump referenced a rule added earlier within the same
dump, kernel would reject the resulting NFT_MSG_DELRULE command.

Catch this by assigning the rule to delete a RULE_ID value if it doesn't
have a handle yet. Since __nft_rule_del() does not duplicate the
nftnl_rule object when creating the NFT_COMPAT_RULE_DELETE command, this
RULE_ID value is added to both NEWRULE and DELRULE commands - exactly
what is needed to establish the reference.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c                                 |  3 +++
 .../ipt-restore/0003-restore-ordering_0        | 18 +++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 90bb0c63c025a..255e51b4d2275 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2199,6 +2199,9 @@ static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule *r)
 
 	nftnl_rule_list_del(r);
 
+	if (!nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE))
+		nftnl_rule_set_u32(r, NFTNL_RULE_ID, ++h->rule_id);
+
 	obj = batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r);
 	if (!obj) {
 		nftnl_rule_free(r);
diff --git a/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0 b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
index 51f2422e15259..3f1d229e915ff 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
@@ -14,7 +14,7 @@ ipt_show() {
 
 $XT_MULTI iptables-restore <<EOF
 *filter
--A FORWARD -m comment --comment "appended rule" -j ACCEPT
+-A FORWARD -m comment --comment "rule 4" -j ACCEPT
 -I FORWARD 1 -m comment --comment "rule 1" -j ACCEPT
 -I FORWARD 2 -m comment --comment "rule 2" -j ACCEPT
 -I FORWARD 3 -m comment --comment "rule 3" -j ACCEPT
@@ -24,7 +24,7 @@ EOF
 EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
 -A FORWARD -m comment --comment "rule 2" -j ACCEPT
 -A FORWARD -m comment --comment "rule 3" -j ACCEPT
--A FORWARD -m comment --comment "appended rule" -j ACCEPT'
+-A FORWARD -m comment --comment "rule 4" -j ACCEPT'
 
 diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
 
@@ -32,11 +32,14 @@ diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
 
 $XT_MULTI iptables-restore --noflush <<EOF
 *filter
+-A FORWARD -m comment --comment "rule 5" -j ACCEPT
 -I FORWARD 1 -m comment --comment "rule 0.5" -j ACCEPT
 -I FORWARD 3 -m comment --comment "rule 1.5" -j ACCEPT
 -I FORWARD 5 -m comment --comment "rule 2.5" -j ACCEPT
 -I FORWARD 7 -m comment --comment "rule 3.5" -j ACCEPT
--I FORWARD 9 -m comment --comment "appended rule 2" -j ACCEPT
+-I FORWARD 9 -m comment --comment "rule 4.5" -j ACCEPT
+-I FORWARD 11 -m comment --comment "rule 5.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 6" -j ACCEPT
 COMMIT
 EOF
 
@@ -47,8 +50,11 @@ EXPECT='-A FORWARD -m comment --comment "rule 0.5" -j ACCEPT
 -A FORWARD -m comment --comment "rule 2.5" -j ACCEPT
 -A FORWARD -m comment --comment "rule 3" -j ACCEPT
 -A FORWARD -m comment --comment "rule 3.5" -j ACCEPT
--A FORWARD -m comment --comment "appended rule" -j ACCEPT
--A FORWARD -m comment --comment "appended rule 2" -j ACCEPT'
+-A FORWARD -m comment --comment "rule 4" -j ACCEPT
+-A FORWARD -m comment --comment "rule 4.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 5.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 6" -j ACCEPT'
 
 diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
 
@@ -78,6 +84,8 @@ diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
 
 $XT_MULTI iptables-restore --noflush <<EOF
 *filter
+-A FORWARD -m comment --comment "appended rule 4" -j ACCEPT
+-D FORWARD 7
 -D FORWARD -m comment --comment "appended rule 1" -j ACCEPT
 -D FORWARD 3
 -I FORWARD 3 -m comment --comment "manually replaced rule 2" -j ACCEPT
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (3 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-27 14:25   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing Phil Sutter
                   ` (18 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

No need for a full cache to serve the list of tables.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 255e51b4d2275..695758831047a 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2046,7 +2046,8 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 
 static struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h)
 {
-	nft_build_cache(h);
+	if (!h->cache->tables)
+		fetch_table_cache(h);
 
 	return h->cache->tables;
 }
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (4 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-27 14:27   ` Florian Westphal
  2019-09-25 21:25 ` [iptables PATCH v2 07/24] nft: Support fetch_rule_cache() per chain Phil Sutter
                   ` (17 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Unless --noflush was given, xtables-restore merely needs the list of
tables to decide whether to delete it or not. Introduce nft_fake_cache()
function which populates table list, initializes chain lists (so
nft_chain_list_get() returns an empty list instead of NULL) and sets
'have_cache' to turn any later calls to nft_build_cache() into nops.

If --noflush was given, call nft_build_cache() just once instead of for
each table line in input.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 695758831047a..02da53e60bc83 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1611,6 +1611,23 @@ retry:
 	h->nft_genid = genid_start;
 }
 
+void nft_fake_cache(struct nft_handle *h)
+{
+	int i;
+
+	fetch_table_cache(h);
+	for (i = 0; i < NFT_TABLE_MAX; i++) {
+		enum nft_table_type type = h->tables[i].type;
+
+		if (!h->tables[i].name)
+			continue;
+
+		h->cache->table[type].chains = nftnl_chain_list_alloc();
+	}
+	h->have_cache = true;
+	mnl_genid_get(h, &h->nft_genid);
+}
+
 void nft_build_cache(struct nft_handle *h)
 {
 	if (!h->have_cache)
diff --git a/iptables/nft.h b/iptables/nft.h
index 43463d7f262e8..bcac8e228c787 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -73,6 +73,7 @@ int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
 	     void *data);
 int nft_init(struct nft_handle *h, const struct builtin_table *t);
 void nft_fini(struct nft_handle *h);
+void nft_fake_cache(struct nft_handle *h);
 void nft_build_cache(struct nft_handle *h);
 
 /*
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 27e65b971727e..7f28a05c68619 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -96,6 +96,11 @@ void xtables_restore_parse(struct nft_handle *h,
 
 	line = 0;
 
+	if (!h->noflush)
+		nft_fake_cache(h);
+	else
+		nft_build_cache(h);
+
 	/* Grab standard input. */
 	while (fgets(buffer, sizeof(buffer), p->in)) {
 		int ret = 0;
@@ -145,8 +150,6 @@ void xtables_restore_parse(struct nft_handle *h,
 			if (p->tablename && (strcmp(p->tablename, table) != 0))
 				continue;
 
-			nft_build_cache(h);
-
 			if (h->noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
 					table);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 07/24] nft: Support fetch_rule_cache() per chain
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (5 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get() Phil Sutter
                   ` (16 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Support passing an nftnl_chain to fetch its rules from kernel.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 02da53e60bc83..7c974af8b4141 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1575,10 +1575,13 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static int fetch_rule_cache(struct nft_handle *h)
+static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c)
 {
 	int i;
 
+	if (c)
+		return nft_rule_list_update(c, h);
+
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
 		enum nft_table_type type = h->tables[i].type;
 
@@ -1599,7 +1602,7 @@ static void __nft_build_cache(struct nft_handle *h)
 retry:
 	mnl_genid_get(h, &genid_start);
 	fetch_chain_cache(h);
-	fetch_rule_cache(h);
+	fetch_rule_cache(h, NULL);
 	h->have_cache = true;
 	mnl_genid_get(h, &genid_stop);
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get()
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (6 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 07/24] nft: Support fetch_rule_cache() per chain Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-30 16:57   ` Pablo Neira Ayuso
  2019-09-30 17:12   ` Pablo Neira Ayuso
  2019-09-25 21:25 ` [iptables PATCH v2 09/24] nft: Support fetch_chain_cache() per table Phil Sutter
                   ` (15 subsequent siblings)
  23 siblings, 2 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

The function is used to return the given table's chains, so fetching
chain cache is enough.

This requires a bunch of manual rule cache updates in different places.
To still support the fake cache logic from xtables-restore, make
fetch_rule_cache() do nothing in case have_cache is set.

Accidental double rule cache updates for the same chain need to be
prevented. This is complicated by the fact that chain's rule list is
managed by libnftnl. Hence the same logic as used for table list, namely
checking list pointer value, can't be used. Instead, simply fetch rules
only if the given chain's rule list is empty. If it isn't, rules have
been fetched before; if it is, a second rule fetch won't hurt.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 7c974af8b4141..729b88d990f9f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1264,6 +1264,7 @@ err:
 
 static struct nftnl_chain *
 nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
+static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c);
 
 int
 nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
@@ -1275,6 +1276,14 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 
 	nft_xt_builtin_init(h, table);
 
+	/* Since ebtables user-defined chain policies are implemented as last
+	 * rule in nftables, rule cache is required here to treat them right. */
+	if (h->family == NFPROTO_BRIDGE) {
+		c = nft_chain_find(h, table, chain);
+		if (c && !nft_chain_builtin(c))
+			fetch_rule_cache(h, c);
+	}
+
 	nft_fn = nft_rule_append;
 
 	r = nft_rule_new(h, chain, table, data);
@@ -1550,6 +1559,9 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	struct nftnl_rule *rule;
 	int ret;
 
+	if (nftnl_rule_lookup_byindex(c, 0))
+		return 0;
+
 	rule = nftnl_rule_alloc();
 	if (!rule)
 		return -1;
@@ -1579,6 +1591,9 @@ static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c)
 {
 	int i;
 
+	if (h->have_cache)
+		return 0;
+
 	if (c)
 		return nft_rule_list_update(c, h);
 
@@ -1670,7 +1685,8 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	nft_build_cache(h);
+	if (!h->have_cache && !h->cache->table[t->type].chains)
+		fetch_chain_cache(h);
 
 	return h->cache->table[t->type].chains;
 }
@@ -1761,6 +1777,7 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 
 	c = nftnl_chain_list_iter_next(iter);
 	while (c) {
+		fetch_rule_cache(h, c);
 		ret = nft_chain_save_rules(h, c, format);
 		if (ret != 0)
 			break;
@@ -1949,6 +1966,10 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 		fprintf(stdout, "Deleting chain `%s'\n",
 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
 
+	/* This triggers required policy rule deletion. */
+	if (h->family == NFPROTO_BRIDGE)
+		fetch_rule_cache(h, c);
+
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c);
@@ -2238,6 +2259,8 @@ nft_rule_find(struct nft_handle *h, struct nftnl_chain *c, void *data, int rulen
 	struct nftnl_rule_iter *iter;
 	bool found = false;
 
+	fetch_rule_cache(h, c);
+
 	if (rulenum >= 0)
 		/* Delete by rule number case */
 		return nftnl_rule_lookup_byindex(c, rulenum);
@@ -3063,6 +3086,8 @@ int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 	else
 		return 0;
 
+	fetch_rule_cache(h, c);
+
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, pval);
 	return 1;
 }
@@ -3402,6 +3427,8 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 	enum nf_inet_hooks hook;
 	int prio;
 
+	fetch_rule_cache(h, c);
+
 	if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
 		return -1;
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 09/24] nft: Support fetch_chain_cache() per table
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (7 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get() Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 10/24] nft: Support fetch_chain_cache() per chain Phil Sutter
                   ` (14 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Allow to pass a builtin_table for which to fetch chains. If given,
initialize only that table's chain list in cache and pass the table
along to nftnl_chain_list_cb() which will filter received chains by
table name.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 729b88d990f9f..6ed05e77008c4 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1352,11 +1352,18 @@ nft_rule_print_save(const struct nftnl_rule *r, enum nft_rule_print type,
 		ops->clear_cs(&cs);
 }
 
+struct nftnl_chain_list_cb_data {
+	struct nft_handle *h;
+	const struct builtin_table *t;
+};
+
 static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
-	struct nft_handle *h = data;
-	const struct builtin_table *t;
+	struct nftnl_chain_list_cb_data *d = data;
+	const struct builtin_table *t = d->t;
+	struct nft_handle *h = d->h;
 	struct nftnl_chain *c;
+	const char *tname;
 
 	c = nftnl_chain_alloc();
 	if (c == NULL)
@@ -1365,8 +1372,13 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	if (nftnl_chain_nlmsg_parse(nlh, c) < 0)
 		goto out;
 
-	t = nft_table_builtin_find(h,
-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
+	tname = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
+
+	if (!t)
+		t = nft_table_builtin_find(h, tname);
+	else if (strcmp(t->name, tname))
+		goto out;
+
 	if (!t)
 		goto out;
 
@@ -1423,29 +1435,40 @@ static int fetch_table_cache(struct nft_handle *h)
 	return 1;
 }
 
-static int fetch_chain_cache(struct nft_handle *h)
+static int fetch_chain_cache(struct nft_handle *h,
+			     const struct builtin_table *t)
 {
+	struct nftnl_chain_list_cb_data d = {
+		.h = h,
+		.t = t,
+	};
 	char buf[16536];
 	struct nlmsghdr *nlh;
 	int i, ret;
 
-	fetch_table_cache(h);
+	if (!t) {
+		fetch_table_cache(h);
 
-	for (i = 0; i < NFT_TABLE_MAX; i++) {
-		enum nft_table_type type = h->tables[i].type;
+		for (i = 0; i < NFT_TABLE_MAX; i++) {
+			enum nft_table_type type = h->tables[i].type;
 
-		if (!h->tables[i].name)
-			continue;
+			if (!h->tables[i].name)
+				continue;
 
-		h->cache->table[type].chains = nftnl_chain_list_alloc();
-		if (!h->cache->table[type].chains)
-			return -1;
+			h->cache->table[type].chains = nftnl_chain_list_alloc();
+			if (!h->cache->table[type].chains)
+				return -1;
+		}
+	} else if (!h->cache->table[t->type].chains) {
+		h->cache->table[t->type].chains = nftnl_chain_list_alloc();
+	} else {
+		return 0;
 	}
 
 	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
 					NLM_F_DUMP, h->seq);
 
-	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, h);
+	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, &d);
 	if (ret < 0 && errno == EINTR)
 		assert(nft_restart(h) >= 0);
 
@@ -1616,7 +1639,7 @@ static void __nft_build_cache(struct nft_handle *h)
 
 retry:
 	mnl_genid_get(h, &genid_start);
-	fetch_chain_cache(h);
+	fetch_chain_cache(h, NULL);
 	fetch_rule_cache(h, NULL);
 	h->have_cache = true;
 	mnl_genid_get(h, &genid_stop);
@@ -1686,7 +1709,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 		return NULL;
 
 	if (!h->have_cache && !h->cache->table[t->type].chains)
-		fetch_chain_cache(h);
+		fetch_chain_cache(h, t);
 
 	return h->cache->table[t->type].chains;
 }
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 10/24] nft: Support fetch_chain_cache() per chain
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (8 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 09/24] nft: Support fetch_chain_cache() per table Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 11/24] nft: Support nft_chain_list_get() " Phil Sutter
                   ` (13 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Accept a specific chain's name to fetch from kernel. If given, prepare a
non-dump request with chain's table and name in payload. Kernel will
return only that chain (if it exists).

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 6ed05e77008c4..f116b940b41fd 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1436,7 +1436,7 @@ static int fetch_table_cache(struct nft_handle *h)
 }
 
 static int fetch_chain_cache(struct nft_handle *h,
-			     const struct builtin_table *t)
+			     const struct builtin_table *t, const char *chain)
 {
 	struct nftnl_chain_list_cb_data d = {
 		.h = h,
@@ -1461,12 +1461,24 @@ static int fetch_chain_cache(struct nft_handle *h,
 		}
 	} else if (!h->cache->table[t->type].chains) {
 		h->cache->table[t->type].chains = nftnl_chain_list_alloc();
-	} else {
-		return 0;
 	}
 
-	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
-					NLM_F_DUMP, h->seq);
+	if (t && chain) {
+		struct nftnl_chain *c = nftnl_chain_alloc();
+
+		if (!c)
+			return -1;
+
+		nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
+						  NLM_F_ACK, h->seq);
+		nftnl_chain_set_str(c, NFTNL_CHAIN_TABLE, t->name);
+		nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain);
+		nftnl_chain_nlmsg_build_payload(nlh, c);
+		nftnl_chain_free(c);
+	} else {
+		nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
+						NLM_F_DUMP, h->seq);
+	}
 
 	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, &d);
 	if (ret < 0 && errno == EINTR)
@@ -1639,7 +1651,7 @@ static void __nft_build_cache(struct nft_handle *h)
 
 retry:
 	mnl_genid_get(h, &genid_start);
-	fetch_chain_cache(h, NULL);
+	fetch_chain_cache(h, NULL, NULL);
 	fetch_rule_cache(h, NULL);
 	h->have_cache = true;
 	mnl_genid_get(h, &genid_stop);
@@ -1709,7 +1721,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 		return NULL;
 
 	if (!h->have_cache && !h->cache->table[t->type].chains)
-		fetch_chain_cache(h, t);
+		fetch_chain_cache(h, t, NULL);
 
 	return h->cache->table[t->type].chains;
 }
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 11/24] nft: Support nft_chain_list_get() per chain
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (9 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 10/24] nft: Support fetch_chain_cache() per chain Phil Sutter
@ 2019-09-25 21:25 ` " Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 12/24] nft: Reduce cache overhead of adding a custom chain Phil Sutter
                   ` (12 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Accept an optional chain name to specifically search for. In most cases,
the full list is not interesting, so make use of kernel's ability to
return only a specific chain.

To avoid duplicate chains in tables' chain lists, add the chain to cache
only if it doesn't exist already in the list. Despite the overhead this
causes, it is still better this way than introducing a routine which
bypasses the cache entirely: Many iptables commands support operating on
either a single or on all chains of a table. This code layout reflects
that by accepting a possibly NULL chain name pointer.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c             | 40 +++++++++++++++++++++++---------------
 iptables/nft.h             |  3 ++-
 iptables/xtables-restore.c |  2 +-
 iptables/xtables-save.c    |  2 +-
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index f116b940b41fd..904068a6404a6 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -750,7 +750,7 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
-	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);
+	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name, NULL);
 	struct nftnl_chain *c;
 	int i;
 
@@ -1361,9 +1361,10 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nftnl_chain_list_cb_data *d = data;
 	const struct builtin_table *t = d->t;
+	struct nftnl_chain_list *list;
 	struct nft_handle *h = d->h;
+	const char *tname, *cname;
 	struct nftnl_chain *c;
-	const char *tname;
 
 	c = nftnl_chain_alloc();
 	if (c == NULL)
@@ -1382,7 +1383,13 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	if (!t)
 		goto out;
 
-	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
+	list = h->cache->table[t->type].chains;
+	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+
+	if (nftnl_chain_list_lookup_byname(list, cname))
+		goto out;
+
+	nftnl_chain_list_add_tail(c, list);
 
 	return MNL_CB_OK;
 out:
@@ -1712,7 +1719,8 @@ static void nft_release_cache(struct nft_handle *h)
 }
 
 struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
-					    const char *table)
+					    const char *table,
+					    const char *chain)
 {
 	const struct builtin_table *t;
 
@@ -1720,8 +1728,8 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	if (!h->have_cache && !h->cache->table[t->type].chains)
-		fetch_chain_cache(h, t, NULL);
+	if (!h->have_cache)
+		fetch_chain_cache(h, t, chain);
 
 	return h->cache->table[t->type].chains;
 }
@@ -1802,7 +1810,7 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, NULL);
 	if (!list)
 		return 0;
 
@@ -1864,7 +1872,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 	nft_fn = nft_rule_flush;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL) {
 		ret = 1;
 		goto err;
@@ -1929,7 +1937,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, NULL);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
@@ -1969,7 +1977,7 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, NULL);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
@@ -2028,7 +2036,7 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
 
 	nft_fn = nft_chain_user_del;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		return 0;
 
@@ -2056,7 +2064,7 @@ nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 {
 	struct nftnl_chain_list *list;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		return NULL;
 
@@ -2602,7 +2610,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
 
@@ -2703,7 +2711,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
 
@@ -3387,7 +3395,7 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		goto err;
 
@@ -3495,7 +3503,7 @@ bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
 {
 	struct nftnl_chain_list *clist;
 
-	clist = nft_chain_list_get(h, tablename);
+	clist = nft_chain_list_get(h, tablename, NULL);
 	if (clist == NULL)
 		return false;
 
diff --git a/iptables/nft.h b/iptables/nft.h
index bcac8e228c787..ae9b1a88ae175 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -96,7 +96,8 @@ struct nftnl_chain;
 
 int nft_chain_set(struct nft_handle *h, const char *table, const char *chain, const char *policy, const struct xt_counters *counters);
 struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
-					    const char *table);
+					    const char *table,
+					    const char *chain);
 int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list);
 int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table);
 int nft_chain_user_del(struct nft_handle *h, const char *chain, const char *table, bool verbose);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 7f28a05c68619..c04d7292e0045 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -62,7 +62,7 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
 {
 	struct nftnl_chain_list *chain_list;
 
-	chain_list = nft_chain_list_get(h, table);
+	chain_list = nft_chain_list_get(h, table, NULL);
 	if (chain_list == NULL)
 		xtables_error(OTHER_PROBLEM, "cannot retrieve chain list\n");
 
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 77b13f7ffbcdd..503ae401737c5 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -82,7 +82,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 		return 0;
 	}
 
-	chain_list = nft_chain_list_get(h, tablename);
+	chain_list = nft_chain_list_get(h, tablename, NULL);
 	if (!chain_list)
 		return 0;
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 12/24] nft: Reduce cache overhead of adding a custom chain
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (10 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 11/24] nft: Support nft_chain_list_get() " Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 13/24] nft: Reduce cache overhead of nft_chain_builtin_init() Phil Sutter
                   ` (11 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Pass the new chain name to nft_chain_list_get() although that doesn't
make sense (it is not supposed to be found). The reason is it avoids
full chain list retrieval from kernel if not present already.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 904068a6404a6..2c05643f7d691 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1937,7 +1937,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table, NULL);
+	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
@@ -1977,7 +1977,7 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table, NULL);
+	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 13/24] nft: Reduce cache overhead of nft_chain_builtin_init()
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (11 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 12/24] nft: Reduce cache overhead of adding a custom chain Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 14/24] nft: Support nft_is_table_compatible() per chain Phil Sutter
                   ` (10 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

There is no need for a full chain cache, fetch only the few builtin
chains that might need to be created.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 2c05643f7d691..6c025478a7a20 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -750,15 +750,16 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
-	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name, NULL);
+	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
 	int i;
 
-	if (!list)
-		return;
-
 	/* Initialize built-in chains if they don't exist yet */
 	for (i=0; i < NF_INET_NUMHOOKS && table->chains[i].name != NULL; i++) {
+		list = nft_chain_list_get(h, table->name,
+					  table->chains[i].name);
+		if (!list)
+			continue;
 
 		c = nftnl_chain_list_lookup_byname(list, table->chains[i].name);
 		if (c != NULL)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 14/24] nft: Support nft_is_table_compatible() per chain
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (12 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 13/24] nft: Reduce cache overhead of nft_chain_builtin_init() Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 15/24] nft: Optimize flushing all chains of a table Phil Sutter
                   ` (9 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

When operating on a single chain only, compatibility checking causes
unwanted overhead by checking all chains of the current table. Avoid
this by accepting the current chain name as parameter and pass it along
to nft_chain_list_get().

While being at it, introduce nft_assert_table_compatible() which
calls xtables_error() in case compatibility check fails. If a chain name
was given, include that in error message.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c          | 32 ++++++++++++++++++++++++--------
 iptables/nft.h          |  5 ++++-
 iptables/xtables-save.c |  2 +-
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 6c025478a7a20..b2b15f08c1637 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2605,12 +2605,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	bool found = false;
 
 	nft_xt_builtin_init(h, table);
+	nft_assert_table_compatible(h, table, chain);
 
 	ops = nft_family_ops_lookup(h->family);
 
-	if (!nft_is_table_compatible(h, table))
-		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
-
 	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
@@ -2708,9 +2706,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
-
-	if (!nft_is_table_compatible(h, table))
-		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
+	nft_assert_table_compatible(h, table, chain);
 
 	list = nft_chain_list_get(h, table, chain);
 	if (!list)
@@ -3500,11 +3496,12 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
+bool nft_is_table_compatible(struct nft_handle *h,
+			     const char *table, const char *chain)
 {
 	struct nftnl_chain_list *clist;
 
-	clist = nft_chain_list_get(h, tablename, NULL);
+	clist = nft_chain_list_get(h, table, chain);
 	if (clist == NULL)
 		return false;
 
@@ -3513,3 +3510,22 @@ bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
 
 	return true;
 }
+
+void nft_assert_table_compatible(struct nft_handle *h,
+				 const char *table, const char *chain)
+{
+	const char *pfx = "", *sfx = "";
+
+	if (nft_is_table_compatible(h, table, chain))
+		return;
+
+	if (chain) {
+		pfx = "chain `";
+		sfx = "' in ";
+	} else {
+		chain = "";
+	}
+	xtables_error(OTHER_PROBLEM,
+		      "%s%s%stable `%s' is incompatible, use 'nft' tool.\n",
+		      pfx, chain, sfx, table);
+}
diff --git a/iptables/nft.h b/iptables/nft.h
index ae9b1a88ae175..44377c0446942 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -200,7 +200,10 @@ int nft_arp_rule_insert(struct nft_handle *h, const char *chain,
 
 void nft_rule_to_arpt_entry(struct nftnl_rule *r, struct arpt_entry *fw);
 
-bool nft_is_table_compatible(struct nft_handle *h, const char *name);
+bool nft_is_table_compatible(struct nft_handle *h,
+			     const char *table, const char *chain);
+void nft_assert_table_compatible(struct nft_handle *h,
+				 const char *table, const char *chain);
 
 int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 			      const char *chain, const char *policy);
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 503ae401737c5..000104d3b51ae 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -76,7 +76,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	if (!nft_table_builtin_find(h, tablename))
 		return 0;
 
-	if (!nft_is_table_compatible(h, tablename)) {
+	if (!nft_is_table_compatible(h, tablename, NULL)) {
 		printf("# Table `%s' is incompatible, use 'nft' tool.\n",
 		       tablename);
 		return 0;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 15/24] nft: Optimize flushing all chains of a table
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (13 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 14/24] nft: Support nft_is_table_compatible() per chain Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 16/24] xtables-restore: Introduce rule counter tokenizer function Phil Sutter
                   ` (8 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Leverage nftables' support for flushing all chains of a table by
omitting NFTNL_RULE_CHAIN attribute in NFT_MSG_DELRULE payload.

The only caveat is with verbose output, as that still requires to have a
list of (existing) chains to iterate over. Apart from that, implementing
this shortcut is pretty straightforward: Don't retrieve a chain list and
just call __nft_rule_flush() directly which doesn't set above attribute
if chain name pointer is NULL.

A bigger deal is keeping rule cache consistent: Instead of just clearing
rule list for each flushed chain, flush_rule_cache() is updated to
iterate over all cached chains of the given table, clearing their rule
lists if not called for a specific chain.

While being at it, sort local variable declarations in nft_rule_flush()
from longest to shortest and drop the loop-local 'chain_name' variable
(but instead use 'chain' function parameter which is not used at that
point).

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

diff --git a/iptables/nft.c b/iptables/nft.c
index b2b15f08c1637..b50ec12ae9d42 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -837,7 +837,7 @@ int nft_init(struct nft_handle *h, const struct builtin_table *t)
 	return 0;
 }
 
-static int __flush_rule_cache(struct nftnl_rule *r, void *data)
+static int ____flush_rule_cache(struct nftnl_rule *r, void *data)
 {
 	nftnl_rule_list_del(r);
 	nftnl_rule_free(r);
@@ -845,9 +845,25 @@ static int __flush_rule_cache(struct nftnl_rule *r, void *data)
 	return 0;
 }
 
-static void flush_rule_cache(struct nftnl_chain *c)
+static int __flush_rule_cache(struct nftnl_chain *c, void *data)
 {
-	nftnl_rule_foreach(c, __flush_rule_cache, NULL);
+	return nftnl_rule_foreach(c, ____flush_rule_cache, NULL);
+}
+
+static int flush_rule_cache(struct nft_handle *h, const char *table,
+			    struct nftnl_chain *c)
+{
+	const struct builtin_table *t;
+
+	if (c)
+		return __flush_rule_cache(c, NULL);
+
+	t = nft_table_builtin_find(h, table);
+	if (!t|| !h->cache->table[t->type].chains)
+		return 0;
+
+	return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
+					__flush_rule_cache, NULL);
 }
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
@@ -1842,7 +1858,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 	struct obj_update *obj;
 	struct nftnl_rule *r;
 
-	if (verbose)
+	if (verbose && chain)
 		fprintf(stdout, "Flushing chain `%s'\n", chain);
 
 	r = nftnl_rule_alloc();
@@ -1850,7 +1866,8 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 		return;
 
 	nftnl_rule_set_str(r, NFTNL_RULE_TABLE, table);
-	nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
+	if (chain)
+		nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
 
 	obj = batch_rule_add(h, NFT_COMPAT_RULE_FLUSH, r);
 	if (!obj) {
@@ -1864,19 +1881,21 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		   bool verbose)
 {
-	int ret = 0;
-	struct nftnl_chain_list *list;
 	struct nftnl_chain_list_iter *iter;
-	struct nftnl_chain *c;
+	struct nftnl_chain_list *list;
+	struct nftnl_chain *c = NULL;
+	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
 
 	nft_fn = nft_rule_flush;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL) {
-		ret = 1;
-		goto err;
+	if (chain || verbose) {
+		list = nft_chain_list_get(h, table, chain);
+		if (list == NULL) {
+			ret = 1;
+			goto err;
+		}
 	}
 
 	if (chain) {
@@ -1885,9 +1904,11 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 			errno = ENOENT;
 			return 0;
 		}
+	}
 
+	if (chain || !verbose) {
 		__nft_rule_flush(h, table, chain, verbose, false);
-		flush_rule_cache(c);
+		flush_rule_cache(h, table, c);
 		return 1;
 	}
 
@@ -1899,11 +1920,10 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 	c = nftnl_chain_list_iter_next(iter);
 	while (c != NULL) {
-		const char *chain_name =
-			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+		chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
 
-		__nft_rule_flush(h, table, chain_name, verbose, false);
-		flush_rule_cache(c);
+		__nft_rule_flush(h, table, chain, verbose, false);
+		flush_rule_cache(h, table, c);
 		c = nftnl_chain_list_iter_next(iter);
 	}
 	nftnl_chain_list_iter_destroy(iter);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 16/24] xtables-restore: Introduce rule counter tokenizer function
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (14 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 15/24] nft: Optimize flushing all chains of a table Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:25 ` [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse Phil Sutter
                   ` (7 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

The same piece of code appears three times, introduce a function to take
care of tokenizing and error reporting.

Pass buffer pointer via reference so it can be updated to point to after
the counters (if found).

While being at it, drop pointless casting when passing pcnt/bcnt to
add_argv().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/iptables-restore.c                   | 35 ++---------------
 iptables/iptables-xml.c                       | 31 +--------------
 .../ipt-restore/0008-restore-counters_0       | 22 +++++++++++
 iptables/xshared.c                            | 38 +++++++++++++++++++
 iptables/xshared.h                            |  1 +
 iptables/xtables-restore.c                    | 35 ++---------------
 6 files changed, 71 insertions(+), 91 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 6bc182bfae4a2..3655b3e84637e 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -313,44 +313,17 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 			int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
-
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer+1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
+			char *parsestart = buffer;
 
 			add_argv(argv[0], 0);
 			add_argv("-t", 0);
 			add_argv(curtable, 0);
 
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
 				add_argv("--set-counters", 0);
-				add_argv((char *) pcnt, 0);
-				add_argv((char *) bcnt, 0);
+				add_argv(pcnt, 0);
+				add_argv(bcnt, 0);
 			}
 
 			add_param_to_argv(parsestart, line);
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 36ad78450b1ef..5255e097eba88 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -644,7 +644,7 @@ iptables_xml_main(int argc, char *argv[])
 			unsigned int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
+			char *parsestart = buffer;
 			char *chain = NULL;
 
 			/* the parser */
@@ -652,34 +652,7 @@ iptables_xml_main(int argc, char *argv[])
 			int quote_open, quoted;
 			char param_buffer[1024];
 
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer + 1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
-
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 
 			/* This is a 'real' parser crafted in artist mode
 			 * not hacker mode. If the author can live with that
diff --git a/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0 b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
new file mode 100755
index 0000000000000..5ac70682b76bf
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+set -e
+
+DUMP="*filter
+:foo - [23:42]
+[13:37] -A foo -j ACCEPT
+COMMIT
+"
+
+EXPECT=":foo - [0:0]
+[0:0] -A foo -j ACCEPT"
+
+$XT_MULTI iptables-restore <<< "$DUMP"
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save --counters | grep foo)
+
+# iptables-*-restore ignores custom chain counters :(
+EXPECT=":foo - [0:0]
+[13:37] -A foo -j ACCEPT"
+
+$XT_MULTI iptables-restore --counters <<< "$DUMP"
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save --counters | grep foo)
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 5e6cd4ae7c908..52730d8a6d526 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -373,6 +373,44 @@ int parse_counters(const char *string, struct xt_counters *ctr)
 	return ret == 2;
 }
 
+/* Tokenize counters argument of typical iptables-restore format rule.
+ *
+ * If *bufferp contains counters, update *pcntp and *bcntp to point at them,
+ * change bytes after counters in *bufferp to nul-bytes, update *bufferp to
+ * point to after the counters and return true.
+ * If *bufferp does not contain counters, return false.
+ * If syntax is wrong in *bufferp, call xtables_error() and hence exit().
+ * */
+bool tokenize_rule_counters(char **bufferp, char **pcntp, char **bcntp, int line)
+{
+	char *ptr, *buffer = *bufferp, *pcnt, *bcnt;
+
+	if (buffer[0] != '[')
+		return false;
+
+	/* we have counters in our input */
+
+	ptr = strchr(buffer, ']');
+	if (!ptr)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need ]\n", line);
+
+	pcnt = strtok(buffer+1, ":");
+	if (!pcnt)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need :\n", line);
+
+	bcnt = strtok(NULL, "]");
+	if (!bcnt)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need ]\n", line);
+
+	*pcntp = pcnt;
+	*bcntp = bcnt;
+	/* start command parsing after counter */
+	*bufferp = ptr + 1;
+
+	return true;
+}
+
+
 inline bool xs_has_arg(int argc, char *argv[])
 {
 	return optind < argc &&
diff --git a/iptables/xshared.h b/iptables/xshared.h
index b08c700e1d8b9..21f4e8fdee67c 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -151,6 +151,7 @@ extern int xtables_lock_or_exit(int wait, struct timeval *tv);
 int parse_wait_time(int argc, char *argv[]);
 void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval);
 int parse_counters(const char *string, struct xt_counters *ctr);
+bool tokenize_rule_counters(char **bufferp, char **pcnt, char **bcnt, int line);
 bool xs_has_arg(int argc, char *argv[]);
 
 extern const struct xtables_afinfo *afinfo;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index c04d7292e0045..3bb901913ed58 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -230,47 +230,20 @@ void xtables_restore_parse(struct nft_handle *h,
 			int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
+			char *parsestart = buffer;
 
 			/* reset the newargv */
 			newargc = 0;
 
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer+1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
-
 			add_argv(argv[0], 0);
 			add_argv("-t", 0);
 			add_argv(curtable->name, 0);
 
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
 				add_argv("--set-counters", 0);
-				add_argv((char *) pcnt, 0);
-				add_argv((char *) bcnt, 0);
+				add_argv(pcnt, 0);
+				add_argv(bcnt, 0);
 			}
 
 			add_param_to_argv(parsestart, line);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (15 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 16/24] xtables-restore: Introduce rule counter tokenizer function Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-30 16:30   ` Pablo Neira Ayuso
  2019-09-25 21:25 ` [iptables PATCH v2 18/24] xtables-restore: Use xt_params->program_name Phil Sutter
                   ` (6 subsequent siblings)
  23 siblings, 1 reply; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

This is a requirement for outsourcing line parsing code into a dedicated
function.

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

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 9d62913461fa4..facad6d02a7ec 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -237,6 +237,7 @@ struct nft_xt_restore_parse {
 	int		testing;
 	const char	*tablename;
 	bool		commit;
+	bool		in_table;
 };
 
 struct nftnl_chain_list;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 3bb901913ed58..2519005590ff1 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -91,7 +91,6 @@ void xtables_restore_parse(struct nft_handle *h,
 {
 	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
-	int in_table = 0;
 	const struct xtc_ops *ops = &xtc_ops;
 
 	line = 0;
@@ -114,7 +113,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			if (verbose)
 				fputs(buffer, stdout);
 			continue;
-		} else if ((strcmp(buffer, "COMMIT\n") == 0) && (in_table)) {
+		} else if ((strcmp(buffer, "COMMIT\n") == 0) && (p->in_table)) {
 			if (!p->testing) {
 				/* Commit per table, although we support
 				 * global commit at once, stick by now to
@@ -128,9 +127,9 @@ void xtables_restore_parse(struct nft_handle *h,
 				if (cb->abort)
 					ret = cb->abort(h);
 			}
-			in_table = 0;
+			p->in_table = 0;
 
-		} else if ((buffer[0] == '*') && (!in_table || !p->commit)) {
+		} else if ((buffer[0] == '*') && (!p->in_table || !p->commit)) {
 			/* New table */
 			char *table;
 
@@ -158,12 +157,12 @@ void xtables_restore_parse(struct nft_handle *h,
 			}
 
 			ret = 1;
-			in_table = 1;
+			p->in_table = 1;
 
 			if (cb->table_new)
 				cb->table_new(h, table);
 
-		} else if ((buffer[0] == ':') && (in_table)) {
+		} else if ((buffer[0] == ':') && (p->in_table)) {
 			/* New chain. */
 			char *policy, *chain = NULL;
 			struct xt_counters count = {};
@@ -226,7 +225,7 @@ void xtables_restore_parse(struct nft_handle *h,
 					      ops->strerror(errno));
 			}
 			ret = 1;
-		} else if (in_table) {
+		} else if (p->in_table) {
 			int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
@@ -281,11 +280,11 @@ void xtables_restore_parse(struct nft_handle *h,
 			exit(1);
 		}
 	}
-	if (in_table && p->commit) {
+	if (p->in_table && p->commit) {
 		fprintf(stderr, "%s: COMMIT expected at line %u\n",
 				xt_params->program_name, line + 1);
 		exit(1);
-	} else if (in_table && cb->commit && !cb->commit(h)) {
+	} else if (p->in_table && cb->commit && !cb->commit(h)) {
 		xtables_error(OTHER_PROBLEM, "%s: final implicit COMMIT failed",
 			      xt_params->program_name);
 	}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 18/24] xtables-restore: Use xt_params->program_name
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (16 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse Phil Sutter
@ 2019-09-25 21:25 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 19/24] xtables-restore: Carry curtable in struct nft_xt_restore_parse Phil Sutter
                   ` (5 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:25 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Instead of setting newargv[0] to argv[0]'s value, just use whatever
xt_params->program_name contains. The latter is arbitrarily defined, but
may still be more correct than real argv[0] which may simply be for
instance xtables-nft-multi. Either way, there is no practical
significance since newargv[0] is used exclusively in debug output.

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

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index facad6d02a7ec..f1efab80ff621 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -263,8 +263,7 @@ struct nft_xt_restore_cb {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb,
-			   int argc, char *argv[]);
+			   struct nft_xt_restore_cb *cb);
 
 void nft_check_xt_legacy(int family, bool is_ipt_save);
 #endif
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 2519005590ff1..48999d1ec8a27 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -86,8 +86,7 @@ static const struct xtc_ops xtc_ops = {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb,
-			   int argc, char *argv[])
+			   struct nft_xt_restore_cb *cb)
 {
 	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
@@ -234,7 +233,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			/* reset the newargv */
 			newargc = 0;
 
-			add_argv(argv[0], 0);
+			add_argv(xt_params->program_name, 0);
 			add_argv("-t", 0);
 			add_argv(curtable->name, 0);
 
@@ -405,7 +404,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	xtables_restore_parse(&h, &p, &restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &restore_cb);
 
 	nft_fini(&h);
 	fclose(p.in);
@@ -471,7 +470,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 
 	nft_init_eb(&h, "ebtables-restore");
 	h.noflush = noflush;
-	xtables_restore_parse(&h, &p, &ebt_restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &ebt_restore_cb);
 	nft_fini(&h);
 
 	return 0;
@@ -495,7 +494,7 @@ int xtables_arp_restore_main(int argc, char *argv[])
 	struct nft_handle h;
 
 	nft_init_arp(&h, "arptables-restore");
-	xtables_restore_parse(&h, &p, &arp_restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &arp_restore_cb);
 	nft_fini(&h);
 
 	return 0;
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 4ae9ff57c0eb3..64e7667a253e7 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -535,7 +535,7 @@ static int xtables_restore_xlate_main(int family, const char *progname,
 
 	printf("# Translated by %s v%s on %s",
 	       argv[0], PACKAGE_VERSION, ctime(&now));
-	xtables_restore_parse(&h, &p, &cb_xlate, argc, argv);
+	xtables_restore_parse(&h, &p, &cb_xlate);
 	printf("# Completed on %s", ctime(&now));
 
 	nft_fini(&h);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 19/24] xtables-restore: Carry curtable in struct nft_xt_restore_parse
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (17 preceding siblings ...)
  2019-09-25 21:25 ` [iptables PATCH v2 18/24] xtables-restore: Use xt_params->program_name Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 20/24] xtables-restore: Introduce line parsing function Phil Sutter
                   ` (4 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

This is a requirement for outsourcing line parsing code into a dedicated
function.

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

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index f1efab80ff621..3be8bafed60e9 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -238,6 +238,7 @@ struct nft_xt_restore_parse {
 	const char	*tablename;
 	bool		commit;
 	bool		in_table;
+	const struct builtin_table *curtable;
 };
 
 struct nftnl_chain_list;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 48999d1ec8a27..3bd8a8925c8bc 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -88,7 +88,6 @@ void xtables_restore_parse(struct nft_handle *h,
 			   struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb)
 {
-	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
 	const struct xtc_ops *ops = &xtc_ops;
 
@@ -139,8 +138,8 @@ void xtables_restore_parse(struct nft_handle *h,
 					"%s: line %u table name invalid\n",
 					xt_params->program_name, line);
 
-			curtable = nft_table_builtin_find(h, table);
-			if (!curtable)
+			p->curtable = nft_table_builtin_find(h, table);
+			if (!p->curtable)
 				xtables_error(PARAMETER_PROBLEM,
 					"%s: line %u table name '%s' invalid\n",
 					xt_params->program_name, line, table);
@@ -186,7 +185,7 @@ void xtables_restore_parse(struct nft_handle *h,
 					   "%s: line %u policy invalid\n",
 					   xt_params->program_name, line);
 
-			if (nft_chain_builtin_find(curtable, chain)) {
+			if (nft_chain_builtin_find(p->curtable, chain)) {
 				if (counters) {
 					char *ctrs;
 					ctrs = strtok(NULL, " \t\n");
@@ -198,7 +197,7 @@ void xtables_restore_parse(struct nft_handle *h,
 
 				}
 				if (cb->chain_set &&
-				    cb->chain_set(h, curtable->name,
+				    cb->chain_set(h, p->curtable->name,
 					          chain, policy, &count) < 0) {
 					xtables_error(OTHER_PROBLEM,
 						      "Can't set policy `%s'"
@@ -208,14 +207,14 @@ void xtables_restore_parse(struct nft_handle *h,
 				}
 				DEBUGP("Setting policy of chain %s to %s\n",
 				       chain, policy);
-			} else if (cb->chain_restore(h, chain, curtable->name) < 0 &&
+			} else if (cb->chain_restore(h, chain, p->curtable->name) < 0 &&
 				   errno != EEXIST) {
 				xtables_error(PARAMETER_PROBLEM,
 					      "cannot create chain "
 					      "'%s' (%s)\n", chain,
 					      strerror(errno));
 			} else if (h->family == NFPROTO_BRIDGE &&
-				   !ebt_set_user_chain_policy(h, curtable->name,
+				   !ebt_set_user_chain_policy(h, p->curtable->name,
 							      chain, policy)) {
 				xtables_error(OTHER_PROBLEM,
 					      "Can't set policy `%s'"
@@ -235,7 +234,7 @@ void xtables_restore_parse(struct nft_handle *h,
 
 			add_argv(xt_params->program_name, 0);
 			add_argv("-t", 0);
-			add_argv(curtable->name, 0);
+			add_argv(p->curtable->name, 0);
 
 			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
@@ -247,7 +246,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			add_param_to_argv(parsestart, line);
 
 			DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-				newargc, curtable->name);
+				newargc, p->curtable->name);
 
 			for (a = 0; a < newargc; a++)
 				DEBUGP("argv[%u]: %s\n", a, newargv[a]);
@@ -270,8 +269,8 @@ void xtables_restore_parse(struct nft_handle *h,
 			free_argv();
 			fflush(stdout);
 		}
-		if (p->tablename && curtable &&
-		    (strcmp(p->tablename, curtable->name) != 0))
+		if (p->tablename && p->curtable &&
+		    (strcmp(p->tablename, p->curtable->name) != 0))
 			continue;
 		if (!ret) {
 			fprintf(stderr, "%s: line %u failed\n",
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 20/24] xtables-restore: Introduce line parsing function
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (18 preceding siblings ...)
  2019-09-25 21:26 ` [iptables PATCH v2 19/24] xtables-restore: Carry curtable in struct nft_xt_restore_parse Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 21/24] tests: shell: Add ipt-restore/0007-flush-noflush_0 Phil Sutter
                   ` (3 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Move the loop code parsing a distinct line of input into a dedicated
function as a preparation for changing input sources. Since loop code
either calls continue or exit() directly, there is no need for a return
code to indicate failure.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Despite the diff's size, this is actually a small change: Pass
--ignore-space-change to git to see how few changes there actually are
besides the necessary indenting level change.
---
 iptables/xtables-restore.c | 354 +++++++++++++++++++------------------
 1 file changed, 180 insertions(+), 174 deletions(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 3bd8a8925c8bc..1a3739b3350c8 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -84,199 +84,205 @@ static const struct xtc_ops xtc_ops = {
 	.strerror	= nft_strerror,
 };
 
-void xtables_restore_parse(struct nft_handle *h,
-			   struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb)
+static void xtables_restore_parse_line(struct nft_handle *h,
+				       struct nft_xt_restore_parse *p,
+				       struct nft_xt_restore_cb *cb,
+				       char *buffer)
 {
-	char buffer[10240];
 	const struct xtc_ops *ops = &xtc_ops;
+	int ret = 0;
+
+	if (buffer[0] == '\n')
+		return;
+	else if (buffer[0] == '#') {
+		if (verbose)
+			fputs(buffer, stdout);
+		return;
+	} else if ((strcmp(buffer, "COMMIT\n") == 0) && (p->in_table)) {
+		if (!p->testing) {
+			/* Commit per table, although we support
+			 * global commit at once, stick by now to
+			 * the existing behaviour.
+			 */
+			DEBUGP("Calling commit\n");
+			if (cb->commit)
+				ret = cb->commit(h);
+		} else {
+			DEBUGP("Not calling commit, testing\n");
+			if (cb->abort)
+				ret = cb->abort(h);
+		}
+		p->in_table = 0;
+
+	} else if ((buffer[0] == '*') && (!p->in_table || !p->commit)) {
+		/* New table */
+		char *table;
+
+		table = strtok(buffer+1, " \t\n");
+		DEBUGP("line %u, table '%s'\n", line, table);
+		if (!table)
+			xtables_error(PARAMETER_PROBLEM,
+				"%s: line %u table name invalid\n",
+				xt_params->program_name, line);
+
+		p->curtable = nft_table_builtin_find(h, table);
+		if (!p->curtable)
+			xtables_error(PARAMETER_PROBLEM,
+				"%s: line %u table name '%s' invalid\n",
+				xt_params->program_name, line, table);
+
+		if (p->tablename && (strcmp(p->tablename, table) != 0))
+			return;
+
+		if (h->noflush == 0) {
+			DEBUGP("Cleaning all chains of table '%s'\n",
+				table);
+			if (cb->table_flush)
+				cb->table_flush(h, table);
+		}
 
-	line = 0;
-
-	if (!h->noflush)
-		nft_fake_cache(h);
-	else
-		nft_build_cache(h);
+		ret = 1;
+		p->in_table = 1;
+
+		if (cb->table_new)
+			cb->table_new(h, table);
+
+	} else if ((buffer[0] == ':') && (p->in_table)) {
+		/* New chain. */
+		char *policy, *chain = NULL;
+		struct xt_counters count = {};
+
+		chain = strtok(buffer+1, " \t\n");
+		DEBUGP("line %u, chain '%s'\n", line, chain);
+		if (!chain)
+			xtables_error(PARAMETER_PROBLEM,
+				   "%s: line %u chain name invalid\n",
+				   xt_params->program_name, line);
+
+		if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
+			xtables_error(PARAMETER_PROBLEM,
+				   "Invalid chain name `%s' "
+				   "(%u chars max)",
+				   chain, XT_EXTENSION_MAXNAMELEN - 1);
+
+		policy = strtok(NULL, " \t\n");
+		DEBUGP("line %u, policy '%s'\n", line, policy);
+		if (!policy)
+			xtables_error(PARAMETER_PROBLEM,
+				   "%s: line %u policy invalid\n",
+				   xt_params->program_name, line);
+
+		if (nft_chain_builtin_find(p->curtable, chain)) {
+			if (counters) {
+				char *ctrs;
+				ctrs = strtok(NULL, " \t\n");
+
+				if (!ctrs || !parse_counters(ctrs, &count))
+					xtables_error(PARAMETER_PROBLEM,
+						   "invalid policy counters "
+						   "for chain '%s'\n", chain);
 
-	/* Grab standard input. */
-	while (fgets(buffer, sizeof(buffer), p->in)) {
-		int ret = 0;
-
-		line++;
-		h->error.lineno = line;
-
-		if (buffer[0] == '\n')
-			continue;
-		else if (buffer[0] == '#') {
-			if (verbose)
-				fputs(buffer, stdout);
-			continue;
-		} else if ((strcmp(buffer, "COMMIT\n") == 0) && (p->in_table)) {
-			if (!p->testing) {
-				/* Commit per table, although we support
-				 * global commit at once, stick by now to
-				 * the existing behaviour.
-				 */
-				DEBUGP("Calling commit\n");
-				if (cb->commit)
-					ret = cb->commit(h);
-			} else {
-				DEBUGP("Not calling commit, testing\n");
-				if (cb->abort)
-					ret = cb->abort(h);
 			}
-			p->in_table = 0;
-
-		} else if ((buffer[0] == '*') && (!p->in_table || !p->commit)) {
-			/* New table */
-			char *table;
-
-			table = strtok(buffer+1, " \t\n");
-			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table)
-				xtables_error(PARAMETER_PROBLEM,
-					"%s: line %u table name invalid\n",
-					xt_params->program_name, line);
-
-			p->curtable = nft_table_builtin_find(h, table);
-			if (!p->curtable)
-				xtables_error(PARAMETER_PROBLEM,
-					"%s: line %u table name '%s' invalid\n",
-					xt_params->program_name, line, table);
-
-			if (p->tablename && (strcmp(p->tablename, table) != 0))
-				continue;
-
-			if (h->noflush == 0) {
-				DEBUGP("Cleaning all chains of table '%s'\n",
-					table);
-				if (cb->table_flush)
-					cb->table_flush(h, table);
-			}
-
-			ret = 1;
-			p->in_table = 1;
-
-			if (cb->table_new)
-				cb->table_new(h, table);
-
-		} else if ((buffer[0] == ':') && (p->in_table)) {
-			/* New chain. */
-			char *policy, *chain = NULL;
-			struct xt_counters count = {};
-
-			chain = strtok(buffer+1, " \t\n");
-			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain)
-				xtables_error(PARAMETER_PROBLEM,
-					   "%s: line %u chain name invalid\n",
-					   xt_params->program_name, line);
-
-			if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Invalid chain name `%s' "
-					   "(%u chars max)",
-					   chain, XT_EXTENSION_MAXNAMELEN - 1);
-
-			policy = strtok(NULL, " \t\n");
-			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy)
-				xtables_error(PARAMETER_PROBLEM,
-					   "%s: line %u policy invalid\n",
-					   xt_params->program_name, line);
-
-			if (nft_chain_builtin_find(p->curtable, chain)) {
-				if (counters) {
-					char *ctrs;
-					ctrs = strtok(NULL, " \t\n");
-
-					if (!ctrs || !parse_counters(ctrs, &count))
-						xtables_error(PARAMETER_PROBLEM,
-							   "invalid policy counters "
-							   "for chain '%s'\n", chain);
-
-				}
-				if (cb->chain_set &&
-				    cb->chain_set(h, p->curtable->name,
-					          chain, policy, &count) < 0) {
-					xtables_error(OTHER_PROBLEM,
-						      "Can't set policy `%s'"
-						      " on `%s' line %u: %s\n",
-						      policy, chain, line,
-						      ops->strerror(errno));
-				}
-				DEBUGP("Setting policy of chain %s to %s\n",
-				       chain, policy);
-			} else if (cb->chain_restore(h, chain, p->curtable->name) < 0 &&
-				   errno != EEXIST) {
-				xtables_error(PARAMETER_PROBLEM,
-					      "cannot create chain "
-					      "'%s' (%s)\n", chain,
-					      strerror(errno));
-			} else if (h->family == NFPROTO_BRIDGE &&
-				   !ebt_set_user_chain_policy(h, p->curtable->name,
-							      chain, policy)) {
+			if (cb->chain_set &&
+			    cb->chain_set(h, p->curtable->name,
+					  chain, policy, &count) < 0) {
 				xtables_error(OTHER_PROBLEM,
 					      "Can't set policy `%s'"
 					      " on `%s' line %u: %s\n",
 					      policy, chain, line,
 					      ops->strerror(errno));
 			}
-			ret = 1;
-		} else if (p->in_table) {
-			int a;
-			char *pcnt = NULL;
-			char *bcnt = NULL;
-			char *parsestart = buffer;
-
-			/* reset the newargv */
-			newargc = 0;
-
-			add_argv(xt_params->program_name, 0);
-			add_argv("-t", 0);
-			add_argv(p->curtable->name, 0);
-
-			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
-			if (counters && pcnt && bcnt) {
-				add_argv("--set-counters", 0);
-				add_argv(pcnt, 0);
-				add_argv(bcnt, 0);
-			}
+			DEBUGP("Setting policy of chain %s to %s\n",
+			       chain, policy);
+		} else if (cb->chain_restore(h, chain, p->curtable->name) < 0 &&
+			   errno != EEXIST) {
+			xtables_error(PARAMETER_PROBLEM,
+				      "cannot create chain "
+				      "'%s' (%s)\n", chain,
+				      strerror(errno));
+		} else if (h->family == NFPROTO_BRIDGE &&
+			   !ebt_set_user_chain_policy(h, p->curtable->name,
+						      chain, policy)) {
+			xtables_error(OTHER_PROBLEM,
+				      "Can't set policy `%s'"
+				      " on `%s' line %u: %s\n",
+				      policy, chain, line,
+				      ops->strerror(errno));
+		}
+		ret = 1;
+	} else if (p->in_table) {
+		int a;
+		char *pcnt = NULL;
+		char *bcnt = NULL;
+		char *parsestart = buffer;
+
+		/* reset the newargv */
+		newargc = 0;
+
+		add_argv(xt_params->program_name, 0);
+		add_argv("-t", 0);
+		add_argv(p->curtable->name, 0);
+
+		tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
+		if (counters && pcnt && bcnt) {
+			add_argv("--set-counters", 0);
+			add_argv(pcnt, 0);
+			add_argv(bcnt, 0);
+		}
+
+		add_param_to_argv(parsestart, line);
 
-			add_param_to_argv(parsestart, line);
+		DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
+			newargc, p->curtable->name);
 
-			DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-				newargc, p->curtable->name);
+		for (a = 0; a < newargc; a++)
+			DEBUGP("argv[%u]: %s\n", a, newargv[a]);
 
-			for (a = 0; a < newargc; a++)
-				DEBUGP("argv[%u]: %s\n", a, newargv[a]);
+		ret = cb->do_command(h, newargc, newargv,
+				    &newargv[2], true);
+		if (ret < 0) {
+			if (cb->abort)
+				ret = cb->abort(h);
+			else
+				ret = 0;
 
-			ret = cb->do_command(h, newargc, newargv,
-					    &newargv[2], true);
 			if (ret < 0) {
-				if (cb->abort)
-					ret = cb->abort(h);
-				else
-					ret = 0;
-
-				if (ret < 0) {
-					fprintf(stderr, "failed to abort "
-							"commit operation\n");
-				}
-				exit(1);
+				fprintf(stderr, "failed to abort "
+						"commit operation\n");
 			}
-
-			free_argv();
-			fflush(stdout);
-		}
-		if (p->tablename && p->curtable &&
-		    (strcmp(p->tablename, p->curtable->name) != 0))
-			continue;
-		if (!ret) {
-			fprintf(stderr, "%s: line %u failed\n",
-					xt_params->program_name, line);
 			exit(1);
 		}
+
+		free_argv();
+		fflush(stdout);
+	}
+	if (p->tablename && p->curtable &&
+	    (strcmp(p->tablename, p->curtable->name) != 0))
+		return;
+	if (!ret) {
+		fprintf(stderr, "%s: line %u failed\n",
+				xt_params->program_name, line);
+		exit(1);
+	}
+}
+
+void xtables_restore_parse(struct nft_handle *h,
+			   struct nft_xt_restore_parse *p,
+			   struct nft_xt_restore_cb *cb)
+{
+	char buffer[10240];
+
+	line = 0;
+
+	if (!h->noflush)
+		nft_fake_cache(h);
+	else
+		nft_build_cache(h);
+
+	/* Grab standard input. */
+	while (fgets(buffer, sizeof(buffer), p->in)) {
+		h->error.lineno = ++line;
+		xtables_restore_parse_line(h, p, cb, buffer);
 	}
 	if (p->in_table && p->commit) {
 		fprintf(stderr, "%s: COMMIT expected at line %u\n",
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 21/24] tests: shell: Add ipt-restore/0007-flush-noflush_0
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (19 preceding siblings ...)
  2019-09-25 21:26 ` [iptables PATCH v2 20/24] xtables-restore: Introduce line parsing function Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 22/24] xtables-restore: Remove some pointless linebreaks Phil Sutter
                   ` (2 subsequent siblings)
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Simple test to make sure iptables-restore does not touch tables it is
not supposed to.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../ipt-restore/0007-flush-noflush_0          | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0

diff --git a/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0 b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
new file mode 100755
index 0000000000000..029db2235b9a4
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
@@ -0,0 +1,42 @@
+#!/bin/bash
+
+# Make sure iptables-restore without --noflush does not flush tables other than
+# those contained in the dump it's reading from
+
+set -e
+
+$XT_MULTI iptables-restore <<EOF
+*nat
+-A POSTROUTING -j ACCEPT
+COMMIT
+EOF
+
+EXPECT="*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+-A POSTROUTING -j ACCEPT
+COMMIT"
+diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -j ACCEPT
+COMMIT
+EOF
+
+EXPECT="*filter
+:INPUT ACCEPT [0:0]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+-A FORWARD -j ACCEPT
+COMMIT
+*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+-A POSTROUTING -j ACCEPT
+COMMIT"
+diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 22/24] xtables-restore: Remove some pointless linebreaks
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (20 preceding siblings ...)
  2019-09-25 21:26 ` [iptables PATCH v2 21/24] tests: shell: Add ipt-restore/0007-flush-noflush_0 Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 23/24] xtables-restore: Allow lines without trailing newline character Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 24/24] xtables-restore: Improve performance of --noflush operation Phil Sutter
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Due to reduced indenting level, some linebreaks are no longer needed.
OTOH, strings should not be split to aid in grepping for error output.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-restore.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 1a3739b3350c8..e9cdf2093bfcb 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -135,8 +135,7 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 			return;
 
 		if (h->noflush == 0) {
-			DEBUGP("Cleaning all chains of table '%s'\n",
-				table);
+			DEBUGP("Cleaning all chains of table '%s'\n", table);
 			if (cb->table_flush)
 				cb->table_flush(h, table);
 		}
@@ -161,8 +160,7 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 
 		if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid chain name `%s' "
-				   "(%u chars max)",
+				   "Invalid chain name `%s' (%u chars max)",
 				   chain, XT_EXTENSION_MAXNAMELEN - 1);
 
 		policy = strtok(NULL, " \t\n");
@@ -179,16 +177,15 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 
 				if (!ctrs || !parse_counters(ctrs, &count))
 					xtables_error(PARAMETER_PROBLEM,
-						   "invalid policy counters "
-						   "for chain '%s'\n", chain);
+						   "invalid policy counters for chain '%s'\n",
+						   chain);
 
 			}
 			if (cb->chain_set &&
 			    cb->chain_set(h, p->curtable->name,
 					  chain, policy, &count) < 0) {
 				xtables_error(OTHER_PROBLEM,
-					      "Can't set policy `%s'"
-					      " on `%s' line %u: %s\n",
+					      "Can't set policy `%s' on `%s' line %u: %s\n",
 					      policy, chain, line,
 					      ops->strerror(errno));
 			}
@@ -197,15 +194,13 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		} else if (cb->chain_restore(h, chain, p->curtable->name) < 0 &&
 			   errno != EEXIST) {
 			xtables_error(PARAMETER_PROBLEM,
-				      "cannot create chain "
-				      "'%s' (%s)\n", chain,
-				      strerror(errno));
+				      "cannot create chain '%s' (%s)\n",
+				      chain, strerror(errno));
 		} else if (h->family == NFPROTO_BRIDGE &&
 			   !ebt_set_user_chain_policy(h, p->curtable->name,
 						      chain, policy)) {
 			xtables_error(OTHER_PROBLEM,
-				      "Can't set policy `%s'"
-				      " on `%s' line %u: %s\n",
+				      "Can't set policy `%s' on `%s' line %u: %s\n",
 				      policy, chain, line,
 				      ops->strerror(errno));
 		}
@@ -238,8 +233,7 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		for (a = 0; a < newargc; a++)
 			DEBUGP("argv[%u]: %s\n", a, newargv[a]);
 
-		ret = cb->do_command(h, newargc, newargv,
-				    &newargv[2], true);
+		ret = cb->do_command(h, newargc, newargv, &newargv[2], true);
 		if (ret < 0) {
 			if (cb->abort)
 				ret = cb->abort(h);
@@ -247,8 +241,8 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 				ret = 0;
 
 			if (ret < 0) {
-				fprintf(stderr, "failed to abort "
-						"commit operation\n");
+				fprintf(stderr,
+					"failed to abort commit operation\n");
 			}
 			exit(1);
 		}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 23/24] xtables-restore: Allow lines without trailing newline character
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (21 preceding siblings ...)
  2019-09-25 21:26 ` [iptables PATCH v2 22/24] xtables-restore: Remove some pointless linebreaks Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  2019-09-25 21:26 ` [iptables PATCH v2 24/24] xtables-restore: Improve performance of --noflush operation Phil Sutter
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Old code in add_param_to_argv() assumed the input line would always end
with a newline character. Without it, the last word of input wasn't
recognized. Fix this by adding a final check for param.len (indicating
leftover data in buffer).

In line parsing code itself, only COMMIT line check required presence of
trailing newline. The replaced conditional is not 100% accurate as it
allows for characters after newline to be present, but since fgets() is
used this shouldn't happen anyway.

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

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 52730d8a6d526..01709288da5dc 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -549,6 +549,10 @@ void add_param_to_argv(char *parsestart, int line)
 		add_argv(param.buffer, 0);
 		param.len = 0;
 	}
+	if (param.len) {
+		param.buffer[param.len] = '\0';
+		add_argv(param.buffer, 0);
+	}
 }
 
 static const char *ipv4_addr_to_string(const struct in_addr *addr,
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index e9cdf2093bfcb..d065e1a921be7 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -98,7 +98,9 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		if (verbose)
 			fputs(buffer, stdout);
 		return;
-	} else if ((strcmp(buffer, "COMMIT\n") == 0) && (p->in_table)) {
+	} else if (p->in_table &&
+		   (strncmp(buffer, "COMMIT", 6) == 0) &&
+		   (buffer[6] == '\0' || buffer[6] == '\n')) {
 		if (!p->testing) {
 			/* Commit per table, although we support
 			 * global commit at once, stick by now to
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [iptables PATCH v2 24/24] xtables-restore: Improve performance of --noflush operation
  2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
                   ` (22 preceding siblings ...)
  2019-09-25 21:26 ` [iptables PATCH v2 23/24] xtables-restore: Allow lines without trailing newline character Phil Sutter
@ 2019-09-25 21:26 ` Phil Sutter
  23 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

The reason for that full cache fetching when called with --noflush even
before looking at any input data was that there might be a command
requiring a rule cache following some rule add/insert ones which don't.
At that point one needs to fetch rules from kernel and try to insert the
local ones at the right spot which is non-trivial.

At the same time there is a performance-critical use-case for --noflush,
namely fast insertion of a bunch of rules in one go, avoiding the
process spawn overhead.

Optimize for this use-case by preloading input into a 64KB buffer to see
if it fits. If so, search for commands requiring a rule cache. If there
are none, skip initial full cache fetching.

The above algorithm may abort at any point, so actual input parsing must
happen in three stages:

1) parse all preloaded lines from 64KB buffer
2) parse any leftover line in line buffer (happens if input exceeds
   the preload buffer size)
3) parse remaining input from input file pointer

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-restore.c | 85 ++++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 7 deletions(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index d065e1a921be7..1b84f78070fa0 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -262,22 +262,93 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 	}
 }
 
+/* return true if given iptables-restore line references a rule */
+static bool is_ruleref_cmd(char *cmd)
+{
+	char c, chain[32];
+	int rulenum, mcount;
+
+	mcount = sscanf(cmd, "-%c %31s %d", &c, chain, &rulenum);
+
+	if (mcount == 3)
+		return true;
+
+	switch (c) {
+	case 'D':
+	case 'C':
+	case 'S':
+	case 'L':
+	case 'Z':
+		return true;
+	}
+
+	return false;
+}
+
+#define PREBUFSIZ	65536
+
 void xtables_restore_parse(struct nft_handle *h,
 			   struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb)
 {
-	char buffer[10240];
-
-	line = 0;
+	char preload_buffer[PREBUFSIZ] = {}, buffer[10240], *ptr;
 
-	if (!h->noflush)
+	if (!h->noflush) {
 		nft_fake_cache(h);
-	else
-		nft_build_cache(h);
+	} else {
+		ssize_t pblen = sizeof(preload_buffer);
+		bool do_cache = false;
+
+		ptr = preload_buffer;
+		while (fgets(buffer, sizeof(buffer), p->in)) {
+			size_t blen = strlen(buffer);
+
+			/* drop trailing newline; add_param_to_argv() changes
+			 * it into nul, causing unpredictable string delimiting
+			 * in preload_buffer */
+			if (buffer[blen - 1] == '\n')
+				buffer[blen - 1] = '\0';
+			else
+				blen++;
 
-	/* Grab standard input. */
+			pblen -= blen;
+			if (pblen <= 0) {
+				/* buffer exhausted */
+				do_cache = true;
+				break;
+			}
+
+			if (is_ruleref_cmd(buffer)) {
+				do_cache = true;
+				break;
+			}
+
+			/* keep strings nul-terminated */
+			memcpy(ptr, buffer, blen);
+			ptr += blen;
+			buffer[0] = '\0';
+		}
+
+		if (do_cache)
+			nft_build_cache(h);
+	}
+
+	line = 0;
+	ptr = preload_buffer;
+	while (*ptr) {
+		h->error.lineno = ++line;
+		DEBUGP("%s: buffered line %d: '%s'\n", __func__, line, ptr);
+		xtables_restore_parse_line(h, p, cb, ptr);
+		ptr += strlen(ptr) + 1;
+	}
+	if (*buffer) {
+		h->error.lineno = ++line;
+		DEBUGP("%s: overrun line %d: '%s'\n", __func__, line, buffer);
+		xtables_restore_parse_line(h, p, cb, buffer);
+	}
 	while (fgets(buffer, sizeof(buffer), p->in)) {
 		h->error.lineno = ++line;
+		DEBUGP("%s: input line %d: '%s'\n", __func__, line, buffer);
 		xtables_restore_parse_line(h, p, cb, buffer);
 	}
 	if (p->in_table && p->commit) {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 01/24] xtables_error() does not return
  2019-09-25 21:25 ` [iptables PATCH v2 01/24] xtables_error() does not return Phil Sutter
@ 2019-09-25 21:31   ` Florian Westphal
  2019-09-25 21:47     ` Phil Sutter
  0 siblings, 1 reply; 38+ messages in thread
From: Florian Westphal @ 2019-09-25 21:31 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> It's a define which resolves into a callback which in turn is declared
> with noreturn attribute. It will never return, therefore drop all
> explicit exit() calls or other dead code immediately following it.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Florian Westphal <fw@strlen.de>

Feel free to push this already.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 01/24] xtables_error() does not return
  2019-09-25 21:31   ` Florian Westphal
@ 2019-09-25 21:47     ` Phil Sutter
  0 siblings, 0 replies; 38+ messages in thread
From: Phil Sutter @ 2019-09-25 21:47 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel

On Wed, Sep 25, 2019 at 11:31:15PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > It's a define which resolves into a callback which in turn is declared
> > with noreturn attribute. It will never return, therefore drop all
> > explicit exit() calls or other dead code immediately following it.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Acked-by: Florian Westphal <fw@strlen.de>
> 
> Feel free to push this already.

DONE. :)

Thanks, Phil

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0
  2019-09-25 21:25 ` [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0 Phil Sutter
@ 2019-09-26  9:07   ` Florian Westphal
  0 siblings, 0 replies; 38+ messages in thread
From: Florian Westphal @ 2019-09-26  9:07 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> This test tended to cause quite excessive load on my system, sometimes
> taking longer than all other tests combined. Even with the reduced
> numbers, it still fails reliably after reverting commit 58d7de0181f61
> ("xtables: handle concurrent ruleset modifications").

Acked-by: Florian Westphal <fw@strlen.de>


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only
  2019-09-25 21:25 ` [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only Phil Sutter
@ 2019-09-27 14:19   ` Florian Westphal
  2019-09-30 16:20     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 38+ messages in thread
From: Florian Westphal @ 2019-09-27 14:19 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> After some changes, one might want to test a single variant only. Allow
> this by supporting -n/--nft and -l/--legacy parameters, each disabling
> the other variant.

Makes sense to me.

Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch
  2019-09-25 21:25 ` [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch Phil Sutter
@ 2019-09-27 14:20   ` Florian Westphal
  2019-09-30 16:36     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 38+ messages in thread
From: Florian Westphal @ 2019-09-27 14:20 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> Another corner-case found when extending restore ordering test: If a
> delete command in a dump referenced a rule added earlier within the same
> dump, kernel would reject the resulting NFT_MSG_DELRULE command.
> 
> Catch this by assigning the rule to delete a RULE_ID value if it doesn't
> have a handle yet. Since __nft_rule_del() does not duplicate the
> nftnl_rule object when creating the NFT_COMPAT_RULE_DELETE command, this
> RULE_ID value is added to both NEWRULE and DELRULE commands - exactly
> what is needed to establish the reference.

Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables
  2019-09-25 21:25 ` [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables Phil Sutter
@ 2019-09-27 14:25   ` Florian Westphal
  0 siblings, 0 replies; 38+ messages in thread
From: Florian Westphal @ 2019-09-27 14:25 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> No need for a full cache to serve the list of tables.

Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing
  2019-09-25 21:25 ` [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing Phil Sutter
@ 2019-09-27 14:27   ` Florian Westphal
  0 siblings, 0 replies; 38+ messages in thread
From: Florian Westphal @ 2019-09-27 14:27 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> Unless --noflush was given, xtables-restore merely needs the list of
> tables to decide whether to delete it or not. Introduce nft_fake_cache()
> function which populates table list, initializes chain lists (so
> nft_chain_list_get() returns an empty list instead of NULL) and sets
> 'have_cache' to turn any later calls to nft_build_cache() into nops.
> 
> If --noflush was given, call nft_build_cache() just once instead of for
> each table line in input.

LGTM.
Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only
  2019-09-27 14:19   ` Florian Westphal
@ 2019-09-30 16:20     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 16:20 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Phil Sutter, netfilter-devel

On Fri, Sep 27, 2019 at 04:19:01PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > After some changes, one might want to test a single variant only. Allow
> > this by supporting -n/--nft and -l/--legacy parameters, each disabling
> > the other variant.
> 
> Makes sense to me.
> 
> Acked-by: Florian Westphal <fw@strlen.de>

OK, please push out this.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse
  2019-09-25 21:25 ` [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse Phil Sutter
@ 2019-09-30 16:30   ` Pablo Neira Ayuso
  2019-09-30 16:31     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 16:30 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

On Wed, Sep 25, 2019 at 11:25:58PM +0200, Phil Sutter wrote:
> This is a requirement for outsourcing line parsing code into a dedicated
> function.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft-shared.h      |  1 +
>  iptables/xtables-restore.c | 17 ++++++++---------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> index 9d62913461fa4..facad6d02a7ec 100644
> --- a/iptables/nft-shared.h
> +++ b/iptables/nft-shared.h
> @@ -237,6 +237,7 @@ struct nft_xt_restore_parse {
>  	int		testing;
>  	const char	*tablename;
>  	bool		commit;
> +	bool		in_table;

I don't think this belong here.

If you want to add a structure, then add something like:

struct nft_xt_parse_ctx {
        char    buffer[10240];
        char    *curtable;
        int     in_table;
};

to store all the internal context, not mix things like internal
parsing context with the object that describes the parser
configuration.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse
  2019-09-30 16:30   ` Pablo Neira Ayuso
@ 2019-09-30 16:31     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 16:31 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Mon, Sep 30, 2019 at 06:30:53PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 25, 2019 at 11:25:58PM +0200, Phil Sutter wrote:
> > This is a requirement for outsourcing line parsing code into a dedicated
> > function.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  iptables/nft-shared.h      |  1 +
> >  iptables/xtables-restore.c | 17 ++++++++---------
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> > index 9d62913461fa4..facad6d02a7ec 100644
> > --- a/iptables/nft-shared.h
> > +++ b/iptables/nft-shared.h
> > @@ -237,6 +237,7 @@ struct nft_xt_restore_parse {
> >  	int		testing;
> >  	const char	*tablename;
> >  	bool		commit;
> > +	bool		in_table;
> 
> I don't think this belong here.
> 
> If you want to add a structure, then add something like:
> 
> struct nft_xt_parse_ctx {
>         char    buffer[10240];
>         char    *curtable;
>         int     in_table;
> };
> 
> to store all the internal context, not mix things like internal
> parsing context with the object that describes the parser
> configuration.

Attaching a patch to constify nft_xt_restore_parse.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 857 bytes --]

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index de889ead7b60..ea9e5bfa6ae1 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -261,7 +261,7 @@ struct nft_xt_restore_cb {
 };
 
 void xtables_restore_parse(struct nft_handle *h,
-			   struct nft_xt_restore_parse *p,
+			   const struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb,
 			   int argc, char *argv[]);
 
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 601c842feab3..d34fad43e0b3 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -85,7 +85,7 @@ static const struct xtc_ops xtc_ops = {
 };
 
 void xtables_restore_parse(struct nft_handle *h,
-			   struct nft_xt_restore_parse *p,
+			   const struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb,
 			   int argc, char *argv[])
 {

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch
  2019-09-27 14:20   ` Florian Westphal
@ 2019-09-30 16:36     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 16:36 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Phil Sutter, netfilter-devel

On Fri, Sep 27, 2019 at 04:20:27PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Another corner-case found when extending restore ordering test: If a
> > delete command in a dump referenced a rule added earlier within the same
> > dump, kernel would reject the resulting NFT_MSG_DELRULE command.
> > 
> > Catch this by assigning the rule to delete a RULE_ID value if it doesn't
> > have a handle yet. Since __nft_rule_del() does not duplicate the
> > nftnl_rule object when creating the NFT_COMPAT_RULE_DELETE command, this
> > RULE_ID value is added to both NEWRULE and DELRULE commands - exactly
> > what is needed to establish the reference.
> 
> Acked-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get()
  2019-09-25 21:25 ` [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get() Phil Sutter
@ 2019-09-30 16:57   ` Pablo Neira Ayuso
  2019-09-30 17:12   ` Pablo Neira Ayuso
  1 sibling, 0 replies; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 16:57 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

On Wed, Sep 25, 2019 at 11:25:49PM +0200, Phil Sutter wrote:
> @@ -2238,6 +2259,8 @@ nft_rule_find(struct nft_handle *h, struct nftnl_chain *c, void *data, int rulen
>  	struct nftnl_rule_iter *iter;
>  	bool found = false;
>  
> +	fetch_rule_cache(h, c);

fetch_rule_cache() does not perform any cache consistency check. This
function is good to be called from nft_build_cache() path. However, if
you call it away from it, you have to be sure the cache you are ending
up with is consistency. There are several netlink dump operations in a
row in fetch_rule_cache(), this is likely to happen I'm afraid.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get()
  2019-09-25 21:25 ` [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get() Phil Sutter
  2019-09-30 16:57   ` Pablo Neira Ayuso
@ 2019-09-30 17:12   ` Pablo Neira Ayuso
  1 sibling, 0 replies; 38+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-30 17:12 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

On Wed, Sep 25, 2019 at 11:25:49PM +0200, Phil Sutter wrote:
> The function is used to return the given table's chains, so fetching
> chain cache is enough.
> 
> This requires a bunch of manual rule cache updates in different places.
> To still support the fake cache logic from xtables-restore, make
> fetch_rule_cache() do nothing in case have_cache is set.
> 
> Accidental double rule cache updates for the same chain need to be
> prevented. This is complicated by the fact that chain's rule list is
> managed by libnftnl. Hence the same logic as used for table list, namely
> checking list pointer value, can't be used. Instead, simply fetch rules
> only if the given chain's rule list is empty. If it isn't, rules have
> been fetched before; if it is, a second rule fetch won't hurt.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 7c974af8b4141..729b88d990f9f 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -1264,6 +1264,7 @@ err:
>  
>  static struct nftnl_chain *
>  nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
> +static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c);
>  
>  int
>  nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> @@ -1275,6 +1276,14 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
>  
>  	nft_xt_builtin_init(h, table);
>  
> +	/* Since ebtables user-defined chain policies are implemented as last
> +	 * rule in nftables, rule cache is required here to treat them right. */
> +	if (h->family == NFPROTO_BRIDGE) {
> +		c = nft_chain_find(h, table, chain);
> +		if (c && !nft_chain_builtin(c))
> +			fetch_rule_cache(h, c);
> +	}
> +
>  	nft_fn = nft_rule_append;
>  
>  	r = nft_rule_new(h, chain, table, data);
> @@ -1550,6 +1559,9 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
>  	struct nftnl_rule *rule;
>  	int ret;
>  
> +	if (nftnl_rule_lookup_byindex(c, 0))
> +		return 0;
> +
>  	rule = nftnl_rule_alloc();
>  	if (!rule)
>  		return -1;
> @@ -1579,6 +1591,9 @@ static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c)
>  {
>  	int i;
>  
> +	if (h->have_cache)
> +		return 0;
> +
>  	if (c)
>  		return nft_rule_list_update(c, h);
>  
> @@ -1670,7 +1685,8 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
>  	if (!t)
>  		return NULL;
>  
> -	nft_build_cache(h);
> +	if (!h->have_cache && !h->cache->table[t->type].chains)
> +		fetch_chain_cache(h);

Could we extend nft_build_cache(...) to be used from everywhere in
this code?

Or add something like:

        nft_build_table_cache(...)
        nft_build_chain_cache(...)
        nft_build_rule_cache(...)

that actually call __nft_build_cache(...) with many parameters to
specify what table/chain/... specifically you need.

I don't have any specific design in mind for this API. However, I
would like to see a single routine to build a cache the way you need.
That single routine will ensure cache consistency, no matter what
configuration of partial cache you need.

While speeding up things, cache consistency needs to be guaranteed.

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, back to index

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 21:25 [iptables PATCH v2 00/24] Improve iptables-nft performance with large rulesets Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 01/24] xtables_error() does not return Phil Sutter
2019-09-25 21:31   ` Florian Westphal
2019-09-25 21:47     ` Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 02/24] tests/shell: Speed up ipt-restore/0004-restore-race_0 Phil Sutter
2019-09-26  9:07   ` Florian Westphal
2019-09-25 21:25 ` [iptables PATCH v2 03/24] tests: shell: Support running for legacy/nft only Phil Sutter
2019-09-27 14:19   ` Florian Westphal
2019-09-30 16:20     ` Pablo Neira Ayuso
2019-09-25 21:25 ` [iptables PATCH v2 04/24] nft: Fix for add and delete of same rule in single batch Phil Sutter
2019-09-27 14:20   ` Florian Westphal
2019-09-30 16:36     ` Pablo Neira Ayuso
2019-09-25 21:25 ` [iptables PATCH v2 05/24] nft: Make nftnl_table_list_get() fetch only tables Phil Sutter
2019-09-27 14:25   ` Florian Westphal
2019-09-25 21:25 ` [iptables PATCH v2 06/24] xtables-restore: Minimize caching when flushing Phil Sutter
2019-09-27 14:27   ` Florian Westphal
2019-09-25 21:25 ` [iptables PATCH v2 07/24] nft: Support fetch_rule_cache() per chain Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get() Phil Sutter
2019-09-30 16:57   ` Pablo Neira Ayuso
2019-09-30 17:12   ` Pablo Neira Ayuso
2019-09-25 21:25 ` [iptables PATCH v2 09/24] nft: Support fetch_chain_cache() per table Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 10/24] nft: Support fetch_chain_cache() per chain Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 11/24] nft: Support nft_chain_list_get() " Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 12/24] nft: Reduce cache overhead of adding a custom chain Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 13/24] nft: Reduce cache overhead of nft_chain_builtin_init() Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 14/24] nft: Support nft_is_table_compatible() per chain Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 15/24] nft: Optimize flushing all chains of a table Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 16/24] xtables-restore: Introduce rule counter tokenizer function Phil Sutter
2019-09-25 21:25 ` [iptables PATCH v2 17/24] xtables-restore: Carry in_table in struct nft_xt_restore_parse Phil Sutter
2019-09-30 16:30   ` Pablo Neira Ayuso
2019-09-30 16:31     ` Pablo Neira Ayuso
2019-09-25 21:25 ` [iptables PATCH v2 18/24] xtables-restore: Use xt_params->program_name Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 19/24] xtables-restore: Carry curtable in struct nft_xt_restore_parse Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 20/24] xtables-restore: Introduce line parsing function Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 21/24] tests: shell: Add ipt-restore/0007-flush-noflush_0 Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 22/24] xtables-restore: Remove some pointless linebreaks Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 23/24] xtables-restore: Allow lines without trailing newline character Phil Sutter
2019-09-25 21:26 ` [iptables PATCH v2 24/24] xtables-restore: Improve performance of --noflush operation Phil Sutter

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org netfilter-devel@archiver.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox