From c74f2b2678f40b80265dd53556f1f778c8e1823f Mon Sep 17 00:00:00 2001 From: Stanislaw Gruszka Date: Fri, 26 Jul 2013 11:00:10 +0200 Subject: genetlink: release cb_lock before requesting additional module Requesting external module with cb_lock taken can result in the deadlock like showed below: [ 2458.111347] Showing all locks held in the system: [ 2458.111347] 1 lock held by NetworkManager/582: [ 2458.111347] #0: (cb_lock){++++++}, at: [] genl_rcv+0x19/0x40 [ 2458.111347] 1 lock held by modprobe/603: [ 2458.111347] #0: (cb_lock){++++++}, at: [] genl_lock_all+0x15/0x30 [ 2461.579457] SysRq : Show Blocked State [ 2461.580103] task PC stack pid father [ 2461.580103] NetworkManager D ffff880034b84500 4040 582 1 0x00000080 [ 2461.580103] ffff8800197ff720 0000000000000046 00000000001d5340 ffff8800197fffd8 [ 2461.580103] ffff8800197fffd8 00000000001d5340 ffff880019631700 7fffffffffffffff [ 2461.580103] ffff8800197ff880 ffff8800197ff878 ffff880019631700 ffff880019631700 [ 2461.580103] Call Trace: [ 2461.580103] [] schedule+0x29/0x70 [ 2461.580103] [] schedule_timeout+0x1c1/0x360 [ 2461.580103] [] ? mark_held_locks+0xbb/0x140 [ 2461.580103] [] ? _raw_spin_unlock_irq+0x2c/0x50 [ 2461.580103] [] ? trace_hardirqs_on_caller+0xfd/0x1c0 [ 2461.580103] [] wait_for_completion_killable+0xe8/0x170 [ 2461.580103] [] ? wake_up_state+0x20/0x20 [ 2461.580103] [] call_usermodehelper_exec+0x1a5/0x210 [ 2461.580103] [] ? wait_for_completion_killable+0x3d/0x170 [ 2461.580103] [] __request_module+0x1b3/0x370 [ 2461.580103] [] ? trace_hardirqs_on_caller+0xfd/0x1c0 [ 2461.580103] [] ctrl_getfamily+0x159/0x190 [ 2461.580103] [] genl_family_rcv_msg+0x1f4/0x2e0 [ 2461.580103] [] ? genl_family_rcv_msg+0x2e0/0x2e0 [ 2461.580103] [] genl_rcv_msg+0x8e/0xd0 [ 2461.580103] [] netlink_rcv_skb+0xa9/0xc0 [ 2461.580103] [] genl_rcv+0x28/0x40 [ 2461.580103] [] netlink_unicast+0xdd/0x190 [ 2461.580103] [] netlink_sendmsg+0x329/0x750 [ 2461.580103] [] sock_sendmsg+0x99/0xd0 [ 2461.580103] [] ? local_clock+0x5f/0x70 [ 2461.580103] [] ? lock_release_non_nested+0x308/0x350 [ 2461.580103] [] ___sys_sendmsg+0x39e/0x3b0 [ 2461.580103] [] ? kvm_clock_read+0x2f/0x50 [ 2461.580103] [] ? sched_clock+0x9/0x10 [ 2461.580103] [] ? sched_clock_local+0x1d/0x80 [ 2461.580103] [] ? sched_clock_cpu+0xa8/0x100 [ 2461.580103] [] ? trace_hardirqs_off+0xd/0x10 [ 2461.580103] [] ? local_clock+0x5f/0x70 [ 2461.580103] [] ? lock_release_holdtime.part.28+0xf/0x1a0 [ 2461.580103] [] ? fget_light+0xf9/0x510 [ 2461.580103] [] ? fget_light+0x3c/0x510 [ 2461.580103] [] __sys_sendmsg+0x42/0x80 [ 2461.580103] [] SyS_sendmsg+0x12/0x20 [ 2461.580103] [] system_call_fastpath+0x16/0x1b [ 2461.580103] modprobe D ffff88000f2c8000 4632 603 602 0x00000080 [ 2461.580103] ffff88000f04fba8 0000000000000046 00000000001d5340 ffff88000f04ffd8 [ 2461.580103] ffff88000f04ffd8 00000000001d5340 ffff8800377d4500 ffff8800377d4500 [ 2461.580103] ffffffff81d0b260 ffffffff81d0b268 ffffffff00000000 ffffffff81d0b2b0 [ 2461.580103] Call Trace: [ 2461.580103] [] schedule+0x29/0x70 [ 2461.580103] [] rwsem_down_write_failed+0xed/0x1a0 [ 2461.580103] [] ? update_cpu_load_active+0x10/0xb0 [ 2461.580103] [] call_rwsem_down_write_failed+0x13/0x20 [ 2461.580103] [] ? down_write+0x9d/0xb2 [ 2461.580103] [] ? genl_lock_all+0x15/0x30 [ 2461.580103] [] genl_lock_all+0x15/0x30 [ 2461.580103] [] genl_register_family+0x53/0x1f0 [ 2461.580103] [] ? 0xffffffffa01dbfff [ 2461.580103] [] genl_register_family_with_ops+0x20/0x80 [ 2461.580103] [] ? 0xffffffffa01dbfff [ 2461.580103] [] nl80211_init+0x24/0xf0 [cfg80211] [ 2461.580103] [] ? 0xffffffffa01dbfff [ 2461.580103] [] cfg80211_init+0x43/0xdb [cfg80211] [ 2461.580103] [] do_one_initcall+0xfa/0x1b0 [ 2461.580103] [] ? set_memory_nx+0x43/0x50 [ 2461.580103] [] load_module+0x1c6f/0x27f0 [ 2461.580103] [] ? store_uevent+0x40/0x40 [ 2461.580103] [] SyS_finit_module+0x86/0xb0 [ 2461.580103] [] system_call_fastpath+0x16/0x1b [ 2461.580103] Sched Debug Version: v0.10, 3.11.0-0.rc1.git4.1.fc20.x86_64 #1 Problem start to happen after adding net-pf-16-proto-16-family-nl80211 alias name to cfg80211 module by below commit (though that commit itself is perfectly fine): commit fb4e156886ce6e8309e912d8b370d192330d19d3 Author: Marcel Holtmann Date: Sun Apr 28 16:22:06 2013 -0700 nl80211: Add generic netlink module alias for cfg80211/nl80211 Reported-and-tested-by: Jeff Layton Reported-by: Richard W.M. Jones Signed-off-by: Stanislaw Gruszka Reviewed-by: Pravin B Shelar Signed-off-by: David S. Miller --- net/netlink/genetlink.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'net/netlink/genetlink.c') diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 2fd6dbea327a..1076fe16b122 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -877,8 +877,10 @@ static int ctrl_getfamily(struct sk_buff *skb, struct genl_info *info) #ifdef CONFIG_MODULES if (res == NULL) { genl_unlock(); + up_read(&cb_lock); request_module("net-pf-%d-proto-%d-family-%s", PF_NETLINK, NETLINK_GENERIC, name); + down_read(&cb_lock); genl_lock(); res = genl_family_find_byname(name); } -- cgit v1.2.1 From e1ee3673a83cc02b6b5e43c9e647d8dd5e1c4e26 Mon Sep 17 00:00:00 2001 From: Pablo Neira Date: Mon, 29 Jul 2013 12:30:04 +0200 Subject: genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE Currently, it is not possible to use neither NLM_F_EXCL nor NLM_F_REPLACE from genetlink. This is due to this checking in genl_family_rcv_msg: if (nlh->nlmsg_flags & NLM_F_DUMP) NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or NLM_F_REPLACE flag is set, genetlink believes that you're requesting a dump and it calls the .dumpit callback. The solution that I propose is to refine this checking to make it stricter: if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) And given the combination NLM_F_REPLACE and NLM_F_EXCL does not make sense to me, it removes the ambiguity. There was a patch that tried to fix this some time ago (0ab03c2 netlink: test for all flags of the NLM_F_DUMP composite) but it tried to resolve this ambiguity in *all* existing netlink subsystems, not only genetlink. That patch was reverted since it broke iproute2, which is using NLM_F_ROOT to request the dump of the routing cache. Signed-off-by: Pablo Neira Ayuso Signed-off-by: David S. Miller --- net/netlink/genetlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/netlink/genetlink.c') diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 1076fe16b122..512718adb0d5 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -571,7 +571,7 @@ static int genl_family_rcv_msg(struct genl_family *family, !capable(CAP_NET_ADMIN)) return -EPERM; - if (nlh->nlmsg_flags & NLM_F_DUMP) { + if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) { struct netlink_dump_control c = { .dump = ops->dumpit, .done = ops->done, -- cgit v1.2.1 From 58ad436fcf49810aa006016107f494c9ac9013db Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 13 Aug 2013 09:04:05 +0200 Subject: genetlink: fix family dump race When dumping generic netlink families, only the first dump call is locked with genl_lock(), which protects the list of families, and thus subsequent calls can access the data without locking, racing against family addition/removal. This can cause a crash. Fix it - the locking needs to be conditional because the first time around it's already locked. A similar bug was reported to me on an old kernel (3.4.47) but the exact scenario that happened there is no longer possible, on those kernels the first round wasn't locked either. Looking at the current code I found the race described above, which had also existed on the old kernel. Cc: stable@vger.kernel.org Reported-by: Andrei Otcheretianski Signed-off-by: Johannes Berg Signed-off-by: David S. Miller --- net/netlink/genetlink.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'net/netlink/genetlink.c') diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 512718adb0d5..f85f8a2ad6cf 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -789,6 +789,10 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb) struct net *net = sock_net(skb->sk); int chains_to_skip = cb->args[0]; int fams_to_skip = cb->args[1]; + bool need_locking = chains_to_skip || fams_to_skip; + + if (need_locking) + genl_lock(); for (i = chains_to_skip; i < GENL_FAM_TAB_SIZE; i++) { n = 0; @@ -810,6 +814,9 @@ errout: cb->args[0] = i; cb->args[1] = n; + if (need_locking) + genl_unlock(); + return skb->len; } -- cgit v1.2.1 From 9d47b380563174e5c15776ce6ca9bab4ee7d59e3 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 21 Aug 2013 16:08:02 +0200 Subject: Revert "genetlink: fix family dump race" This reverts commit 58ad436fcf49810aa006016107f494c9ac9013db. It turns out that the change introduced a potential deadlock by causing a locking dependency with netlink's cb_mutex. I can't seem to find a way to resolve this without doing major changes to the locking, so revert this. Signed-off-by: Johannes Berg Acked-by: Pravin B Shelar Signed-off-by: David S. Miller --- net/netlink/genetlink.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'net/netlink/genetlink.c') diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index f85f8a2ad6cf..512718adb0d5 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -789,10 +789,6 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb) struct net *net = sock_net(skb->sk); int chains_to_skip = cb->args[0]; int fams_to_skip = cb->args[1]; - bool need_locking = chains_to_skip || fams_to_skip; - - if (need_locking) - genl_lock(); for (i = chains_to_skip; i < GENL_FAM_TAB_SIZE; i++) { n = 0; @@ -814,9 +810,6 @@ errout: cb->args[0] = i; cb->args[1] = n; - if (need_locking) - genl_unlock(); - return skb->len; } -- cgit v1.2.1