* [PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties
@ 2018-02-12 8:56 Ying Xue
2018-02-12 10:10 ` Kirill Tkhai
0 siblings, 1 reply; 3+ messages in thread
From: Ying Xue @ 2018-02-12 8:56 UTC (permalink / raw)
To: davem, 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>
---
v2: The whole operation of setting bearer/media properties has been
protected under RTNL, as per feedback from David M.
net/tipc/bearer.c | 84 ++++++++++++++++++++++++++++++-----------------
net/tipc/bearer.h | 4 +++
net/tipc/net.c | 15 +++++++--
net/tipc/net.h | 1 +
net/tipc/netlink_compat.c | 14 ++++----
5 files changed, 79 insertions(+), 39 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index c800147..e12d7eb 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,20 +835,27 @@ 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_enable(struct sk_buff *skb, struct genl_info *info)
+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;
char *bearer;
@@ -890,17 +897,24 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
}
- rtnl_lock();
err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
- if (err) {
- rtnl_unlock();
+ if (err)
return err;
- }
- rtnl_unlock();
return 0;
}
+int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+{
+ int err;
+
+ rtnl_lock();
+ err = __tipc_nl_bearer_enable(skb, info);
+ rtnl_unlock();
+
+ return err;
+}
+
int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
{
int err;
@@ -944,7 +958,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;
@@ -965,22 +979,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]);
@@ -989,11 +998,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)
{
@@ -1115,7 +1134,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;
@@ -1133,22 +1152,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]);
@@ -1157,7 +1171,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 42d6eee..a53613d 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -188,15 +188,19 @@ 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_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);
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);
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
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index e48f0b2..750e37c 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -332,7 +332,9 @@ static int tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
return -EINVAL;
+ rtnl_lock();
err = __tipc_nl_compat_doit(cmd, msg);
+ rtnl_unlock();
if (err)
return err;
@@ -722,13 +724,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);
}
@@ -1089,12 +1091,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:
@@ -1148,12 +1150,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] 3+ messages in thread
* Re: [PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties
2018-02-12 8:56 [PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties Ying Xue
@ 2018-02-12 10:10 ` Kirill Tkhai
2018-02-12 15:08 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Kirill Tkhai @ 2018-02-12 10:10 UTC (permalink / raw)
To: Ying Xue, davem, jon.maloy; +Cc: netdev, syzkaller-bugs, tipc-discussion
Hi, Ying,
On 12.02.2018 11:56, 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>
> ---
> v2: The whole operation of setting bearer/media properties has been
> protected under RTNL, as per feedback from David M.
>
> net/tipc/bearer.c | 84 ++++++++++++++++++++++++++++++-----------------
> net/tipc/bearer.h | 4 +++
> net/tipc/net.c | 15 +++++++--
> net/tipc/net.h | 1 +
> net/tipc/netlink_compat.c | 14 ++++----
> 5 files changed, 79 insertions(+), 39 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index c800147..e12d7eb 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,20 +835,27 @@ 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_enable(struct sk_buff *skb, struct genl_info *info)
> +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;
> char *bearer;
> @@ -890,17 +897,24 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
> }
>
> - rtnl_lock();
> err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
> - if (err) {
> - rtnl_unlock();
> + if (err)
> return err;
> - }
> - rtnl_unlock();
>
> return 0;
This err branch looks excess. It was before your patch, but in case of you change this place,
can't we stop having it? it looks like we can simply do the below here:
err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
return err;
> }
>
> +int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> +{
> + int err;
> +
> + rtnl_lock();
> + err = __tipc_nl_bearer_enable(skb, info);
> + rtnl_unlock();
> +
> + return err;
> +}
> +
> int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
> {
> int err;
> @@ -944,7 +958,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;
> @@ -965,22 +979,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]);
> @@ -989,11 +998,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)
> {
> @@ -1115,7 +1134,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;
> @@ -1133,22 +1152,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]);
> @@ -1157,7 +1171,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 42d6eee..a53613d 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -188,15 +188,19 @@ 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_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);
> 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);
> 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
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index e48f0b2..750e37c 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -332,7 +332,9 @@ static int tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
> if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
> return -EINVAL;
>
> + rtnl_lock();
> err = __tipc_nl_compat_doit(cmd, msg);
> + rtnl_unlock();
rtnl_lock() is frequently accessed mutex used everywhere in network subsystem,
and it would be good to minimize the time it's held. Function __tipc_nl_compat_doit()
allocates memory and these allocations may envoke memory reclain, which may
take much time. The lock will be held all the time, and it's bad for other
rtnl users.
Since __tipc_nl_compat_doit() allocates constant amount of memory, we can do
all the allocations before taking rtnl_lock, and this may reduce the time
we are owning the lock in significant way. Can't we do this in this patchset?
(I don't think we can do this in the same patch, but one more pre-patch may
be introduced). The same for kfree*() and memset(), they also do not need
rtnl_lock().
> if (err)
> return err;
>
> @@ -722,13 +724,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);
> }
>
> @@ -1089,12 +1091,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:
> @@ -1148,12 +1150,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:
>
Thanks,
Kirill
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties
2018-02-12 10:10 ` Kirill Tkhai
@ 2018-02-12 15:08 ` David Miller
0 siblings, 0 replies; 3+ messages in thread
From: David Miller @ 2018-02-12 15:08 UTC (permalink / raw)
To: ktkhai; +Cc: ying.xue, jon.maloy, netdev, syzkaller-bugs, tipc-discussion
From: Kirill Tkhai <ktkhai@virtuozzo.com>
Date: Mon, 12 Feb 2018 13:10:34 +0300
> This err branch looks excess. It was before your patch, but in case of you change this place,
> can't we stop having it? it looks like we can simply do the below here:
>
> err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
> return err;
Or even better, straight:
return tipc_enable_bearer(net, bearer, domain, prio, attrs);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-12 15:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 8:56 [PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties Ying Xue
2018-02-12 10:10 ` Kirill Tkhai
2018-02-12 15:08 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.