Re: [PATCH net 3/7] netfilter: nf_nat: unregister and release hooks on error

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: nf_nat: unregister and release hooks on error

This defers assigning nat_proto_net->nat_hook_ops until the nested nat hook
has actually been inserted, and adds an error path that unregisters the base
nat hooks and frees the nat_ops array plus the per-hook private data when
insertion fails.  The code change itself looks right and restores the
"nat_hook_ops != NULL implies users > 0" invariant.

The question below is only about the changelog wording.

> However, sashiko reports that nf_hook_entries_insert_raw() fails, then
> ->users count remains at zero and nf_nat_unregister_fn() could hit
> WARN_ON() since base nat hooks with no users is unexpected in the
> unregistration path.

[Severity: Medium]
Is this WARN_ON() actually reachable from the described sequence?

The WARN is the users == 0 check at the top of nf_nat_unregister_fn():

	mutex_lock(&nf_nat_proto_mutex);
	if (WARN_ON(nat_proto_net->users == 0))
		goto unlock;

Every in-tree caller seems to unregister only ops whose registration
returned 0.  ipt_nat_register_lookups() (and the ip6 counterpart) unwinds
with:

	ret = nf_nat_ipv4_register_fn(net, &ops[i]);
	if (ret) {
		while (i)
			nf_nat_ipv4_unregister_fn(net, &ops[--i]);

so the failing index is never unregistered, and for i == 0 nothing is
unregistered at all.

nf_nat_inet_register_fn() in nf_nat_proto.c unregisters only the IPv6 half
on IPv4 failure, and that half had returned 0, so users for IPv6 is at
least 1:

	ret = nf_nat_register_fn(net, NFPROTO_IPV4, ops, nf_nat_ipv4_ops,
				 ARRAY_SIZE(nf_nat_ipv4_ops));
	if (ret)
		nf_nat_unregister_fn(net, NFPROTO_IPV6, ops,
					ARRAY_SIZE(nf_nat_ipv6_ops));

For nft_chain_nat, nf_tables_addchain()'s err_register_hook path does
nft_chain_del() plus nft_trans_destroy() without calling
nf_tables_unregister_hook(), and nf_tables_table_enable() disables only the
chains that registered successfully.

The leftover state after the pre-patch failure also stays self-consistent:
nat_hook_ops is non-NULL with users == 0, so the next successful
nf_nat_register_fn() takes the existing-ops path, bumps users to 1 without
re-registering base hooks, and its matching unregister brings users back to
0 with exactly one nf_unregister_net_hooks().

> +err_unregister_hooks:
> +	if (nat_proto_net->nat_hook_ops) {
> +		mutex_unlock(&nf_nat_proto_mutex);
> +		return ret;
> +	}
> +	nf_unregister_net_hooks(net, nat_ops, ops_count);
> +err_free_hooks:
> +	mutex_unlock(&nf_nat_proto_mutex);
> +	for (i = 0; i < ops_count; i++) {
> +		priv = nat_ops[i].priv;
> +		kfree_rcu(priv, rcu_head);
> +	}
> +	kfree_rcu(nat_ops, rcu);

Isn't the failure this new path actually fixes a leak rather than a WARN?

On the first nat registration for a family in a netns, an
nf_hook_entries_insert_raw() failure (for example the allocation inside
nf_hook_entries_grow()) previously left the base nat hooks registered in
that netns forever, still traversing every packet, and leaked both the
kmemdup_array()'d nat_ops array and the four nf_nat_lookup_hook_priv
allocations.  Nothing reclaims them later, since nat_net_ops declares only
.id and .size:

	static struct pernet_operations nat_net_ops = {
		.id = &nat_net_id,
		.size = sizeof(struct nat_net),
	};

Could the changelog describe that signature instead, so anyone bisecting or
backporting looks for leaked hook registrations and memory rather than a
WARN splat?