[PATCH 0/2] fix generic netlink locking issue(s)

Here's another attempt at fixing the genetlink locking issue,
hopefully better tested this time. Sorry about the mess the 
previous version caused.

I really didn't find a way to "simply" add locking, no matter
which way I turn genetlink is special because it would then use
the same locks "inside" and "outside" the generic netlink family;
"inside" because I'm trying to protect the otherwise unlocked
dump call, and "outside" because it itself is a generic netlink
family so needs to protect things there.

As a result, I've turned to RCU and (hopefully) made it safe.
The unregistration can get fairly expensive with all the calls
to synchronize_rcu(), but I don't see any better way, and it's
hopefully really rare.

The only place that now uses RCU is ctrl_dumpfamily(), but it'd
be possible to use it in other places. I didn't want to do it
in this patch, but I also don't see any candidates where that 
would really make sense. 

The first patch should obviously go into 3.11, I'll let you 
decide about the second. This one might actually be easier to
backport than the original one, but it'd still have to be done
carefully.

johannes

From: Johannes Berg <redacted>

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 <redacted>
---
 net/netlink/genetlink.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f85f8a2..512718a 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;
 }
 

-- 
1.8.4.rc2

From: Johannes Berg <redacted>

In my previous commit 58ad436fcf49810aa006016107f494c9ac9013db
("genetlink: fix family dump race") I attempted to solve an
issue in generic netlink that could lead to crashes, but it
turns out that this introduced a possibility for deadlock. As
I haven't found a way to actually add locking without causing
that, convert the family, family ops/mcast group lists all to
use RCU, so the family dump code can simply use RCU protection
instead of locking.

Signed-off-by: Johannes Berg <redacted>
---
 net/netlink/genetlink.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 512718a..2027964 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c

@@ -222,7 +222,7 @@ int genl_register_mc_group(struct genl_family *family,
 
 	grp->id = id;
 	set_bit(id, mc_groups);
-	list_add_tail(&grp->list, &family->mcast_groups);
+	list_add_tail_rcu(&grp->list, &family->mcast_groups);
 	grp->family = family;
 
 	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, grp);

@@ -246,7 +246,7 @@ static void __genl_unregister_mc_group(struct genl_family *family,
 	netlink_table_ungrab();
 
 	clear_bit(grp->id, mc_groups);
-	list_del(&grp->list);
+	list_del_rcu(&grp->list);
 	genl_ctrl_event(CTRL_CMD_DELMCAST_GRP, grp);
 	grp->id = 0;
 	grp->family = NULL;

@@ -272,6 +272,7 @@ void genl_unregister_mc_group(struct genl_family *family,
 	genl_lock_all();
 	__genl_unregister_mc_group(family, grp);
 	genl_unlock_all();
+	synchronize_rcu();
 }
 EXPORT_SYMBOL(genl_unregister_mc_group);
 

@@ -281,6 +282,7 @@ static void genl_unregister_mc_groups(struct genl_family *family)
 
 	list_for_each_entry_safe(grp, tmp, &family->mcast_groups, list)
 		__genl_unregister_mc_group(family, grp);
+	synchronize_rcu();
 }
 
 /**

@@ -318,7 +320,7 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
 	genl_lock_all();
-	list_add_tail(&ops->ops_list, &family->ops_list);
+	list_add_tail_rcu(&ops->ops_list, &family->ops_list);
 	genl_unlock_all();
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);

@@ -351,9 +353,10 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 	genl_lock_all();
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
-			list_del(&ops->ops_list);
+			list_del_rcu(&ops->ops_list);
 			genl_unlock_all();
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
+			synchronize_rcu();
 			return 0;
 		}
 	}

@@ -418,7 +421,7 @@ int genl_register_family(struct genl_family *family)
 	} else
 		family->attrbuf = NULL;
 
-	list_add_tail(&family->family_list, genl_family_chain(family->id));
+	list_add_tail_rcu(&family->family_list, genl_family_chain(family->id));
 	genl_unlock_all();
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);

@@ -498,7 +501,8 @@ int genl_unregister_family(struct genl_family *family)
 		if (family->id != rc->id || strcmp(rc->name, family->name))
 			continue;
 
-		list_del(&rc->family_list);
+		list_del_rcu(&rc->family_list);
+		synchronize_rcu();
 		INIT_LIST_HEAD(&family->ops_list);
 		genl_unlock_all();
 

@@ -692,7 +696,7 @@ static int ctrl_fill_info(struct genl_family *family, u32 portid, u32 seq,
 		if (nla_ops == NULL)
 			goto nla_put_failure;
 
-		list_for_each_entry(ops, &family->ops_list, ops_list) {
+		list_for_each_entry_rcu(ops, &family->ops_list, ops_list) {
 			struct nlattr *nest;
 
 			nest = nla_nest_start(skb, idx++);

@@ -718,7 +722,7 @@ static int ctrl_fill_info(struct genl_family *family, u32 portid, u32 seq,
 		if (nla_grps == NULL)
 			goto nla_put_failure;
 
-		list_for_each_entry(grp, &family->mcast_groups, list) {
+		list_for_each_entry_rcu(grp, &family->mcast_groups, list) {
 			struct nlattr *nest;
 
 			nest = nla_nest_start(skb, idx++);

@@ -790,9 +794,10 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int chains_to_skip = cb->args[0];
 	int fams_to_skip = cb->args[1];
 
+	rcu_read_lock();
 	for (i = chains_to_skip; i < GENL_FAM_TAB_SIZE; i++) {
 		n = 0;
-		list_for_each_entry(rt, genl_family_chain(i), family_list) {
+		list_for_each_entry_rcu(rt, genl_family_chain(i), family_list) {
 			if (!rt->netnsok && !net_eq(net, &init_net))
 				continue;
 			if (++n < fams_to_skip)

@@ -807,6 +812,8 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	}
 
 errout:
+	rcu_read_unlock();
+
 	cb->args[0] = i;
 	cb->args[1] = n;
 

-- 
1.8.4.rc2

On 21.08.2013 16:08, Johannes Berg wrote:

Here's another attempt at fixing the genetlink locking issue,
hopefully better tested this time. Sorry about the mess the 
previous version caused.

I really didn't find a way to "simply" add locking, no matter
which way I turn genetlink is special because it would then use
the same locks "inside" and "outside" the generic netlink family;
"inside" because I'm trying to protect the otherwise unlocked
dump call, and "outside" because it itself is a generic netlink
family so needs to protect things there.

As a result, I've turned to RCU and (hopefully) made it safe.
The unregistration can get fairly expensive with all the calls
to synchronize_rcu(), but I don't see any better way, and it's
hopefully really rare.

The only place that now uses RCU is ctrl_dumpfamily(), but it'd
be possible to use it in other places. I didn't want to do it
in this patch, but I also don't see any candidates where that 
would really make sense. 

The first patch should obviously go into 3.11, I'll let you 
decide about the second. This one might actually be easier to
backport than the original one, but it'd still have to be done
carefully.

I applied both patches.
The lockdep issue vanished and i still can write emails :-)

Thanks Johannes.

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

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

This looks like more general problem and not specific to genl_ctrl
family. I have couple of fixes for issues you are seeing, I will send
out those patches soon.

On Wed, Aug 21, 2013 at 12:05 PM, Oliver Hartkopp
[off-list ref] wrote:

On 21.08.2013 16:08, Johannes Berg wrote:

quoted

Here's another attempt at fixing the genetlink locking issue,
hopefully better tested this time. Sorry about the mess the
previous version caused.

I really didn't find a way to "simply" add locking, no matter
which way I turn genetlink is special because it would then use
the same locks "inside" and "outside" the generic netlink family;
"inside" because I'm trying to protect the otherwise unlocked
dump call, and "outside" because it itself is a generic netlink
family so needs to protect things there.

As a result, I've turned to RCU and (hopefully) made it safe.
The unregistration can get fairly expensive with all the calls
to synchronize_rcu(), but I don't see any better way, and it's
hopefully really rare.

The only place that now uses RCU is ctrl_dumpfamily(), but it'd
be possible to use it in other places. I didn't want to do it
in this patch, but I also don't see any candidates where that
would really make sense.

The first patch should obviously go into 3.11, I'll let you
decide about the second. This one might actually be easier to
backport than the original one, but it'd still have to be done
carefully.

I applied both patches.
The lockdep issue vanished and i still can write emails :-)

Thanks Johannes.

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

quoted

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

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

On Wed, Aug 21, 2013 at 7:08 AM, Johannes Berg
[off-list ref] wrote:

From: Johannes Berg <redacted>

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 <redacted>

Acked-by: Pravin B Shelar <redacted>

On Wed, Aug 21, 2013 at 7:08 AM, Johannes Berg
[off-list ref] wrote:

From: Johannes Berg <redacted>

In my previous commit 58ad436fcf49810aa006016107f494c9ac9013db
("genetlink: fix family dump race") I attempted to solve an
issue in generic netlink that could lead to crashes, but it
turns out that this introduced a possibility for deadlock. As
I haven't found a way to actually add locking without causing
that, convert the family, family ops/mcast group lists all to
use RCU, so the family dump code can simply use RCU protection
instead of locking.

Signed-off-by: Johannes Berg <redacted>

I send out a fix which fixes genl locking for dump operation and it
might fix this issue, Can you try this?

http://marc.info/?l=linux-netdev&m=137714389705485&w=2

On Wed, 2013-08-21 at 15:53 -0700, Pravin Shelar wrote:

This looks like more general problem and not specific to genl_ctrl
family. I have couple of fixes for issues you are seeing, I will send
out those patches soon.

No, it's not a general problem.

You don't _want_ dump generally locked, it's not really useful. Netlink
guarantees with the cb_mutex that it can't go away, so locking dump at
the generic netlink level doesn't buy the average generic netlink user
at all.

I think that the problem is that generic netlink is "inside itself" in a
manner of speaking.

OTOH, I'm not sure I fully understand what would happen if a family is
deleted while something inside of it is dumping. Even the module unload
wouldn't be prevented since netlink_dump_start() uses THIS_MODULE (which
is really NULL since generic netlink is built into the kernel), so maybe
you're right about the locking, but then your patch still wouldn't be
correct because the family might still go away even if you lock the
genl_lock() every round.

I think to fix that additional problem we need something like the
(untested!) patch below (which is missing documentation updates) in
addition to fix that problem.

johannes

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 93024a4..0bc3a53 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h

@@ -42,6 +42,7 @@ struct genl_info;
  * @ops_list: list of all assigned operations
  * @family_list: family list
  * @mcast_groups: multicast groups list
+ * @module: the module containing this family
  */
 struct genl_family {
 	unsigned int		id;

@@ -61,6 +62,7 @@ struct genl_family {
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
 	struct list_head	mcast_groups;	/* private */
+	struct module *		module;		/* private */
 };
 
 /**

@@ -121,9 +123,24 @@ struct genl_ops {
 	struct list_head	ops_list;
 };
 
-extern int genl_register_family(struct genl_family *family);
-extern int genl_register_family_with_ops(struct genl_family *family,
+extern int __genl_register_family(struct genl_family *family);
+extern int __genl_register_family_with_ops(struct genl_family *family,
 	struct genl_ops *ops, size_t n_ops);
+
+static inline int genl_register_family(struct genl_family *family)
+{
+	family->module = THIS_MODULE;
+	return __genl_register_family(family);
+}
+
+static inline int genl_register_family_with_ops(struct genl_family *family,
+						struct genl_ops *ops,
+						size_t n_ops)
+{
+	family->module = THIS_MODULE;
+	return __genl_register_family_with_ops(family, ops, n_ops);
+}
+
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 2fd6dbe..8bab493 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c

@@ -374,7 +374,7 @@ EXPORT_SYMBOL(genl_unregister_ops);
  *
  * Return 0 on success or a negative error code.
  */
-int genl_register_family(struct genl_family *family)
+int __genl_register_family(struct genl_family *family)
 {
 	int err = -EINVAL;
 

@@ -430,7 +430,7 @@ errout_locked:
 errout:
 	return err;
 }
-EXPORT_SYMBOL(genl_register_family);
+EXPORT_SYMBOL(__genl_register_family);
 
 /**
  * genl_register_family_with_ops - register a generic netlink family

@@ -457,12 +457,12 @@ EXPORT_SYMBOL(genl_register_family);
  *
  * Return 0 on success or a negative error code.
  */
-int genl_register_family_with_ops(struct genl_family *family,
+int __genl_register_family_with_ops(struct genl_family *family,
 	struct genl_ops *ops, size_t n_ops)
 {
 	int err, i;
 
-	err = genl_register_family(family);
+	err = __genl_register_family(family);
 	if (err)
 		return err;
 

@@ -476,7 +476,7 @@ err_out:
 	genl_unregister_family(family);
 	return err;
 }
-EXPORT_SYMBOL(genl_register_family_with_ops);
+EXPORT_SYMBOL(__genl_register_family_with_ops);
 
 /**
  * genl_unregister_family - unregister generic netlink family

On Wed, 2013-08-21 at 21:32 -0700, Pravin Shelar wrote:

I send out a fix which fixes genl locking for dump operation and it
might fix this issue, Can you try this?

I don't think that fix is correct. I don't have a copy, so let me reply
here.

http://marc.info/?l=linux-netdev&m=137714389705485&w=2

In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, 
But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.

I don't think either of those statements are true - dump() has alway
taken a shortcut in netlink_recvmsg() and doesn't go into genl_rcv(), so
how could your patch have changed locking? Therefore, it can't have been
expected either.

For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.

This might help for generic netlink itself, but I'm not convinced that
it's really useful for other families. I've fixed bugs like in commit
3a5a423bb958ad22eeccca66c533e85bf69ba10e, which actually made it in
after your commit, but per above I don't think it was actually a problem
introduced by your commit.

johannes

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 21 Aug 2013 16:08:02 +0200

From: Johannes Berg <redacted>

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 <redacted>

So I've applied this since everyone agrees that we should revert
this change.

How to deal with the problem we were attempting to solve is still
under discussion.  It seems that even if we go to RCU we must also
address to module reference count issue.

I think the existing locking is very messy, and RCU looks a lot
cleaner and has potential for future improvements to the scalability
of dumps.

So I'd like to propose that we combine Johannes's RCU conversion
with some variant of the module reference count fix.

Can you guys work together and come up with something I can apply?

Thanks.

On Thu, 2013-08-22 at 13:26 -0700, David Miller wrote:

How to deal with the problem we were attempting to solve is still
under discussion.  It seems that even if we go to RCU we must also
address to module reference count issue.

Yes, that's a separate issue. However, I think we should also check in
more detail the dumpit locking issue Pravin pointed out - before his
changes there was indeed the genl lock used as the cb_mutex. As I've
said over in the other thread, I'm not sure that change was actually
useful - it sounded like he confused kernel sockets and userland sockets
here? (Or maybe I am?!)

OTOH, we've already fixed the race conditions that resulted from his
patch, at least in nl80211. You might remember the issue Linus ran into
with the attrbuf, it's looking like that issue was because he changed
generic netlink to no longer use the genl_lock as the cb_mutex.

I think the existing locking is very messy, and RCU looks a lot
cleaner and has potential for future improvements to the scalability
of dumps.

I agree, though we're not all that interested in generic netlink family
scalability I think, we have less than a dozen families, so this
shouldn't really be an issue.

So I'd like to propose that we combine Johannes's RCU conversion
with some variant of the module reference count fix.

Can you guys work together and come up with something I can apply?

Sure, that in itself isn't really a problem, but if we don't take
Pravin's patch to "revert" the cb_mutex change in his parallel_ops
changes, then we definitely need to audit all generic netlink dumpit
implementations in all users to see if they have similar races to
nl80211. I originally thought that it was an nl80211 problem, but I'm
now convinced that it wasn't. Still the new code in nl80211 is probably
nicer, and we can probably make it parallel_ops now due to these changes
but I'm not convinced we can audit all genl families.

If it wasn't that so much time has already passed since the parallel_ops
changes I'd almost suggest reverting those altogether and addressing the
locking properly ...

johannes

On Thu, Aug 22, 2013 at 1:36 PM, Johannes Berg
[off-list ref] wrote:

On Thu, 2013-08-22 at 13:26 -0700, David Miller wrote:

quoted

How to deal with the problem we were attempting to solve is still
under discussion.  It seems that even if we go to RCU we must also
address to module reference count issue.

Yes, that's a separate issue. However, I think we should also check in
more detail the dumpit locking issue Pravin pointed out - before his
changes there was indeed the genl lock used as the cb_mutex. As I've
said over in the other thread, I'm not sure that change was actually
useful - it sounded like he confused kernel sockets and userland sockets
here? (Or maybe I am?!)

OTOH, we've already fixed the race conditions that resulted from his
patch, at least in nl80211. You might remember the issue Linus ran into
with the attrbuf, it's looking like that issue was because he changed
generic netlink to no longer use the genl_lock as the cb_mutex.

quoted

I think the existing locking is very messy, and RCU looks a lot
cleaner and has potential for future improvements to the scalability
of dumps.

I agree, though we're not all that interested in generic netlink family
scalability I think, we have less than a dozen families, so this
shouldn't really be an issue.

quoted

So I'd like to propose that we combine Johannes's RCU conversion
with some variant of the module reference count fix.

Can you guys work together and come up with something I can apply?

Sure, that in itself isn't really a problem, but if we don't take
Pravin's patch to "revert" the cb_mutex change in his parallel_ops
changes, then we definitely need to audit all generic netlink dumpit
implementations in all users to see if they have similar races to
nl80211. I originally thought that it was an nl80211 problem, but I'm
now convinced that it wasn't. Still the new code in nl80211 is probably
nicer, and we can probably make it parallel_ops now due to these changes
but I'm not convinced we can audit all genl families.

I have sent fixes for genl-locking.

If it wasn't that so much time has already passed since the parallel_ops
changes I'd almost suggest reverting those altogether and addressing the
locking properly ...

I think genl-parallel ops patch decouple genl-locking from netlink
lock which simplifies it. This also allows existing genl-family
gradually converted to parallel-ops. Once all are moved to parallel
ops, we can get rid of genl_mutex completely.

Passing mutex after one module to another is messy and confusing.
After genl-parallel op changes this is done only by rtnl module.
I will try to get rid of this cb_mutex parameter so that each layer
can have independent locks. I need to check rtnl module for that.