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

[Phil Sutter <phil@nwl.cc>]

On Sat, Sep 23, 2023 at 01:04:37PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> >  	table = nft_table_lookup(net, nla[NFTA_RULE_TABLE], family, genmask, 0);
> >  	if (IS_ERR(table)) {
> >  		NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_TABLE]);
> > -		return PTR_ERR(table);
> > +		return ERR_CAST(table);
> >  	}
> 
> 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.

> > +	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
> > +		struct netlink_dump_control c = {
> > +			.start= nf_tables_dumpreset_rules_start,
> > +			.dump = nf_tables_dumpreset_rules,
> > +			.done = nf_tables_dump_rules_done,
> > +			.module = THIS_MODULE,
> > +			.data = (void *)nla,
> > +		};
> > +
> > +		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);
> > +	}
> > +
> > +	if (!nla[NFTA_RULE_TABLE])
> > +		return -EINVAL;
> > +
> > +	tablename = nla_strdup(nla[NFTA_RULE_TABLE], GFP_ATOMIC);
> > +	if (!tablename)
> > +		return -ENOMEM;
> > +	spin_lock(&nft_net->reset_lock);
> 
> 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?

But he didn't get a reply. :(

What is the relation to this being a distinct function? Can't one have
the same callback function once with type CB_RCU and once as CB_MUTEX?
nfnetlink doesn't seem to care.

> > +	spin_unlock(&nft_net->reset_lock);
> > +	if (IS_ERR(skb2))
> > +		return PTR_ERR(skb2);
> 
> MIssing kfree(tablename)

Thanks! 

Cheers, Phil