Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module

[Florian Westphal <fw@strlen.de>]

Peter Zijlstra <peterz@infradead.org> wrote:
> > rtnetlink_rcv_msg:
> > 
> > 4406                         dumpit = READ_ONCE(handlers[type].dumpit);
> > 4407                         if (!dumpit)
> > 4408                                 goto err_unlock;
> > 4409                         owner = READ_ONCE(handlers[type].owner);
> 
> So what stops the CPU from hoisting this load before the dumpit load?

I was under impression READ_ONCE also includes rmb but I see i was
wrong.

> > I don't want dumpit function address to be visible before owner.
> > Does that make sense?
> 
> And no. That's insane, how can it ever observe an incomplete tab in the
> first place.
> 
> The problem is that __rtnl_register() and rtnl_unregister are broken.
> 
> __rtnl_register() publishes the tab before it initializes it; allowing
> people to observe the thing incomplete.
>
> Also, are we required to hold rtnl_lock() across __rtnl_register()? I'd
> hope so, otherwise what stops concurrent allocations and leaking of tab?

I don't think these ever acquired rtnl mutex.
Hostorically the rtnl callbacks were statically allocated and only ran
from initcalls.

Use of of kmalloc came later, and then use in modules.

> rtnl_unregister() should then RCU free the tab.

I do not think that will work since that will make it behave like
rtnl_unregister_all(), i.e. removes all callbacks of the family.

> None of that is happening, so what is that RCU stuff supposed to do?

Its supposed to delay rmmod until all places that are still executing a
registered callback are done.