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

On Tue, Aug 18, 2026 at 3:48 AM Jagielski, Jedrzej
<[email protected]> wrote:
>
> >From: Kitszel, Przemyslaw <[email protected]>
> >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 <[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
>
> 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?
>