[PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Pravin B Shelar]

netlink dump operations take module as parameter to hold
reference for entire netlink dump duration.
Currently it holds ref only on genl module which is not correct
when we use ops registered to genl from another module.
Following patch adds module pointer to genl_ops so that netlink
can hold ref count on it.

CC: Jesse Gross <jesse@nicira.com>
CC: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
v1-v2:
No need to ref count genl module.
---
 include/net/genetlink.h |   20 +++++++++++++++++---
 net/netlink/genetlink.c |   20 +++++++++++---------
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 93024a4..ab8a7c5 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -61,6 +61,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;
 };
 
 /**
@@ -121,9 +122,22 @@ 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,
-	struct genl_ops *ops, size_t n_ops);
+extern int __genl_register_family(struct genl_family *family, struct module *module);
+
+static inline int genl_register_family(struct genl_family *family)
+{
+	return __genl_register_family(family, THIS_MODULE);
+}
+
+extern int __genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops, struct module *module);
+
+static inline int genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops)
+{
+	return __genl_register_family_with_ops(family, ops, n_ops, THIS_MODULE);
+}
+
 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 3669039..d491604 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -364,7 +364,7 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 EXPORT_SYMBOL(genl_unregister_ops);
 
 /**
- * genl_register_family - register a generic netlink family
+ * __genl_register_family - register a generic netlink family
  * @family: generic netlink family
  *
  * Registers the specified family after validating it first. Only one
@@ -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, struct module *module)
 {
 	int err = -EINVAL;
 
@@ -418,6 +418,7 @@ int genl_register_family(struct genl_family *family)
 	} else
 		family->attrbuf = NULL;
 
+	family->module = module;
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
 	genl_unlock_all();
 
@@ -430,10 +431,10 @@ 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
+ * __genl_register_family_with_ops - register a generic netlink family
  * @family: generic netlink family
  * @ops: operations to be registered
  * @n_ops: number of elements to register
@@ -457,12 +458,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,
-	struct genl_ops *ops, size_t n_ops)
+int __genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops, struct module *module)
 {
 	int err, i;
 
-	err = genl_register_family(family);
+	err = __genl_register_family(family, module);
 	if (err)
 		return err;
 
@@ -476,7 +477,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
@@ -612,7 +613,8 @@ static int genl_family_rcv_msg(struct genl_family *family,
 			c.done = ops->done;
 		}
 
-		rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
+		c.module = family->module;
+		rc = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
 		if (!family->parallel_ops)
 			genl_lock();
 		return rc;
-- 
1.7.1

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Johannes Berg]

> +extern int __genl_register_family(struct genl_family *family, struct module *module);

Why add an extra argument instead of just assigning family->module in
the inline wrapper(s):

static inline int genl_register_family(struct genl_family *family)
{
	family->module = THIS_MODULE;
	return __genl_register_family(family);
}

Although I guess it doesn't really make a big difference. Just curious
if there was a reason you decided to do it this way.

johannes

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Pravin Shelar]

On Fri, Aug 23, 2013 at 12:23 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> +extern int __genl_register_family(struct genl_family *family, struct module *module);
>
> Why add an extra argument instead of just assigning family->module in
> the inline wrapper(s):
>
> static inline int genl_register_family(struct genl_family *family)
> {
>         family->module = THIS_MODULE;
>         return __genl_register_family(family);
> }
>
This is better, I will send updated patch.

[PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Pravin B Shelar]

netlink dump operations take module as parameter to hold
reference for entire netlink dump duration.
Currently it holds ref only on genl module which is not correct
when we use ops registered to genl from another module.
Following patch adds module pointer to genl_ops so that netlink
can hold ref count on it.

CC: Jesse Gross <jesse@nicira.com>
CC: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
 include/net/genetlink.h |   21 ++++++++++++++++++---
 net/netlink/genetlink.c |   39 ++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 93024a4..7f57b2c 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -119,13 +119,28 @@ struct genl_ops {
 					 struct netlink_callback *cb);
 	int		       (*done)(struct netlink_callback *cb);
 	struct list_head	ops_list;
+	struct module		*module;
 };
 
 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);
+
+extern int __genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops, struct module *module);
+
+static inline int genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops)
+{
+	return __genl_register_family_with_ops(family, ops, n_ops, THIS_MODULE);
+}
+
 extern int genl_unregister_family(struct genl_family *family);
-extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
+extern int __genl_register_ops(struct genl_family *f, struct genl_ops *ops,
+			       struct module *module);
+static inline int genl_register_ops(struct genl_family *f, struct genl_ops *ops)
+{
+	return __genl_register_ops(f, ops, THIS_MODULE);
+}
+
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_register_mc_group(struct genl_family *family,
 				  struct genl_multicast_group *grp);
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 3669039..23430c7 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -284,7 +284,7 @@ static void genl_unregister_mc_groups(struct genl_family *family)
 }
 
 /**
- * genl_register_ops - register generic netlink operations
+ * __genl_register_ops - register generic netlink operations
  * @family: generic netlink family
  * @ops: operations to be registered
  *
@@ -298,7 +298,8 @@ static void genl_unregister_mc_groups(struct genl_family *family)
  *
  * Returns 0 on success or a negative error code.
  */
-int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
+int __genl_register_ops(struct genl_family *family, struct genl_ops *ops,
+			struct module *module)
 {
 	int err = -EINVAL;
 
@@ -317,6 +318,7 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
+	ops->module = module;
 	genl_lock_all();
 	list_add_tail(&ops->ops_list, &family->ops_list);
 	genl_unlock_all();
@@ -326,7 +328,7 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 errout:
 	return err;
 }
-EXPORT_SYMBOL(genl_register_ops);
+EXPORT_SYMBOL(__genl_register_ops);
 
 /**
  * genl_unregister_ops - unregister generic netlink operations
@@ -433,7 +435,7 @@ errout:
 EXPORT_SYMBOL(genl_register_family);
 
 /**
- * genl_register_family_with_ops - register a generic netlink family
+ * __genl_register_family_with_ops - register a generic netlink family
  * @family: generic netlink family
  * @ops: operations to be registered
  * @n_ops: number of elements to register
@@ -457,8 +459,8 @@ EXPORT_SYMBOL(genl_register_family);
  *
  * Return 0 on success or a negative error code.
  */
-int genl_register_family_with_ops(struct genl_family *family,
-	struct genl_ops *ops, size_t n_ops)
+int __genl_register_family_with_ops(struct genl_family *family,
+	struct genl_ops *ops, size_t n_ops, struct module *module)
 {
 	int err, i;
 
@@ -467,7 +469,7 @@ int genl_register_family_with_ops(struct genl_family *family,
 		return err;
 
 	for (i = 0; i < n_ops; ++i, ++ops) {
-		err = genl_register_ops(family, ops);
+		err = __genl_register_ops(family, ops, module);
 		if (err)
 			goto err_out;
 	}
@@ -476,7 +478,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
@@ -558,11 +560,14 @@ static int genl_lock_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 static int genl_lock_done(struct netlink_callback *cb)
 {
 	struct genl_ops *ops = cb->data;
-	int rc;
+	int rc = 0;
 
-	genl_lock();
-	rc = ops->done(cb);
-	genl_unlock();
+	if (ops->done) {
+		genl_lock();
+		rc = ops->done(cb);
+		genl_unlock();
+	}
+	module_put(ops->module);
 	return rc;
 }
 
@@ -605,14 +610,18 @@ static int genl_family_rcv_msg(struct genl_family *family,
 			genl_unlock();
 			c.data = ops;
 			c.dump = genl_lock_dumpit;
-			if (ops->done)
-				c.done = genl_lock_done;
+			c.done = genl_lock_done;
+			c.module = THIS_MODULE;
+
+			if (!try_module_get(ops->module))
+				return -EPROTONOSUPPORT;
 		} else {
 			c.dump = ops->dumpit;
 			c.done = ops->done;
+			c.module = ops->module;
 		}
 
-		rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
+		rc = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
 		if (!family->parallel_ops)
 			genl_lock();
 		return rc;
-- 
1.7.1

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Johannes Berg]

On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
> netlink dump operations take module as parameter to hold
> reference for entire netlink dump duration.
> Currently it holds ref only on genl module which is not correct
> when we use ops registered to genl from another module.
> Following patch adds module pointer to genl_ops so that netlink
> can hold ref count on it.
> 
> CC: Jesse Gross <jesse@nicira.com>
> CC: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> ---
>  include/net/genetlink.h |   21 ++++++++++++++++++---
>  net/netlink/genetlink.c |   39 ++++++++++++++++++++++++---------------
>  2 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index 93024a4..7f57b2c 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -119,13 +119,28 @@ struct genl_ops {
>  					 struct netlink_callback *cb);
>  	int		       (*done)(struct netlink_callback *cb);
>  	struct list_head	ops_list;
> +	struct module		*module;
>  };

This may be more correct than my patch, but I'm not sure it's worth
spending the memory. Is there going to be any generic netlink family
that actually puts operations into a different module than the family
itself? I doubt that.

> @@ -605,14 +610,18 @@ static int genl_family_rcv_msg(struct genl_family *family,
>  			genl_unlock();
>  			c.data = ops;
>  			c.dump = genl_lock_dumpit;
> -			if (ops->done)
> -				c.done = genl_lock_done;
> +			c.done = genl_lock_done;
> +			c.module = THIS_MODULE;

THIS_MODULE here is useless, this code is always built-in.

> +			if (!try_module_get(ops->module))
> +				return -EPROTONOSUPPORT;

Why open-code it? You can just point c.module to the ops module here as
well (because generic netlink is built-in) and save yourself the
try_module_get stuff.

johannes

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Pravin Shelar]

On Thu, Aug 22, 2013 at 12:40 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
>> netlink dump operations take module as parameter to hold
>> reference for entire netlink dump duration.
>> Currently it holds ref only on genl module which is not correct
>> when we use ops registered to genl from another module.
>> Following patch adds module pointer to genl_ops so that netlink
>> can hold ref count on it.
>>
>> CC: Jesse Gross <jesse@nicira.com>
>> CC: Johannes Berg <johannes.berg@intel.com>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> ---
>>  include/net/genetlink.h |   21 ++++++++++++++++++---
>>  net/netlink/genetlink.c |   39 ++++++++++++++++++++++++---------------
>>  2 files changed, 42 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
>> index 93024a4..7f57b2c 100644
>> --- a/include/net/genetlink.h
>> +++ b/include/net/genetlink.h
>> @@ -119,13 +119,28 @@ struct genl_ops {
>>                                        struct netlink_callback *cb);
>>       int                    (*done)(struct netlink_callback *cb);
>>       struct list_head        ops_list;
>> +     struct module           *module;
>>  };
>
> This may be more correct than my patch, but I'm not sure it's worth
> spending the memory. Is there going to be any generic netlink family
> that actually puts operations into a different module than the family
> itself? I doubt that.
>
I tried to do same, but I do not have access to ops in
genl_lock_done() function. therefore I decided to store direct pointer
to module in ops struct.

>> @@ -605,14 +610,18 @@ static int genl_family_rcv_msg(struct genl_family *family,
>>                       genl_unlock();
>>                       c.data = ops;
>>                       c.dump = genl_lock_dumpit;
>> -                     if (ops->done)
>> -                             c.done = genl_lock_done;
>> +                     c.done = genl_lock_done;
>> +                     c.module = THIS_MODULE;
>
> THIS_MODULE here is useless, this code is always built-in.
>
>> +                     if (!try_module_get(ops->module))
>> +                             return -EPROTONOSUPPORT;
>
> Why open-code it? You can just point c.module to the ops module here as
> well (because generic netlink is built-in) and save yourself the
> try_module_get stuff.
>

In locked genl case genl-dump operation has call graph as follows:
netlink_dump() -> genl_lock_dumpit() -> ops->dump()
Therefore I need to take ref on genl module and ops->module.

In case of parallel_ops been true, genl_lock_dumpit() is not used so I
have directly set c.module to ops->module.

I know this code is bit complicated, but I could not find way to simplify it.

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Johannes Berg]

On Thu, 2013-08-22 at 10:44 -0700, Pravin Shelar wrote:

> > This may be more correct than my patch, but I'm not sure it's worth
> > spending the memory. Is there going to be any generic netlink family
> > that actually puts operations into a different module than the family
> > itself? I doubt that.
> >
> I tried to do same, but I do not have access to ops in
> genl_lock_done() function. therefore I decided to store direct pointer
> to module in ops struct.

You mean the family? I still don't like this, it'll be a massive
increase in memory for something like nl80211 that has a lot of
operations. In fact I think we should get rid of being allowed to
register single ops and just force a single array to remove the linked
list as well.

> >> +                     c.module = THIS_MODULE;
> >
> > THIS_MODULE here is useless, this code is always built-in.
> >
> >> +                     if (!try_module_get(ops->module))
> >> +                             return -EPROTONOSUPPORT;
> >
> > Why open-code it? You can just point c.module to the ops module here as
> > well (because generic netlink is built-in) and save yourself the
> > try_module_get stuff.
> >
> 
> In locked genl case genl-dump operation has call graph as follows:
> netlink_dump() -> genl_lock_dumpit() -> ops->dump()
> Therefore I need to take ref on genl module and ops->module.

There's no "genl module", generic netlink is always built in. That was
my point, you don't need to hold any reference to the "genl module"
since it doesn't exist, so you can just point to the ops (family)
module.

johannes

Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.

[Pravin Shelar]

On Thu, Aug 22, 2013 at 10:49 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-08-22 at 10:44 -0700, Pravin Shelar wrote:
>
>> > This may be more correct than my patch, but I'm not sure it's worth
>> > spending the memory. Is there going to be any generic netlink family
>> > that actually puts operations into a different module than the family
>> > itself? I doubt that.
>> >
>> I tried to do same, but I do not have access to ops in
>> genl_lock_done() function. therefore I decided to store direct pointer
>> to module in ops struct.
>
> You mean the family? I still don't like this, it'll be a massive
> increase in memory for something like nl80211 that has a lot of
> operations. In fact I think we should get rid of being allowed to
> register single ops and just force a single array to remove the linked
> list as well.
>
Sorry, I meant I do not have access to genl-family in genl_lock_done().
I am using c.data to store ops pointer. Thats why module pointer was
store in ops.

But considering we do not need to hold ref on genl, I can move module
pointer to family.

>> >> +                     c.module = THIS_MODULE;
>> >
>> > THIS_MODULE here is useless, this code is always built-in.
>> >
>> >> +                     if (!try_module_get(ops->module))
>> >> +                             return -EPROTONOSUPPORT;
>> >
>> > Why open-code it? You can just point c.module to the ops module here as
>> > well (because generic netlink is built-in) and save yourself the
>> > try_module_get stuff.
>> >
>>
>> In locked genl case genl-dump operation has call graph as follows:
>> netlink_dump() -> genl_lock_dumpit() -> ops->dump()
>> Therefore I need to take ref on genl module and ops->module.
>
> There's no "genl module", generic netlink is always built in. That was
> my point, you don't need to hold any reference to the "genl module"
> since it doesn't exist, so you can just point to the ops (family)
> module.
>
ok, this make sense.
I will send updated patch.