* linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage!
@ 2014-08-06 17:29 Andrey Wagin
2014-08-06 17:52 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Wagin @ 2014-08-06 17:29 UTC (permalink / raw)
To: netdev
[ 40.072199] ===============================
[ 40.072350] [ INFO: suspicious RCU usage. ]
[ 40.072501] 3.16.0-next-20140806 #1 Not tainted
[ 40.072659] -------------------------------
[ 40.072807] net/netlink/diag.c:106 suspicious
rcu_dereference_protected() usage!
[ 40.073047]
other info that might help us debug this:
[ 40.073276]
rcu_scheduler_active = 1, debug_locks = 0
[ 40.073494] 4 locks held by criu/2838:
[ 40.073635] #0: (sock_diag_mutex){+.+.+.}, at:
[<ffffffff81689afb>] sock_diag_rcv+0x1b/0x40
[ 40.074226] #1: (sock_diag_table_mutex){+.+.+.}, at:
[<ffffffff81689c6d>] sock_diag_rcv_msg+0x6d/0x140
[ 40.074803] #2: (nlk->cb_mutex){+.+.+.}, at: [<ffffffff816a28f1>]
netlink_dump+0x21/0x2d0
[ 40.075382] #3: (nl_table_lock){.+.?..}, at: [<ffffffffa0286601>]
netlink_diag_dump+0x31/0xb0 [netlink_diag]
[ 40.076351]
stack backtrace:
[ 40.076862] CPU: 0 PID: 2838 Comm: criu Not tainted 3.16.0-next-20140806 #1
[ 40.077276] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[ 40.077645] 0000000000000000 000000007979a988 ffff88003a213990
ffffffff817a9c04
[ 40.078548] ffff88003a2e4800 ffff88003a2139c0 ffffffff810ecc47
0000000000000000
[ 40.079419] ffff88003fb324e8 0000000000000ec0 0000000000000000
ffff88003a213a30
[ 40.080258] Call Trace:
[ 40.080559] [<ffffffff817a9c04>] dump_stack+0x4d/0x66
[ 40.080918] [<ffffffff810ecc47>] lockdep_rcu_suspicious+0xe7/0x120
[ 40.081313] [<ffffffffa0286585>] __netlink_diag_dump+0x265/0x2b0
[netlink_diag]
[ 40.081899] [<ffffffffa0286667>] netlink_diag_dump+0x97/0xb0 [netlink_diag]
[ 40.082360] [<ffffffff816a29e7>] netlink_dump+0x117/0x2d0
[ 40.082822] [<ffffffff816a366b>] __netlink_dump_start+0x1ab/0x220
[ 40.083346] [<ffffffffa028607c>]
netlink_diag_handler_dump+0x7c/0xa0 [netlink_diag]
[ 40.083935] [<ffffffffa02865d0>] ? __netlink_diag_dump+0x2b0/0x2b0
[netlink_diag]
[ 40.084547] [<ffffffff81689c87>] sock_diag_rcv_msg+0x87/0x140
[ 40.084914] [<ffffffff81689c00>] ? sock_diag_unregister+0x60/0x60
[ 40.085322] [<ffffffff816a55a9>] netlink_rcv_skb+0xa9/0xc0
[ 40.085709] [<ffffffff81689b0a>] sock_diag_rcv+0x2a/0x40
[ 40.086080] [<ffffffff816a4b80>] netlink_unicast+0x100/0x1e0
[ 40.086444] [<ffffffff816a4fb0>] netlink_sendmsg+0x350/0x790
[ 40.086847] [<ffffffff8165209c>] sock_sendmsg+0x9c/0xe0
[ 40.087244] [<ffffffff811e49af>] ? might_fault+0x5f/0xb0
[ 40.087600] [<ffffffff811e49f8>] ? might_fault+0xa8/0xb0
[ 40.087954] [<ffffffff811e49af>] ? might_fault+0x5f/0xb0
[ 40.088345] [<ffffffff81652fe9>] ___sys_sendmsg+0x3a9/0x3c0
[ 40.088716] [<ffffffff810ef3c1>] ? __lock_acquire+0x501/0x1cc0
[ 40.089102] [<ffffffff81063f34>] ? __do_page_fault+0x254/0x5b0
[ 40.089480] [<ffffffff811e49af>] ? might_fault+0x5f/0xb0
[ 40.089841] [<ffffffff8110a7de>] ? rcu_read_lock_held+0x6e/0x80
[ 40.090238] [<ffffffff81256e6e>] ? __fget_light+0xbe/0xe0
[ 40.090597] [<ffffffff816537d1>] __sys_sendmsg+0x51/0x90
[ 40.090951] [<ffffffff81653822>] SyS_sendmsg+0x12/0x20
[ 40.091367] [<ffffffff817b3b69>] system_call_fastpath+0x16/0x1b
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage! 2014-08-06 17:29 linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage! Andrey Wagin @ 2014-08-06 17:52 ` Cong Wang 2014-08-06 19:51 ` Thomas Graf 0 siblings, 1 reply; 6+ messages in thread From: Cong Wang @ 2014-08-06 17:52 UTC (permalink / raw) To: Andrey Wagin; +Cc: netdev, Thomas Graf On Wed, Aug 6, 2014 at 10:29 AM, Andrey Wagin <avagin@gmail.com> wrote: > [ 40.072199] =============================== > [ 40.072350] [ INFO: suspicious RCU usage. ] > [ 40.072501] 3.16.0-next-20140806 #1 Not tainted > [ 40.072659] ------------------------------- > [ 40.072807] net/netlink/diag.c:106 suspicious > rcu_dereference_protected() usage! > [ 40.073047] > other info that might help us debug this: > > [ 40.073276] > rcu_scheduler_active = 1, debug_locks = 0 > [ 40.073494] 4 locks held by criu/2838: > [ 40.073635] #0: (sock_diag_mutex){+.+.+.}, at: > [<ffffffff81689afb>] sock_diag_rcv+0x1b/0x40 > [ 40.074226] #1: (sock_diag_table_mutex){+.+.+.}, at: > [<ffffffff81689c6d>] sock_diag_rcv_msg+0x6d/0x140 > [ 40.074803] #2: (nlk->cb_mutex){+.+.+.}, at: [<ffffffff816a28f1>] > netlink_dump+0x21/0x2d0 > [ 40.075382] #3: (nl_table_lock){.+.?..}, at: [<ffffffffa0286601>] > netlink_diag_dump+0x31/0xb0 [netlink_diag] > [ 40.076351] Looks like we should hold rcu_read_lock() before calling __netlink_diag_dump(). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage! 2014-08-06 17:52 ` Cong Wang @ 2014-08-06 19:51 ` Thomas Graf 2014-08-06 21:20 ` Cong Wang 0 siblings, 1 reply; 6+ messages in thread From: Thomas Graf @ 2014-08-06 19:51 UTC (permalink / raw) To: Cong Wang; +Cc: Andrey Wagin, netdev On 08/06/14 at 10:52am, Cong Wang wrote: > On Wed, Aug 6, 2014 at 10:29 AM, Andrey Wagin <avagin@gmail.com> wrote: > > [ 40.072199] =============================== > > [ 40.072350] [ INFO: suspicious RCU usage. ] > > [ 40.072501] 3.16.0-next-20140806 #1 Not tainted > > [ 40.072659] ------------------------------- > > [ 40.072807] net/netlink/diag.c:106 suspicious > > rcu_dereference_protected() usage! > > [ 40.073047] > > other info that might help us debug this: > > > > [ 40.073276] > > rcu_scheduler_active = 1, debug_locks = 0 > > [ 40.073494] 4 locks held by criu/2838: > > [ 40.073635] #0: (sock_diag_mutex){+.+.+.}, at: > > [<ffffffff81689afb>] sock_diag_rcv+0x1b/0x40 > > [ 40.074226] #1: (sock_diag_table_mutex){+.+.+.}, at: > > [<ffffffff81689c6d>] sock_diag_rcv_msg+0x6d/0x140 > > [ 40.074803] #2: (nlk->cb_mutex){+.+.+.}, at: [<ffffffff816a28f1>] > > netlink_dump+0x21/0x2d0 > > [ 40.075382] #3: (nl_table_lock){.+.?..}, at: [<ffffffffa0286601>] > > netlink_diag_dump+0x31/0xb0 [netlink_diag] > > [ 40.076351] > > > Looks like we should hold rcu_read_lock() before calling __netlink_diag_dump(). netlink_diag_dump() still acquires nl_table_lock which is pointless as a separate mutex has been introduced to protect mutations. I will send a patch to RCU'ify it properly. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage! 2014-08-06 19:51 ` Thomas Graf @ 2014-08-06 21:20 ` Cong Wang 2014-08-06 23:18 ` [PATCH net] netlink: hold nl_sock_hash_lock during diag dump Thomas Graf 0 siblings, 1 reply; 6+ messages in thread From: Cong Wang @ 2014-08-06 21:20 UTC (permalink / raw) To: Thomas Graf; +Cc: Andrey Wagin, netdev [-- Attachment #1: Type: text/plain, Size: 477 bytes --] On Wed, Aug 6, 2014 at 12:51 PM, Thomas Graf <tgraf@suug.ch> wrote: > On 08/06/14 at 10:52am, Cong Wang wrote: >> >> Looks like we should hold rcu_read_lock() before calling __netlink_diag_dump(). > > netlink_diag_dump() still acquires nl_table_lock which is pointless as > a separate mutex has been introduced to protect mutations. I will send > a patch to RCU'ify it properly. Agreed, but that's likely a net-next material. I think we need the following quick fix for net. [-- Attachment #2: netlink-diag.diff --] [-- Type: text/plain, Size: 1268 bytes --] diff --git a/net/netlink/diag.c b/net/netlink/diag.c index 7301850..2d94c49 100644 --- a/net/netlink/diag.c +++ b/net/netlink/diag.c @@ -103,7 +103,7 @@ static int __netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb, { struct netlink_table *tbl = &nl_table[protocol]; struct rhashtable *ht = &tbl->hash; - const struct bucket_table *htbl = rht_dereference(ht->tbl, ht); + const struct bucket_table *htbl; struct net *net = sock_net(skb->sk); struct netlink_diag_req *req; struct netlink_sock *nlsk; @@ -112,8 +112,11 @@ static int __netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb, req = nlmsg_data(cb->nlh); + rcu_read_lock(); + htbl = rht_dereference_rcu(ht->tbl, ht); + for (i = 0; i < htbl->size; i++) { - rht_for_each_entry(nlsk, htbl->buckets[i], ht, node) { + rht_for_each_entry_rcu(nlsk, htbl->buckets[i], node) { sk = (struct sock *)nlsk; if (!net_eq(sock_net(sk), net)) @@ -129,12 +132,14 @@ static int __netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb, NLM_F_MULTI, sock_i_ino(sk)) < 0) { ret = 1; + rcu_read_unlock(); goto done; } num++; } } + rcu_read_unlock(); sk_for_each_bound(sk, &tbl->mc_list) { if (sk_hashed(sk)) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net] netlink: hold nl_sock_hash_lock during diag dump 2014-08-06 21:20 ` Cong Wang @ 2014-08-06 23:18 ` Thomas Graf 2014-08-07 2:18 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Thomas Graf @ 2014-08-06 23:18 UTC (permalink / raw) To: Cong Wang, davem; +Cc: Andrey Wagin, netdev On 08/06/14 at 02:20pm, Cong Wang wrote: > On Wed, Aug 6, 2014 at 12:51 PM, Thomas Graf <tgraf@suug.ch> wrote: > > On 08/06/14 at 10:52am, Cong Wang wrote: > >> > >> Looks like we should hold rcu_read_lock() before calling __netlink_diag_dump(). > > > > netlink_diag_dump() still acquires nl_table_lock which is pointless as > > a separate mutex has been introduced to protect mutations. I will send > > a patch to RCU'ify it properly. > > Agreed, but that's likely a net-next material. > > I think we need the following quick fix for net. I came up with the exact same patch as you first but eventually figured holding the mutex directly as below ias actually better in this case: [PATCH net] netlink: hold nl_sock_hash_lock during diag dump Although RCU protection would be possible during diag dump, doing so allows for concurrent table mutations which can render the in-table offset between individual Netlink messages invalid and thus cause legitimate sockets to be skipped in the dump. Since the diag dump is relatively low volume and consistency is more important than performance, the table mutex is held during dump. Reported-by: Andrey Wagin <avagin@gmail.com> Signed-off-by: Thomas Graf <tgraf@suug.ch> Fixes: e341694e3eb57fc ("netlink: Convert netlink_lookup() to use RCU protected hash table") --- net/netlink/af_netlink.c | 1 + net/netlink/af_netlink.h | 1 + net/netlink/diag.c | 3 +++ 3 files changed, 5 insertions(+) diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 479a344..a324b4b 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -104,6 +104,7 @@ static atomic_t nl_table_users = ATOMIC_INIT(0); /* Protects netlink socket hash table mutations */ DEFINE_MUTEX(nl_sk_hash_lock); +EXPORT_SYMBOL_GPL(nl_sk_hash_lock); static int lockdep_nl_sk_hash_is_held(void) { diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h index 60f631f..b20a173 100644 --- a/net/netlink/af_netlink.h +++ b/net/netlink/af_netlink.h @@ -73,5 +73,6 @@ struct netlink_table { extern struct netlink_table *nl_table; extern rwlock_t nl_table_lock; +extern struct mutex nl_sk_hash_lock; #endif diff --git a/net/netlink/diag.c b/net/netlink/diag.c index 7301850..de8c74a 100644 --- a/net/netlink/diag.c +++ b/net/netlink/diag.c @@ -170,6 +170,7 @@ static int netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) req = nlmsg_data(cb->nlh); + mutex_lock(&nl_sk_hash_lock); read_lock(&nl_table_lock); if (req->sdiag_protocol == NDIAG_PROTO_ALL) { @@ -183,6 +184,7 @@ static int netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) } else { if (req->sdiag_protocol >= MAX_LINKS) { read_unlock(&nl_table_lock); + mutex_unlock(&nl_sk_hash_lock); return -ENOENT; } @@ -190,6 +192,7 @@ static int netlink_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) } read_unlock(&nl_table_lock); + mutex_unlock(&nl_sk_hash_lock); return skb->len; } -- 1.9.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] netlink: hold nl_sock_hash_lock during diag dump 2014-08-06 23:18 ` [PATCH net] netlink: hold nl_sock_hash_lock during diag dump Thomas Graf @ 2014-08-07 2:18 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2014-08-07 2:18 UTC (permalink / raw) To: tgraf; +Cc: cwang, avagin, netdev From: Thomas Graf <tgraf@suug.ch> Date: Thu, 7 Aug 2014 00:18:47 +0100 > [PATCH net] netlink: hold nl_sock_hash_lock during diag dump > > Although RCU protection would be possible during diag dump, doing > so allows for concurrent table mutations which can render the > in-table offset between individual Netlink messages invalid and > thus cause legitimate sockets to be skipped in the dump. > > Since the diag dump is relatively low volume and consistency is > more important than performance, the table mutex is held during > dump. > > Reported-by: Andrey Wagin <avagin@gmail.com> > Signed-off-by: Thomas Graf <tgraf@suug.ch> > Fixes: e341694e3eb57fc ("netlink: Convert netlink_lookup() to use RCU protected hash table") Applied, thanks Thomas. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-07 2:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-06 17:29 linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage! Andrey Wagin 2014-08-06 17:52 ` Cong Wang 2014-08-06 19:51 ` Thomas Graf 2014-08-06 21:20 ` Cong Wang 2014-08-06 23:18 ` [PATCH net] netlink: hold nl_sock_hash_lock during diag dump Thomas Graf 2014-08-07 2:18 ` 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.