Messages in this thread Patch in this message |  | | Date | Mon, 16 Mar 2026 18:45:48 +0000 | | From | Paul Moses <> | | Subject | Re: [PATCH net 1/2] net-shapers: clear hierarchy pointer and defer flush frees with RCU |
| |
> This is not the right fix. The shaper hierarchy as a while is not under > RCU. The problem is that we take a ref on netdev and then lock it, > assuming that it's still alive. But it may have gotten unregistered in > the meantime. The correct fix is to check that the netdev is still > alive after we lock the binding or take RCU from the Netlink side.
Ok I see it now, I didn't care about anything except queue because it's the only path that affected both drivers. This is an entirely different issue.
1. net_shaper_nl_pre_doit() → net_shaper_ctx_setup() gets dev = netdev_get_by_index(...) (ref only, no alive check) 2. Before doit runs, unregister can do: - unlist_netdevice(dev) (dev.c:12388) - dev->reg_state = NETREG_UNREGISTERING 3. Doit then runs: - net_shaper_lock(binding) - continues without checking reg_state - may call ops->set/delete/group() on a dying device
Here's the flow of reported issue:
1) A userspace GET doit path does this:
net_shaper_nl_get_doit() -> rcu_read_lock() -> net_shaper_lookup() -> net_shaper_hierarchy() -> READ_ONCE(dev->net_shaper_hierarchy) -> xa_get_mark() / xa_load() -> dereference hierarchy->shapers -> rcu_read_unlock()
That can race with netdevice unregister teardown:
net_shaper_flush_netdev() -> net_shaper_flush() -> xa_for_each(...) { __xa_erase(...) kfree(cur) } -> kfree(hierarchy)
The problem is that readers walk the published hierarchy locklessly under an RCU read-side section, but teardown reclaims both the shapers and the hierarchy with plain kfree() rather than kfree_rcu().
2) The original flush path does this:
net_shaper_flush() -> hierarchy = net_shaper_hierarchy(binding) -> ... free shapers ... -> kfree(hierarchy) -> no WRITE_ONCE(dev->net_shaper_hierarchy, NULL)
So a later GET reader can still do:
net_shaper_hierarchy() -> return stale non-NULL pointer
and then walk the freed hierarchy through xa_* operations.
The only remaining issue I found after fully reviewing this is the dump path, but I have not been able to reproduce it so far:
- kfree_rcu() only protects readers that have already entered rcu_read_lock() - In the old net_shaper_nl_get_dumpit(), hierarchy was loaded before rcu_read_lock() - So this sequence is possible: 1. dump path reads the hierarchy pointer 2. gets preempted 3. teardown detaches the pointer and queues kfree_rcu() 4. the grace period ends and the object is freed 5. dump resumes, enters rcu_read_lock(), and dereferences the stale pointer
diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index ab0de415546d6..452557c52488b 100644 --- a/net/shaper/shaper.c +++ b/net/shaper/shaper.c @@ -779,11 +779,13 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb, /* Don't error out dumps performed before any set operation. */ binding = net_shaper_binding_from_ctx(ctx); + rcu_read_lock(); hierarchy = net_shaper_hierarchy(binding); - if (!hierarchy) + if (!hierarchy) { + rcu_read_unlock(); return 0; + } - rcu_read_lock(); for (; (shaper = xa_find(&hierarchy->shapers, &ctx->start_index, U32_MAX, XA_PRESENT)); ctx->start_index++) { ret = net_shaper_fill_one(skb, binding, shaper, info); -- 2.53.GIT
So I still have no more changes besides possibly the inclusion of this patch. Thanks, Paul
|  |