Re: [PATCH] net/mlx5: Fix rev_entry reference leak in mlx5_tc_ct_shared_counter_get()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message misidentifies the condition that makes the
  leak reachable.
- [Medium] The Fixes: tag points at the wrong commit.

--

net/mlx5: Fix rev_entry reference leak in mlx5_tc_ct_shared_counter_get()

This adds a mlx5_tc_ct_entry_put() on the fall-through path of
mlx5_tc_ct_shared_counter_get() so that the reverse entry reference taken by
mlx5_tc_ct_entry_get() is dropped when the shared counter cannot be reused.
The put itself looks correct; the questions below are about the described
trigger condition and the Fixes tag.

> Fixes: 1edae2335adf ("net/mlx5e: CT: Use the same counter for both directions")
> Cc: stable@vger.kernel.org

[Severity: Medium]
Is this the right commit to blame?  What this patch balances is the entry
reference taken by mlx5_tc_ct_entry_get() in
mlx5_tc_ct_shared_counter_get(), and that reference-taking lookup plus the
matching put on the success path came from a2173131526d ("net/mlx5e: CT:
manage the lifetime of the ct entry object"), not from 1edae2335adf.

git blame on the pre-patch file attributes this whole block to
a2173131526d:

	spin_lock_bh(&ct_priv->ht_lock);
	rev_entry = mlx5_tc_ct_entry_get(ct_priv, &rev_tuple);

	if (IS_ERR(rev_entry)) {
		spin_unlock_bh(&ct_priv->ht_lock);
		goto create_counter;
	}

	if (rev_entry && refcount_inc_not_zero(&rev_entry->counter->refcount)) {
		...
		mlx5_tc_ct_entry_put(rev_entry);
		return shared_counter;
	}

1edae2335adf only contributed the tuple-reversal code above it; at that
point the reverse tuple was looked up with a plain rhashtable_lookup_fast()
and no entry reference was taken, so there was nothing to leak.

With Cc: stable, does the current tag select the wrong backport range?
Trees that contain 1edae2335adf but not a2173131526d (1edae2335adf landed
in v5.9, a2173131526d in v5.12) would be picked, and there
mlx5_tc_ct_shared_counter_get() holds no rev_entry reference at all, so the
hunk cannot apply as written and a hand-resolved backport that added the put
anyway would underflow the rev_entry refcount.

> When the reverse entry is found but its counter is already being
> released, refcount_inc_not_zero() fails and the reference taken by
> mlx5_tc_ct_entry_get() is never dropped before falling through to
> create_counter.

[Severity: Medium]
Can the "counter is already being released" case actually happen for a
non-NULL rev_entry?

mlx5_tc_ct_entry_get() only returns a non-NULL, non-ERR entry after both the
valid test and the entry refcount succeed:

	if (entry && mlx5_tc_ct_entry_valid(entry) &&
	    refcount_inc_not_zero(&entry->refcnt)) {
		return entry;

So the returned rev_entry is VALID and holds a live reference, which means
it cannot be inside mlx5_tc_ct_entry_del() - the only teardown site calling
mlx5_tc_ct_counter_put().  The other mlx5_tc_ct_counter_put() call, in the
err_orig path of mlx5_tc_ct_entry_add_rules(), runs before
MLX5_CT_ENTRY_FLAG_VALID is set, so such an entry is never returned by the
lookup either.  A shared counter owned by a live VALID entry therefore
always has refcount >= 1.

Is the reachable trigger instead the private-counter case?
mlx5_tc_ct_counter_create() kzallocs the counter and never calls
refcount_set():

	counter->is_shared = false;
	counter->counter = mlx5_fc_create(ct_priv->dev, true);

so private counters keep refcount == 0 permanently, and
mlx5_tc_ct_entry_add_rules() picks between the two kinds at runtime:

	if (nf_ct_acct_enabled(dev_net(ct_priv->netdev)))
		entry->counter = mlx5_tc_ct_counter_create(ct_priv);
	else
		entry->counter = mlx5_tc_ct_shared_counter_get(ct_priv, entry);

nf_ct_acct_enabled() reads net->ct.sysctl_acct, the writable per-netns
net.netfilter.nf_conntrack_acct sysctl, so both counter kinds coexist after
accounting is toggled from enabled to disabled.  A new entry then takes the
shared path, finds a live reverse entry owning a private refcount == 0
counter, refcount_inc_not_zero() fails, and the rev_entry reference is
leaked.

Could the changelog describe that path instead?  As written the fixed branch
reads like dead code, which makes reachability hard to judge for stable, and
it hides the companion cleanup question: should
mlx5_tc_ct_counter_create() refcount_set() the private counter, or should
the consumer test is_shared explicitly?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> index 6c87a1c7db095..c1841ce74d9ac 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> @@ -1082,6 +1082,9 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
>  
>  	spin_unlock_bh(&ct_priv->ht_lock);
>  
> +	if (rev_entry)
> +		mlx5_tc_ct_entry_put(rev_entry);
> +
>  create_counter:
>  
>  	shared_counter = mlx5_tc_ct_counter_create(ct_priv);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917113131.2149024-1-vulab%40iscas.ac.cn