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

[syzbot <syzbot+ff16b505ec9152e5f448@syzkaller.appspotmail.com>]

> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git main

This crash does not have a reproducer. I cannot test it.

>
> From: Brian Witte <brianwitte@mailfence.com>
> Subject: netfilter: nf_tables: use dedicated mutex for reset operations
>
> 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