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

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 <[email protected]>

--

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?