From 575a1e5589f813af7e838c045863b510b4740353 Mon Sep 17 00:00:00 2001
From: Phil Sutter <phil@nwl.cc>
Date: Mon, 5 Oct 2020 16:06:49 +0200
Subject: [PATCH] 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 <phil@nwl.cc>
(cherry picked from commit dac904bdcd9a18aabafee7275ccf0c2bd53800f3)
Conflicts:
iptables/nft.c
-> Upstream changed good/bad return codes of nft_chain_restore()
function.
Signed-off-by: Phil Sutter <psutter@redhat.com>
---
iptables/nft.c | 58 ++++++++++---------
.../ipt-restore/0016-concurrent-restores_0 | 53 +++++++++++++++++
2 files changed, 83 insertions(+), 28 deletions(-)
create mode 100755 iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
diff --git a/iptables/nft.c b/iptables/nft.c
index d661ac2cafda6..dc5490c085364 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -283,7 +283,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;
@@ -1650,7 +1649,7 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
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;
@@ -1672,7 +1671,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
return;
}
- obj->implicit = implicit;
+ obj->skip = skip;
}
int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
@@ -1768,17 +1767,12 @@ 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;
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 -1;
@@ -1786,20 +1780,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 0;
+ __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 -1;
- list = nft_chain_list_get(h, table, chain);
- if (list)
- nftnl_chain_list_add(c, list);
+ obj->skip = !created;
return 0;
}
@@ -2693,11 +2693,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);
@@ -2723,14 +2718,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:
@@ -2742,7 +2745,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:
break;
}
diff --git a/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0 b/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
new file mode 100755
index 0000000000000..53ec12fa368af
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
@@ -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
--
2.28.0