[PATCH v3 1/3] genetlink: document parallel_ops

[PATCH v3 1/3] genetlink: document parallel_ops

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The kernel-doc for the parallel_ops family struct member is
missing, add it.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/genetlink.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 84125088c309..2ea2c55bdc87 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -27,6 +27,8 @@ struct genl_info;
  * @maxattr: maximum number of attributes supported
  * @netnsok: set to true if the family can handle network
  *	namespaces and should be presented in all of them
+ * @parallel_ops: operations can be called in parallel and aren't
+ *	synchronized by the core genetlink code
  * @pre_doit: called before an operation's doit callback, it may
  *	do additional, common, filtering and return an error
  * @post_doit: called after an operation's doit callback, it may
-- 
2.1.4

[PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Jeff Layton reported that he could trigger the multicast unbind warning
in generic netlink using trinity. I originally thought it was a race
condition between unregistering the generic netlink family and closing
the socket, but there's a far simpler explanation: genetlink currently
allows subscribing to groups that don't (yet) exist, and the warning is
triggered when unsubscribing again while the group still doesn't exist.

Originally, I had a warning in the subscribe case and accepted it out of
userspace API concerns, but the warning was of course wrong and removed
later.

However, I now think that allowing userspace to subscribe to groups that
don't exist is wrong and could possibly become a security problem:
Consider a (new) genetlink family implementing a permission check in
the mcast_bind() function similar to the like the audit code does today;
it would be possible to bypass the permission check by guessing the ID
and subscribing to the group it exists. This is only possible in case a
family like that would be dynamically loaded, but it doesn't seem like a
huge stretch, for example wireless may be loaded when you plug in a USB
device.

To avoid this reject such subscription attempts.

If this ends up causing userspace issues we may need to add a workaround
in af_netlink to deny such requests but not return an error.

Reported-by: Jeff Layton <jeff.layton@primarydata.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/netlink/genetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 2e11061ef885..c18d3f5624b2 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -985,7 +985,7 @@ static struct genl_multicast_group genl_ctrl_groups[] = {
 
 static int genl_bind(struct net *net, int group)
 {
-	int i, err = 0;
+	int i, err = -ENOENT;
 
 	down_read(&cb_lock);
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
-- 
2.1.4

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 16 Jan 2015 11:37:13 +0100

> From: Johannes Berg <johannes.berg@intel.com>
> 
> Jeff Layton reported that he could trigger the multicast unbind warning
> in generic netlink using trinity. I originally thought it was a race
> condition between unregistering the generic netlink family and closing
> the socket, but there's a far simpler explanation: genetlink currently
> allows subscribing to groups that don't (yet) exist, and the warning is
> triggered when unsubscribing again while the group still doesn't exist.
> 
> Originally, I had a warning in the subscribe case and accepted it out of
> userspace API concerns, but the warning was of course wrong and removed
> later.
> 
> However, I now think that allowing userspace to subscribe to groups that
> don't exist is wrong and could possibly become a security problem:
> Consider a (new) genetlink family implementing a permission check in
> the mcast_bind() function similar to the like the audit code does today;
> it would be possible to bypass the permission check by guessing the ID
> and subscribing to the group it exists. This is only possible in case a
> family like that would be dynamically loaded, but it doesn't seem like a
> huge stretch, for example wireless may be loaded when you plug in a USB
> device.
> 
> To avoid this reject such subscription attempts.
> 
> If this ends up causing userspace issues we may need to add a workaround
> in af_netlink to deny such requests but not return an error.
> 
> Reported-by: Jeff Layton <jeff.layton@primarydata.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Applied.

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Bjørn Mork]

Johannes Berg <johannes@sipsolutions.net> writes:

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 2e11061ef885..c18d3f5624b2 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -985,7 +985,7 @@ static struct genl_multicast_group genl_ctrl_groups[] = {
>  
>  static int genl_bind(struct net *net, int group)
>  {
> -	int i, err = 0;
> +	int i, err = -ENOENT;
>  
>  	down_read(&cb_lock);
>  	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {

This change cause serious problems for acpid, as reported on
https://bugzilla.kernel.org/show_bug.cgi?id=92121

The end user visible result is that acpid fails to detect events like AC
power disconnect, battery alarms etc, which can be a bit of a problem if
you trust a battery alarm to trigger an emergency hibernation.  As I
happen to do...

Given the timing, I request that this patch is reverted for v3.19.
Thanks.

Sample acpid debug output, tracing the socket calls, running on a plain
v3.19-rc7:

nemi:/tmp# strace -s 128 -e trace=socket,bind,sendmsg,recvmsg -f acpid -l -d
socket(PF_LOCAL, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 3
Deprecated /proc/acpi/event was not found.  Trying netlink and the input layer...
input layer /dev/input/event0 (AT Translated Set 2 keyboard) opened successfully, fd 4
input layer /dev/input/event1 (Lid Switch) opened successfully, fd 5
input layer /dev/input/event10 (HDA Intel Dock Headphone) opened successfully, fd 6
input layer /dev/input/event11 (HDA Intel Headphone) opened successfully, fd 7
input layer /dev/input/event2 (Sleep Button) opened successfully, fd 8
input layer /dev/input/event3 (Power Button) opened successfully, fd 9
input layer /dev/input/event4 (ThinkPad Extra Buttons) opened successfully, fd 10
input layer /dev/input/event6 (Video Bus) opened successfully, fd 11
input layer /dev/input/event8 (HDA Intel Mic) opened successfully, fd 12
input layer /dev/input/event9 (HDA Intel Dock Mic) opened successfully, fd 13
inotify fd: 14
inotify wd: 1
socket(PF_NETLINK, SOCK_RAW|SOCK_CLOEXEC, NETLINK_GENERIC) = 15
bind(15, {sa_family=AF_NETLINK, pid=0, groups=00000000}, 12) = 0
sendmsg(15, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"$\0\0\0\20\0\5\0\21\10\322T\0\0\0\0\3\0\0\0\17\0\2\0acpi_event\0\0", 36}], msg_controllen=0, msg_flags=0}, 0) = 36
recvmsg(15, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"h\0\0\0\20\0\0\0\21\10\322T\314l\0\0\1\2\0\0\17\0\2\0acpi_event\0\0\6\0\1\0\23\0\0\0\10\0\3\0\1\0\0\0\10\0\4\0\0\0\0\0\10\0\5\0\1\0\0\0$\0\7\0 \0\1\0\10\0\2\0\2\0\0\0\22\0\1\0acpi_mc_group\0\0\0", 16384}], msg_controllen=0, msg_flags=MSG_CMSG_CLOEXEC}, MSG_CMSG_CLOEXEC) = 104
socket(PF_NETLINK, SOCK_RAW|SOCK_CLOEXEC, NETLINK_GENERIC) = 15
bind(15, {sa_family=AF_NETLINK, pid=0, groups=00000002}, 12) = -1 ENOENT (No such file or directory)
Cannot bind netlink socket: No such file or directory
acpid: cannot open generic netlink socket
socket(PF_LOCAL, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0) = 16
bind(16, {sa_family=AF_LOCAL, sun_path="/var/run/acpid.socket"}, 110) = 0
acpid: starting up with netlink and the input layer
parsing conf file /etc/acpi/events/powerbtn-acpi-support
acpid: skipping non-file /etc/acpi/events/CVS
parsing conf file /etc/acpi/events/any
parsing conf file /etc/acpi/events/generic-hibernatebtn
parsing conf file /etc/acpi/events/low_battery
parsing conf file /etc/acpi/events/lidbtn
parsing conf file /etc/acpi/events/sleepbtn
acpid: 6 rules loaded
acpid: waiting for events: event logging is on



Bjørn

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:
> Johannes Berg <johannes@sipsolutions.net> writes:
>
>> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
>> index 2e11061ef885..c18d3f5624b2 100644
>> --- a/net/netlink/genetlink.c
>> +++ b/net/netlink/genetlink.c
>> @@ -985,7 +985,7 @@ static struct genl_multicast_group genl_ctrl_groups[] = {
>>  
>>  static int genl_bind(struct net *net, int group)
>>  {
>> -	int i, err = 0;
>> +	int i, err = -ENOENT;
>>  
>>  	down_read(&cb_lock);
>>  	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
>
> This change cause serious problems for acpid, as reported on
> https://bugzilla.kernel.org/show_bug.cgi?id=92121

Ah, I see this bug is already fixed by commit 8b7c36d810c6 ("netlink:
fix wrong subscription bitmask to group mapping in").  Your change was
obviously correct, and found the long standing off by one bug.  Thanks.

Sorry about the noise.  I should have checked the current "net" first.


Bjørn

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Johannes Berg]

On Wed, 2015-02-04 at 16:36 +0100, Bjørn Mork wrote:

> >> -	int i, err = 0;
> >> +	int i, err = -ENOENT;
> >>  
> >>  	down_read(&cb_lock);
> >>  	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
> >
> > This change cause serious problems for acpid, as reported on
> > https://bugzilla.kernel.org/show_bug.cgi?id=92121
> 
> Ah, I see this bug is already fixed by commit 8b7c36d810c6 ("netlink:
> fix wrong subscription bitmask to group mapping in").  Your change was
> obviously correct, and found the long standing off by one bug.  Thanks.
> 
> Sorry about the noise.  I should have checked the current "net" first.

Interesting. I was completely willing to entertain the notion that some
userspace might be broken and be attempting to subscribe to a (static
through the hacks we had to put in or "I think I know it already")
group.

Have you checked acpid with the bitmap fix?

johannes

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Bjørn Mork]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2015-02-04 at 16:36 +0100, Bjørn Mork wrote:
>
>> >> -	int i, err = 0;
>> >> +	int i, err = -ENOENT;
>> >>  
>> >>  	down_read(&cb_lock);
>> >>  	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
>> >
>> > This change cause serious problems for acpid, as reported on
>> > https://bugzilla.kernel.org/show_bug.cgi?id=92121
>> 
>> Ah, I see this bug is already fixed by commit 8b7c36d810c6 ("netlink:
>> fix wrong subscription bitmask to group mapping in").  Your change was
>> obviously correct, and found the long standing off by one bug.  Thanks.
>> 
>> Sorry about the noise.  I should have checked the current "net" first.
>
> Interesting. I was completely willing to entertain the notion that some
> userspace might be broken and be attempting to subscribe to a (static
> through the hacks we had to put in or "I think I know it already")
> group.

Me to.  So I went through the whole loop, checking that acpid did
everything by the book, adding a debug message to genl_bind() only to
see that it was called with '1' instead of the expected '2'.  Then
looking at af_netlink.c and its history. Etc.

> Have you checked acpid with the bitmap fix?

No, not yet. But I went down far enough into this that I actually wrote
the exact same patch as Pablo. Had to scratch my head when I couldn't
cherry-pick it into net because it was already there :-)

So I am pretty sure Pablo's patch fixes the problem.

This was never noticed before because there are only two users,
netfilter and generic netlink,  and both were willing to accept the
off-by-one values without much fuzz.


Bjørn

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:

> So I am pretty sure Pablo's patch fixes the problem.

Confirmed now.

Running acpid on v3.19-rc7 + commit 8b7c36d810c6 ("netlink: fix
wrong subscription bitmask to group mapping in") works fine:

nemi:/tmp# strace -s 128 -e trace=socket,bind,sendmsg,recvmsg -f acpid -l -d
socket(PF_LOCAL, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 3
Deprecated /proc/acpi/event was not found.  Trying netlink and the input layer...
input layer /dev/input/event0 (AT Translated Set 2 keyboard) opened successfully, fd 4
input layer /dev/input/event1 (Lid Switch) opened successfully, fd 5
input layer /dev/input/event10 (HDA Intel Headphone) opened successfully, fd 6
input layer /dev/input/event2 (Sleep Button) opened successfully, fd 7
input layer /dev/input/event3 (Power Button) opened successfully, fd 8
input layer /dev/input/event4 (Video Bus) opened successfully, fd 9
input layer /dev/input/event5 (ThinkPad Extra Buttons) opened successfully, fd 10
input layer /dev/input/event7 (HDA Intel Mic) opened successfully, fd 11
input layer /dev/input/event8 (HDA Intel Dock Mic) opened successfully, fd 12
input layer /dev/input/event9 (HDA Intel Dock Headphone) opened successfully, fd 13
inotify fd: 14
inotify wd: 1
socket(PF_NETLINK, SOCK_RAW|SOCK_CLOEXEC, NETLINK_GENERIC) = 15
bind(15, {sa_family=AF_NETLINK, pid=0, groups=00000000}, 12) = 0
sendmsg(15, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"$\0\0\0\20\0\5\0oD\322T\0\0\0\0\3\0\0\0\17\0\2\0acpi_event\0\0", 36}], msg_controllen=0, msg_flags=0}, 0) = 36
recvmsg(15, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"h\0\0\0\20\0\0\0oD\322T@\20\0\0\1\2\0\0\17\0\2\0acpi_event\0\0\6\0\1\0\23\0\0\0\10\0\3\0\1\0\0\0\10\0\4\0\0\0\0\0\10\0\5\0\1\0\0\0$\0\7\0 \0\1\0\10\0\2\0\2\0\0\0\22\0\1\0acpi_mc_group\0\0\0", 16384}], msg_controllen=0, msg_flags=MSG_CMSG_CLOEXEC}, MSG_CMSG_CLOEXEC) = 104
socket(PF_NETLINK, SOCK_RAW|SOCK_CLOEXEC, NETLINK_GENERIC) = 15
bind(15, {sa_family=AF_NETLINK, pid=0, groups=00000002}, 12) = 0
netlink opened successfully
socket(PF_LOCAL, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0) = 16
bind(16, {sa_family=AF_LOCAL, sun_path="/var/run/acpid.socket"}, 110) = 0
acpid: starting up with netlink and the input layer
parsing conf file /etc/acpi/events/powerbtn-acpi-support
acpid: skipping non-file /etc/acpi/events/CVS
parsing conf file /etc/acpi/events/any
parsing conf file /etc/acpi/events/generic-hibernatebtn
parsing conf file /etc/acpi/events/low_battery
parsing conf file /etc/acpi/events/lidbtn
parsing conf file /etc/acpi/events/sleepbtn
acpid: 6 rules loaded
acpid: waiting for events: event logging is on



And the acpi events are of course received as expected.  Thanks again
for making the kernel more robust.



Bjørn

Re: [PATCH v3 2/3] genetlink: disallow subscribing to unknown mcast groups

[Johannes Berg]

On Wed, 2015-02-04 at 16:55 +0100, Bjørn Mork wrote:

> This was never noticed before because there are only two users,
> netfilter and generic netlink,  and both were willing to accept the
> off-by-one values without much fuzz.

Generic netlink didn't even have it until my patchset.

I think audit also uses the callback, but only for permissions checks
and doesn't care about the group at all.

johannes

[PATCH v3 3/3] genetlink: synchronize socket closing and family removal

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

In addition to the problem Jeff Layton reported, I looked at the code
and reproduced the same warning by subscribing and removing the genl
family with a socket still open. This is a fairly tricky race which
originates in the fact that generic netlink allows the family to go
away while sockets are still open - unlike regular netlink which has
a module refcount for every open socket so in general this cannot be
triggered.

Trying to resolve this issue by the obvious locking isn't possible as
it will result in deadlocks between unregistration and group unbind
notification (which incidentally lockdep doesn't find due to the home
grown locking in the netlink table.)

To really resolve this, introduce a "closing socket" reference counter
(for generic netlink only, as it's the only affected family) in the
core netlink code and use that in generic netlink to wait for all the
sockets that are being closed at the same time as a generic netlink
family is removed.

This fixes the race that when a socket is closed, it will should call
the unbind, but if the family is removed at the same time the unbind
will not find it, leading to the warning. The real problem though is
that in this case the unbind could actually find a new family that is
registered to have a multicast group with the same ID, and call its
mcast_unbind() leading to confusing.

Also remove the warning since it would still trigger, but is now no
longer a problem.

This also moves the code in af_netlink.c to before unreferencing the
module to avoid having the same problem in the normal non-genl case.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/genetlink.h |  4 ++++
 include/net/genetlink.h   |  5 ++++-
 net/netlink/af_netlink.c  | 24 +++++++++++++++++-------
 net/netlink/af_netlink.h  |  1 +
 net/netlink/genetlink.c   | 16 +++++++++-------
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/include/linux/genetlink.h b/include/linux/genetlink.h
index 55b685719d52..09460d6d6682 100644
--- a/include/linux/genetlink.h
+++ b/include/linux/genetlink.h
@@ -11,6 +11,10 @@ extern void genl_unlock(void);
 extern int lockdep_genl_is_held(void);
 #endif
 
+/* for synchronisation between af_netlink and genetlink */
+extern atomic_t genl_sk_destructing_cnt;
+extern wait_queue_head_t genl_sk_destructing_waitq;
+
 /**
  * rcu_dereference_genl - rcu_dereference with debug checking
  * @p: The pointer to read, prior to dereferencing
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 2ea2c55bdc87..6c92415311ca 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -35,7 +35,10 @@ struct genl_info;
  *	undo operations done by pre_doit, for example release locks
  * @mcast_bind: a socket bound to the given multicast group (which
  *	is given as the offset into the groups array)
- * @mcast_unbind: a socket was unbound from the given multicast group
+ * @mcast_unbind: a socket was unbound from the given multicast group.
+ *	Note that unbind() will not be called symmetrically if the
+ *	generic netlink family is removed while there are still open
+ *	sockets.
  * @attrbuf: buffer to store parsed attributes
  * @family_list: family list
  * @mcgrps: multicast groups used by this family (private)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 01b702d63457..0a91ea36ac55 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -61,6 +61,7 @@
 #include <linux/rhashtable.h>
 #include <asm/cacheflush.h>
 #include <linux/hash.h>
+#include <linux/genetlink.h>
 
 #include <net/net_namespace.h>
 #include <net/sock.h>
@@ -1089,6 +1090,8 @@ static void netlink_remove(struct sock *sk)
 		__sk_del_bind_node(sk);
 		netlink_update_listeners(sk);
 	}
+	if (sk->sk_protocol == NETLINK_GENERIC)
+		atomic_inc(&genl_sk_destructing_cnt);
 	netlink_table_ungrab();
 }
 
@@ -1212,6 +1215,20 @@ static int netlink_release(struct socket *sock)
 	 * will be purged.
 	 */
 
+	/* must not acquire netlink_table_lock in any way again before unbind
+	 * and notifying genetlink is done as otherwise it might deadlock
+	 */
+	if (nlk->netlink_unbind) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, nlk->groups))
+				nlk->netlink_unbind(sock_net(sk), i + 1);
+	}
+	if (sk->sk_protocol == NETLINK_GENERIC &&
+	    atomic_dec_return(&genl_sk_destructing_cnt) == 0)
+		wake_up(&genl_sk_destructing_waitq);
+
 	sock->sk = NULL;
 	wake_up_interruptible_all(&nlk->wait);
 
@@ -1247,13 +1264,6 @@ static int netlink_release(struct socket *sock)
 		netlink_table_ungrab();
 	}
 
-	if (nlk->netlink_unbind) {
-		int i;
-
-		for (i = 0; i < nlk->ngroups; i++)
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_unbind(sock_net(sk), i + 1);
-	}
 	kfree(nlk->groups);
 	nlk->groups = NULL;
 
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 7518375782f5..89008405d6b4 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -2,6 +2,7 @@
 #define _AF_NETLINK_H
 
 #include <linux/rhashtable.h>
+#include <linux/atomic.h>
 #include <net/sock.h>
 
 #define NLGRPSZ(x)	(ALIGN(x, sizeof(unsigned long) * 8) / 8)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index c18d3f5624b2..ee57459fc258 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -23,6 +23,9 @@
 static DEFINE_MUTEX(genl_mutex); /* serialization of message processing */
 static DECLARE_RWSEM(cb_lock);
 
+atomic_t genl_sk_destructing_cnt = ATOMIC_INIT(0);
+DECLARE_WAIT_QUEUE_HEAD(genl_sk_destructing_waitq);
+
 void genl_lock(void)
 {
 	mutex_lock(&genl_mutex);
@@ -435,15 +438,18 @@ int genl_unregister_family(struct genl_family *family)
 
 	genl_lock_all();
 
-	genl_unregister_mc_groups(family);
-
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
 		if (family->id != rc->id || strcmp(rc->name, family->name))
 			continue;
 
+		genl_unregister_mc_groups(family);
+
 		list_del(&rc->family_list);
 		family->n_ops = 0;
-		genl_unlock_all();
+		up_write(&cb_lock);
+		wait_event(genl_sk_destructing_waitq,
+			   atomic_read(&genl_sk_destructing_cnt) == 0);
+		genl_unlock();
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family, NULL, 0);
@@ -1014,7 +1020,6 @@ static int genl_bind(struct net *net, int group)
 static void genl_unbind(struct net *net, int group)
 {
 	int i;
-	bool found = false;
 
 	down_read(&cb_lock);
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
@@ -1027,14 +1032,11 @@ static void genl_unbind(struct net *net, int group)
 
 				if (f->mcast_unbind)
 					f->mcast_unbind(net, fam_grp);
-				found = true;
 				break;
 			}
 		}
 	}
 	up_read(&cb_lock);
-
-	WARN_ON(!found);
 }
 
 static int __net_init genl_pernet_init(struct net *net)
-- 
2.1.4

Re: [PATCH v3 3/3] genetlink: synchronize socket closing and family removal

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 16 Jan 2015 11:37:14 +0100

> From: Johannes Berg <johannes.berg@intel.com>
> 
> In addition to the problem Jeff Layton reported, I looked at the code
> and reproduced the same warning by subscribing and removing the genl
> family with a socket still open. This is a fairly tricky race which
> originates in the fact that generic netlink allows the family to go
> away while sockets are still open - unlike regular netlink which has
> a module refcount for every open socket so in general this cannot be
> triggered.
> 
> Trying to resolve this issue by the obvious locking isn't possible as
> it will result in deadlocks between unregistration and group unbind
> notification (which incidentally lockdep doesn't find due to the home
> grown locking in the netlink table.)
> 
> To really resolve this, introduce a "closing socket" reference counter
> (for generic netlink only, as it's the only affected family) in the
> core netlink code and use that in generic netlink to wait for all the
> sockets that are being closed at the same time as a generic netlink
> family is removed.
> 
> This fixes the race that when a socket is closed, it will should call
> the unbind, but if the family is removed at the same time the unbind
> will not find it, leading to the warning. The real problem though is
> that in this case the unbind could actually find a new family that is
> registered to have a multicast group with the same ID, and call its
> mcast_unbind() leading to confusing.
> 
> Also remove the warning since it would still trigger, but is now no
> longer a problem.
> 
> This also moves the code in af_netlink.c to before unreferencing the
> module to avoid having the same problem in the normal non-genl case.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Applied.

Re: [PATCH v3 1/3] genetlink: document parallel_ops

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 16 Jan 2015 11:37:12 +0100

> From: Johannes Berg <johannes.berg@intel.com>
> 
> The kernel-doc for the parallel_ops family struct member is
> missing, add it.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Applied.