Re: [nf PATCH v2 8/8] netfilter: nf_tables: Add locking for NFT_MSG_GETSETELEM_RESET requests

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Sep 28, 2023 at 07:46:30PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > +static int nf_tables_dumpreset_set(struct sk_buff *skb,
> > > +				   struct netlink_callback *cb)
> > > +{
> > > +	struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));
> > > +	struct nft_set_dump_ctx *dump_ctx = cb->data;
> > > +	int ret, skip = cb->args[0];
> > > +
> > > +	mutex_lock(&nft_net->commit_mutex);
> > > +	ret = nf_tables_dump_set(skb, cb);
> > > +	mutex_unlock(&nft_net->commit_mutex);
> > > +
> > > +	if (cb->args[0] > skip)
> > > +		audit_log_nft_set_reset(dump_ctx->ctx.table, cb->seq,
> > > +					cb->args[0] - skip);
> > > +
> > 
> > Once commit_mutex is dropped, parallel user can
> > delete table, and ctx.table references garbage.
> 
> This path should hold rcu read lock.

Then it would splat on first mutex_lock().

> I think spinlock would be better, we would just spin for very little
> time here for another thread to complete the reset, and the race is
> fixed.
> 
> The use of commit_mutex here is confusing is really misleading to the
> reader, this is also not the commit path.

I'd say its semantically the same thing, we alter state.

We even emit audit records to tell userspace that there is state
change.

Also, are you sure spinlock is appropriate here?

For full-ruleset resets we might be doing quite some
traverals.

But, no problem, I'm fine with spinlock as well.