Re: [PATCH net v2 2/3] net/sched: cls_u32: fix linked hash table refcount leak

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/sched: cls_u32: fix linked hash table refcount leak

The patch drops the reference on n->ht_down at the errunbind label of
u32_change(), so that a newly created filter carrying TCA_U32_LINK does not
leave the linked tc_u_hnode refcount elevated when u32_replace_hw_knode()
fails.

Closes: https://sashiko.dev/#/patchset/20260807100356.1083774-1-jedrzej.jagielski%40intel.com

Two questions, one about the Fixes tag and one about the sibling error path
in the same function.

> Fixes: af69afc51a56 ("net/sched: cls_u32: Fix reference counter leak leading to overflow")

[Severity: Medium]
Does this SHA resolve anywhere?  In this tree:

  $ git show af69afc51a56
  fatal: ambiguous argument 'af69afc51a56': unknown revision or path not
  in the working tree.

The commit whose subject matches verbatim is 04c55383fa56 ("net/sched:
cls_u32: Fix reference counter leak leading to overflow", Lee Jones,
2023-06-08), and that one resolves fine, so it looks like the digits in the
tag got mangled.

Beyond the typo, is 04c55383fa56 the right blame target?  The leak fixed
here is on the u32_replace_hw_knode() failure path of the new-node branch of
u32_change().  git blame of that area points at the addition of the
errunbind label (9cb36faedeaf) and, for the offload error goto itself, at
d34e3e181395, whereas 04c55383fa56 only reordered the INDEV/LINK validation
inside u32_set_parms().

Since the patch carries Cc: <stable@vger.kernel.org>, would it make sense to
correct the tag so the stable tooling can compute the affected trees?

> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 1374da97836eb..f8be2894db85b 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -875,7 +875,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		      struct netlink_ext_ack *extack)
>  {
>  	struct tc_u_common *tp_c = tp->data;
> -	struct tc_u_hnode *ht;
> +	struct tc_u_hnode *ht, *ht_down;
>  	struct tc_u_knode *n;
>  	struct tc_u32_sel *s;
>  	struct nlattr *opt = tca[TCA_OPTIONS];

[ ... ]

> @@ -1192,6 +1192,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  
>  errunbind:
>  	u32_unbind_filter(tp, n, tb);
> +	ht_down = rtnl_dereference(n->ht_down);
> +	if (ht_down && refcount_dec_and_test(&ht_down->refcnt))
> +		kfree(ht_down);

[Severity: Medium]
This isn't a bug introduced by this patch, but with the create path now
balanced, does the sibling "replace existing knode" branch of u32_change()
still hold an unowned reference on the linked hash table?  That branch does:

  net/sched/cls_u32.c:u32_change() {
	err = u32_replace_hw_knode(tp, new, userflags, extack);
	if (err) {
		u32_unbind_filter(tp, new, tb);

		if (tb[TCA_U32_LINK]) {
			struct tc_u_hnode *ht_old;

			ht_old = rtnl_dereference(n->ht_down);
			if (ht_old)
				refcount_inc(&ht_old->refcnt);
		}
		__u32_destroy_key(new);
		return err;
	}

The increment is applied to n->ht_down, i.e. the old node's linked table,
while the only later release is __u32_destroy_key(new), which drops
new->ht_down.

Walking it through for n->ht_down == A and a request linking to B:

  u32_init_knode()      new->ht_down = A, refcount_inc(A)
  u32_set_parms()       refcount_inc(B), new->ht_down = B,
                        refcount_dec(A)      /* consumes the ref above */
  error handler         refcount_inc(A)      /* no owner */
  __u32_destroy_key()   drops new->ht_down == B

so A ends up permanently at +1.  The same +1 happens for TCA_U32_LINK with
handle 0, where new->ht_down becomes NULL.

Does that then make the linked table undeletable, given u32_delete():

  net/sched/cls_u32.c:u32_delete() {
	if (refcount_dec_if_one(&ht->refcnt)) {
		u32_destroy_hnode(tp, ht, extack);
	} else {
		NL_SET_ERR_MSG_MOD(extack, "Can not delete in-use filter");
		return -EBUSY;
	}

Each failed offload on a change of an existing key node with TCA_U32_LINK
would add another reference, and the tc_u_hnode is then leaked.

For the record, this block is deleted by the later patch in the series,
"net/sched: cls_u32: remove erroneous refcount_inc()", so the two paths are
only inconsistent at this commit.  Would it be worth reordering the series
so the two error paths are never out of sync?
-- 
This is an AI-generated review.