Re: linux-next: net/netlink/diag.c:106 suspicious rcu_dereference_protected() usage!

[Cong Wang <cwang@twopensource.com>]

[-- 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))