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: 58d7de0181 ("xtables: handle concurrent ruleset modifications")
Signed-off-by: Phil Sutter <phil@nwl.cc>
This commit is contained in:
Phil Sutter
2020-10-05 16:06:49 +02:00
parent c6cff7ddd4
commit dac904bdcd
2 changed files with 83 additions and 28 deletions
+30 -28
View File
@@ -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;
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 (h->family == NFPROTO_BRIDGE)
nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
__nft_rule_flush(h, table, chain, false, created);
if (!created)
return 1;
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:
@@ -0,0 +1,53 @@
#!/bin/bash
set -e
RS="*filter
:INPUT ACCEPT [12024:3123388]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [12840:2144421]
:FOO - [0:0]
:BAR0 - [0:0]
:BAR1 - [0:0]
:BAR2 - [0:0]
:BAR3 - [0:0]
:BAR4 - [0:0]
:BAR5 - [0:0]
:BAR6 - [0:0]
:BAR7 - [0:0]
:BAR8 - [0:0]
:BAR9 - [0:0]
"
RS1="$RS
-X BAR3
-X BAR6
-X BAR9
-A FOO -s 9.9.0.1/32 -j BAR1
-A FOO -s 9.9.0.2/32 -j BAR2
-A FOO -s 9.9.0.4/32 -j BAR4
-A FOO -s 9.9.0.5/32 -j BAR5
-A FOO -s 9.9.0.7/32 -j BAR7
-A FOO -s 9.9.0.8/32 -j BAR8
COMMIT
"
RS2="$RS
-X BAR2
-X BAR5
-X BAR7
-A FOO -s 9.9.0.1/32 -j BAR1
-A FOO -s 9.9.0.3/32 -j BAR3
-A FOO -s 9.9.0.4/32 -j BAR4
-A FOO -s 9.9.0.6/32 -j BAR6
-A FOO -s 9.9.0.8/32 -j BAR8
-A FOO -s 9.9.0.9/32 -j BAR9
COMMIT
"
for n in $(seq 1 10); do
$XT_MULTI iptables-restore --noflush -w <<< "$RS1" &
$XT_MULTI iptables-restore --noflush -w <<< "$RS2" &
wait -n
wait -n
done