From dac904bdcd9a18aabafee7275ccf0c2bd53800f3 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 5 Oct 2020 16:06:49 +0200 Subject: nft: Fix for concurrent noflush restore calls Transaction refresh was broken with regards to nft_chain_restore(): It created a rule flush batch object only if the chain was found in cache and a chain add object only if the chain was not found. Yet with concurrent ruleset updates, one has to expect both situations: * If a chain vanishes, the rule flush job must be skipped and instead the chain add job become active. * If a chain appears, the chain add job must be skipped and instead rules flushed. Change the code accordingly: Create both batch objects and set their 'skip' field depending on the situation in cache and adjust both in nft_refresh_transaction(). As a side-effect, the implicit rule flush becomes explicit and all handling of implicit batch jobs is dropped along with the related field indicating such. Reuse the 'implicit' parameter of __nft_rule_flush() to control the initial 'skip' field value instead. A subtle caveat is vanishing of existing chains: Creating the chain add job based on the chain in cache causes a netlink message containing that chain's handle which the kernel dislikes. Therefore unset the chain's handle in that case. Fixes: 58d7de0181f61 ("xtables: handle concurrent ruleset modifications") Signed-off-by: Phil Sutter --- iptables/nft.c | 58 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 28 deletions(-) (limited to 'iptables/nft.c') diff --git a/iptables/nft.c b/iptables/nft.c index ccbbccb5..39882a44 100644 --- a/iptables/nft.c +++ b/iptables/nft.c @@ -265,7 +265,6 @@ struct obj_update { struct list_head head; enum obj_update_type type:8; uint8_t skip:1; - uint8_t implicit:1; unsigned int seq; union { struct nftnl_table *table; @@ -1623,7 +1622,7 @@ struct nftnl_set *nft_set_batch_lookup_byid(struct nft_handle *h, static void __nft_rule_flush(struct nft_handle *h, const char *table, - const char *chain, bool verbose, bool implicit) + const char *chain, bool verbose, bool skip) { struct obj_update *obj; struct nftnl_rule *r; @@ -1645,7 +1644,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table, return; } - obj->implicit = implicit; + obj->skip = skip; } struct nft_rule_flush_data { @@ -1748,19 +1747,14 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table) { struct nftnl_chain_list *list; + struct obj_update *obj; struct nftnl_chain *c; bool created = false; nft_xt_builtin_init(h, table); c = nft_chain_find(h, table, chain); - if (c) { - /* Apparently -n still flushes existing user defined - * chains that are redefined. - */ - if (h->noflush) - __nft_rule_flush(h, table, chain, false, true); - } else { + if (!c) { c = nftnl_chain_alloc(); if (!c) return 0; @@ -1768,20 +1762,26 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table nftnl_chain_set_str(c, NFTNL_CHAIN_TABLE, table); nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain); created = true; - } - if (h->family == NFPROTO_BRIDGE) - nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT); + list = nft_chain_list_get(h, table, chain); + if (list) + nftnl_chain_list_add(c, list); + } else { + /* If the chain should vanish meanwhile, kernel genid changes + * and the transaction is refreshed enabling the chain add + * object. With the handle still set, kernel interprets it as a + * chain replace job and errors since it is not found anymore. + */ + nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE); + } - if (!created) - return 1; + __nft_rule_flush(h, table, chain, false, created); - if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c)) + obj = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c); + if (!obj) return 0; - list = nft_chain_list_get(h, table, chain); - if (list) - nftnl_chain_list_add(c, list); + obj->skip = !created; /* the core expects 1 for success and 0 for error */ return 1; @@ -2638,11 +2638,6 @@ static void nft_refresh_transaction(struct nft_handle *h) h->error.lineno = 0; list_for_each_entry_safe(n, tmp, &h->obj_list, head) { - if (n->implicit) { - batch_obj_del(h, n); - continue; - } - switch (n->type) { case NFT_COMPAT_TABLE_FLUSH: tablename = nftnl_table_get_str(n->table, NFTNL_TABLE_NAME); @@ -2668,14 +2663,22 @@ static void nft_refresh_transaction(struct nft_handle *h) c = nft_chain_find(h, tablename, chainname); if (c) { - /* -restore -n flushes existing rules from redefined user-chain */ - __nft_rule_flush(h, tablename, - chainname, false, true); n->skip = 1; } else if (!c) { n->skip = 0; } break; + case NFT_COMPAT_RULE_FLUSH: + tablename = nftnl_rule_get_str(n->rule, NFTNL_RULE_TABLE); + if (!tablename) + continue; + + chainname = nftnl_rule_get_str(n->rule, NFTNL_RULE_CHAIN); + if (!chainname) + continue; + + n->skip = !nft_chain_find(h, tablename, chainname); + break; case NFT_COMPAT_TABLE_ADD: case NFT_COMPAT_CHAIN_ADD: case NFT_COMPAT_CHAIN_ZERO: @@ -2687,7 +2690,6 @@ static void nft_refresh_transaction(struct nft_handle *h) case NFT_COMPAT_RULE_INSERT: case NFT_COMPAT_RULE_REPLACE: case NFT_COMPAT_RULE_DELETE: - case NFT_COMPAT_RULE_FLUSH: case NFT_COMPAT_SET_ADD: case NFT_COMPAT_RULE_LIST: case NFT_COMPAT_RULE_CHECK: -- cgit v1.2.3