From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module Date: Tue, 7 Nov 2017 10:43:39 +0100 Message-ID: <20171107094339.GL9424@breakpoint.cc> References: <20171106105113.20476-1-fw@strlen.de> <20171106105113.20476-3-fw@strlen.de> <20171106124454.GI3165@worktop.lehotels.local> <20171107061156.GK9424@breakpoint.cc> <20171107091004.GI3326@worktop> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Westphal , netdev@vger.kernel.org To: Peter Zijlstra Return-path: Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:49768 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933592AbdKGJoP (ORCPT ); Tue, 7 Nov 2017 04:44:15 -0500 Content-Disposition: inline In-Reply-To: <20171107091004.GI3326@worktop> Sender: netdev-owner@vger.kernel.org List-ID: Peter Zijlstra 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.