Re: [PATCH net 13/15] netfilter: nf_tables: fix memleak when rule deletion fails with hardware offload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message calls the bug fixed in nft_delrule()
  (net/netfilter/nf_tables_api.c) a plain "memleak", but the old error…

--

netfilter: nf_tables: fix memleak when rule deletion fails with hardware offload

The patch moves the flow rule allocation in nft_delrule() so it runs after
the rule has been deactivated. This means the -ENOENT error path no longer
drops an already built flow rule. nft_flow_rule_destroy() now accepts a
NULL flow, and the NULL check in the nf_tables_newrule() error path is
removed.

Link: https://patch.msgid.link/20261028062204.1079089-1-hdthky0@gmail.com/

> Allocate flow rule object after transaction has been allocation and rule
> deactivation has been performed, otherwise memleak is possible.  Let the

[Severity: Low]

The code change looks correct. Should the commit message also mention the
net_device reference leak?

Before this patch, when nf_tables_delrule_deactivate() returned -ENOENT in
nft_delrule(), only the transaction was freed:

	err = nf_tables_delrule_deactivate(ctx, rule);
	if (err < 0) {
		nft_trans_destroy(trans);
		return err;
	}

nft_trans_destroy() only does kfree(trans). The flow rule already stored
in nft_trans_flow_rule(trans) never reached nft_flow_rule_destroy().

Take a rule in an NFT_CHAIN_HW_OFFLOAD chain that uses fwd or dup to a
device. For that rule, nft_fwd_dup_netdev_offload() in
net/netfilter/nf_dup_netdev.c holds a device reference that only
nft_flow_rule_destroy() drops:

	dev = dev_get_by_index(ctx->net, oif);
	...
	/* nft_flow_rule_destroy() releases the reference on this device. */
	entry->dev = dev;

So each failed delete leaked a netdev refcount as well as the memory. A
leaked refcount can leave unregister_netdevice() waiting for the device to
become free, and that blocks netns teardown.

This path is easy to reach. __nft_rule_lookup() matches only on
rule->handle and does no genmask check. Deleting the same handle twice in
one batch therefore hits the -ENOENT path in nft_delrule().

Could the changelog mention the netdev reference leak, so stable
backporters can judge the impact of this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103140308.591690-3-pablo%40netfilter.org