[PATCH nf-next] netfilter: nf_tables: use dedicated mutex for reset operations

[Brian Witte <brianwitte@mailfence.com>]

Add a dedicated reset_mutex to serialize reset operations instead of
reusing the commit_mutex. This fixes a circular locking dependency
between commit_mutex, nfnl_subsys_ipset, and nlk_cb_mutex-NETFILTER
that could lead to deadlock when nft reset, ipset list, and
iptables-nft with set match run concurrently:

  CPU0 (nft reset):        nlk_cb_mutex -> commit_mutex
  CPU1 (ipset list):       nfnl_subsys_ipset -> nlk_cb_mutex
  CPU2 (iptables -m set):  commit_mutex -> nfnl_subsys_ipset

The reset_mutex only serializes concurrent reset operations to prevent
counter underruns, which is all that's needed. Breaking the commit_mutex
dependency in the dump-reset path eliminates the circular lock chain.

Reported-by: syzbot+ff16b505ec9152e5f448@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ff16b505ec9152e5f448
Signed-off-by: Brian Witte <brianwitte@mailfence.com>
---
 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 31906f90706e..85cdd93e564b 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1931,6 +1931,7 @@ struct nftables_pernet {
 	struct list_head	module_list;
 	struct list_head	notify_list;
 	struct mutex		commit_mutex;
+	struct mutex		reset_mutex;
 	u64			table_handle;
 	u64			tstamp;
 	unsigned int		gc_seq;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index be4924aeaf0e..c82b7875c49c 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3907,13 +3907,12 @@ static int nf_tables_dumpreset_rules(struct sk_buff *skb,
 	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));
 	int ret;
 
-	/* Mutex is held is to prevent that two concurrent dump-and-reset calls
-	 * do not underrun counters and quotas. The commit_mutex is used for
-	 * the lack a better lock, this is not transaction path.
+	/* Mutex is held to prevent that two concurrent dump-and-reset calls
+	 * do not underrun counters and quotas.
 	 */
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 	ret = nf_tables_dump_rules(skb, cb);
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 
 	return ret;
 }
@@ -4057,9 +4056,9 @@ static int nf_tables_getrule_reset(struct sk_buff *skb,
 	if (!try_module_get(THIS_MODULE))
 		return -EINVAL;
 	rcu_read_unlock();
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 	skb2 = nf_tables_getrule_single(portid, info, nla, true);
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 	rcu_read_lock();
 	module_put(THIS_MODULE);
 
@@ -6346,7 +6345,7 @@ static int nf_tables_dumpreset_set(struct sk_buff *skb,
 	struct nft_set_dump_ctx *dump_ctx = cb->data;
 	int ret, skip = cb->args[0];
 
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 
 	ret = nf_tables_dump_set(skb, cb);
 
@@ -6354,7 +6353,7 @@ static int nf_tables_dumpreset_set(struct sk_buff *skb,
 		audit_log_nft_set_reset(dump_ctx->ctx.table, cb->seq,
 					cb->args[0] - skip);
 
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 
 	return ret;
 }
@@ -6671,7 +6670,7 @@ static int nf_tables_getsetelem_reset(struct sk_buff *skb,
 	if (!try_module_get(THIS_MODULE))
 		return -EINVAL;
 	rcu_read_unlock();
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 	rcu_read_lock();
 
 	err = nft_set_dump_ctx_init(&dump_ctx, skb, info, nla, true);
@@ -6690,7 +6689,7 @@ static int nf_tables_getsetelem_reset(struct sk_buff *skb,
 
 out_unlock:
 	rcu_read_unlock();
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 	rcu_read_lock();
 	module_put(THIS_MODULE);
 
@@ -8552,9 +8551,9 @@ static int nf_tables_dumpreset_obj(struct sk_buff *skb,
 	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));
 	int ret;
 
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 	ret = nf_tables_dump_obj(skb, cb);
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 
 	return ret;
 }
@@ -8693,9 +8692,9 @@ static int nf_tables_getobj_reset(struct sk_buff *skb,
 	if (!try_module_get(THIS_MODULE))
 		return -EINVAL;
 	rcu_read_unlock();
-	mutex_lock(&nft_net->commit_mutex);
+	mutex_lock(&nft_net->reset_mutex);
 	skb2 = nf_tables_getobj_single(portid, info, nla, true);
-	mutex_unlock(&nft_net->commit_mutex);
+	mutex_unlock(&nft_net->reset_mutex);
 	rcu_read_lock();
 	module_put(THIS_MODULE);
 
@@ -12194,6 +12193,7 @@ static int __net_init nf_tables_init_net(struct net *net)
 	INIT_LIST_HEAD(&nft_net->module_list);
 	INIT_LIST_HEAD(&nft_net->notify_list);
 	mutex_init(&nft_net->commit_mutex);
+	mutex_init(&nft_net->reset_mutex);
 	net->nft.base_seq = 1;
 	nft_net->gc_seq = 0;
 	nft_net->validate_state = NFT_VALIDATE_SKIP;
-- 
2.47.3