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

[Florian Westphal <fw@strlen.de>]

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);
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?