[GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

I am currently trying to write a module which communicates with user space using NETLINK_GENERIC. This module (dev_mgr) manages virtual devices which are also supposed to use genetlink for communication with user space.

I want to do something like that:
dev_mgr <- receives message from user space to create new device
dev_mgr	inside 'doit' handler:
	1. creates device
	2. registers new genetlink family for the device
	3. returns family name and id to user

This should work similarly for device removal.

After few reboots I found out that 2. blocks on genl_mutex, since this mutex is already acquired when genl_register_family is called (by genl_rcv).

I do not see why registering new family (when processing message for another family) should be a problem. In fact from genl_lock and genl_trylock occurrence it seems that genl_mutex is mostly used for syncing access to family list and also for message processing.
Since I am not (yet) familiar enough with (ge)netlink internals I am asking:
Would it make sense to split the mutex into two, one for family list and one for messaging, so it would be possible to change families when processing the message?

Simple split could introduce possible danger of user removing family inside processing of the message for this particular family, but would this really be a danger?

--
Richard

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Patrick McHardy]

Richard MUSIL wrote:
> I am currently trying to write a module which communicates with user space using NETLINK_GENERIC. This module (dev_mgr) manages virtual devices which are also supposed to use genetlink for communication with user space.
>
> I want to do something like that:
> dev_mgr <- receives message from user space to create new device
> dev_mgr	inside 'doit' handler:
> 	1. creates device
> 	2. registers new genetlink family for the device
> 	3. returns family name and id to user
>
> This should work similarly for device removal.
>
> After few reboots I found out that 2. blocks on genl_mutex, since this mutex is already acquired when genl_register_family is called (by genl_rcv).
>
> I do not see why registering new family (when processing message for another family) should be a problem. In fact from genl_lock and genl_trylock occurrence it seems that genl_mutex is mostly used for syncing access to family list and also for message processing.
> Since I am not (yet) familiar enough with (ge)netlink internals I am asking:
> Would it make sense to split the mutex into two, one for family list and one for messaging, so it would be possible to change families when processing the message?
>
> Simple split could introduce possible danger of user removing family inside processing of the message for this particular family, but would this really be a danger?
>   

The usual way to do this for auto-loading of modules that register
things that take a mutex that is already held during netlink queue
processing, like qdiscs, classifiers, .. is:

- look for <qdisc/classifier/...>, if not found:
- drop mutex (using the __ unlock variant to avoid reentering queue 
processing)
- perform module loading (which takes the mutex and registers itself)
- grab mutex again
- look for <qdisc/classifier/...> again
- if not found return -ENOENT
- if found drop reference, return -EAGAIN

The caller is changed to handle -EAGAIN by replaying the entire
request. Your problem sounds very similar, look at net/sched/sch_api.c
for an example.

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Patrick McHardy wrote:
> Richard MUSIL wrote:
>> I am currently trying to write a module which communicates with user
>> space using NETLINK_GENERIC. This module (dev_mgr) manages virtual
>> devices which are also supposed to use genetlink for communication
>> with user space.
>>
>> I want to do something like that:
>> dev_mgr <- receives message from user space to create new device
>> dev_mgr    inside 'doit' handler:
>>     1. creates device
>>     2. registers new genetlink family for the device
>>     3. returns family name and id to user
>>
>> This should work similarly for device removal.
>>
>> After few reboots I found out that 2. blocks on genl_mutex, since this
>> mutex is already acquired when genl_register_family is called (by
>> genl_rcv).
>>
>> I do not see why registering new family (when processing message for
>> another family) should be a problem. In fact from genl_lock and
>> genl_trylock occurrence it seems that genl_mutex is mostly used for
>> syncing access to family list and also for message processing.
>> Since I am not (yet) familiar enough with (ge)netlink internals I am
>> asking:
>> Would it make sense to split the mutex into two, one for family list
>> and one for messaging, so it would be possible to change families when
>> processing the message?
>>
>> Simple split could introduce possible danger of user removing family
>> inside processing of the message for this particular family, but would
>> this really be a danger?
>>   
> 
> The usual way to do this for auto-loading of modules that register
> things that take a mutex that is already held during netlink queue
> processing, like qdiscs, classifiers, .. is:
> 
> - look for <qdisc/classifier/...>, if not found:
> - drop mutex (using the __ unlock variant to avoid reentering queue
> processing)
> - perform module loading (which takes the mutex and registers itself)
> - grab mutex again
> - look for <qdisc/classifier/...> again
> - if not found return -ENOENT
> - if found drop reference, return -EAGAIN
> 
> The caller is changed to handle -EAGAIN by replaying the entire
> request. Your problem sounds very similar, look at net/sched/sch_api.c
> for an example.

The aforementioned mutex is local to genetlink module, so I cannot temporarily drop it, call the stuff and grab it again (which was mine original thought too).
In fact the only way to go around (without changing the genetlink) seems to schedule the family registration to some other context outside message processing. But this would be clearly much more complex than doing it directly in message handler and also a bit against "ease of use" which genetlink is supposed to offer.

My question was if its really necessary to sync both message processing and genetlink family management on one primitive. I believe it is not, but I would rather be happy if someone who maintains it confirm this theory. Meanwhile I am going to do quick mod to genetlink and if it goes well, post the patch, which seems to be quite simple.

--
Richard

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Patrick McHardy]

[ Please quote and break your lines appropriately ]

Richard MUSIL wrote:
> Patrick McHardy wrote:
>>
>>The usual way to do this for auto-loading of modules that register
>>things that take a mutex that is already held during netlink queue
>>processing, like qdiscs, classifiers, .. is:
>>
>>- look for <qdisc/classifier/...>, if not found:
>>- drop mutex (using the __ unlock variant to avoid reentering queue
>>processing)
>>- perform module loading (which takes the mutex and registers itself)
>>- grab mutex again
>>- look for <qdisc/classifier/...> again
>>- if not found return -ENOENT
>>- if found drop reference, return -EAGAIN
>>
>>The caller is changed to handle -EAGAIN by replaying the entire
>>request. Your problem sounds very similar, look at net/sched/sch_api.c
>>for an example.
> 
> 
> The aforementioned mutex is local to genetlink module, so I cannot temporarily drop it, call the stuff and grab it again (which was mine original thought too).

Export the lock/unlock/.. functions. You'll also need a new version
similar to __rtnl_unlock.

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Patrick McHardy wrote:
> [ Please quote and break your lines appropriately ]

Oops, sorry for that, definitely was not intentional :(.

> Export the lock/unlock/.. functions. You'll also need a new version 
> similar to __rtnl_unlock.

Patrick, you might feel, I am not reading your lines, but in fact I do.
The problem is that I do not feel competent to follow/propose such
changes. So what I propose here (in included patch) is the least change
scenario, which I can think of and on which I feel safe.

If there are some other changes required, as you suggested for example
exporting lock from genetlink module, I hope authors of genetlink will
comment on that. Currently, I do not see any reason for that, but this
could be due to my limited knowledge.

--
Richard

>From a02ef65329fa33591247f9f3a39f2917afe1ce89 Mon Sep 17 00:00:00 2001
From: Richard Musil <richard.musil@st.com>
Date: Fri, 20 Jul 2007 17:44:20 +0200
Subject: [PATCH] Added separate mutex for syncing the access to family
list (registration, unregistration, ops, etc.)
This change allows family/ops registration/unregistration in 'doit',
'dumpit' message handlers both which hold message sync. mutex. Original
version used only one global mutex for both managing the families and
processing the messages and led to lock when message processing handler
tried to change families.
---
 net/netlink/genetlink.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..5ee18eb 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,18 @@ static void genl_unlock(void)
 		genl_sock->sk_data_ready(genl_sock, 0);
 }
 
+static DEFINE_MUTEX(genl_fam_mutex);	/* serialization for family list management */
+
+static inline void genl_fam_lock(void)
+{
+	mutex_lock(&genl_fam_mutex);
+}
+
+static inline genl_fam_unlock(void)
+{
+	mutex_unlock(&genl_fam_mutex);
+}
+
 #define GENL_FAM_TAB_SIZE	16
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
@@ -150,9 +162,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
-	genl_lock();
+	genl_fam_lock();
 	list_add_tail(&ops->ops_list, &family->ops_list);
-	genl_unlock();
+	genl_fam_unlock();
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
 	err = 0;
@@ -180,16 +192,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 {
 	struct genl_ops *rc;
 
-	genl_lock();
+	genl_fam_lock();
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
 			list_del(&ops->ops_list);
-			genl_unlock();
+			genl_fam_unlock();
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
 			return 0;
 		}
 	}
-	genl_unlock();
+	genl_fam_unlock();
 
 	return -ENOENT;
 }
@@ -217,7 +229,7 @@ int genl_register_family(struct genl_family *family)
 
 	INIT_LIST_HEAD(&family->ops_list);
 
-	genl_lock();
+	genl_fam_lock();
 
 	if (genl_family_find_byname(family->name)) {
 		err = -EEXIST;
@@ -251,14 +263,14 @@ int genl_register_family(struct genl_family *family)
 		family->attrbuf = NULL;
 
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
-	genl_unlock();
+	genl_fam_unlock();
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
 
 	return 0;
 
 errout_locked:
-	genl_unlock();
+	genl_fam_unlock();
 errout:
 	return err;
 }
@@ -275,7 +287,7 @@ int genl_unregister_family(struct genl_family *family)
 {
 	struct genl_family *rc;
 
-	genl_lock();
+	genl_fam_lock();
 
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
 		if (family->id != rc->id || strcmp(rc->name, family->name))
@@ -283,14 +295,14 @@ int genl_unregister_family(struct genl_family *family)
 
 		list_del(&rc->family_list);
 		INIT_LIST_HEAD(&family->ops_list);
-		genl_unlock();
+		genl_fam_unlock();
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
 		return 0;
 	}
 
-	genl_unlock();
+	genl_fam_unlock();
 
 	return -ENOENT;
 }
@@ -425,7 +437,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int fams_to_skip = cb->args[1];
 
 	if (chains_to_skip != 0)
-		genl_lock();
+		genl_fam_lock();
 
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
 		if (i < chains_to_skip)
@@ -445,7 +457,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 
 errout:
 	if (chains_to_skip != 0)
-		genl_unlock();
+		genl_fam_unlock();
 
 	cb->args[0] = i;
 	cb->args[1] = n;
-- 
1.5.1.6

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Thomas Graf]

* Richard MUSIL <richard.musil@st.com> 2007-07-20 18:15
> Patrick McHardy wrote:
> > Export the lock/unlock/.. functions. You'll also need a new version 
> > similar to __rtnl_unlock.
> 
> Patrick, you might feel, I am not reading your lines, but in fact I do.
> The problem is that I do not feel competent to follow/propose such
> changes. So what I propose here (in included patch) is the least change
> scenario, which I can think of and on which I feel safe.
> 
> If there are some other changes required, as you suggested for example
> exporting lock from genetlink module, I hope authors of genetlink will
> comment on that. Currently, I do not see any reason for that, but this
> could be due to my limited knowledge.

Actually there is no reason to not use separate locks for the
message serialization and the protection of the list of registered
families. There is only one lock simply for the reason that I've
never thought of anybody could think of registering a new genetlink
family while processing a message.

Alternatively you could also postpone the registration of the new
genetlink family to a workqueue.

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Thomas Graf wrote:
> Actually there is no reason to not use separate locks for the
> message serialization and the protection of the list of registered
> families. There is only one lock simply for the reason that I've
> never thought of anybody could think of registering a new genetlink
> family while processing a message.

Thomas,
I have been giving it a second thought and came up with something more
complex. The idea is to have locking granularity at the level of
individual families.

Message processing will lock only the family it currently processes
*and* global messaging lock, while family management routines will take
global family lock, and particular lock for family.
This should allow running registration/dereg. from message handler as
long as the family involved is not the same family in which context the
message is being processed.

On the other hand, using lock per family, ensures that message handlers
are not (accidentally) called with invalid references. This should not
be so much problem for dumpit handler, but doit uses for example family
attrs. So I added another patch which implements above described
functionality below.

> Alternatively you could also postpone the registration of the new
> genetlink family to a workqueue.

To be honest, I cannot judge whether the additional complexity I propose
outweighs the gains. In my book, it definitely does, since I like the
easiness of doit, dumpit handlers and implementation using those is
pretty straightforward.
In the long term I believe that refining the locking could also help
in situations where there is heavy traffic over genetlink, then
all family manipulations will not be blocked (which is currently the case).

Let me know, if you accept it as a patch, or should I eventually go for
plan B ;).

--
Richard


>From 63b3ee722402533aed6e137347e41ab1a1fa1127 Mon Sep 17 00:00:00 2001
From: Richard Musil <richard.musil@st.com>
Date: Mon, 23 Jul 2007 15:12:09 +0200
Subject: [PATCH] Added private mutex for each genetlink family (struct genl_family).
This mutex is used to synchronize access to particular family between message
processing handlers and management routines for families
(registering/unregistering families/ops).

This should ensure that another family can be registered or unregistered from
inside genetlink message handler. Trying to register or unregister family
from its own handler will still cause deadlock.
---
 include/net/genetlink.h |    1 +
 net/netlink/genetlink.c |   98 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 70 insertions(+), 29 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct mutex		lock;		/* private */
 };
 
 /**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 5ee18eb..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -40,16 +40,30 @@ static void genl_unlock(void)
 
 static DEFINE_MUTEX(genl_fam_mutex);	/* serialization for family list management */
 
-static inline void genl_fam_lock(void)
+static inline void genl_fam_lock(struct genl_family *family)
 {
 	mutex_lock(&genl_fam_mutex);
+	if (family)
+		mutex_lock(&family->lock);
 }
 
-static inline genl_fam_unlock(void)
+static inline void genl_fam_unlock(struct genl_family *family)
 {
+	if (family)
+		mutex_unlock(&family->lock);
 	mutex_unlock(&genl_fam_mutex);
 }
 
+static inline void genl_onefam_lock(struct genl_family *family)
+{
+	mutex_lock(&family->lock);
+}
+
+static inline void genl_onefam_unlock(struct genl_family *family)
+{
+	mutex_unlock(&family->lock);
+}
+
 #define GENL_FAM_TAB_SIZE	16
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
@@ -162,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
-	genl_fam_lock();
+	genl_fam_lock(family);
 	list_add_tail(&ops->ops_list, &family->ops_list);
-	genl_fam_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
 	err = 0;
@@ -192,16 +206,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 {
 	struct genl_ops *rc;
 
-	genl_fam_lock();
+	genl_fam_lock(family);
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
 			list_del(&ops->ops_list);
-			genl_fam_unlock();
+			genl_fam_unlock(family);
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
 			return 0;
 		}
 	}
-	genl_fam_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -228,8 +242,9 @@ int genl_register_family(struct genl_family *family)
 		goto errout;
 
 	INIT_LIST_HEAD(&family->ops_list);
+	mutex_init(&family->lock);
 
-	genl_fam_lock();
+	genl_fam_lock(family);
 
 	if (genl_family_find_byname(family->name)) {
 		err = -EEXIST;
@@ -263,14 +278,14 @@ int genl_register_family(struct genl_family *family)
 		family->attrbuf = NULL;
 
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
-	genl_fam_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
 
 	return 0;
 
 errout_locked:
-	genl_fam_unlock();
+	genl_fam_unlock(family);
 errout:
 	return err;
 }
@@ -287,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
 {
 	struct genl_family *rc;
 
-	genl_fam_lock();
+	genl_fam_lock(family);
 
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
 		if (family->id != rc->id || strcmp(rc->name, family->name))
@@ -295,14 +310,16 @@ int genl_unregister_family(struct genl_family *family)
 
 		list_del(&rc->family_list);
 		INIT_LIST_HEAD(&family->ops_list);
-		genl_fam_unlock();
+
+		genl_fam_unlock(family);
+		mutex_destroy(&family->lock);
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
 		return 0;
 	}
 
-	genl_fam_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -315,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err;
 
+	genl_fam_lock(NULL);
 	family = genl_family_find_byid(nlh->nlmsg_type);
-	if (family == NULL)
+	if (family == NULL) {
+		genl_fam_unlock(NULL);
 		return -ENOENT;
+	}
+
+	/* get particular family lock, but release global family lock
+	 * so registering operations for other families are possible */
+	genl_onefam_lock(family);
+	genl_fam_unlock(NULL);
 
 	hdrlen = GENL_HDRLEN + family->hdrsize;
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) {
+		err = -EINVAL;
+		goto unlock_out;
+	}
 
 	ops = genl_get_cmd(hdr->cmd, family);
-	if (ops == NULL)
-		return -EOPNOTSUPP;
+	if (ops == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if ((ops->flags & GENL_ADMIN_PERM) &&
-	    security_netlink_recv(skb, CAP_NET_ADMIN))
-		return -EPERM;
+	    security_netlink_recv(skb, CAP_NET_ADMIN)) {
+		err = -EPERM;
+		goto unlock_out;
+	}
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
-		if (ops->dumpit == NULL)
-			return -EOPNOTSUPP;
+		if (ops->dumpit == NULL) {
+			err = -EOPNOTSUPP;
+			goto unlock_out;
+		}
 
-		return netlink_dump_start(genl_sock, skb, nlh,
+		err = netlink_dump_start(genl_sock, skb, nlh,
 					  ops->dumpit, ops->done);
+		goto unlock_out;
 	}
 
-	if (ops->doit == NULL)
-		return -EOPNOTSUPP;
+	if (ops->doit == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if (family->attrbuf) {
 		err = nlmsg_parse(nlh, hdrlen, family->attrbuf, family->maxattr,
 				  ops->policy);
 		if (err < 0)
-			return err;
+			goto unlock_out;
 	}
 
 	info.snd_seq = nlh->nlmsg_seq;
@@ -356,7 +392,11 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = family->attrbuf;
 
-	return ops->doit(skb, &info);
+	err = ops->doit(skb, &info);
+
+unlock_out:
+	genl_onefam_unlock(family);
+	return err;
 }
 
 static void genl_rcv(struct sock *sk, int len)
@@ -437,7 +477,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int fams_to_skip = cb->args[1];
 
 	if (chains_to_skip != 0)
-		genl_fam_lock();
+		genl_fam_lock(NULL);
 
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
 		if (i < chains_to_skip)
@@ -457,7 +497,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 
 errout:
 	if (chains_to_skip != 0)
-		genl_fam_unlock();
+		genl_fam_unlock(NULL);
 
 	cb->args[0] = i;
 	cb->args[1] = n;
-- 
1.5.1.6

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Thomas Graf]

* Richard MUSIL <richard.musil@st.com> 2007-07-23 18:45
> I have been giving it a second thought and came up with something more
> complex. The idea is to have locking granularity at the level of
> individual families.

I agree in general, it would make up a better solution.

However, your initial patch allows operations and families to be
unregistered while message of the same family are being processed
which must not be allowed.

Please provide a new overall patch which is not based on your
initial patch so I can review your idea properly.

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Thomas Graf wrote:
> Please provide a new overall patch which is not based on your
> initial patch so I can review your idea properly.

Here it goes (merging two previous patches). I have diffed
against v2.6.22, which I am using currently as my base:

 include/net/genetlink.h |    1 +
 net/netlink/genetlink.c |  106 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 80 insertions(+), 27 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct mutex		lock;		/* private */
 };
 
 /**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,32 @@ static void genl_unlock(void)
 		genl_sock->sk_data_ready(genl_sock, 0);
 }
 
+static DEFINE_MUTEX(genl_fam_mutex);	/* serialization for family list management */
+
+static inline void genl_fam_lock(struct genl_family *family)
+{
+	mutex_lock(&genl_fam_mutex);
+	if (family)
+		mutex_lock(&family->lock);
+}
+
+static inline void genl_fam_unlock(struct genl_family *family)
+{
+	if (family)
+		mutex_unlock(&family->lock);
+	mutex_unlock(&genl_fam_mutex);
+}
+
+static inline void genl_onefam_lock(struct genl_family *family)
+{
+	mutex_lock(&family->lock);
+}
+
+static inline void genl_onefam_unlock(struct genl_family *family)
+{
+	mutex_unlock(&family->lock);
+}
+
 #define GENL_FAM_TAB_SIZE	16
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
@@ -150,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_add_tail(&ops->ops_list, &family->ops_list);
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
 	err = 0;
@@ -180,16 +206,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 {
 	struct genl_ops *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
 			list_del(&ops->ops_list);
-			genl_unlock();
+			genl_fam_unlock(family);
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
 			return 0;
 		}
 	}
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
 		goto errout;
 
 	INIT_LIST_HEAD(&family->ops_list);
+	mutex_init(&family->lock);
 
-	genl_lock();
+	genl_fam_lock(family);
 
 	if (genl_family_find_byname(family->name)) {
 		err = -EEXIST;
@@ -251,14 +278,14 @@ int genl_register_family(struct genl_family *family)
 		family->attrbuf = NULL;
 
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
 
 	return 0;
 
 errout_locked:
-	genl_unlock();
+	genl_fam_unlock(family);
 errout:
 	return err;
 }
@@ -275,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
 {
 	struct genl_family *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
 		if (family->id != rc->id || strcmp(rc->name, family->name))
@@ -283,14 +310,16 @@ int genl_unregister_family(struct genl_family *family)
 
 		list_del(&rc->family_list);
 		INIT_LIST_HEAD(&family->ops_list);
-		genl_unlock();
+
+		genl_fam_unlock(family);
+		mutex_destroy(&family->lock);
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
 		return 0;
 	}
 
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -303,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err;
 
+	genl_fam_lock(NULL);
 	family = genl_family_find_byid(nlh->nlmsg_type);
-	if (family == NULL)
+	if (family == NULL) {
+		genl_fam_unlock(NULL);
 		return -ENOENT;
+	}
+
+	/* get particular family lock, but release global family lock
+	 * so registering operations for other families are possible */
+	genl_onefam_lock(family);
+	genl_fam_unlock(NULL);
 
 	hdrlen = GENL_HDRLEN + family->hdrsize;
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) {
+		err = -EINVAL;
+		goto unlock_out;
+	}
 
 	ops = genl_get_cmd(hdr->cmd, family);
-	if (ops == NULL)
-		return -EOPNOTSUPP;
+	if (ops == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if ((ops->flags & GENL_ADMIN_PERM) &&
-	    security_netlink_recv(skb, CAP_NET_ADMIN))
-		return -EPERM;
+	    security_netlink_recv(skb, CAP_NET_ADMIN)) {
+		err = -EPERM;
+		goto unlock_out;
+	}
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
-		if (ops->dumpit == NULL)
-			return -EOPNOTSUPP;
+		if (ops->dumpit == NULL) {
+			err = -EOPNOTSUPP;
+			goto unlock_out;
+		}
 
-		return netlink_dump_start(genl_sock, skb, nlh,
+		err = netlink_dump_start(genl_sock, skb, nlh,
 					  ops->dumpit, ops->done);
+		goto unlock_out;
 	}
 
-	if (ops->doit == NULL)
-		return -EOPNOTSUPP;
+	if (ops->doit == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if (family->attrbuf) {
 		err = nlmsg_parse(nlh, hdrlen, family->attrbuf, family->maxattr,
 				  ops->policy);
 		if (err < 0)
-			return err;
+			goto unlock_out;
 	}
 
 	info.snd_seq = nlh->nlmsg_seq;
@@ -344,7 +392,11 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = family->attrbuf;
 
-	return ops->doit(skb, &info);
+	err = ops->doit(skb, &info);
+
+unlock_out:
+	genl_onefam_unlock(family);
+	return err;
 }
 
 static void genl_rcv(struct sock *sk, int len)
@@ -425,7 +477,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int fams_to_skip = cb->args[1];
 
 	if (chains_to_skip != 0)
-		genl_lock();
+		genl_fam_lock(NULL);
 
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
 		if (i < chains_to_skip)
@@ -445,7 +497,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 
 errout:
 	if (chains_to_skip != 0)
-		genl_unlock();
+		genl_fam_unlock(NULL);
 
 	cb->args[0] = i;
 	cb->args[1] = n;

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Hello Thomas,

I wonder, if you had time to take a look at the patch I posted back then.

Richard

----

Thomas Graf wrote:
> > Please provide a new overall patch which is not based on your
> > initial patch so I can review your idea properly.

Here it goes (merging two previous patches). I have diffed
against v2.6.22, which I am using currently as my base:

 include/net/genetlink.h |    1 +
 net/netlink/genetlink.c |  106 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 80 insertions(+), 27 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct mutex		lock;		/* private */
 };
 
 /**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,32 @@ static void genl_unlock(void)
 		genl_sock->sk_data_ready(genl_sock, 0);
 }
 
+static DEFINE_MUTEX(genl_fam_mutex);	/* serialization for family list management */
+
+static inline void genl_fam_lock(struct genl_family *family)
+{
+	mutex_lock(&genl_fam_mutex);
+	if (family)
+		mutex_lock(&family->lock);
+}
+
+static inline void genl_fam_unlock(struct genl_family *family)
+{
+	if (family)
+		mutex_unlock(&family->lock);
+	mutex_unlock(&genl_fam_mutex);
+}
+
+static inline void genl_onefam_lock(struct genl_family *family)
+{
+	mutex_lock(&family->lock);
+}
+
+static inline void genl_onefam_unlock(struct genl_family *family)
+{
+	mutex_unlock(&family->lock);
+}
+
 #define GENL_FAM_TAB_SIZE	16
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
@@ -150,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_add_tail(&ops->ops_list, &family->ops_list);
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
 	err = 0;
@@ -180,16 +206,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 {
 	struct genl_ops *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
 			list_del(&ops->ops_list);
-			genl_unlock();
+			genl_fam_unlock(family);
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
 			return 0;
 		}
 	}
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
 		goto errout;
 
 	INIT_LIST_HEAD(&family->ops_list);
+	mutex_init(&family->lock);
 
-	genl_lock();
+	genl_fam_lock(family);
 
 	if (genl_family_find_byname(family->name)) {
 		err = -EEXIST;
@@ -251,14 +278,14 @@ int genl_register_family(struct genl_family *family)
 		family->attrbuf = NULL;
 
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
 
 	return 0;
 
 errout_locked:
-	genl_unlock();
+	genl_fam_unlock(family);
 errout:
 	return err;
 }
@@ -275,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
 {
 	struct genl_family *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
 		if (family->id != rc->id || strcmp(rc->name, family->name))
@@ -283,14 +310,16 @@ int genl_unregister_family(struct genl_family *family)
 
 		list_del(&rc->family_list);
 		INIT_LIST_HEAD(&family->ops_list);
-		genl_unlock();
+
+		genl_fam_unlock(family);
+		mutex_destroy(&family->lock);
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
 		return 0;
 	}
 
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -303,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err;
 
+	genl_fam_lock(NULL);
 	family = genl_family_find_byid(nlh->nlmsg_type);
-	if (family == NULL)
+	if (family == NULL) {
+		genl_fam_unlock(NULL);
 		return -ENOENT;
+	}
+
+	/* get particular family lock, but release global family lock
+	 * so registering operations for other families are possible */
+	genl_onefam_lock(family);
+	genl_fam_unlock(NULL);
 
 	hdrlen = GENL_HDRLEN + family->hdrsize;
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) {
+		err = -EINVAL;
+		goto unlock_out;
+	}
 
 	ops = genl_get_cmd(hdr->cmd, family);
-	if (ops == NULL)
-		return -EOPNOTSUPP;
+	if (ops == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if ((ops->flags & GENL_ADMIN_PERM) &&
-	    security_netlink_recv(skb, CAP_NET_ADMIN))
-		return -EPERM;
+	    security_netlink_recv(skb, CAP_NET_ADMIN)) {
+		err = -EPERM;
+		goto unlock_out;
+	}
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
-		if (ops->dumpit == NULL)
-			return -EOPNOTSUPP;
+		if (ops->dumpit == NULL) {
+			err = -EOPNOTSUPP;
+			goto unlock_out;
+		}
 
-		return netlink_dump_start(genl_sock, skb, nlh,
+		err = netlink_dump_start(genl_sock, skb, nlh,
 					  ops->dumpit, ops->done);
+		goto unlock_out;
 	}
 
-	if (ops->doit == NULL)
-		return -EOPNOTSUPP;
+	if (ops->doit == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if (family->attrbuf) {
 		err = nlmsg_parse(nlh, hdrlen, family->attrbuf, family->maxattr,
 				  ops->policy);
 		if (err < 0)
-			return err;
+			goto unlock_out;
 	}
 
 	info.snd_seq = nlh->nlmsg_seq;
@@ -344,7 +392,11 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = family->attrbuf;
 
-	return ops->doit(skb, &info);
+	err = ops->doit(skb, &info);
+
+unlock_out:
+	genl_onefam_unlock(family);
+	return err;
 }
 
 static void genl_rcv(struct sock *sk, int len)
@@ -425,7 +477,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int fams_to_skip = cb->args[1];
 
 	if (chains_to_skip != 0)
-		genl_lock();
+		genl_fam_lock(NULL);
 
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
 		if (i < chains_to_skip)
@@ -445,7 +497,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 
 errout:
 	if (chains_to_skip != 0)
-		genl_unlock();
+		genl_fam_unlock(NULL);
 
 	cb->args[0] = i;
 	cb->args[1] = n;

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Thomas Graf]

* Richard MUSIL <richard.musil@st.com> 2007-07-24 13:09
> Thomas Graf wrote:
> > Please provide a new overall patch which is not based on your
> > initial patch so I can review your idea properly.
> 
> Here it goes (merging two previous patches). I have diffed
> against v2.6.22, which I am using currently as my base:

Sorry for taking so long.

> @@ -150,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
>  	if (ops->policy)
>  		ops->flags |= GENL_CMD_CAP_HASPOL;
>  
> -	genl_lock();
> +	genl_fam_lock(family);
>  	list_add_tail(&ops->ops_list, &family->ops_list);
> -	genl_unlock();
> +	genl_fam_unlock(family);

For registering operations, it is sufficient to just acquire the
family lock, the family itself can't disappear while holding it.

> @@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
>  		goto errout;
>  
>  	INIT_LIST_HEAD(&family->ops_list);
> +	mutex_init(&family->lock);
>  
> -	genl_lock();
> +	genl_fam_lock(family);
>  
>  	if (genl_family_find_byname(family->name)) {
>  		err = -EEXIST;
> @@ -251,14 +278,14 @@ int genl_register_family(struct genl_family *family)
>  		family->attrbuf = NULL;
>  
>  	list_add_tail(&family->family_list, genl_family_chain(family->id));
> -	genl_unlock();
> +	genl_fam_unlock(family);

This looks good.

> @@ -303,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  	struct genlmsghdr *hdr = nlmsg_data(nlh);
>  	int hdrlen, err;
>  
> +	genl_fam_lock(NULL);
>  	family = genl_family_find_byid(nlh->nlmsg_type);
> -	if (family == NULL)
> +	if (family == NULL) {
> +		genl_fam_unlock(NULL);
>  		return -ENOENT;
> +	}
> +
> +	/* get particular family lock, but release global family lock
> +	 * so registering operations for other families are possible */
> +	genl_onefam_lock(family);
> +	genl_fam_unlock(NULL);

I don't like having two locks for something as trivial as this.
Basically the only reason the global lock is required here is to
protect from family removal which can be avoided otherwise by
using RCU list operations.

Therefore, I'd propose the following lock semantics:
Use own global mutex to protect writing to the family list, make
reading side lockless using rcu for use when looking up family
upon mesage processing. Use a family lock to protect writing to
operations list and serialize messae processing with unregister
operations.

Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

[Richard MUSIL]

Thomas Graf wrote:
>> @@ -150,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
>>  	if (ops->policy)
>>  		ops->flags |= GENL_CMD_CAP_HASPOL;
>>  
>> -	genl_lock();
>> +	genl_fam_lock(family);
>>  	list_add_tail(&ops->ops_list, &family->ops_list);
>> -	genl_unlock();
>> +	genl_fam_unlock(family);
> 
> For registering operations, it is sufficient to just acquire the
> family lock, the family itself can't disappear while holding it.

I agree.

>> @@ -303,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>>  	struct genlmsghdr *hdr = nlmsg_data(nlh);
>>  	int hdrlen, err;
>>  
>> +	genl_fam_lock(NULL);
>>  	family = genl_family_find_byid(nlh->nlmsg_type);
>> -	if (family == NULL)
>> +	if (family == NULL) {
>> +		genl_fam_unlock(NULL);
>>  		return -ENOENT;
>> +	}
>> +
>> +	/* get particular family lock, but release global family lock
>> +	 * so registering operations for other families are possible */
>> +	genl_onefam_lock(family);
>> +	genl_fam_unlock(NULL);
> 
> I don't like having two locks for something as trivial as this.
> Basically the only reason the global lock is required here is to
> protect from family removal which can be avoided otherwise by
> using RCU list operations.
> 
> Therefore, I'd propose the following lock semantics:
> Use own global mutex to protect writing to the family list, make
> reading side lockless using rcu for use when looking up family
> upon mesage processing. Use a family lock to protect writing to
> operations list and serialize messae processing with unregister
> operations.

I was not aware of RCU lists, but after looking at them, I consider your
proposal to be better. I guess, you would rather write the patch
yourself, so I will wait.

Thanks for help,
Richard