* Re: [PATCH net] net/sched: cls_u32: take a reference on the hash table in u32_change()
2026-09-05 16:38 [PATCH net] net/sched: cls_u32: take a reference on the hash table in u32_change() Norbert Szetei
@ 2026-09-09 12:39 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 12:39 UTC (permalink / raw)
To: norbert
Cc: netdev, jhs, jiri, davem, edumazet, kuba, pabeni, horms,
xiyou.wangcong, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 0
- [High] The new pin in u32_change() (refcount_inc(&ht->refcnt) /
refcount_dec(&ht->refcnt) around u32_set_parms()) assumes every drop…
- [Medium] Commit message understates the reachability of the bug being
fixed.
--
net/sched: cls_u32: take a reference on the hash table in u32_change()
u32_change() holds the target tc_u_hnode in a local variable while
u32_set_parms() may drop rtnl to load an action module, so a concurrent
RTM_DELTFILTER can free it. The patch pins the hnode with
refcount_inc()/refcount_dec() around the u32_set_parms() call.
Two questions below, one on the changelog and one on the new pin itself.
> u32_change() keeps the target hash table in a local variable without
> taking a reference. u32_set_parms() may then drop rtnl to load an action
> module, and on a failed load returns without asking the caller to
> replay (-ENOENT). A concurrent RTM_DELTFILTER can free the table in
> that window, and u32_change() then removes the node id it reserved from
> the freed table.
[Severity: Medium]
Is the race really limited to the "failed load" case? tc_action_load_ops()
in net/sched/act_api.c drops rtnl in both outcomes:
net/sched/act_api.c:tc_action_load_ops() {
...
rtnl_unlock();
request_module("act_%s", act_name);
rtnl_lock();
...
if (a_o != NULL) {
module_put(a_o->owner);
return ERR_PTR(-EAGAIN);
}
#endif
NL_SET_ERR_MSG(extack, "Failed to load TC action module");
return ERR_PTR(-ENOENT);
}
In u32_change() any non-zero err from u32_set_parms() skips the
if (err == 0) block and falls through errunbind -> errout -> erridr, where
idr_remove(&ht->handle_idr, handle) dereferences ht. So the -EAGAIN path
(module present and loaded successfully, request replayed afterwards) also
touches ht after rtnl was dropped, and that is the common case on a normal
system.
Would it be worth describing the window as "u32_set_parms() may drop rtnl"
without tying it to -ENOENT, so backport triage sees the wider trigger?
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index a3e65c8cf29ef..0e9128f2f33e8 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -1180,8 +1180,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> }
> #endif
>
> + /* u32_set_parms() may drop rtnl; keep ht alive across it */
> + refcount_inc(&ht->refcnt);
> err = u32_set_parms(net, tp, n, tb, tca[TCA_RATE],
> flags, n->flags, extack);
> + refcount_dec(&ht->refcnt);
[Severity: High]
Does this pin hold when the hnode being pinned belongs to a different
tcf_proto?
The changelog states that a racing deletion "fails its
refcount_dec_if_one() and returns -EBUSY", which is true for u32_delete()
of a non-root hnode. But u32_destroy() drops the root hnode reference
unconditionally:
net/sched/cls_u32.c:u32_destroy() {
if (root_ht && refcount_dec_and_test(&root_ht->refcnt))
u32_destroy_hnode(tp, root_ht, extack);
...
}
tc_u_common is shared by all cls_u32 tcf_protos on the same qdisc/block,
and u32_change() resolves TCA_U32_HASH through the shared list without any
is_root or owner check:
net/sched/cls_u32.c:u32_change() {
...
ht = u32_lookup_ht(tp->data, TC_U32_HTID(htid));
if (!ht) {
NL_SET_ERR_MSG_MOD(extack, "Specified hash table not found");
return -EINVAL;
}
...
}
net/sched/cls_u32.c:u32_lookup_ht() {
for (ht = rtnl_dereference(tp_c->hlist);
ht;
ht = rtnl_dereference(ht->next))
if (ht->handle == handle)
break;
...
}
So a filter added at prio 2 with "ht 800:" can pin prio 1's root hnode.
If prio 1 is deleted in the rtnl-dropped window (RTM_DELTFILTER with
tcm_handle == 0 -> tcf_chain_tp_remove() + tcf_proto_put() ->
tcf_proto_destroy() -> u32_destroy()), the sequence looks like:
u32_change() refcount_inc() refcnt 1 -> 2
u32_destroy() refcount_dec_and_test() false refcnt 2 -> 1
u32_change() refcount_dec() refcnt 1 -> 0
Two things follow. First, u32_destroy_hnode() is skipped, so the hnode
stays linked in tp_c->hlist with no owner and u32_clear_hnode(),
u32_clear_hw_hnode(), idr_remove(&tp_c->handle_idr, ...), idr_destroy()
and kfree_rcu() never run.
Second, the trailing refcount_dec() takes the count 1 -> 0:
include/linux/refcount.h:__refcount_dec() {
int old = atomic_fetch_sub_release(1, &r->refs);
...
if (unlikely(old <= 1))
refcount_warn_saturate(r, REFCOUNT_DEC_LEAK);
}
That warns and sets REFCOUNT_SATURATED without freeing anything, and once
saturated no later refcount_dec_and_test() in u32_destroy() can reach zero:
net/sched/cls_u32.c:u32_destroy() {
...
if (refcount_dec_and_test(&ht->refcnt))
kfree_rcu(ht, rcu);
...
}
Can this leak the hnode (plus its idr and any knodes later inserted into
it) for the lifetime of the block, and splat under panic_on_warn? Would
handling the "we dropped the last reference" case, or re-looking up ht
after u32_set_parms() instead of carrying a borrowed pointer, avoid this?
As a smaller note on the same two lines: the reference is released before
the remaining ht dereferences it is meant to cover, namely
idr_remove(&ht->handle_idr, handle) at erridr and the
&ht->ht[TC_U32_HASH(handle)] insertion, so it relies on u32_set_parms()
always having re-acquired rtnl by the time it returns.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/79DB27AC-0FC0-42B2-A795-A8B585B73CFB%40doyensec.com
^ permalink raw reply [flat|nested] 2+ messages in thread