netdev - [PATCH net] netlink: hold nl_sock_hash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140806231847.GA3703@casper.infradead.org>
Date:	Thu, 7 Aug 2014 00:18:47 +0100
From:	Thomas Graf <tgraf@...g.ch>
To:	Cong Wang <cwang@...pensource.com>, davem@...emloft.net
Cc:	Andrey Wagin <avagin@...il.com>, netdev <netdev@...r.kernel.org>
Subject: [PATCH net] netlink: hold nl_sock_hash_lock during diag dump

On 08/06/14 at 02:20pm, Cong Wang wrote:
> On Wed, Aug 6, 2014 at 12:51 PM, Thomas Graf <tgraf@...g.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@...il.com>
Signed-off-by: Thomas Graf <tgraf@...g.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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html