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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171107061156.GK9424@breakpoint.cc>
Date:   Tue, 7 Nov 2017 07:11:56 +0100
From:   Florian Westphal <fw@...len.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module

Peter Zijlstra <peterz@...radead.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?