* [PATCH net v4 1/7] tipc: Refactor __tipc_nl_compat_doit
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
@ 2018-02-14 5:37 ` Ying Xue
2018-02-14 5:37 ` [PATCH net v4 2/7] tipc: Introduce __tipc_nl_bearer_disable Ying Xue
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:37 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 2/7] tipc: Introduce __tipc_nl_bearer_disable
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
2018-02-14 5:37 ` [PATCH net v4 1/7] tipc: Refactor __tipc_nl_compat_doit Ying Xue
@ 2018-02-14 5:37 ` Ying Xue
2018-02-14 5:38 ` [PATCH net v4 3/7] tipc: Introduce __tipc_nl_bearer_enable Ying Xue
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:37 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 3/7] tipc: Introduce __tipc_nl_bearer_enable
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
2018-02-14 5:37 ` [PATCH net v4 1/7] tipc: Refactor __tipc_nl_compat_doit Ying Xue
2018-02-14 5:37 ` [PATCH net v4 2/7] tipc: Introduce __tipc_nl_bearer_disable Ying Xue
@ 2018-02-14 5:38 ` Ying Xue
2018-02-14 5:38 ` [PATCH net v4 4/7] tipc: Introduce __tipc_nl_bearer_set Ying Xue
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:38 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 4/7] tipc: Introduce __tipc_nl_bearer_set
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
` (2 preceding siblings ...)
2018-02-14 5:38 ` [PATCH net v4 3/7] tipc: Introduce __tipc_nl_bearer_enable Ying Xue
@ 2018-02-14 5:38 ` Ying Xue
2018-02-14 5:38 ` [PATCH net v4 5/7] tipc: Introduce __tipc_nl_media_set Ying Xue
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:38 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 5/7] tipc: Introduce __tipc_nl_media_set
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
` (3 preceding siblings ...)
2018-02-14 5:38 ` [PATCH net v4 4/7] tipc: Introduce __tipc_nl_bearer_set Ying Xue
@ 2018-02-14 5:38 ` Ying Xue
2018-02-14 5:38 ` [PATCH net v4 6/7] tipc: Introduce __tipc_nl_net_set Ying Xue
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:38 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 6/7] tipc: Introduce __tipc_nl_net_set
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
` (4 preceding siblings ...)
2018-02-14 5:38 ` [PATCH net v4 5/7] tipc: Introduce __tipc_nl_media_set Ying Xue
@ 2018-02-14 5:38 ` Ying Xue
2018-02-14 5:38 ` [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
2018-02-14 19:46 ` [PATCH net v4 0/7] " David Miller
7 siblings, 0 replies; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:38 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
` (5 preceding siblings ...)
2018-02-14 5:38 ` [PATCH net v4 6/7] tipc: Introduce __tipc_nl_net_set Ying Xue
@ 2018-02-14 5:38 ` Ying Xue
2018-02-14 10:16 ` Kirill Tkhai
2018-02-14 19:46 ` [PATCH net v4 0/7] " David Miller
7 siblings, 1 reply; 10+ messages in thread
From: Ying Xue @ 2018-02-14 5:38 UTC (permalink / raw)
To: davem, ktkhai, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties
2018-02-14 5:38 ` [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
@ 2018-02-14 10:16 ` Kirill Tkhai
0 siblings, 0 replies; 10+ messages in thread
From: Kirill Tkhai @ 2018-02-14 10:16 UTC (permalink / raw)
To: Ying Xue, davem, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties
2018-02-14 5:37 [PATCH net v4 0/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
` (6 preceding siblings ...)
2018-02-14 5:38 ` [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during setting link properties Ying Xue
@ 2018-02-14 19:46 ` David Miller
7 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-02-14 19:46 UTC (permalink / raw)
To: ying.xue; +Cc: ktkhai, jon.maloy, netdev, syzkaller-bugs, tipc-discussion
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.
^ permalink raw reply [flat|nested] 10+ messages in thread