[PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties

[PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties

[Ying Xue]

At present it's unsafe to configure link properties through netlink
as the entire setting process is not under RTNL lock protection. Now
TIPC supports two different sets of netlink APIs at the same time, and
they share the same set of backend functions to configure bearer,
media and net properties. In order to solve the missing RTNL issue,
we have to make the whole __tipc_nl_compat_doit() protected by RTNL,
which means any function called within it cannot take RTNL any more.
So in the series we first introduce the following new functions which
doesn't hold RTNl lock:

 - __tipc_nl_bearer_disable()
 - __tipc_nl_bearer_enable()
 - __tipc_nl_bearer_set()
 - __tipc_nl_media_set()
 - __tipc_nl_net_set()

Meanwhile, __tipc_nl_compat_doit() has been reconstructed to minimize
the time of holding RTNL lock.

Changes in v4:
 - Per suggestion of Kirill Tkhai, divided original big one patch into
   seven small ones so that they can be easily reviewed.

Changes in v3: 
 - Optimized return method of __tipc_nl_bearer_enable() regarding
   the comments from David M and Kirill Tkhai
 - Moved the allocations of memory in __tipc_nl_compat_doit() out
   of RTNL lock to minimize the time of holding RTNL lock according
   to the suggestion of Kirill Tkhai.

Changes in v2:
 - The whole operation of setting bearer/media properties has been
   protected under RTNL, as per feedback from David M.

Ying Xue (7):
  tipc: Refactor __tipc_nl_compat_doit
  tipc: Introduce __tipc_nl_bearer_disable
  tipc: Introduce __tipc_nl_bearer_enable
  tipc: Introduce __tipc_nl_bearer_set
  tipc: Introduce __tipc_nl_media_set
  tipc: Introduce __tipc_nl_net_set
  tipc: Fix missing RTNL lock protection during setting link properties

 net/tipc/bearer.c         | 82 +++++++++++++++++++++++++++++------------------
 net/tipc/bearer.h         |  4 +++
 net/tipc/net.c            | 15 +++++++--
 net/tipc/net.h            |  1 +
 net/tipc/netlink_compat.c | 43 +++++++++++++------------
 5 files changed, 91 insertions(+), 54 deletions(-)

-- 
2.7.4

[PATCH net v4 1/7] tipc: Refactor __tipc_nl_compat_doit

[Ying Xue]

As preparation for adding RTNL to make (*cmd->transcode)() and
(*cmd->transcode)() constantly protected by RTNL lock, we move out of
memory allocations existing between them as many as possible so that
the time of holding RTNL can be minimized in __tipc_nl_compat_doit().

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/netlink_compat.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index e48f0b2..9741690 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -285,10 +285,6 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!trans_buf)
 		return -ENOMEM;
 
-	err = (*cmd->transcode)(cmd, trans_buf, msg);
-	if (err)
-		goto trans_out;
-
 	attrbuf = kmalloc((tipc_genl_family.maxattr + 1) *
 			sizeof(struct nlattr *), GFP_KERNEL);
 	if (!attrbuf) {
@@ -296,27 +292,32 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 		goto trans_out;
 	}
 
-	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
-			(const struct nlattr *)trans_buf->data,
-			trans_buf->len, NULL, NULL);
-	if (err)
-		goto parse_out;
-
 	doit_buf = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!doit_buf) {
 		err = -ENOMEM;
-		goto parse_out;
+		goto attrbuf_out;
 	}
 
-	doit_buf->sk = msg->dst_sk;
-
 	memset(&info, 0, sizeof(info));
 	info.attrs = attrbuf;
 
+	err = (*cmd->transcode)(cmd, trans_buf, msg);
+	if (err)
+		goto doit_out;
+
+	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
+			(const struct nlattr *)trans_buf->data,
+			trans_buf->len, NULL, NULL);
+	if (err)
+		goto doit_out;
+
+	doit_buf->sk = msg->dst_sk;
+
 	err = (*cmd->doit)(doit_buf, &info);
+doit_out:
 
 	kfree_skb(doit_buf);
-parse_out:
+attrbuf_out:
 	kfree(attrbuf);
 trans_out:
 	kfree_skb(trans_buf);
-- 
2.7.4

[PATCH net v4 2/7] tipc: Introduce __tipc_nl_bearer_disable

[Ying Xue]

Introduce __tipc_nl_bearer_disable() which doesn't hold RTNL lock.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/bearer.c | 19 +++++++++++++------
 net/tipc/bearer.h |  1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index c800147..61b6625 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -813,7 +813,7 @@ int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -835,19 +835,26 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	bearer = tipc_bearer_find(net, name);
-	if (!bearer) {
-		rtnl_unlock();
+	if (!bearer)
 		return -EINVAL;
-	}
 
 	bearer_disable(net, bearer);
-	rtnl_unlock();
 
 	return 0;
 }
 
+int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_disable(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 42d6eee..bcc6d5f 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -188,6 +188,7 @@ extern struct tipc_media udp_media_info;
 #endif
 
 int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
-- 
2.7.4

[PATCH net v4 3/7] tipc: Introduce __tipc_nl_bearer_enable

[Ying Xue]

Introduce __tipc_nl_bearer_enable() which doesn't hold RTNL lock.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/bearer.c | 17 ++++++++++-------
 net/tipc/bearer.h |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 61b6625..faf8fa0 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -855,7 +855,7 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *bearer;
@@ -897,15 +897,18 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 			prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
 	}
 
+	return tipc_enable_bearer(net, bearer, domain, prio, attrs);
+}
+
+int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
 	rtnl_lock();
-	err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
-	if (err) {
-		rtnl_unlock();
-		return err;
-	}
+	err = __tipc_nl_bearer_enable(skb, info);
 	rtnl_unlock();
 
-	return 0;
+	return err;
 }
 
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index bcc6d5f..fc81150 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -190,6 +190,7 @@ extern struct tipc_media udp_media_info;
 int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
-- 
2.7.4

[PATCH net v4 4/7] tipc: Introduce __tipc_nl_bearer_set

[Ying Xue]

Introduce __tipc_nl_bearer_set() which doesn't holding RTNL lock.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/bearer.c | 23 ++++++++++++++---------
 net/tipc/bearer.h |  1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index faf8fa0..f92c9c5 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -954,7 +954,7 @@ int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
-int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -975,22 +975,17 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	b = tipc_bearer_find(net, name);
-	if (!b) {
-		rtnl_unlock();
+	if (!b)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_BEARER_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_BEARER_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			b->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -999,11 +994,21 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			b->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
 
+int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
 static int __tipc_nl_add_media(struct tipc_nl_msg *msg,
 			       struct tipc_media *media, int nlflags)
 {
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index fc81150..cc0f529 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -194,6 +194,7 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
-- 
2.7.4

[PATCH net v4 5/7] tipc: Introduce __tipc_nl_media_set

[Ying Xue]

Introduce __tipc_nl_media_set() which doesn't hold RTNL lock.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/bearer.c | 23 ++++++++++++++---------
 net/tipc/bearer.h |  1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index f92c9c5..3e3dce3 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -1130,7 +1130,7 @@ int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -1148,22 +1148,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_MEDIA_NAME]);
 
-	rtnl_lock();
 	m = tipc_media_find(name);
-	if (!m) {
-		rtnl_unlock();
+	if (!m)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_MEDIA_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_MEDIA_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			m->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -1172,7 +1167,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			m->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
+
+int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_media_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index cc0f529..a53613d 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -200,6 +200,7 @@ int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_media_set_priority(const char *name, u32 new_value);
 int tipc_media_set_window(const char *name, u32 new_value);
-- 
2.7.4

[PATCH net v4 6/7] tipc: Introduce __tipc_nl_net_set

[Ying Xue]

Introduce __tipc_nl_net_set() which doesn't hold RTNL lock.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/net.c | 15 ++++++++++++---
 net/tipc/net.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/tipc/net.c b/net/tipc/net.c
index 719c592..1a2fde0 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -200,7 +200,7 @@ int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
-int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
 	struct tipc_net *tn = net_generic(net, tipc_net_id);
@@ -241,10 +241,19 @@ int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 		if (!tipc_addr_node_valid(addr))
 			return -EINVAL;
 
-		rtnl_lock();
 		tipc_net_start(net, addr);
-		rtnl_unlock();
 	}
 
 	return 0;
 }
+
+int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_net_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/net.h b/net/tipc/net.h
index c7c2549..c0306aa 100644
--- a/net/tipc/net.h
+++ b/net/tipc/net.h
@@ -47,5 +47,6 @@ void tipc_net_stop(struct net *net);
 
 int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
 
 #endif
-- 
2.7.4

[PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties

[Ying Xue]

Currently when user changes link properties, TIPC first checks if
user's command message contains media name or bearer name through
tipc_media_find() or tipc_bearer_find() which is protected by RTNL
lock. But when tipc_nl_compat_link_set() conducts the checking with
the two functions, it doesn't hold RTNL lock at all, as a result,
the following complaints were reported:

audit: type=1400 audit(1514679888.244:9): avc:  denied  { write } for
pid=3194 comm="syzkaller021477" path="socket:[11143]" dev="sockfs"
ino=11143 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tcontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tclass=netlink_generic_socket permissive=1
=============================
WARNING: suspicious RCU usage
4.15.0-rc5+ #152 Not tainted
-----------------------------
net/tipc/bearer.c:177 suspicious rcu_dereference_protected() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
2 locks held by syzkaller021477/3194:
  #0:  (cb_lock){++++}, at: [<00000000d20133ea>] genl_rcv+0x19/0x40
net/netlink/genetlink.c:634
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_lock
net/netlink/genetlink.c:33 [inline]
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_rcv_msg+0x115/0x140
net/netlink/genetlink.c:622

stack backtrace:
CPU: 1 PID: 3194 Comm: syzkaller021477 Not tainted 4.15.0-rc5+ #152
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:17 [inline]
  dump_stack+0x194/0x257 lib/dump_stack.c:53
  lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
  tipc_bearer_find+0x2b4/0x3b0 net/tipc/bearer.c:177
  tipc_nl_compat_link_set+0x329/0x9f0 net/tipc/netlink_compat.c:729
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:288 [inline]
  tipc_nl_compat_doit+0x15b/0x660 net/tipc/netlink_compat.c:335
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1119 [inline]
  tipc_nl_compat_recv+0x112f/0x18f0 net/tipc/netlink_compat.c:1201
  genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
  genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624
  netlink_rcv_skb+0x21e/0x460 net/netlink/af_netlink.c:2408
  genl_rcv+0x28/0x40 net/netlink/genetlink.c:635
  netlink_unicast_kernel net/netlink/af_netlink.c:1275 [inline]
  netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1301
  netlink_sendmsg+0xa4a/0xe60 net/netlink/af_netlink.c:1864
  sock_sendmsg_nosec net/socket.c:636 [inline]
  sock_sendmsg+0xca/0x110 net/socket.c:646
  sock_write_iter+0x31a/0x5d0 net/socket.c:915
  call_write_iter include/linux/fs.h:1772 [inline]
  new_sync_write fs/read_write.c:469 [inline]
  __vfs_write+0x684/0x970 fs/read_write.c:482
  vfs_write+0x189/0x510 fs/read_write.c:544
  SYSC_write fs/read_write.c:589 [inline]
  SyS_write+0xef/0x220 fs/read_write.c:581
  do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
  do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
  entry_SYSENTER_compat+0x54/0x63 arch/x86/entry/entry_64_compat.S:129

In order to correct the mistake, __tipc_nl_compat_doit() has been
protected by RTNL lock, which means the whole operation of setting
bearer/media properties is under RTNL protection.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Reported-by: syzbot <syzbot+6345fd433db009b29413@syzkaller.appspotmail.com>
---
 net/tipc/netlink_compat.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 9741690..4492cda 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -301,6 +301,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 	memset(&info, 0, sizeof(info));
 	info.attrs = attrbuf;
 
+	rtnl_lock();
 	err = (*cmd->transcode)(cmd, trans_buf, msg);
 	if (err)
 		goto doit_out;
@@ -315,6 +316,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 
 	err = (*cmd->doit)(doit_buf, &info);
 doit_out:
+	rtnl_unlock();
 
 	kfree_skb(doit_buf);
 attrbuf_out:
@@ -723,13 +725,13 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
 
 	media = tipc_media_find(lc->name);
 	if (media) {
-		cmd->doit = &tipc_nl_media_set;
+		cmd->doit = &__tipc_nl_media_set;
 		return tipc_nl_compat_media_set(skb, msg);
 	}
 
 	bearer = tipc_bearer_find(msg->net, lc->name);
 	if (bearer) {
-		cmd->doit = &tipc_nl_bearer_set;
+		cmd->doit = &__tipc_nl_bearer_set;
 		return tipc_nl_compat_bearer_set(skb, msg);
 	}
 
@@ -1090,12 +1092,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_ENABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_CONFIG;
-		doit.doit = tipc_nl_bearer_enable;
+		doit.doit = __tipc_nl_bearer_enable;
 		doit.transcode = tipc_nl_compat_bearer_enable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_DISABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_NAME;
-		doit.doit = tipc_nl_bearer_disable;
+		doit.doit = __tipc_nl_bearer_disable;
 		doit.transcode = tipc_nl_compat_bearer_disable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SHOW_LINK_STATS:
@@ -1149,12 +1151,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_SET_NODE_ADDR:
 		msg->req_type = TIPC_TLV_NET_ADDR;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SET_NETID:
 		msg->req_type = TIPC_TLV_UNSIGNED;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_GET_NETID:
-- 
2.7.4

Re: [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties

[Kirill Tkhai]

On 14.02.2018 08:38, Ying Xue wrote:
> Currently when user changes link properties, TIPC first checks if
> user's command message contains media name or bearer name through
> tipc_media_find() or tipc_bearer_find() which is protected by RTNL
> lock. But when tipc_nl_compat_link_set() conducts the checking with
> the two functions, it doesn't hold RTNL lock at all, as a result,
> the following complaints were reported:
> 
> audit: type=1400 audit(1514679888.244:9): avc:  denied  { write } for
> pid=3194 comm="syzkaller021477" path="socket:[11143]" dev="sockfs"
> ino=11143 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tcontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tclass=netlink_generic_socket permissive=1
> =============================
> WARNING: suspicious RCU usage
> 4.15.0-rc5+ #152 Not tainted
> -----------------------------
> net/tipc/bearer.c:177 suspicious rcu_dereference_protected() usage!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 2, debug_locks = 1
> 2 locks held by syzkaller021477/3194:
>   #0:  (cb_lock){++++}, at: [<00000000d20133ea>] genl_rcv+0x19/0x40
> net/netlink/genetlink.c:634
>   #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_lock
> net/netlink/genetlink.c:33 [inline]
>   #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_rcv_msg+0x115/0x140
> net/netlink/genetlink.c:622
> 
> stack backtrace:
> CPU: 1 PID: 3194 Comm: syzkaller021477 Not tainted 4.15.0-rc5+ #152
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:17 [inline]
>   dump_stack+0x194/0x257 lib/dump_stack.c:53
>   lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
>   tipc_bearer_find+0x2b4/0x3b0 net/tipc/bearer.c:177
>   tipc_nl_compat_link_set+0x329/0x9f0 net/tipc/netlink_compat.c:729
>   __tipc_nl_compat_doit net/tipc/netlink_compat.c:288 [inline]
>   tipc_nl_compat_doit+0x15b/0x660 net/tipc/netlink_compat.c:335
>   tipc_nl_compat_handle net/tipc/netlink_compat.c:1119 [inline]
>   tipc_nl_compat_recv+0x112f/0x18f0 net/tipc/netlink_compat.c:1201
>   genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
>   genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624
>   netlink_rcv_skb+0x21e/0x460 net/netlink/af_netlink.c:2408
>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:635
>   netlink_unicast_kernel net/netlink/af_netlink.c:1275 [inline]
>   netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1301
>   netlink_sendmsg+0xa4a/0xe60 net/netlink/af_netlink.c:1864
>   sock_sendmsg_nosec net/socket.c:636 [inline]
>   sock_sendmsg+0xca/0x110 net/socket.c:646
>   sock_write_iter+0x31a/0x5d0 net/socket.c:915
>   call_write_iter include/linux/fs.h:1772 [inline]
>   new_sync_write fs/read_write.c:469 [inline]
>   __vfs_write+0x684/0x970 fs/read_write.c:482
>   vfs_write+0x189/0x510 fs/read_write.c:544
>   SYSC_write fs/read_write.c:589 [inline]
>   SyS_write+0xef/0x220 fs/read_write.c:581
>   do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
>   do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
>   entry_SYSENTER_compat+0x54/0x63 arch/x86/entry/entry_64_compat.S:129
> 
> In order to correct the mistake, __tipc_nl_compat_doit() has been
> protected by RTNL lock, which means the whole operation of setting
> bearer/media properties is under RTNL protection.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Reported-by: syzbot <syzbot+6345fd433db009b29413@syzkaller.appspotmail.com>
> ---
>  net/tipc/netlink_compat.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index 9741690..4492cda 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -301,6 +301,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
>  	memset(&info, 0, sizeof(info));
>  	info.attrs = attrbuf;
>  
> +	rtnl_lock();
>  	err = (*cmd->transcode)(cmd, trans_buf, msg);
>  	if (err)
>  		goto doit_out;
> @@ -315,6 +316,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
>  
>  	err = (*cmd->doit)(doit_buf, &info);
>  doit_out:
> +	rtnl_unlock();
>  
>  	kfree_skb(doit_buf);
>  attrbuf_out:
> @@ -723,13 +725,13 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
>  
>  	media = tipc_media_find(lc->name);
>  	if (media) {
> -		cmd->doit = &tipc_nl_media_set;
> +		cmd->doit = &__tipc_nl_media_set;
>  		return tipc_nl_compat_media_set(skb, msg);
>  	}
>  
>  	bearer = tipc_bearer_find(msg->net, lc->name);
>  	if (bearer) {
> -		cmd->doit = &tipc_nl_bearer_set;
> +		cmd->doit = &__tipc_nl_bearer_set;
>  		return tipc_nl_compat_bearer_set(skb, msg);
>  	}
>  
> @@ -1090,12 +1092,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
>  		return tipc_nl_compat_dumpit(&dump, msg);
>  	case TIPC_CMD_ENABLE_BEARER:
>  		msg->req_type = TIPC_TLV_BEARER_CONFIG;
> -		doit.doit = tipc_nl_bearer_enable;
> +		doit.doit = __tipc_nl_bearer_enable;
>  		doit.transcode = tipc_nl_compat_bearer_enable;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_DISABLE_BEARER:
>  		msg->req_type = TIPC_TLV_BEARER_NAME;
> -		doit.doit = tipc_nl_bearer_disable;
> +		doit.doit = __tipc_nl_bearer_disable;
>  		doit.transcode = tipc_nl_compat_bearer_disable;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_SHOW_LINK_STATS:
> @@ -1149,12 +1151,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
>  		return tipc_nl_compat_dumpit(&dump, msg);
>  	case TIPC_CMD_SET_NODE_ADDR:
>  		msg->req_type = TIPC_TLV_NET_ADDR;
> -		doit.doit = tipc_nl_net_set;
> +		doit.doit = __tipc_nl_net_set;
>  		doit.transcode = tipc_nl_compat_net_set;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_SET_NETID:
>  		msg->req_type = TIPC_TLV_UNSIGNED;
> -		doit.doit = tipc_nl_net_set;
> +		doit.doit = __tipc_nl_net_set;
>  		doit.transcode = tipc_nl_compat_net_set;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_GET_NETID:

It looks like this matches the idea, David suggested in reply to v1.
Also, I don't see more .doit acquiring rtnl_lock(), all are converted.

For the whole series:

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

Thanks.

Re: [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties

[David Miller]

From: Ying Xue <ying.xue@windriver.com>
Date: Wed, 14 Feb 2018 13:37:57 +0800

> At present it's unsafe to configure link properties through netlink
> as the entire setting process is not under RTNL lock protection. Now
> TIPC supports two different sets of netlink APIs at the same time, and
> they share the same set of backend functions to configure bearer,
> media and net properties. In order to solve the missing RTNL issue,
> we have to make the whole __tipc_nl_compat_doit() protected by RTNL,
> which means any function called within it cannot take RTNL any more.
> So in the series we first introduce the following new functions which
> doesn't hold RTNl lock:
> 
>  - __tipc_nl_bearer_disable()
>  - __tipc_nl_bearer_enable()
>  - __tipc_nl_bearer_set()
>  - __tipc_nl_media_set()
>  - __tipc_nl_net_set()
> 
> Meanwhile, __tipc_nl_compat_doit() has been reconstructed to minimize
> the time of holding RTNL lock.
> 
> Changes in v4:
>  - Per suggestion of Kirill Tkhai, divided original big one patch into
>    seven small ones so that they can be easily reviewed.
> 
> Changes in v3: 
>  - Optimized return method of __tipc_nl_bearer_enable() regarding
>    the comments from David M and Kirill Tkhai
>  - Moved the allocations of memory in __tipc_nl_compat_doit() out
>    of RTNL lock to minimize the time of holding RTNL lock according
>    to the suggestion of Kirill Tkhai.
> 
> Changes in v2:
>  - The whole operation of setting bearer/media properties has been
>    protected under RTNL, as per feedback from David M.

Series applied, thank you.