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

[Florian Westphal <fw@strlen.de>]

Brian Witte <brianwitte@mailfence.com> wrote:
> 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>

This needs more work:

-----------------------------
net/netfilter/nf_tables_api.c:1002 RCU-list traversed in non-reader section!!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
1 lock held by nft/17539:
 #0: ffff888132018368 (&nft_net->reset_mutex){+.+.}-{4:4}, at: nf_tables_getobj_reset+0x19e/0x5a0 [nf_tables]

stack backtrace:
CPU: 4 UID: 0 PID: 17539 Comm: nft Not tainted 6.19.0-rc6+ #9 PREEMPT(full)
Call Trace:
 lockdep_rcu_suspicious.cold+0x4f/0xb1
 nft_table_lookup.part.0+0x1e7/0x220 [nf_tables]
 nf_tables_getobj_single+0x196/0x5a0 [nf_tables]
 nf_tables_getobj_reset+0x1b1/0x5a0 [nf_tables]
 nfnetlink_rcv_msg+0x49e/0xf00

Please run nftables.git tests/shell/run-tests.sh with

CONFIG_PROVE_LOCKING=y
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_LIST=y

This warning is not a false positive, the list traversal was
fine for reset case because we held the transaction mutex.

Now that we don't, we need to hold rcu_read_lock().

Maybe its worth investigating if we should instead protect
only the reset action itself, i.e. add private reset spinlocks
in nft_quota_do_dump() et al?