Re: [PATCH net 03/11] netfilter: nft_flow_offload: drop flowtable reference on init error path

[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] One sentence in the commit message names the wrong callback.

--

netfilter: nft_flow_offload: drop flowtable reference on init error path

In nft_flow_offload_init(), nft_use_inc() raises the flowtable use count
before nf_ct_netns_get() is called. If nf_ct_netns_get() fails, that count
is never dropped. The patch saves the return value of nf_ct_netns_get() and
calls nft_use_dec() on failure, so the inc/dec pairing is restored inside
the init callback.

> The upper layers do not balance it either: nf_tables_newexpr()
> clears expr->ops when the expression init callback fails, so the
> nft_expr_more() iteration in nft_rule_expr_deactivate() and
> nf_tables_rule_destroy() stops right before the failed expression
> and its ->destroy callback, which would drop the reference, never
> runs.

[Severity: Low]
This isn't a bug, but does this paragraph name the wrong callback? In
net/netfilter/nft_flow_offload.c, the ->destroy callback only drops the
conntrack netns reference:

static void nft_flow_offload_destroy(const struct nft_ctx *ctx,
				     const struct nft_expr *expr)
{
	nf_ct_netns_put(ctx->net, ctx->family);
}

The flowtable use count is dropped through ->deactivate instead.
nft_flow_offload_deactivate() calls nf_tables_deactivate_flowtable(), and
that function does the decrement:

net/netfilter/nf_tables_api.c:nf_tables_deactivate_flowtable() {
	switch (phase) {
	case NFT_TRANS_PREPARE_ERROR:
	case NFT_TRANS_PREPARE:
	case NFT_TRANS_ABORT:
	case NFT_TRANS_RELEASE:
		nft_use_dec(&flowtable->use);
	...
}

On the nf_tables_newrule() error path, the callback that gets skipped for
the failed expression is this one:

	nft_rule_expr_deactivate(&ctx, rule, NFT_TRANS_PREPARE_ERROR);

It is skipped because nf_tables_newexpr() cleared expr->ops, which ends the
nft_expr_more() walk.

The code change looks correct. Could the commit message say that the
skipped ->deactivate callback is what would drop the reference, rather than
->destroy? That would help anyone checking the fix against
nft_flow_offload_destroy(), for example during a stable backport.

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