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

[Florian Westphal <fw@strlen.de>]

Florian Westphal <fw@strlen.de> wrote:
> 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.

Actually, Brian, could you please try and see if the following isn't
yielding a saner result?

First, issue 'git revert' for bd662c4218f9, 3d483faa6663, 3cb03edb4de3

(3d483faa6663 has minor conflict, easy to resolve, keep nft_base_seq()
 but move to conditional).

Then, use the previous version of your patch but do use the nft_pernet
spinlock added in this change here.

I think this will give us a 4-part series that is more obvious wrt.
what is going on.

The spinlock fix would then be limited to adding locked sections where
needed, rather than mixing new lock with partial reverts of a few chunks
from the previous attempt.

The 3 reverts could optionally be squased into one commit, clearly saying
that it reverts those 3 changes (and why).

Thanks.