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

[Florian Westphal <fw@strlen.de>]

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?

> +	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:

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

MIssing kfree(tablename)

> +	buf = kasprintf(GFP_ATOMIC, "%s:%u", tablename, nft_net->base_seq);
> +	audit_log_nfcfg(buf, family, 1, AUDIT_NFT_OP_RULE_RESET, GFP_ATOMIC);
> +	kfree(buf);
> +	kfree(tablename);
> +
> +	return nfnetlink_unicast(skb2, info->net, portid);
>  }
>  
>  void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule)
> @@ -8950,7 +9017,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
>  		.policy		= nft_rule_policy,
>  	},
>  	[NFT_MSG_GETRULE_RESET] = {
> -		.call		= nf_tables_getrule,
> +		.call		= nf_tables_getrule_reset,
>  		.type		= NFNL_CB_RCU,

This can be changed to CB_MUTEX, we can serialize by commit_mutex then.