Re: [PATCH net v2 1/2] net/sched: cls_u32: fix duplicate handle when node ID pool is exhausted

On Fri, Aug 21, 2026 at 5:04 PM Jamal Hadi Salim <[email protected]> wrote:
>
> To: [email protected]
> Cc: Jamal Hadi Salim <[email protected]>
> Cc: Jiri Pirko <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Simon Horman <[email protected]>
> Cc: Cong Wang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Victor Nogueira <[email protected]>
>
> gen_new_kid() falls back to returning max (htid | 0xFFF) when both
> idr_alloc_u32() ranges are full, instead of reporting an error.
> u32_change() trusts that value and inserts a new knode with a handle
> that is already live in the hash table, breaking handle uniqueness
> within the table's node ID space.
>

Sashiko is right that using /tmp/u32_batch.txt may pose a security
threat. Better add it now as opposed to followup.
I will send a v3 and also add the minor change that was suggested by
sashiko for the extack messages.
The rest of the sashiko suggestions are followups.

pw-bot: cr

cheers,
jamal
> The handle was never reserved in ht->handle_idr, so every later error
> path that does idr_remove(&ht->handle_idr, handle) removes the
> reservation of a different, live knode, which is then reused — one
> failed add compounds into further duplicates.
>
> The 4095 limit is per (table, bucket) — ht->handle_idr is per hash
> table and the range is derived from htid (bucketid), so a table with
> divisor 256 can legitimately hold 256*4095 knodes.
>
> The sibling helper gen_new_htid() has the same silent in-band failure:
> it returns 0 when the tp_c handle pool (1..0x7FF) is full, and
> u32_init() publishes the root hash table with handle 0 without
> checking.  Two root tables with handle 0 alias in u32_lookup_ht(),
> allowing cross-tcf_proto knode add/lookup/delete.  Add the same
> exhaustion check that the divisor path already has.
>
> Return an error so u32_change() fails with ENOSPC/ENOMEM when the
> node ID space is exhausted, and so u32_init() fails with -ENOMEM
> when the hash table ID space is exhausted.
>
> Conditions to recreate the bug:
> - CONFIG_NET_SCHED=y, CONFIG_CLS_U32=y (or =m with module loaded)
> - Create a clsact qdisc on a device, then add 4095 u32 filters with
>   auto-generated handles to fill the node ID space for the root hash
>   table (single bucket). The 4096th auto-handle filter add triggers
>   the duplicate handle (fh 800::fff reused). Reachable at Level 2
>   (unshare -Urn, namespace-local CAP_NET_ADMIN).
> - For gen_new_htid: create 2047 u32 proto entries on the same block
>   to fill the tp_c handle pool, then create one more. The root table
>   gets handle 0 and aliases with other handle-0 root tables.
>
> Fixes: 7801db8aec95 ("net_sched: avoid generating same handle for u32 filters")
> Reported-by: [email protected]
> Tested-by: Victor Nogueira <[email protected]>
> Signed-off-by: Jamal Hadi Salim <[email protected]>
> ---
> v1 -> v2:
> - Commit message: removed incorrect "4095-knode cap" claim (knodes is a
>   liveness counter, not a limit; 4095 is per (table, bucket) via
>   ht->handle_idr/htid); reworded to "handle uniqueness within node ID
>   space".
> - Commit message: added IDR-desync paragraph (max handle never reserved
>   -> erridr removes other live knode's reservation -> reuse compounds).
> - Commit message: noted ENOSPC/ENOMEM both reachable (idr_get_free/
>   radix_tree_extend).
> - Fixes: e7614370d6f04 -> 7801db8aec95 (duplicate-handle bug predates
>   the IDR conversion; e761 only added the IDR-desync consequence).
> - Added NL_SET_ERR_MSG_MOD(extack, "Hash table node ID pool exhausted")
>   at both gen_new_kid() call sites; bare -ENOSPC -> "No space left on
>   device" was confusing.
> - Folded gen_new_htid() exhaustion check in u32_init() (Sashiko nipa
>   main-1-0): same silent in-band failure pattern — gen_new_htid()
>   returns 0 when tp_c handle pool full, u32_init() published root table
>   with handle 0 without checking, causing cross-tcf_proto aliasing.
> - Renewed tdc test (v1 tdc failed in long-running sweep, prompting v2).
> ---
>  net/sched/cls_u32.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index c297d7dbcf91..1a72817bd449 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -370,6 +370,10 @@ static int u32_init(struct tcf_proto *tp)
>
>         refcount_set(&root_ht->refcnt, 1);
>         root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
> +       if (root_ht->handle == 0) {
> +               kfree(root_ht);
> +               return -ENOMEM;
> +       }
>         root_ht->prio = tp->prio;
>         root_ht->is_root = true;
>         idr_init(&root_ht->handle_idr);
> @@ -695,16 +699,19 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
>         return ret;
>  }
>
> -static u32 gen_new_kid(struct tc_u_hnode *ht, u32 htid)
> +static u32 gen_new_kid(struct tc_u_hnode *ht, u32 htid, int *err)
>  {
>         u32 index = htid | 0x800;
>         u32 max = htid | 0xFFF;
>
> +       *err = 0;
> +
>         if (idr_alloc_u32(&ht->handle_idr, NULL, &index, max, GFP_KERNEL)) {
>                 index = htid + 1;
> -               if (idr_alloc_u32(&ht->handle_idr, NULL, &index, max,
> -                                GFP_KERNEL))
> -                       index = max;
> +               *err = idr_alloc_u32(&ht->handle_idr, NULL, &index, max,
> +                                    GFP_KERNEL);
> +               if (*err)
> +                       return 0;
>         }
>
>         return index;
> @@ -1079,7 +1086,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>                  * handle which is used to uniquely identify the match entry.
>                  */
>                 if (!TC_U32_NODE(handle)) {
> -                       handle = gen_new_kid(ht, htid);
> +                       handle = gen_new_kid(ht, htid, &err);
> +                       if (err) {
> +                               NL_SET_ERR_MSG_MOD(extack, "Hash table node ID pool exhausted");
> +                               return err;
> +                       }
>                 } else {
>                         handle = htid | TC_U32_NODE(handle);
>                         err = idr_alloc_u32(&ht->handle_idr, NULL, &handle,
> @@ -1091,7 +1102,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>                 /* The user did not give us a handle; lets just generate one
>                  * from the table's pool of nodeids.
>                  */
> -               handle = gen_new_kid(ht, htid);
> +               handle = gen_new_kid(ht, htid, &err);
> +               if (err) {
> +                       NL_SET_ERR_MSG_MOD(extack, "Hash table node ID pool exhausted");
> +                       return err;
> +               }
>         }
>
>         if (tb[TCA_U32_SEL] == NULL) {
> --
> 2.43.0
>