Re: [PATCH net 1/2] net/sched: cls_u32: fix manual hash table handle IDR aliasing

Le ven. 18 sept. 2026 à 10:22, Jamal Hadi Salim <[email protected]> a écrit :
>
> On Fri, Sep 18, 2026 at 1:37 AM Alexandre Ferrieux
> <[email protected]> wrote:
> >
> > Le jeu. 17 sept. 2026 à 14:08, Simon Horman <[email protected]> a écrit :
> > >
> > > On Wed, Sep 16, 2026 at 06:01:14AM -0400, Jamal Hadi Salim wrote:
> > > > A u32 hash table created with an explicit handle ('tc filter add ...
> > > > handle 801: u32 divisor N') keys its IDR entry on the raw handle, while
> > > > the destroy paths free it under handle2id(handle). The two key domains
> > > > disagree for handles in the 0x800..0xFFF htid range:
> > > > handle2id() folds them back into the auto-allocated id space (1..0x7FF).
> > > >
> > > > A manual table therefore leaves its raw-keyed IDR entry unreachable on
> > > > delete (a permanent leak), and its delete can drop the idr entry of an
> > > > unrelated live auto table. A later auto allocation can then hand out a
> > > > handle that aliases the live manual table; u32_lookup_ht() first-match
> > > > routes lookups and TCA_U32_LINK for that htid to the wrong table.
> > > >
> > > > Key the divisor-path alloc on handle2id(handle) so allocation and
> > > > removal share one key domain. A manual handle that maps onto an id
> > > > already in use is rejected with -ENOSPC, and auto allocation skips ids
> > > > held by live manual tables.
> > > >
> > > > Conditions to recreate:
> > > >   ip link add test0 type dummy
> > > >   tc qdisc add dev test0 clsact
> > > >   tc filter add dev test0 ingress protocol ip pref 1 \
> > > >           handle 801: u32 divisor 16
> > > >   tc filter add dev test0 ingress protocol ip pref 2 u32 divisor 16
> > > >   tc -d filter show dev test0 ingress | grep 'fh 801:'
> > > >   # unpatched: two live tables with handle 0x80100000 (the pref 2 root
> > > >   # hnode is auto-allocated id 1); patched: the auto hnode takes id 2.
> > > >
> > > > Also tested with a poc with a live u32 table on the block, add/delete a manual
> > > > table 'handle 901: u32 divisor 1' twice; unpatched, the re-add fails with
> > > > -ENOSPC because the raw key leaked on the first delete.
> > > >
> > > > Fixes: 73af53d82076 ("net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.")
> > > > Reported-by: Sashiko (gemini + nipa) <[email protected]>
> > > > Closes: https://sashiko.dev/#/patchset/[email protected]
> > > > Reviewed-by: Victor Nogueira <[email protected]>
> > > > Tested-by: hybris <[email protected]>
> > > > Signed-off-by: Jamal Hadi Salim <[email protected]>
> > >
> > > Reviewed-by: Simon Horman <[email protected]>
> > >
> >
> > Hi Jamal, Simon,
> >
> > Thanks a lot for the fix.
> > One nit I'd like to pick is the "Fixes:" header.
> > Specifically, it mentions my patch:
> >
> >      Fixes: 73af53d82076 ("net: sched: cls_u32: Fix u32's systematic
> > failure to free IDR entries for hnodes.")
> >
> > However, what this does fix is *not* a regression introduced by the
> > above patch,
>
> This patch fixes the alloc/remove key-domain asymmetry introduced by
> 73af53d82076
> Prior to your patch, the alloc/remove key domain was symmetric; you
> forgot to convert the "else {" part in your conversion, which this
> patch fixes. The comment tries to explain this detail.
>
> cheers,
> jamal

Oh, I stand corrected. Thank you !

-Alex