rtnetlink: rework handler (un)registering

PATCH In review Florian Westphal 3 participants 10 messages Dec 2, 2017 – Dec 4, 2017

Subsystems (7)

Networking Network drivers IPv4/IPv6 Ethernet bridge +3 more subsystems

lore

Share & tools

lore

Florian Westphal

author

2017-12-02 20:44 UTC #0 lore raw

cover rtnetlink: rework handler (un)registering

Peter Zijlstra reported (referring to commit 019a316992ee0d983, "rtnetlink: add reference counting to prevent module unload while dump is in progress"):

1) it not in fact a refcount, so using refcount_t is silly
2) there is a distinct lack of memory barriers, so we can easily observe the decrement while the msg_handler is still in progress.
3) waiting with a schedule()/yield() loop is complete crap and subject life-locks, imagine doing that rtnl_unregister_all() from a RT task.

In ancient times rtnetlink exposed a statically-sized table with preset doit/dumpit handlers to be called for a protocol/type pair.

Later the rtnl_register interface was added and the table was allocated on demand. Eventually these were also used by modules.

Problem is that nothing prevents module unload while a netlink dump is in progress. netlink dumps can be span multiple recv calls and netlink core saves the to-be-repeated dumper address for later invocation.

To prevent rmmod the netlink core expects callers to pass in the owning module so a reference can be taken.

So far rtnetlink wasn't doing this, add new interface to pass THIS_MODULE. Moreover, when converting parts of the rtnetlink handling to rcu this code gained way too many READ_ONCE spots, remove them and the extra refcounting.

Take a module reference when running dumpit and doit callbacks and never alter content of rtnl_link structures after they have been published via rcu_assign_pointer.

Based partially on earlier patch from Peter.

 include/net/rtnetlink.h |    4 
 net/bridge/br_mdb.c     |    6 -
 net/can/gw.c            |   14 +-
 net/core/rtnetlink.c    |  270 ++++++++++++++++++++++++++++++------------------
 net/decnet/dn_dev.c     |    9 +
 net/decnet/dn_fib.c     |    6 -
 net/decnet/dn_route.c   |    8 -
 net/ipv6/addrconf.c     |   44 +++++--
 net/ipv6/addrlabel.c    |   13 +-
 net/ipv6/ip6_fib.c      |    4 
 net/ipv6/route.c        |   20 ++-
 net/mpls/af_mpls.c      |   15 +-
 net/phonet/pn_netlink.c |   21 ++-
 net/qrtr/qrtr.c         |    8 +
 14 files changed, 282 insertions(+), 160 deletions(-)

Florian Westphal

author

2017-12-02 20:45 UTC #1 lore raw

PATCH 1/4 net: rtnetlink: use rcu to free rtnl message handlers

rtnetlink is littered with READ_ONCE() because we can have read accesses while another cpu can write to the structure we're reading by (un)registering doit or dumpit handlers.

This patch changes this so that (un)registering cpu allocates a new structure and then publishes it via rcu_assign_pointer, i.e. once another cpu can see such pointer no modifications will occur anymore.

based on initial patch from Peter Zijlstra.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

---
 net/core/rtnetlink.c | 154 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 53 deletions(-)

net/core/rtnetlink.c +101 ~~-53~~

index dabba2a91fc8..ff292d3f2c41 100644
@@ -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,20 @@ 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(rtnl_msg_handlers[protocol]);
+	if (!tab)
+		tab = rcu_dereference_rtnl(rtnl_msg_handlers[PF_UNSPEC]);
+
+	return tab[msgtype];
+}
+
 /**
  * __rtnl_register - Register a rtnetlink message type
  * @protocol: Protocol family or PF_UNSPEC

@@ -166,28 +181,52 @@ 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, *old;
 	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);
 	}
 
+	old = rtnl_dereference(tab[msgindex]);
+	if (old) {
+		link = kmemdup(old, sizeof(*old), GFP_KERNEL);
+		if (!link)
+			goto unlock;
+	} else {
+		link = kzalloc(sizeof(*link), GFP_KERNEL);
+		if (!link)
+			goto unlock;
+	}
+
+	WARN_ON(doit && link->doit && link->doit != doit);
 	if (doit)
-		tab[msgindex].doit = doit;
+		link->doit = doit;
+	WARN_ON(dumpit && link->dumpit && link->dumpit != dumpit);
 	if (dumpit)
-		tab[msgindex].dumpit = dumpit;
-	tab[msgindex].flags |= flags;
+		link->dumpit = dumpit;
 
-	return 0;
+	link->flags |= flags;
+
+	/* publish protocol:msgtype */
+	rcu_assign_pointer(tab[msgindex], link);
+	ret = 0;
+	if (old)
+		kfree_rcu(old, rcu);
+unlock:
+	rtnl_unlock();
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__rtnl_register);

@@ -220,24 +259,25 @@ 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);
 	rtnl_unlock();
 
+	kfree_rcu(link, rcu);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister);

@@ -251,20 +291,29 @@ 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();
 
 	while (refcount_read(&rtnl_msg_handlers_ref[protocol]) > 1)
 		schedule();
-	kfree(handlers);
+	kfree(tab);
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister_all);

@@ -2973,18 +3022,26 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
 		s_idx = 1;
 
 	for (idx = 1; idx <= RTNL_FAMILY_MAX; idx++) {
+		struct rtnl_link **tab;
 		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)
+		if (type < 0 || type >= RTM_NR_MSGTYPES)
 			continue;
 
-		dumpit = READ_ONCE(handlers[type].dumpit);
+		tab = rcu_dereference_rtnl(rtnl_msg_handlers[idx]);
+		if (!tab)
+			continue;
+
+		link = tab[type];
+		if (!link)
+			continue;
+
+		dumpit = link->dumpit;
 		if (!dumpit)
 			continue;

@@ -4314,7 +4371,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;

@@ -4338,32 +4395,20 @@ 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) {
+		link = rtnl_get_link(family, type);
+		if (!link || !link->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)
 				goto err_unlock;
 		}
+		dumpit = link->dumpit;
 
 		refcount_inc(&rtnl_msg_handlers_ref[family]);

@@ -4384,33 +4429,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->doit;
 		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:

Signature · 1 line

2.13.6

Florian Westphal

author

2017-12-02 20:45 UTC #2 lore raw

PATCH 2/4 rtnetlink: get reference on module before invoking handlers

In reply to Florian Westphal

Add yet another rtnl_register function. It will be used by modules that can be removed.

The passed module struct is used to prevent module unload while a netlink dump is in progress or when a DOIT_UNLOCKED doit callback is called.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

---
 include/net/rtnetlink.h |   2 +
 net/core/rtnetlink.c    | 113 +++++++++++++++++++++++++++++++++---------------
 2 files changed, 80 insertions(+), 35 deletions(-)

include/net/rtnetlink.h +2 -0

index ead018744ff5..e326b3f9eb5f 100644
@@ -17,6 +17,8 @@ int __rtnl_register(int protocol, int msgtype,
 		    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
+int rtnl_register_module(struct module *owner, int protocol, int msgtype,
+			 rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_unregister(int protocol, int msgtype);
 void rtnl_unregister_all(int protocol);

net/core/rtnetlink.c +78 ~~-35~~

199 lines not on this page · raw message

Signature · 1 line

2.13.6

Florian Westphal

author

2017-12-02 20:45 UTC #3 lore raw

PATCH 3/4 net: use rtnl_register_module where needed

In reply to Florian Westphal

all of these can be compiled as a module, so use new _module version to make sure module can no longer be removed while callback/dump is in use.

Signed-off-by Florian Westphal <[email protected]>

---
 net/bridge/br_mdb.c     |  6 +++---
 net/can/gw.c            | 14 ++++++++++----
 net/decnet/dn_dev.c     |  9 ++++++---
 net/decnet/dn_fib.c     |  6 ++++--
 net/decnet/dn_route.c   |  8 ++++----
 net/mpls/af_mpls.c      | 15 +++++++++------
 net/phonet/pn_netlink.c | 21 +++++++++++++--------
 net/qrtr/qrtr.c         |  8 ++++++--
 8 files changed, 55 insertions(+), 32 deletions(-)

net/bridge/br_mdb.c +3 -3

index b0f4c734900b..6d9f48bd374a 100644
@@ -760,9 +760,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 void br_mdb_init(void)
 {
-	rtnl_register(PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, 0);
-	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, 0);
-	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, 0);
 }
 
 void br_mdb_uninit(void)

net/can/gw.c +10 -4

index 73a02af4b5d7..398dd0395ad9 100644
@@ -1014,6 +1014,8 @@ static struct pernet_operations cangw_pernet_ops = {
 
 static __init int cgw_module_init(void)
 {
+	int ret;
+
 	/* sanitize given module parameter */
 	max_hops = clamp_t(unsigned int, max_hops, CGW_MIN_HOPS, CGW_MAX_HOPS);

@@ -1031,15 +1033,19 @@ static __init int cgw_module_init(void)
 	notifier.notifier_call = cgw_notifier;
 	register_netdevice_notifier(&notifier);
 
-	if (__rtnl_register(PF_CAN, RTM_GETROUTE, NULL, cgw_dump_jobs, 0)) {
+	ret = rtnl_register_module(THIS_MODULE, PF_CAN, RTM_GETROUTE,
+				   NULL, cgw_dump_jobs, 0);
+	if (ret) {
 		unregister_netdevice_notifier(&notifier);
 		kmem_cache_destroy(cgw_cache);
 		return -ENOBUFS;
 	}
 
-	/* Only the first call to __rtnl_register can fail */
-	__rtnl_register(PF_CAN, RTM_NEWROUTE, cgw_create_job, NULL, 0);
-	__rtnl_register(PF_CAN, RTM_DELROUTE, cgw_remove_job, NULL, 0);
+	/* Only the first call to rtnl_register_module can fail */
+	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_NEWROUTE,
+			     cgw_create_job, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_DELROUTE,
+			     cgw_remove_job, NULL, 0);
 
 	return 0;
 }

net/decnet/dn_dev.c +6 -3

index 9153247dad28..d1885cf59319 100644
@@ -1418,9 +1418,12 @@ void __init dn_dev_init(void)
 
 	dn_dev_devices_on();
 
-	rtnl_register(PF_DECnet, RTM_NEWADDR, dn_nl_newaddr, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_DELADDR, dn_nl_deladdr, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_GETADDR, NULL, dn_nl_dump_ifaddr, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_NEWADDR,
+			     dn_nl_newaddr, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_DELADDR,
+			     dn_nl_deladdr, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETADDR,
+			     NULL, dn_nl_dump_ifaddr, 0);
 
 	proc_create("decnet_dev", S_IRUGO, init_net.proc_net, &dn_dev_seq_fops);

net/decnet/dn_fib.c +4 -2

index b37a1b833c77..fce94cbd4378 100644
@@ -792,8 +792,10 @@ void __init dn_fib_init(void)
 
 	register_dnaddr_notifier(&dn_fib_dnaddr_notifier);
 
-	rtnl_register(PF_DECnet, RTM_NEWROUTE, dn_fib_rtm_newroute, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_DELROUTE, dn_fib_rtm_delroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_NEWROUTE,
+			     dn_fib_rtm_newroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_DELROUTE,
+			     dn_fib_rtm_delroute, NULL, 0);
 }

net/decnet/dn_route.c +4 -4

index 4b3ca70be723..73160d4aebbe 100644
@@ -1923,11 +1923,11 @@ void __init dn_route_init(void)
 		    &dn_rt_cache_seq_fops);
 
 #ifdef CONFIG_DECNET_ROUTER
-	rtnl_register(PF_DECnet, RTM_GETROUTE, dn_cache_getroute,
-		      dn_fib_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETROUTE,
+			     dn_cache_getroute, dn_fib_dump, 0);
 #else
-	rtnl_register(PF_DECnet, RTM_GETROUTE, dn_cache_getroute,
-		      dn_cache_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETROUTE,
+			     dn_cache_getroute, dn_cache_dump, 0);
 #endif
 }

net/mpls/af_mpls.c +9 -6

index 8ca9915befc8..5dce8336d33f 100644
@@ -2510,12 +2510,15 @@ static int __init mpls_init(void)
 
 	rtnl_af_register(&mpls_af_ops);
 
-	rtnl_register(PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0);
-	rtnl_register(PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0);
-	rtnl_register(PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes,
-		      0);
-	rtnl_register(PF_MPLS, RTM_GETNETCONF, mpls_netconf_get_devconf,
-		      mpls_netconf_dump_devconf, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_NEWROUTE,
+			     mpls_rtm_newroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_DELROUTE,
+			     mpls_rtm_delroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETROUTE,
+			     mpls_getroute, mpls_dump_routes, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETNETCONF,
+			     mpls_netconf_get_devconf,
+			     mpls_netconf_dump_devconf, 0);
 	err = ipgre_tunnel_encap_add_mpls_ops();
 	if (err)
 		pr_err("Can't add mpls over gre tunnel ops\n");

net/phonet/pn_netlink.c +13 -8

index da754fc926e7..871eaf2cb85e 100644
@@ -299,16 +299,21 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 int __init phonet_netlink_register(void)
 {
-	int err = __rtnl_register(PF_PHONET, RTM_NEWADDR, addr_doit,
-				  NULL, 0);
+	int err = rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWADDR,
+				       addr_doit, NULL, 0);
 	if (err)
 		return err;
 
-	/* Further __rtnl_register() cannot fail */
-	__rtnl_register(PF_PHONET, RTM_DELADDR, addr_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0);
-	__rtnl_register(PF_PHONET, RTM_NEWROUTE, route_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_DELROUTE, route_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_GETROUTE, NULL, route_dumpit, 0);
+	/* Further rtnl_register_module() cannot fail */
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELADDR,
+			     addr_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETADDR,
+			     NULL, getaddr_dumpit, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWROUTE,
+			     route_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELROUTE,
+			     route_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETROUTE,
+			     NULL, route_dumpit, 0);
 	return 0;
 }

net/qrtr/qrtr.c +6 -2

index 77ab05e23001..5fb3929e3d7d 100644
@@ -1116,9 +1116,13 @@ static int __init qrtr_proto_init(void)
 		return rc;
 	}
 
-	rtnl_register(PF_QIPCRTR, RTM_NEWADDR, qrtr_addr_doit, NULL, 0);
+	rc = rtnl_register_module(THIS_MODULE, PF_QIPCRTR, RTM_NEWADDR, qrtr_addr_doit, NULL, 0);
+	if (rc) {
+		sock_unregister(qrtr_family.family);
+		proto_unregister(&qrtr_proto);
+	}
 
-	return 0;
+	return rc;
 }
 postcore_initcall(qrtr_proto_init);

Signature · 1 line

2.13.6

David Ahern

2017-12-04 02:56 UTC #4 lore raw

Re: [PATCH net-next 3/4] net: use rtnl_register_module where needed

Quoted 5 lines · Florian Westphal

all of these can be compiled as a module, so use new _module version to make sure module can no longer be removed while callback/dump is in use.
Signed-off-by Florian Westphal <[email protected]>

Quoted 10 lines

---
 net/bridge/br_mdb.c     |  6 +++---
 net/can/gw.c            | 14 ++++++++++----
 net/decnet/dn_dev.c     |  9 ++++++---
 net/decnet/dn_fib.c     |  6 ++++--
 net/decnet/dn_route.c   |  8 ++++----
 net/mpls/af_mpls.c      | 15 +++++++++------
 net/phonet/pn_netlink.c | 21 +++++++++++++--------
 net/qrtr/qrtr.c         |  8 ++++++--
 8 files changed, 55 insertions(+), 32 deletions(-)

ipv6 is missing from the list. Is the thought that IPv6 can be compiled as a module but can't be removed?

David Ahern

2017-12-04 02:57 UTC #5 lore raw

Re: [PATCH net-next 3/4] net: use rtnl_register_module where needed

Quoted 17 lines · David Ahern

Quoted 5 lines · Florian Westphal
all of these can be compiled as a module, so use new _module version to make sure module can no longer be removed while callback/dump is in use.
Signed-off-by Florian Westphal <[email protected]>
Quoted 10 lines
---
 net/bridge/br_mdb.c     |  6 +++---
 net/can/gw.c            | 14 ++++++++++----
 net/decnet/dn_dev.c     |  9 ++++++---
 net/decnet/dn_fib.c     |  6 ++++--
 net/decnet/dn_route.c   |  8 ++++----
 net/mpls/af_mpls.c      | 15 +++++++++------
 net/phonet/pn_netlink.c | 21 +++++++++++++--------
 net/qrtr/qrtr.c         |  8 ++++++--
 8 files changed, 55 insertions(+), 32 deletions(-)

Quoted 3 lines

ipv6 is missing from the list. Is the thought that IPv6 can be compiled as a module but can't be removed?

nm. ipv6 is converted to use _module in patch 4.

Florian Westphal

author

2017-12-02 20:45 UTC #6 lore raw

PATCH 4/4 rtnetlink: remove __rtnl_register

In reply to Florian Westphal

This removes __rtnl_register and switches callers to either rtnl_register or rtnl_register_module.

Also, rtnl_register() will now print an error if memory allocation failed rather than panic the kernel.

Signed-off-by Florian Westphal <[email protected]>

---
 include/net/rtnetlink.h |  2 --
 net/core/rtnetlink.c    | 33 ++++++++-------------------------
 net/ipv6/addrconf.c     | 44 ++++++++++++++++++++++++++++++--------------
 net/ipv6/addrlabel.c    | 13 ++++++-------
 net/ipv6/ip6_fib.c      |  4 ++--
 net/ipv6/route.c        | 20 +++++++++++++++-----
 6 files changed, 61 insertions(+), 55 deletions(-)

include/net/rtnetlink.h +0 -2

index e326b3f9eb5f..14b6b3af8918 100644
@@ -13,8 +13,6 @@ enum rtnl_link_flags {
 	RTNL_FLAG_DOIT_UNLOCKED = 1,
 };
 
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_register_module(struct module *owner, int protocol, int msgtype,

net/core/rtnetlink.c +8 ~~-25~~

index de6390365c90..fb2d61df1e2f 100644
@@ -238,7 +238,7 @@ int rtnl_register_module(struct module *owner,
 EXPORT_SYMBOL_GPL(rtnl_register_module);
 
 /**
- * __rtnl_register - Register a rtnetlink message type
+ * rtnl_register - Register a rtnetlink message type
  * @protocol: Protocol family or PF_UNSPEC
  * @msgtype: rtnetlink message type
  * @doit: Function pointer called for each request message

@@ -252,35 +252,18 @@ EXPORT_SYMBOL_GPL(rtnl_register_module);
  * The special protocol family PF_UNSPEC may be used to define fallback
  * function pointers for the case when no entry for the specific protocol
  * family exists.
- *
- * Returns 0 on success or a negative error code.
- */
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func doit, rtnl_dumpit_func dumpit,
-		    unsigned int flags)
-{
-	return rtnl_register_internal(NULL, protocol, msgtype,
-				      doit, dumpit, flags);
-}
-EXPORT_SYMBOL_GPL(__rtnl_register);
-
-/**
- * rtnl_register - Register a rtnetlink message type
- *
- * Identical to __rtnl_register() but panics on failure. This is useful
- * as failure of this function is very unlikely, it can only happen due
- * to lack of memory when allocating the chain to store all message
- * handlers for a protocol. Meant for use in init functions where lack
- * of memory implies no sense in continuing.
  */
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func doit, rtnl_dumpit_func dumpit,
 		   unsigned int flags)
 {
-	if (__rtnl_register(protocol, msgtype, doit, dumpit, flags) < 0)
-		panic("Unable to register rtnetlink message handler, "
-		      "protocol = %d, message type = %d\n",
-		      protocol, msgtype);
+	int err;
+
+	err = rtnl_register_internal(NULL, protocol, msgtype, doit, dumpit,
+				     flags);
+	if (err)
+		pr_err("Unable to register rtnetlink message handler, "
+		       "protocol = %d, message type = %d\n", protocol, msgtype);
 }
 EXPORT_SYMBOL_GPL(rtnl_register);

net/ipv6/addrconf.c +30 ~~-14~~

57 lines not on this page · raw message

net/ipv6/addrlabel.c +6 -7

index 00e1f8ee08f8..303fcce5beef 100644
@@ -549,11 +549,10 @@ static int ip6addrlbl_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 void __init ipv6_addr_label_rtnl_register(void)
 {
-	__rtnl_register(PF_INET6, RTM_NEWADDRLABEL, ip6addrlbl_newdel,
-			NULL, RTNL_FLAG_DOIT_UNLOCKED);
-	__rtnl_register(PF_INET6, RTM_DELADDRLABEL, ip6addrlbl_newdel,
-			NULL, RTNL_FLAG_DOIT_UNLOCKED);
-	__rtnl_register(PF_INET6, RTM_GETADDRLABEL, ip6addrlbl_get,
-			ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_NEWADDRLABEL, ip6addrlbl_newdel,
+		      NULL, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_DELADDRLABEL, ip6addrlbl_newdel,
+		      NULL, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_GETADDRLABEL, ip6addrlbl_get,
+		      ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
 }
-

net/ipv6/ip6_fib.c +2 -2

index c43cbaedfa35..a64d559fa513 100644
@@ -2142,8 +2142,8 @@ int __init fib6_init(void)
 	if (ret)
 		goto out_kmem_cache_create;
 
-	ret = __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
-			      0);
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETROUTE, NULL,
+				   inet6_dump_fib, 0);
 	if (ret)
 		goto out_unregister_subsys;

net/ipv6/route.c +15 -5

index 46fd53b268da..b3f4d19b3ca5 100644
@@ -4772,11 +4772,20 @@ int __init ip6_route_init(void)
 	if (ret)
 		goto fib6_rules_init;
 
-	ret = -ENOBUFS;
-	if (__rtnl_register(PF_INET6, RTM_NEWROUTE, inet6_rtm_newroute, NULL, 0) ||
-	    __rtnl_register(PF_INET6, RTM_DELROUTE, inet6_rtm_delroute, NULL, 0) ||
-	    __rtnl_register(PF_INET6, RTM_GETROUTE, inet6_rtm_getroute, NULL,
-			    RTNL_FLAG_DOIT_UNLOCKED))
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_NEWROUTE,
+				   inet6_rtm_newroute, NULL, 0);
+	if (ret < 0)
+		goto out_register_late_subsys;
+
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_DELROUTE,
+				   inet6_rtm_delroute, NULL, 0);
+	if (ret < 0)
+		goto out_register_late_subsys;
+
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETROUTE,
+				   inet6_rtm_getroute, NULL,
+				   RTNL_FLAG_DOIT_UNLOCKED);
+	if (ret < 0)
 		goto out_register_late_subsys;
 
 	ret = register_netdevice_notifier(&ip6_route_dev_notifier);

@@ -4794,6 +4803,7 @@ int __init ip6_route_init(void)
 	return ret;
 
 out_register_late_subsys:
+	rtnl_unregister_all(PF_INET6);
 	unregister_pernet_subsys(&ip6_route_net_late_ops);
 fib6_rules_init:
 	fib6_rules_cleanup();

Signature · 1 line

2.13.6

David Miller

maintainer

2017-12-04 16:34 UTC #7 lore raw

In reply to Florian Westphal

From: Florian Westphal <[email protected]>
Date: Sat, 2 Dec 2017 21:44:04 +0100

Quoted 26 lines

Peter Zijlstra reported (referring to commit 019a316992ee0d983, "rtnetlink: add reference counting to prevent module unload while dump is in progress"):
1) it not in fact a refcount, so using refcount_t is silly
2) there is a distinct lack of memory barriers, so we can easily observe the decrement while the msg_handler is still in progress.
3) waiting with a schedule()/yield() loop is complete crap and subject life-locks, imagine doing that rtnl_unregister_all() from a RT task.
In ancient times rtnetlink exposed a statically-sized table with preset doit/dumpit handlers to be called for a protocol/type pair.
Later the rtnl_register interface was added and the table was allocated on demand. Eventually these were also used by modules.
Problem is that nothing prevents module unload while a netlink dump is in progress. netlink dumps can be span multiple recv calls and netlink core saves the to-be-repeated dumper address for later invocation.
To prevent rmmod the netlink core expects callers to pass in the owning module so a reference can be taken.
So far rtnetlink wasn't doing this, add new interface to pass THIS_MODULE. Moreover, when converting parts of the rtnetlink handling to rcu this code gained way too many READ_ONCE spots, remove them and the extra refcounting.

Quoted 5 lines

Take a module reference when running dumpit and doit callbacks and never alter content of rtnl_link structures after they have been published via rcu_assign_pointer.
Based partially on earlier patch from Peter.

Series applied, thanks Florian.

BTW, why did you use rtnl_register_module() for most of ipv6 but specifically not for the net/ipv6/addrconf.c stuff?

Just curious.

Florian Westphal

author

2017-12-04 16:54 UTC #8 lore raw

Quoted 33 lines · David Miller

From: Florian Westphal <[email protected]>
Date: Sat, 2 Dec 2017 21:44:04 +0100
Quoted 30 lines
Peter Zijlstra reported (referring to commit 019a316992ee0d983, "rtnetlink: add reference counting to prevent module unload while dump is in progress"):
1) it not in fact a refcount, so using refcount_t is silly
2) there is a distinct lack of memory barriers, so we can easily observe the decrement while the msg_handler is still in progress.
3) waiting with a schedule()/yield() loop is complete crap and subject life-locks, imagine doing that rtnl_unregister_all() from a RT task.
In ancient times rtnetlink exposed a statically-sized table with preset doit/dumpit handlers to be called for a protocol/type pair.
Later the rtnl_register interface was added and the table was allocated on demand. Eventually these were also used by modules.
Problem is that nothing prevents module unload while a netlink dump is in progress. netlink dumps can be span multiple recv calls and netlink core saves the to-be-repeated dumper address for later invocation.
To prevent rmmod the netlink core expects callers to pass in the owning module so a reference can be taken.
So far rtnetlink wasn't doing this, add new interface to pass THIS_MODULE. Moreover, when converting parts of the rtnetlink handling to rcu this code gained way too many READ_ONCE spots, remove them and the extra refcounting.
Take a module reference when running dumpit and doit callbacks and never alter content of rtnl_link structures after they have been published via rcu_assign_pointer.

Quoted 6 lines

Quoted 1 line
Based partially on earlier patch from Peter.
Series applied, thanks Florian.
BTW, why did you use rtnl_register_module() for most of ipv6 but specifically not for the net/ipv6/addrconf.c stuff?

Right, originally I did not convert ipv6 at all, as even if its a module it cannot be unloaded.

Then I thought it might be good to also get rid of of panic() in rtnl_register (which made __rtnl_register redundant).

I switched to _module() where error handling wasn't too hard (i.e., where we can make module insertion fail without adding too many
error unwinding stunts).

The only difference is that theoretically some addrlabel rtnetlink ops are not available if registration fails, but thats not really worse than before as this used to panic() instead :-)

I can send a followup patch, to convert all of ipv6, should be easy to propagate errors from addrlabel initialisation to its callsite.

David Miller

maintainer

2017-12-04 16:57 UTC #9 lore raw

From: Florian Westphal <[email protected]>
Date: Mon, 4 Dec 2017 17:53:04 +0100

Quoted 6 lines

The only difference is that theoretically some addrlabel rtnetlink ops are not available if registration fails, but thats not really worse than before as this used to panic() instead :-)
I can send a followup patch, to convert all of ipv6, should be easy to propagate errors from addrlabel initialisation to its callsite.

Yes, please make ipv6 consistent in this area when you get a chance.

Thanks.