From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module Date: Tue, 7 Nov 2017 15:57:36 +0100 Message-ID: <20171107145736.GN3165@worktop.lehotels.local> 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> <20171107094751.GM9424@breakpoint.cc> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org To: Florian Westphal Return-path: Received: from merlin.infradead.org ([205.233.59.134]:33352 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934337AbdKGO5j (ORCPT ); Tue, 7 Nov 2017 09:57:39 -0500 Content-Disposition: inline In-Reply-To: <20171107094751.GM9424@breakpoint.cc> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Nov 07, 2017 at 10:47:51AM +0100, Florian Westphal wrote: > I would expect this to trigger all the time, due to > > rtnl_register(AF_INET, RTM_GETROUTE, ... > rtnl_register(AF_INET, RTM_GETADDR, ... Ah, sure, then something like so then... There's bound to be bugs there too, as I pretty much typed this without thinking, but it should show the idea. --- diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 5ace48926b19..de1336775602 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); @@ -127,7 +128,7 @@ bool lockdep_rtnl_is_held(void) EXPORT_SYMBOL(lockdep_rtnl_is_held); #endif /* #ifdef CONFIG_PROVE_LOCKING */ -static struct rtnl_link __rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1]; +static struct rtnl_link __rcu **rtnl_msg_handlers[RTNL_FAMILY_MAX + 1]; static refcount_t rtnl_msg_handlers_ref[RTNL_FAMILY_MAX + 1]; static inline int rtm_msgindex(int msgtype) @@ -144,6 +145,23 @@ static inline int rtm_msgindex(int msgtype) return msgindex; } +static struct rtnl_link *rtnl_get_link(int protocol, int msgtype) +{ + struct rtnl_link **tab; + + if (protocol >= ARRAY_SIZE(rtnl_msg_handlers)) + protocol = PF_UNSPEC; + + tab = rcu_dereference(rtnl_msg_handlers[protocol]); + if (!tab) { + tab = rcu_dereference(rtnl_msg_handlers[PF_UNSPECl]); + if (!tab) + return NULL; + } + + return rcu_dereference(handlers[rtm_msgindex(msgtype)]); +} + /** * __rtnl_register - Register a rtnetlink message type * @protocol: Protocol family or PF_UNSPEC @@ -166,28 +184,39 @@ int __rtnl_register(int protocol, int msgtype, rtnl_doit_func doit, rtnl_dumpit_func dumpit, unsigned int flags) { - struct rtnl_link *tab; + struct rtnl_link **tab, *link; int msgindex; + int ret = -ENOBUFS; BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX); msgindex = rtm_msgindex(msgtype); - tab = rcu_dereference_raw(rtnl_msg_handlers[protocol]); + rtnl_lock(); + tab = rtnl_msg_handlers[protocol]; if (tab == NULL) { - tab = kcalloc(RTM_NR_MSGTYPES, sizeof(*tab), GFP_KERNEL); - if (tab == NULL) - return -ENOBUFS; + tab = kcalloc(RTM_NR_MSGTYPES, sizeof(void *), GFP_KERNEL); + if (!tab) + goto unlock; + /* ensures we see the 0 stores */ rcu_assign_pointer(rtnl_msg_handlers[protocol], tab); } - if (doit) - tab[msgindex].doit = doit; - if (dumpit) - tab[msgindex].dumpit = dumpit; - tab[msgindex].flags |= flags; + WARN_ONCE(tab[msgindex], "Double allocated rtnl(%d:%d)\n", protocol, msgtype); + link = kzalloc(sizeof(struct rtnl_link), GFP_KERNEL); + if (!link) + goto unlock; - return 0; + link->doit = doit; + link->dumpit = dumpit; + link->flags |= flags; + /* publish protocol:msgtype */ + rcu_assign_pointer(tab[msgindex], link); + ret = 0; +unlock: + rtnl_unlock(); + + return ret; } EXPORT_SYMBOL_GPL(__rtnl_register); @@ -220,22 +249,22 @@ EXPORT_SYMBOL_GPL(rtnl_register); */ int rtnl_unregister(int protocol, int msgtype) { - struct rtnl_link *handlers; + struct rtnl_link **tab, *link; int msgindex; BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX); msgindex = rtm_msgindex(msgtype); rtnl_lock(); - handlers = rtnl_dereference(rtnl_msg_handlers[protocol]); - if (!handlers) { + tab = rtnl_dereference(rtnl_msg_handlers[protocol]); + if (!tab) { rtnl_unlock(); return -ENOENT; } - handlers[msgindex].doit = NULL; - handlers[msgindex].dumpit = NULL; - handlers[msgindex].flags = 0; + link = tab[msgindex]; + rcu_assign_pointer(tab[msgindex], NULL); + kfree_rcu(link, rcu); rtnl_unlock(); return 0; @@ -251,13 +280,22 @@ EXPORT_SYMBOL_GPL(rtnl_unregister); */ void rtnl_unregister_all(int protocol) { - struct rtnl_link *handlers; + struct rtnl_link **tab, *link; + int msgindex; BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX); rtnl_lock(); - handlers = rtnl_dereference(rtnl_msg_handlers[protocol]); + tab = rtnl_msg_handlers[protocol]; RCU_INIT_POINTER(rtnl_msg_handlers[protocol], NULL); + for (msgindex = 0; msgindex < RTM_NR_MSGTYPES; msgindex++) { + link = tab[msgindex]; + if (!link) + continue; + + rcu_assign_pointer(tab[msgindex], NULL); + kfree_rcu(link, rcu); + } rtnl_unlock(); synchronize_net(); @@ -2830,17 +2868,17 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb) for (idx = 1; idx <= RTNL_FAMILY_MAX; idx++) { int type = cb->nlh->nlmsg_type-RTM_BASE; - struct rtnl_link *handlers; + struct rtnl_link *link; rtnl_dumpit_func dumpit; if (idx < s_idx || idx == PF_PACKET) continue; - handlers = rtnl_dereference(rtnl_msg_handlers[idx]); - if (!handlers) + link = rtnl_get_link(idx, type); + if (!link) continue; - dumpit = READ_ONCE(handlers[type].dumpit); + dumpit = link->dumpit; if (!dumpit) continue; @@ -4155,7 +4193,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, struct netlink_ext_ack *extack) { struct net *net = sock_net(skb->sk); - struct rtnl_link *handlers; + struct rtnl_link *link; int err = -EOPNOTSUPP; rtnl_doit_func doit; unsigned int flags; @@ -4179,32 +4217,19 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (family >= ARRAY_SIZE(rtnl_msg_handlers)) - family = PF_UNSPEC; - rcu_read_lock(); - handlers = rcu_dereference(rtnl_msg_handlers[family]); - if (!handlers) { - family = PF_UNSPEC; - handlers = rcu_dereference(rtnl_msg_handlers[family]); - } - if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) { struct sock *rtnl; rtnl_dumpit_func dumpit; u16 min_dump_alloc = 0; - dumpit = READ_ONCE(handlers[type].dumpit); - if (!dumpit) { - family = PF_UNSPEC; - handlers = rcu_dereference(rtnl_msg_handlers[PF_UNSPEC]); - if (!handlers) - goto err_unlock; - - dumpit = READ_ONCE(handlers[type].dumpit); - if (!dumpit) + link = rtnl_get_link(family, type); + if (!link || !link->dumpit) { + link = rtnl_get_link(PF_UNSPEC, type); + if (!link || !link->dumpit) goto err_unlock; } + dumpit = link->dumpit; refcount_inc(&rtnl_msg_handlers_ref[family]); @@ -4225,33 +4250,36 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, return err; } - doit = READ_ONCE(handlers[type].doit); - if (!doit) { + link = rtnl_get_link(family, type); + if (!link || !link->doit) { family = PF_UNSPEC; - handlers = rcu_dereference(rtnl_msg_handlers[family]); + link = rtnl_get_link(PF_UNSPEC, type); + if (!link || !link->doit) + goto out_unlock; } - flags = READ_ONCE(handlers[type].flags); + flags = link->flags; if (flags & RTNL_FLAG_DOIT_UNLOCKED) { refcount_inc(&rtnl_msg_handlers_ref[family]); - doit = READ_ONCE(handlers[type].doit); + doit = link->oit; rcu_read_unlock(); if (doit) err = doit(skb, nlh, extack); refcount_dec(&rtnl_msg_handlers_ref[family]); return err; } - rcu_read_unlock(); rtnl_lock(); - handlers = rtnl_dereference(rtnl_msg_handlers[family]); - if (handlers) { - doit = READ_ONCE(handlers[type].doit); - if (doit) - err = doit(skb, nlh, extack); - } + link = rtnl_get_link(family, type); + if (link && link->doit) + err = link->doit(skb, nlh, extack); rtnl_unlock(); + + return err; + +out_unlock: + rcu_read_unlock(); return err; err_unlock: