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