[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?