On Sat, Sep 23, 2023 at 06:18:13PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> 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.
