diff mbox series

[iptables] xtables: Introduce per table chain caches

Message ID 20181115114624.31813-1-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series [iptables] xtables: Introduce per table chain caches | expand

Commit Message

Phil Sutter Nov. 15, 2018, 11:46 a.m. UTC 
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>
---
 iptables/nft-shared.h      |   3 +-
 iptables/nft.c             | 161 +++++++++++++++++--------------------
 iptables/nft.h             |  10 ++-
 iptables/xtables-restore.c |  16 ++--
 iptables/xtables-save.c    |  12 +--
 5 files changed, 96 insertions(+), 106 deletions(-)
diff mbox series

Patch

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 e8538d38e0109..e320e073ac054 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -673,15 +673,18 @@  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 +785,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;
+			}
+		}
 	}
 }
 
@@ -1271,8 +1280,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)
@@ -1281,7 +1291,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:
@@ -1290,35 +1311,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] = {
@@ -1326,8 +1346,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;
@@ -1341,13 +1360,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;
 
@@ -1358,7 +1372,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);
 	}
 
@@ -1529,7 +1543,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;
@@ -1543,21 +1557,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;
@@ -1573,6 +1582,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;
 
@@ -1591,9 +1601,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;
@@ -1615,7 +1625,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;
 
@@ -1625,8 +1635,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);
 
@@ -1634,9 +1642,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;
 
@@ -1671,8 +1676,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;
@@ -1683,14 +1687,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;
 
@@ -1708,11 +1707,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,
@@ -2324,7 +2323,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)
@@ -2335,8 +2336,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 =
@@ -2353,8 +2352,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;
@@ -2469,7 +2466,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)
@@ -2487,13 +2486,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;
 
@@ -3072,7 +3067,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;
 
@@ -3084,11 +3079,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;
@@ -3229,7 +3219,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;
 
@@ -3239,12 +3229,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 9b4ba5f9a63eb..980b38dcce1e1 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);