Re: [PATCH v2 nf-next] netfilter: nf_tables: use dedicated spinlock for reset operations

[Florian Westphal <fw@strlen.de>]

Brian Witte <brianwitte@mailfence.com> wrote:
> Replace commit_mutex with a dedicated reset_lock spinlock for reset
> operations. This fixes a circular locking dependency between
> commit_mutex, nfnl_subsys_ipset, and nlk_cb_mutex-NETFILTER 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
> 
> Using a spinlock instead of a mutex means we stay in the RCU read-side
> critical section throughout, eliminating the try_module_get/module_put
> and rcu_read_unlock/rcu_read_lock dance that was needed to handle the
> sleeping mutex.
> 
> Reported-by: syzbot+ff16b505ec9152e5f448@syzkaller.appspotmail.com
> Fixes: https://syzkaller.appspot.com/bug?extid=ff16b505ec9152e5f448

'Fixes' tag should point at the commit adding the bug.

Fixes: bd662c4218f9 netfilter: nf_tables: Add locking for NFT_MSG_GETOBJ_RESET requests
Fixes: 3d483faa6663 netfilter: nf_tables: Add locking for NFT_MSG_GETSETELEM_RESET requests
Fixes: 3cb03edb4de3 netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests

No need to resend, I can mangle this locally.

> -	mutex_lock(&nft_net->commit_mutex);
> +	spin_lock(&nft_net->reset_lock);
>  	ret = nf_tables_dump_rules(skb, cb);
> -	mutex_unlock(&nft_net->commit_mutex);
> +	spin_unlock(&nft_net->reset_lock);

I *think* its fine, because concurrent resets make no sense.

In case we get reports wrt. long spin time the lock/unlock
pairs will have to be pushed down, like in the previous version.