[PATCH net v2 0/3] net/sched: u32_change() fixes

[PATCH net v2 0/3] net/sched: u32_change() fixes

[Jedrzej Jagielski]

sorry for spam, previous series get treated as incomplete so resending

ALREADY ON NETDEV, SASHIKO RUN

This series fixes three bugs in cls_u32's u32_change() function:

Patch 1 fixes u32_replace_hw_knode() being fed the wrong flags variable.
Commit 695176bfe5de ("net_sched: refactor TC action init API") introduced
a function parameter `flags` that shadowed the local variable previously
set from TCA_U32_FLAGS. This caused tc_skip_sw() to always return false
during offload, silently ignoring skip_sw/skip_hw attributes. This fix
uncovered following 2 issues happening on u32_replace_hw_knode()
failure.

Patch 2 fixes a refcount leak of the linked hash table (n->ht_down) when
creating a new filter and u32_replace_hw_knode() fails. The error path
frees the node with kfree() but never drops the reference acquired by
u32_set_parms(), leaking the tc_u_hnode.

Patch 3 removes a spurious refcount_inc() in the update error path added
by commit e8d3d78c19be ("net: sched: cls_u32: Undo refcount decrement in
case update failed"). That commit misidentified an already-balanced
refcount (incremented by u32_init_knode, decremented by u32_set_parms)
as needing restoration on failure. The extra increment has no matching
decrement, permanently elevating the hash table's refcount.

Jedrzej Jagielski (3):
  net/sched: cls_u32: feed u32_replace_hw_knode() with correct set of
    flags
  net/sched: cls_u32: fix linked hash table refcount leak
  net/sched: cls_u32: remove erroneous refcount_inc()

 net/sched/cls_u32.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH net v2 1/3] net/sched: cls_u32: feed u32_replace_hw_knode() with correct set of flags

[Jedrzej Jagielski]

Fix u32_change() to pass proper variable to u32_replace_hw_knode().

When trying to offload u32 cmd by adding skip_sw attributes it gets
ignored and it cannot be correctly processed by HW and tc_cls_u32_offload
struct which is then passed to driver lacks skip_sw var enabled.

u32_replace_hw_knode is fed with the TCA_ACT_* flags (bits 16+)
instead of TCA_CLS_* flags (bits 0-4) which actually stores the flags
parsed by tc, so tc_skip_sw() always returns false. This leads to
ignoring some of the attibutes which are meant to be configured on filter
setup.

None of the TCA_ACT_FLAGS_* is actually used within
u32_replace_hw_knode(), so there is no point in passing them.

Looks like commit 695176bfe5de ("net_sched: refactor TC action init API")
shadowed the local flags variable which used to be set with
nla_get_u32(tb[TCA_U32_FLAGS]) with the flags as the new function param
while not replacing for none of the u32_replace_hw_knode() calls.

Cc: <stable@vger.kernel.org>
Fixes: 695176bfe5de ("net_sched: refactor TC action init API")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 net/sched/cls_u32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 8f30cc82181d..dc6e455e64ec 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -935,7 +935,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		u32_bind_filter(tp, new, base, tb);
 
-		err = u32_replace_hw_knode(tp, new, flags, extack);
+		err = u32_replace_hw_knode(tp, new, userflags, extack);
 		if (err) {
 			u32_unbind_filter(tp, new, tb);
 
@@ -1161,7 +1161,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
-		err = u32_replace_hw_knode(tp, n, flags, extack);
+		err = u32_replace_hw_knode(tp, n, userflags, extack);
 		if (err)
 			goto errunbind;
 
-- 
2.31.1

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

[Jedrzej Jagielski]

When a new filter is created with TCA_U32_LINK, u32_set_parms() resolves
the linked hash table and increments its reference count, storing it in
n->ht_down.

If u32_replace_hw_knode() subsequently fails, execution jumps to the
errunbind label which frees the node with kfree(n) but never drops the
reference on n->ht_down. This leaves the tc_u_hnode refcount permanently
elevated, preventing it from being freed when the hash table is later
deleted.

Drop the ht_down reference at errunbind before freeing the node.

Cc: <stable@vger.kernel.org>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260807100356.1083774-1-jedrzej.jagielski%40intel.com
Fixes: af69afc51a56 ("net/sched: cls_u32: Fix reference counter leak leading to overflow")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 net/sched/cls_u32.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index dc6e455e64ec..9539dce217df 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];
@@ -1185,6 +1185,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);
 
 #ifdef CONFIG_CLS_U32_MARK
 	free_percpu(n->pcpu_success);
-- 
2.31.1

[PATCH net v2 3/3] net/sched: cls_u32: remove erroneous refcount_inc()

[Jedrzej Jagielski]

Remove the redundant refcount_inc(&ht_old->refcnt) in the
u32_replace_hw_knode() error path during filter update.

Commit e8d3d78c19be ("net: sched: cls_u32: Undo refcount decrement in case update failed")
added this increment to "undo" a decrement performed by u32_set_parms()
on the original node's linked hash table but apparently this decrement
is already balanced by u32_init_knode(), which increments the same
refcount when cloning the original node:

These cancel out, leaving the original hash table's refcount unchanged.
The additional refcount_inc in the error path has no matching decrement,
leaving the tc_u_hnode refcount permanently elevated by 1.  This
prevents the hash table from ever being freed, leaking memory.

__u32_destroy_key(new) already correctly drops new->ht_down's refcount
(the new linked hash table set by u32_set_parms), so no manual refcount
fixup is needed.

Cc: <stable@vger.kernel.org>
Fixes: e8d3d78c19be ("net: sched: cls_u32: Undo refcount decrement in case update failed")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260807100356.1083774-1-jedrzej.jagielski%40intel.com
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 net/sched/cls_u32.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9539dce217df..69470d49ee9b 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -938,14 +938,6 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		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;
 		}
-- 
2.31.1

Re: [PATCH net v2 0/3] net/sched: u32_change() fixes

[Przemek Kitszel]

On 8/13/26 14:22, Jedrzej Jagielski wrote:
> sorry for spam, previous series get treated as incomplete so resending
> 
> ALREADY ON NETDEV, SASHIKO RUN

hehe
ofc, the above does no harm here

for the series:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

--

Sashiko has found one more *preexisting* bug, perhaps it could be
handled by one more fix in future series?:
https://sashiko.dev/#/patchset/20260813122242.1690024-1-jedrzej.jagielski%40intel.com

 > @@ -938,14 +938,6 @@ static int u32_change(struct net *net, struct 
sk_buff *in_skb,
 >  		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;
 >  		}
This isn't a bug introduced by this patch, but looking at u32_change(), 
there
appears to be a pre-existing issue with how manual hash table handles are
tracked in the IDR.
When a user creates a u32 hash table with a manual handle (for example,
0x80100000), u32_change() inserts the full handle directly into
tp_c->handle_idr:
net/sched/cls_u32.c:u32_change() {
     ...
		} else {
			err = idr_alloc_u32(&tp_c->handle_idr, ht, &handle,
					    handle, GFP_KERNEL);
     ...
}
However, when destroying the node later, u32_destroy_hnode() removes the
mapped ID rather than the full handle:
net/sched/cls_u32.c:u32_destroy_hnode() {
     ...
		idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
     ...
}
For a handle like 0x80100000, handle2id() returns 1. Does this mean the IDR
entry at 0x80100000 is permanently leaked, while index 1 is erroneously 
freed?
If index 1 is already in use by an active auto-generated hash table, could
freeing it here allow gen_new_htid() to issue the same handle again to a new
hash table, causing collisions?

RE: [PATCH net v2 0/3] net/sched: u32_change() fixes

[Jagielski, Jedrzej]

>From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> 
>Sent: Monday, August 17, 2026 2:43 PM
>
>On 8/13/26 14:22, Jedrzej Jagielski wrote:
>> sorry for spam, previous series get treated as incomplete so resending
>> 
>> ALREADY ON NETDEV, SASHIKO RUN
>
>hehe
>ofc, the above does no harm here

:D

>
>for the series:
>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>
>--
>
>Sashiko has found one more *preexisting* bug, perhaps it could be
>handled by one more fix in future series?:
>https://sashiko.dev/#/patchset/20260813122242.1690024-1-jedrzej.jagielski%40intel.com

yeah, i saw that and i thought about extending the series 

>
> > @@ -938,14 +938,6 @@ static int u32_change(struct net *net, struct 
>sk_buff *in_skb,
> >  		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;
> >  		}
>This isn't a bug introduced by this patch, but looking at u32_change(), 
>there
>appears to be a pre-existing issue with how manual hash table handles are
>tracked in the IDR.
>When a user creates a u32 hash table with a manual handle (for example,
>0x80100000), u32_change() inserts the full handle directly into
>tp_c->handle_idr:
>net/sched/cls_u32.c:u32_change() {
>     ...
>		} else {
>			err = idr_alloc_u32(&tp_c->handle_idr, ht, &handle,
>					    handle, GFP_KERNEL);
>     ...
>}
>However, when destroying the node later, u32_destroy_hnode() removes the
>mapped ID rather than the full handle:
>net/sched/cls_u32.c:u32_destroy_hnode() {
>     ...
>		idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
>     ...
>}
>For a handle like 0x80100000, handle2id() returns 1. Does this mean the IDR
>entry at 0x80100000 is permanently leaked, while index 1 is erroneously 
>freed?
>If index 1 is already in use by an active auto-generated hash table, could
>freeing it here allow gen_new_htid() to issue the same handle again to a new
>hash table, causing collisions?

Re: [PATCH net v2 0/3] net/sched: u32_change() fixes

[Jamal Hadi Salim]

On Tue, Aug 18, 2026 at 3:48 AM Jagielski, Jedrzej
<jedrzej.jagielski@intel.com> wrote:
>
> >From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> >Sent: Monday, August 17, 2026 2:43 PM
> >
> >On 8/13/26 14:22, Jedrzej Jagielski wrote:
> >> sorry for spam, previous series get treated as incomplete so resending
> >>
> >> ALREADY ON NETDEV, SASHIKO RUN
> >
> >hehe
> >ofc, the above does no harm here
>
> :D
>
> >
> >for the series:
> >Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> >
> >--
> >
> >Sashiko has found one more *preexisting* bug, perhaps it could be
> >handled by one more fix in future series?:
> >https://sashiko.dev/#/patchset/20260813122242.1690024-1-jedrzej.jagielski%40intel.com
>
> yeah, i saw that and i thought about extending the series

If i am counting correctly there are now 3 more "preexisting" issues
that require follow-up. Are you going to do this?

Regarding the other legitimate issues Sashiko raised:
1) Please double-check everything an AI claims, especially if you used
one to generate these changes. It is very confident and _does_ make up
shit. Example, based on what i have seen being posted it will make up
fixes tags that look serious but dont exist;-> so you need to fix that
id.
Along the same lines: My assumption is you are actually testign this
against something like ixgbe and additionaly  a device that cant
offload? Adding a tested-by (if possible by someone else) would be a
plus. And a tdc test  would be appreciated as well; the skip_sw reject
path is only the user-visible half that seems possible to create a
test before when things just returned success to now returning an
error.

2) Can you please add the comment in the commit i asked before to
explain the user visible changes. In case my or Sashiko message wasnt
clear; before when you ran:
"tc filter add dev eth0 ... u32 ... skip_sw" it used to succeed (exit
0) and then the rule was installed in software with a skip_sw flag but
was used by neither s/w nor h/w.
After your changes the on a device/block that can't offload, that same
command _now fails_ with -EOPNOTSUPP or -EINVAL.

Something along the lines of:
Because skip_sw is now honored, "tc ... u32 ... skip_sw" add/replace
commands that previously succeeded (and were silently skipped by the
software path) will now return -EOPNOTSUPP/-EINVAL when the block or
driver cannot offload the filter.

basically you need to resend with a v3. Add at least one tdc test.

cheers,
jamal
> >
> > > @@ -938,14 +938,6 @@ static int u32_change(struct net *net, struct
> >sk_buff *in_skb,
> > >             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;
> > >             }
> >This isn't a bug introduced by this patch, but looking at u32_change(),
> >there
> >appears to be a pre-existing issue with how manual hash table handles are
> >tracked in the IDR.
> >When a user creates a u32 hash table with a manual handle (for example,
> >0x80100000), u32_change() inserts the full handle directly into
> >tp_c->handle_idr:
> >net/sched/cls_u32.c:u32_change() {
> >     ...
> >               } else {
> >                       err = idr_alloc_u32(&tp_c->handle_idr, ht, &handle,
> >                                           handle, GFP_KERNEL);
> >     ...
> >}
> >However, when destroying the node later, u32_destroy_hnode() removes the
> >mapped ID rather than the full handle:
> >net/sched/cls_u32.c:u32_destroy_hnode() {
> >     ...
> >               idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
> >     ...
> >}
> >For a handle like 0x80100000, handle2id() returns 1. Does this mean the IDR
> >entry at 0x80100000 is permanently leaked, while index 1 is erroneously
> >freed?
> >If index 1 is already in use by an active auto-generated hash table, could
> >freeing it here allow gen_new_htid() to issue the same handle again to a new
> >hash table, causing collisions?
>

Re: [PATCH net v2 1/3] net/sched: cls_u32: feed u32_replace_hw_knode() with correct set of flags

[Paolo Abeni]

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: feed u32_replace_hw_knode() with correct set of flags

The patch changes the two u32_replace_hw_knode() calls in u32_change() to
pass userflags (the TCA_CLS_FLAGS_* set parsed from TCA_U32_FLAGS) instead
of flags (the TCA_ACT_FLAGS_* set passed in as a function parameter), so
that tc_skip_sw() inside the helper looks at the bits userspace actually
set.

Two questions below about what becomes reachable once those flags are
correct.

> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index c297d7dbcf916..1374da97836eb 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -935,7 +935,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  
>  		u32_bind_filter(tp, new, base, tb);
>  
> -		err = u32_replace_hw_knode(tp, new, flags, extack);
> +		err = u32_replace_hw_knode(tp, new, userflags, extack);
>  		if (err) {
>  			u32_unbind_filter(tp, new, tb);
>  

[Severity: High]

Does this make the error path of the replace branch reachable from
userspace for the first time, and can it leave the old knode installed
while its hardware rule is gone?

Before the change, flags only carried TCA_ACT_FLAGS_* bits, so
tc_skip_sw(flags) in u32_replace_hw_knode() was false, tc_setup_cb_add()
ran with err_stop == false and could not return an error, and the
skip_sw && !IN_HW -EINVAL check in the helper was dead code.

With userflags, skip_sw is true for a skip_sw filter, tc_setup_cb_add()
propagates the first driver error, and u32_replace_hw_knode() takes:

net/sched/cls_u32.c:u32_replace_hw_knode() {
	err = tc_setup_cb_add(block, tp, TC_SETUP_CLSU32, &cls_u32, skip_sw,
			      &n->flags, &n->in_hw_count, true);
	if (err) {
		u32_remove_hw_knode(tp, n, NULL);
		return err;
	}
}

Here n is the replacement node, and u32_init_knode() copied the handle
from the old one:

	new->handle = n->handle;

so the TC_CLSU32_DELETE_KNODE emitted by u32_remove_hw_knode() names the
handle of the knode that is still linked in ht->ht[]. u32_change() then
only unbinds and frees new and returns the error, so the old knode stays
in the software list with TCA_CLS_FLAGS_SKIP_SW set, and u32_classify()
skips SKIP_SW knodes. Would that filter then match neither in software
nor in hardware, while userspace was told the command failed?

There also looks like accounting fallout on the same path.
u32_init_knode() copies the flags too:

	new->flags = n->flags;

so new->flags can carry TCA_CLS_FLAGS_IN_HW while new->in_hw_count is
still 0. The teardown then reaches
tc_setup_cb_destroy() -> tc_cls_offload_cnt_reset(), which decrements
purely on the inherited bit:

net/sched/cls_api.c:tc_cls_offload_cnt_reset() {
	spin_lock(&tp->lock);
	tcf_block_offload_dec(block, flags);
	*cnt = 0;
	spin_unlock(&tp->lock);
}

Can this decrement block->offloadcnt for a node that never incremented
it, and then have the old knode (whose flags still carry IN_HW)
decrement it a second time when it is eventually deleted, leaving
tcf_block_offload_in_use() permanently true and later binds of
non-offload-capable devices to the block failing with -EOPNOTSUPP?

For comparison, cls_flower builds fnew->flags only from the netlink
attributes rather than inheriting them from fold, so it does not have
the accounting half of this.

The later patches in this range only adjust ht_down refcounting, so this
error path is unchanged by them.

> @@ -1168,7 +1168,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		struct tc_u_knode __rcu **ins;
>  		struct tc_u_knode *pins;
>  
> -		err = u32_replace_hw_knode(tp, n, flags, extack);
> +		err = u32_replace_hw_knode(tp, n, userflags, extack);
>  		if (err)
>  			goto errunbind;
>  

[Severity: High]

This isn't a bug introduced by this patch, but now that the real
TCA_CLS_FLAGS_* set reaches these call sites, the TCA_CLS_FLAGS_SKIP_HW
bit is visible here and still nothing acts on it. Should these calls be
gated on tc_skip_hw()?

Neither helper looks at it. u32_replace_hw_knode() and
u32_replace_hw_hnode() only derive:

	bool skip_sw = tc_skip_sw(flags);

and the core does not filter on the classifier's behalf either.
tc_setup_cb_add() only checks the block state before dispatching:

net/sched/cls_api.c:tc_setup_cb_add() {
	/* Make sure all netdevs sharing this block are offload-capable. */
	if (block->nooffloaddevcnt && err_stop) {
		ok_count = -EOPNOTSUPP;
		goto err_unlock;
	}

	ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
}

The peer classifiers gate the call themselves, for example cls_flower:

	if (!tc_skip_hw(fnew->flags))
		err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack);

and cls_matchall:

	if (!tc_skip_hw(new->flags))
		err = mall_replace_hw_filter(tp, new, cookie, extack);

Does a "tc filter add ... u32 skip_hw ..." command therefore still get
pushed to TC_SETUP_CLSU32 drivers and pick up TCA_CLS_FLAGS_IN_HW plus
in_hw_count / block->offloadcnt increments, contradicting the documented
meaning of the flag in include/uapi/linux/pkt_cls.h?

The two consumers of the same flags field also disagree. On block
unbind, tcf_block_playback_offloads() -> u32_reoffload(add=false) does
honour the bit:

net/sched/cls_u32.c:u32_reoffload() {
			if (tc_skip_hw(n->flags))
				continue;
}

so a knode that was offloaded by the add path is skipped here, and its
IN_HW bit, in_hw_count and block->offloadcnt contribution are never
released. Can that leave tcf_block_offload_in_use() true forever and
reject later binds of non-offload-capable devices to that shared block
with -EOPNOTSUPP?

The same missing guard applies to the u32_replace_hw_hnode() call in the
TCA_U32_DIVISOR branch of u32_change(), which is likewise given
userflags with no skip_hw check.
-- 
This is an AI-generated review.

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

[Paolo Abeni]

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.

Re: [PATCH net v2 0/3] net/sched: u32_change() fixes

[Paolo Abeni]

On 8/18/26 9:46 AM, Jagielski, Jedrzej wrote:
>> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> 
>> Sent: Monday, August 17, 2026 2:43 PM
>>
>> On 8/13/26 14:22, Jedrzej Jagielski wrote:
>>> sorry for spam, previous series get treated as incomplete so resending
>>>
>>> ALREADY ON NETDEV, SASHIKO RUN
>>
>> hehe
>> ofc, the above does no harm here
> 
> :D
> 
>>
>> for the series:
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>
>> --
>>
>> Sashiko has found one more *preexisting* bug, perhaps it could be
>> handled by one more fix in future series?:
>> https://sashiko.dev/#/patchset/20260813122242.1690024-1-jedrzej.jagielski%40intel.com
WRT to sashiko, please note that there 2 public sashiko instances; the
above and:

https://netdev-ai.bots.linux.dev/sashiko/

both are feeded into out CI, but the most relevant one is the latter, as
it uses multiple models and feeds in the result from the former.

/P