Re: [PATCH net 1/2] net-shapers: clear hierarchy pointer and defer flush frees with RCU

[Paul Moses]

> 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