From a09e8ae2a1e2ff589af839ad3460493fc04306d7 Mon Sep 17 00:00:00 2001

From: Phil Sutter <phil@nwl.cc>

Date: Thu, 15 Nov 2018 14:53:02 +0100

Subject: [PATCH] xtables: Introduce per table chain caches

Being able to omit the previously obligatory table name check when

iterating over the chain cache might help restore performance with large

rulesets in xtables-save and -restore.

There is one subtle quirk in the code: flush_chain_cache() did free the

global chain cache if not called with a table name but didn't if a table

name was given even if it emptied the chain cache. In other places,

chain_cache being non-NULL prevented a cache update from happening, so

this patch establishes the same behaviour (for each individual chain

cache) since otherwise unexpected cache updates lead to weird problems.

Signed-off-by: Phil Sutter <phil@nwl.cc>

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

(cherry picked from commit c58ecf9f8bcb7619a27ef8ffaddf847a562475a5)

Signed-off-by: Phil Sutter <psutter@redhat.com>

---

 iptables/nft-shared.h      |   3 +-

 iptables/nft.c             | 160 +++++++++++++++++--------------------

 iptables/nft.h             |  10 ++-

 iptables/xtables-restore.c |  16 ++--

 iptables/xtables-save.c    |  12 +--

 5 files changed, 95 insertions(+), 106 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h

index e3ecdb4d23df3..9a61d8d2863e3 100644

--- a/iptables/nft-shared.h

+++ b/iptables/nft-shared.h

@@ -251,7 +251,8 @@ struct nftnl_chain_list;

 struct nft_xt_restore_cb {

 	void (*table_new)(struct nft_handle *h, const char *table);

-	struct nftnl_chain_list *(*chain_list)(struct nft_handle *h);

+	struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,

+					       const char *table);

 	void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,

 			  const char *chain);

 	int (*chain_user_flush)(struct nft_handle *h,

diff --git a/iptables/nft.c b/iptables/nft.c

index 6863d851e44c2..36529048a0ca6 100644

--- a/iptables/nft.c

+++ b/iptables/nft.c

@@ -673,15 +673,17 @@ nft_chain_builtin_find(struct builtin_table *t, const char *chain)

 static void nft_chain_builtin_init(struct nft_handle *h,

 				   struct builtin_table *table)

 {

-	struct nftnl_chain_list *list = nft_chain_list_get(h);

+	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);

 	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++) {

-		c = nft_chain_list_find(list, table->name,

-					table->chains[i].name);

+		c = nft_chain_list_find(list, table->chains[i].name);

 		if (c != NULL)

 			continue;

@@ -782,27 +784,33 @@ static void flush_rule_cache(struct nft_handle *h, const char *tablename)

 static int __flush_chain_cache(struct nftnl_chain *c, void *data)

 {

-	const char *tablename = data;

-

-	if (!strcmp(nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE), tablename)) {

-		nftnl_chain_list_del(c);

-		nftnl_chain_free(c);

-	}

+	nftnl_chain_list_del(c);

+	nftnl_chain_free(c);

 	return 0;

 }

 static void flush_chain_cache(struct nft_handle *h, const char *tablename)

 {

-	if (!h->chain_cache)

-		return;

+	int i;

-	if (tablename) {

-		nftnl_chain_list_foreach(h->chain_cache, __flush_chain_cache,

-					 (void *)tablename);

-	} else {

-		nftnl_chain_list_free(h->chain_cache);

-		h->chain_cache = NULL;

+	for (i = 0; i < NFT_TABLE_MAX; i++) {

+		if (h->tables[i].name == NULL)

+			continue;

+

+		if (tablename && strcmp(h->tables[i].name, tablename))

+			continue;

+

+		if (h->tables[i].chain_cache) {

+			if (tablename) {

+				nftnl_chain_list_foreach(h->tables[i].chain_cache,

+							 __flush_chain_cache, NULL);

+				break;

+			} else {

+				nftnl_chain_list_free(h->tables[i].chain_cache);

+				h->tables[i].chain_cache = NULL;

+			}

+		}

 	}

 }

@@ -1244,8 +1252,9 @@ nft_rule_print_save(const struct nftnl_rule *r, enum nft_rule_print type,

 static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)

 {

+	struct nft_handle *h = data;

+	struct builtin_table *t;

 	struct nftnl_chain *c;

-	struct nftnl_chain_list *list = data;

 	c = nftnl_chain_alloc();

 	if (c == NULL)

@@ -1254,7 +1263,18 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)

 	if (nftnl_chain_nlmsg_parse(nlh, c) < 0)

 		goto out;

-	nftnl_chain_list_add_tail(c, list);

+	t = nft_table_builtin_find(h,

+			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));

+	if (!t)

+		goto out;

+

+	if (!t->chain_cache) {

+		t->chain_cache = nftnl_chain_list_alloc();

+		if (!t->chain_cache)

+			goto out;

+	}

+

+	nftnl_chain_list_add_tail(c, t->chain_cache);

 	return MNL_CB_OK;

 out:

@@ -1263,35 +1283,34 @@ err:

 	return MNL_CB_OK;

 }

-struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h)

+struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,

+					    const char *table)

 {

 	char buf[16536];

 	struct nlmsghdr *nlh;

-	struct nftnl_chain_list *list;

+	struct builtin_table *t;

 	int ret;

-	if (h->chain_cache)

-		return h->chain_cache;

-retry:

-	list = nftnl_chain_list_alloc();

-	if (list == NULL) {

-		errno = ENOMEM;

+	t = nft_table_builtin_find(h, table);

+	if (!t)

 		return NULL;

-	}

+	if (t->chain_cache)

+		return t->chain_cache;

+retry:

 	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, list);

+	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, h);

 	if (ret < 0 && errno == EINTR) {

 		assert(nft_restart(h) >= 0);

-		nftnl_chain_list_free(list);

 		goto retry;

 	}

-	h->chain_cache = list;

+	if (!t->chain_cache)

+		t->chain_cache = nftnl_chain_list_alloc();

-	return list;

+	return t->chain_cache;

 }

 static const char *policy_name[NF_ACCEPT+1] = {

@@ -1299,8 +1318,7 @@ static const char *policy_name[NF_ACCEPT+1] = {

 	[NF_ACCEPT] = "ACCEPT",

 };

-int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list,

-		   const char *table)

+int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list)

 {

 	struct nftnl_chain_list_iter *iter;

 	struct nft_family_ops *ops;

@@ -1314,13 +1332,8 @@ int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list,

 	c = nftnl_chain_list_iter_next(iter);

 	while (c != NULL) {

-		const char *chain_table =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *policy = NULL;

-		if (strcmp(table, chain_table) != 0)

-			goto next;

-

 		if (nft_chain_builtin(c)) {

 			uint32_t pol = NF_ACCEPT;

@@ -1331,7 +1344,7 @@ int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list,

 		if (ops->save_chain)

 			ops->save_chain(c, policy);

-next:

+

 		c = nftnl_chain_list_iter_next(iter);

 	}

@@ -1502,7 +1515,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);

+	list = nft_chain_list_get(h, table);

 	if (list == NULL) {

 		ret = 1;

 		goto err;

@@ -1516,21 +1529,16 @@ 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 *table_name =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *chain_name =

 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);

-		if (strcmp(table, table_name) != 0)

-			goto next;

-

 		if (chain != NULL && strcmp(chain, chain_name) != 0)

 			goto next;

 		if (verbose)

 			fprintf(stdout, "Flushing chain `%s'\n", chain_name);

-		__nft_rule_flush(h, table_name, chain_name);

+		__nft_rule_flush(h, table, chain_name);

 		if (chain != NULL)

 			break;

@@ -1546,6 +1554,7 @@ err:

 int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table)

 {

+	struct nftnl_chain_list *list;

 	struct nftnl_chain *c;

 	int ret;

@@ -1564,9 +1573,9 @@ 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);

-	nft_chain_list_get(h);

-

-	nftnl_chain_list_add(c, h->chain_cache);

+	list = nft_chain_list_get(h, table);

+	if (list)

+		nftnl_chain_list_add(c, list);

 	/* the core expects 1 for success and 0 for error */

 	return ret == 0 ? 1 : 0;

@@ -1588,7 +1597,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);

+	list = nft_chain_list_get(h, table);

 	if (list == NULL)

 		goto err;

@@ -1598,8 +1607,6 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,

 	c = nftnl_chain_list_iter_next(iter);

 	while (c != NULL) {

-		const char *table_name =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *chain_name =

 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);

@@ -1607,9 +1614,6 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,

 		if (nft_chain_builtin(c))

 			goto next;

-		if (strcmp(table, table_name) != 0)

-			goto next;

-

 		if (chain != NULL && strcmp(chain, chain_name) != 0)

 			goto next;

@@ -1644,8 +1648,7 @@ err:

 }

 struct nftnl_chain *

-nft_chain_list_find(struct nftnl_chain_list *list,

-		    const char *table, const char *chain)

+nft_chain_list_find(struct nftnl_chain_list *list, const char *chain)

 {

 	struct nftnl_chain_list_iter *iter;

 	struct nftnl_chain *c;

@@ -1656,14 +1659,9 @@ nft_chain_list_find(struct nftnl_chain_list *list,

 	c = nftnl_chain_list_iter_next(iter);

 	while (c != NULL) {

-		const char *table_name =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *chain_name =

 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);

-		if (strcmp(table, table_name) != 0)

-			goto next;

-

 		if (strcmp(chain, chain_name) != 0)

 			goto next;

@@ -1681,11 +1679,11 @@ nft_chain_find(struct nft_handle *h, const char *table, const char *chain)

 {

 	struct nftnl_chain_list *list;

-	list = nft_chain_list_get(h);

+	list = nft_chain_list_get(h, table);

 	if (list == NULL)

 		return NULL;

-	return nft_chain_list_find(list, table, chain);

+	return nft_chain_list_find(list, chain);

 }

 bool nft_chain_exists(struct nft_handle *h,

@@ -2297,7 +2295,9 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,

 		return 1;

 	}

-	list = nft_chain_list_get(h);

+	list = nft_chain_list_get(h, table);

+	if (!list)

+		goto err; /* XXX: return 0 instead? */

 	iter = nftnl_chain_list_iter_create(list);

 	if (iter == NULL)

@@ -2308,8 +2308,6 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,

 	c = nftnl_chain_list_iter_next(iter);

 	while (c != NULL) {

-		const char *chain_table =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *chain_name =

 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);

 		uint32_t policy =

@@ -2326,8 +2324,6 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,

 		if (nftnl_chain_get(c, NFTNL_CHAIN_HOOKNUM))

 			basechain = true;

-		if (strcmp(table, chain_table) != 0)

-			goto next;

 		if (chain) {

 			if (strcmp(chain, chain_name) != 0)

 				goto next;

@@ -2442,7 +2438,9 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,

 		return 0;

 	}

-	list = nft_chain_list_get(h);

+	list = nft_chain_list_get(h, table);

+	if (!list)

+		goto err; /* XXX: correct? */

 	/* Dump policies and custom chains first */

 	if (!rulenum)

@@ -2460,13 +2458,9 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,

 	c = nftnl_chain_list_iter_next(iter);

 	while (c != NULL) {

-		const char *chain_table =

-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);

 		const char *chain_name =

 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);

-		if (strcmp(table, chain_table) != 0)

-			goto next;

 		if (chain && strcmp(chain, chain_name) != 0)

 			goto next;

@@ -3045,7 +3039,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);

+	list = nft_chain_list_get(h, table);

 	if (list == NULL)

 		goto err;

@@ -3057,11 +3051,6 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,

 	while (c != NULL) {

 		const char *chain_name =

 			nftnl_chain_get(c, NFTNL_CHAIN_NAME);

-		const char *chain_table =

-			nftnl_chain_get(c, NFTNL_CHAIN_TABLE);

-

-		if (strcmp(table, chain_table) != 0)

-			goto next;

 		if (chain != NULL && strcmp(chain, chain_name) != 0)

 			goto next;

@@ -3202,7 +3191,7 @@ static int nft_are_chains_compatible(struct nft_handle *h, const char *tablename

 	struct nftnl_chain *chain;

 	int ret = 0;

-	list = nft_chain_list_get(h);

+	list = nft_chain_list_get(h, tablename);

 	if (list == NULL)

 		return -1;

@@ -3212,12 +3201,7 @@ static int nft_are_chains_compatible(struct nft_handle *h, const char *tablename

 	chain = nftnl_chain_list_iter_next(iter);

 	while (chain != NULL) {

-		const char *chain_table;

-

-		chain_table = nftnl_chain_get_str(chain, NFTNL_CHAIN_TABLE);

-

-		if (strcmp(chain_table, tablename) ||

-		    !nft_chain_builtin(chain))

+		if (!nft_chain_builtin(chain))

 			goto next;

 		ret = nft_is_chain_compatible(h, chain);

diff --git a/iptables/nft.h b/iptables/nft.h

index 052105fc6f3cd..6229221bd51f7 100644

--- a/iptables/nft.h

+++ b/iptables/nft.h

@@ -25,6 +25,7 @@ struct builtin_table {

 	const char *name;

 	struct builtin_chain chains[NF_INET_NUMHOOKS];

 	bool initialized;

+	struct nftnl_chain_list *chain_cache;

 };

 struct nft_handle {

@@ -38,7 +39,6 @@ struct nft_handle {

 	struct list_head	err_list;

 	struct nft_family_ops	*ops;

 	struct builtin_table	*tables;

-	struct nftnl_chain_list	*chain_cache;

 	struct nftnl_rule_list	*rule_cache;

 	bool			restore;

 	int8_t			config_done;

@@ -78,9 +78,11 @@ struct builtin_table *nft_table_builtin_find(struct nft_handle *h, const char *t

 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);

-struct nftnl_chain *nft_chain_list_find(struct nftnl_chain_list *list, const char *table, const char *chain);

-int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list, const char *table);

+struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,

+					    const char *table);

+struct nftnl_chain *nft_chain_list_find(struct nftnl_chain_list *list,

+					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);

 int nft_chain_user_flush(struct nft_handle *h, struct nftnl_chain_list *list,

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c

index f529774054215..a46a92955a01a 100644

--- a/iptables/xtables-restore.c

+++ b/iptables/xtables-restore.c

@@ -56,11 +56,12 @@ static void print_usage(const char *name, const char *version)

 			"	   [ --ipv6 ]\n", name);

 }

-static struct nftnl_chain_list *get_chain_list(struct nft_handle *h)

+static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,

+					       const char *table)

 {

 	struct nftnl_chain_list *chain_list;

-	chain_list = nft_chain_list_get(h);

+	chain_list = nft_chain_list_get(h, table);

 	if (chain_list == NULL)

 		xtables_error(OTHER_PROBLEM, "cannot retrieve chain list\n");

@@ -72,7 +73,7 @@ static void chain_delete(struct nftnl_chain_list *clist, const char *curtable,

 {

 	struct nftnl_chain *chain_obj;

-	chain_obj = nft_chain_list_find(clist, curtable, chain);

+	chain_obj = nft_chain_list_find(clist, chain);

 	/* This chain has been found, delete from list. Later

 	 * on, unvisited chains will be purged out.

 	 */

@@ -112,9 +113,6 @@ void xtables_restore_parse(struct nft_handle *h,

 	line = 0;

-	if (cb->chain_list)

-		chain_list = cb->chain_list(h);

-

 	/* Grab standard input. */

 	while (fgets(buffer, sizeof(buffer), p->in)) {

 		int ret = 0;

@@ -165,6 +163,9 @@ void xtables_restore_parse(struct nft_handle *h,

 			if (p->tablename && (strcmp(p->tablename, table) != 0))

 				continue;

+			if (cb->chain_list)

+				chain_list = cb->chain_list(h, table);

+

 			if (noflush == 0) {

 				DEBUGP("Cleaning all chains of table '%s'\n",

 					table);

@@ -197,8 +198,7 @@ void xtables_restore_parse(struct nft_handle *h,

 				if (cb->chain_del)

 					cb->chain_del(chain_list, curtable->name,

 						      chain);

-			} else if (nft_chain_list_find(chain_list,

-						       curtable->name, chain)) {

+			} else if (nft_chain_list_find(chain_list, chain)) {

 				chain_exists = true;

 				/* Apparently -n still flushes existing user

 				 * defined chains that are redefined. Otherwise,

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c

index bed3ee0318995..d121d50e180ff 100644

--- a/iptables/xtables-save.c

+++ b/iptables/xtables-save.c

@@ -73,7 +73,9 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)

 		return 0;

 	}

-	chain_list = nft_chain_list_get(h);

+	chain_list = nft_chain_list_get(h, tablename);

+	if (!chain_list)

+		return 0;

 	time_t now = time(NULL);

@@ -83,7 +85,7 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)

 	/* Dump out chain names first,

 	 * thereby preventing dependency conflicts */

-	nft_chain_save(h, chain_list, tablename);

+	nft_chain_save(h, chain_list);

 	nft_rule_save(h, tablename, counters ? 0 : FMT_NOCOUNTS);

 	now = time(NULL);

@@ -257,7 +259,7 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters

 		return 0;

 	}

-	chain_list = nft_chain_list_get(h);

+	chain_list = nft_chain_list_get(h, tablename);

 	if (first) {

 		now = time(NULL);

@@ -272,7 +274,7 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters

 	/* Dump out chain names first,

 	 * thereby preventing dependency conflicts */

-	nft_chain_save(h, chain_list, tablename);

+	nft_chain_save(h, chain_list);

 	nft_rule_save(h, tablename, format);

 	printf("\n");

 	return 0;

@@ -399,7 +401,7 @@ int xtables_arp_save_main(int argc, char **argv)

 	}

 	printf("*filter\n");

-	nft_chain_save(&h, nft_chain_list_get(&h), "filter");

+	nft_chain_save(&h, nft_chain_list_get(&h, "filter"));

 	nft_rule_save(&h, "filter", show_counters ? 0 : FMT_NOCOUNTS);

 	printf("\n");

 	nft_fini(&h);

-- 

2.21.0