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

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Nov 07, 2017 at 07:11:56AM +0100, Florian Westphal wrote:
> Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Nov 06, 2017 at 11:51:07AM +0100, Florian Westphal wrote:
> > > @@ -180,6 +164,12 @@ int __rtnl_register(int protocol, int msgtype,
> > >  		rcu_assign_pointer(rtnl_msg_handlers[protocol], tab);
> > >  	}
> > >  
> > > +	WARN_ON(tab[msgindex].owner && tab[msgindex].owner != owner);
> > > +
> > > +	tab[msgindex].owner = owner;
> > > +	/* make sure owner is always visible first */
> > > +	smp_wmb();
> > > +
> > >  	if (doit)
> > >  		tab[msgindex].doit = doit;
> > >  	if (dumpit)
> > 
> > > @@ -235,6 +279,9 @@ int rtnl_unregister(int protocol, int msgtype)
> > >  	handlers[msgindex].doit = NULL;
> > >  	handlers[msgindex].dumpit = NULL;
> > >  	handlers[msgindex].flags = 0;
> > > +	/* make sure we clear owner last */
> > > +	smp_wmb();
> > > +	handlers[msgindex].owner = NULL;
> > >  	rtnl_unlock();
> > >  
> > >  	return 0;
> > 
> > These wmb()'s don't make sense; and the comments are incomplete. What do
> > they pair with? Who cares about this ordering?
> 
> 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?

> 4410                 }
> ..
> 4417                 if (!try_module_get(owner))
> 4418                         err = -EPROTONOSUPPORT;
> 4419 
> 
> 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?

Also, rtnl_register() doesn't seen to employ rtnl_lock() and panic()
WTF?!

rtnl_unregister() should then RCU free the tab.

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