Re: [PATCH net] net/sched: cls_u32: take a reference on the hash table in u32_change()

[netdev-bot+sashiko@kernel.org]

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