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:47:51 +0100 Message-ID: <20171107094751.GM9424@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> <20171107092458.GF3857@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]:49784 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756363AbdKGJs0 (ORCPT ); Tue, 7 Nov 2017 04:48:26 -0500 Content-Disposition: inline In-Reply-To: <20171107092458.GF3857@worktop> Sender: netdev-owner@vger.kernel.org List-ID: Peter Zijlstra wrote: > Something like the below would go some way toward sanitizing this stuff; > rcu_assign_pointer() is a store-release, meaning it happens after > everything coming before. > > Therefore, when you observe that tab (through rcu_dereference) you're > guaranteed to see the thing initialized. The memory ordering on the > consume side is through an address dependency; we need to have completed > the load of the tab pointer before we can compute the address of its > members and load from there, these are not things a CPU is allowed to > reorder (lets forget about Alpha). > > Quite possibly, if rtnl_unregister() is called from module unload, this > is broken; in that case we'd need something like: > > rcu_assign_pointer(rtnl_msg_handler[protocol], NULL); > /* > * Ensure nobody can still observe our old protocol handler > * before continuing to free the module that includes the > * functions called from it. > */ > synchronize_rcu(); > > --- > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 5ace48926b19..25391c7b9c5d 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -63,6 +63,7 @@ struct rtnl_link { > rtnl_doit_func doit; > rtnl_dumpit_func dumpit; > unsigned int flags; > + struct rcu_head rcu; > }; > > static DEFINE_MUTEX(rtnl_mutex); > @@ -172,14 +173,15 @@ int __rtnl_register(int protocol, int msgtype, > BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX); > msgindex = rtm_msgindex(msgtype); > > - tab = rcu_dereference_raw(rtnl_msg_handlers[protocol]); > - if (tab == NULL) { > - tab = kcalloc(RTM_NR_MSGTYPES, sizeof(*tab), GFP_KERNEL); > - if (tab == NULL) > - return -ENOBUFS; > + if (WARN_ONCE(rtnl_msg_handler[protocol], > + "Double registration for protocol: %d\n", protcol)) > + return -EEXIST; I would expect this to trigger all the time, due to rtnl_register(AF_INET, RTM_GETROUTE, ... rtnl_register(AF_INET, RTM_GETADDR, ... etc. > @@ -227,15 +231,13 @@ int rtnl_unregister(int protocol, int msgtype) > msgindex = rtm_msgindex(msgtype); > > rtnl_lock(); > - handlers = rtnl_dereference(rtnl_msg_handlers[protocol]); > + handlers = rtnl_msg_handlers[protocol]; > if (!handlers) { > rtnl_unlock(); > return -ENOENT; > } > - > - handlers[msgindex].doit = NULL; > - handlers[msgindex].dumpit = NULL; > - handlers[msgindex].flags = 0; > + rcu_assign_pointer(rtnl_msg_handler[protocol], NULL); > + kfree_rcu(handlers, rcu); This unregisters all handlers of "protocol" instead of "protocol:msgtype".