Re: [nf PATCH 2/5] netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sat, Sep 23, 2023 at 06:18:13PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > Can you split that into another patch?
> > 
> > You mean the whole creation of nf_tables_getrule_single()? Because the
> > above change is only required due to the changed return type.
> 
> Yes, I was wondering if there is a way to convert the return type
> in a different patch.
> 
> If its too costly, don't bother.
> 
> > > Hmm. Stupid question.  Why do we need a spinlock to serialize?
> > > This is now a distinct function, so:
> > 
> > On Tue, Sep 05, 2023 at 11:11:07PM +0200, Phil Sutter wrote:
> > [...]
> > > I guess NFNL_CB_MUTEX is a no go because it locks down the whole
> > > subsystem, right?

NFNL_CB_MUTEX takes the global subsystem mutex, see
net/netfilter/nfnetlink.c

                case NFNL_CB_MUTEX:
                        rcu_read_unlock();
                        nfnl_lock(subsys_id);
                        ...

This does not help either for netlink dumps, because NFNL_CB_MUTEX
only guarantees that the first netlink dump chunk holds the mutex
while follow up calls to netlink_recvmsg() would be lockless.

Note, Florian updated nf_tables to use a per-netns mutex only.

> If thats really a concern. alernative would be to do same thing as
> nft_netlink_dump_start_rcu(), i.e. use _RCU as-is and then switch
> from rcu to module reference held, plus, in your case, the transaction
> mutex.
> 
> Actually I like that better because we already use this pattern and
> afaics all dumpers call rcu_read_lock for us; i.e.:
> 
> callback_that_might_reset()
> {
> 	try_module_get ...
> 	rcu_read_unlock()
> 	mutex_lock(net->commit_mutex)
> 	  dumper();
> 	mutex_unlock(net->commit_mutex)
> 	rcu_read_lock();
> 	module_put()
> }
>
> should do the trick.

Idiom above LGTM, *except for net->commit_mutex*. Please do not use
->commit_mutex: This will stall ruleset updates for no reason, netlink
dump would grab and release such mutex for each netlink_recvmsg() call
and netlink dump side will always retry because of NLM_F_EINTR.