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@xxxxxx>
---
Changes since v1:
- Get rid of a stray closing brace (a leftover from debug output
removal).
---
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 e8538d38e0109..5e55ec13d0da5 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;
+ }
+ }
}
}
@@ -1271,8 +1279,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 +1290,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 +1310,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 +1345,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 +1359,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 +1371,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 +1542,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 +1556,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 +1581,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 +1600,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 +1624,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 +1634,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 +1641,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 +1675,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 +1686,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 +1706,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 +2322,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 +2335,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 +2351,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 +2465,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 +2485,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 +3066,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 +3078,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 +3218,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 +3228,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);
--
2.19.0
