Patchwork [1/2] genl: Fix genl dumpit() locking.

login
register
mail settings
Submitter Pravin B Shelar
Date Aug. 22, 2013, 9:25 p.m.
Message ID <1377206736-18357-1-git-send-email-pshelar@nicira.com>
Download mbox | patch
Permalink /patch/269210/
State Superseded
Delegated to: David Miller
Headers show

Comments

Pravin B Shelar - Aug. 22, 2013, 9:25 p.m.
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.
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.

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 change.
---
 net/netlink/genetlink.c |   46 +++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 41 insertions(+), 5 deletions(-)
Johannes Berg - Aug. 23, 2013, 7:20 a.m.
> @@ -572,15 +594,29 @@ static int genl_family_rcv_msg(struct
> genl_family *family,
>                 return -EPERM;
>  
>         if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
> -               struct netlink_dump_control c = {
> -                       .dump = ops->dumpit,
> -                       .done = ops->done,
> -               };
> +               struct netlink_dump_control c;
> +               int rc;
>  
>                 if (ops->dumpit == NULL)
>                         return -EOPNOTSUPP;
>  
> -               return netlink_dump_start(net->genl_sock, skb, nlh,
> &c);
> +               memset(&c, 0, sizeof(c));
> +               if (!family->parallel_ops) {
> +                       genl_unlock();
> +                       c.data = ops;
> +                       c.dump = genl_lock_dumpit;
> +                       if (ops->done)
> +                               c.done = genl_lock_done;
> +               } else {
> +                       c.dump = ops->dumpit;
> +                       c.done = ops->done;
> +               }
> +
> +               rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
> +               if (!family->parallel_ops)
> +                       genl_lock();
> +               return rc;
> +

I think this piece would be easier to read if you call
netlink_dump_start() separately in the two branches. If you also move
the "c" variable into the branches then you can keep initializing it
with an explicit initializer which would also be more readable IMHO.

Either way, this seems fine. I still think that not assigning cb_mutex
wasn't the smartest idea, but I'll reply over in the other thread.

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
Pravin B Shelar - Aug. 23, 2013, 5:05 p.m.
On Fri, Aug 23, 2013 at 12:20 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> @@ -572,15 +594,29 @@ static int genl_family_rcv_msg(struct
>> genl_family *family,
>>                 return -EPERM;
>>
>>         if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
>> -               struct netlink_dump_control c = {
>> -                       .dump = ops->dumpit,
>> -                       .done = ops->done,
>> -               };
>> +               struct netlink_dump_control c;
>> +               int rc;
>>
>>                 if (ops->dumpit == NULL)
>>                         return -EOPNOTSUPP;
>>
>> -               return netlink_dump_start(net->genl_sock, skb, nlh,
>> &c);
>> +               memset(&c, 0, sizeof(c));
>> +               if (!family->parallel_ops) {
>> +                       genl_unlock();
>> +                       c.data = ops;
>> +                       c.dump = genl_lock_dumpit;
>> +                       if (ops->done)
>> +                               c.done = genl_lock_done;
>> +               } else {
>> +                       c.dump = ops->dumpit;
>> +                       c.done = ops->done;
>> +               }
>> +
>> +               rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
>> +               if (!family->parallel_ops)
>> +                       genl_lock();
>> +               return rc;
>> +
>
> I think this piece would be easier to read if you call
> netlink_dump_start() separately in the two branches. If you also move
> the "c" variable into the branches then you can keep initializing it
> with an explicit initializer which would also be more readable IMHO.
>
ok, I will update patch.

> Either way, this seems fine. I still think that not assigning cb_mutex
> wasn't the smartest idea, but I'll reply over in the other thread.
--
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

Patch

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f85f8a2..3669039 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -544,6 +544,28 @@  void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
 }
 EXPORT_SYMBOL(genlmsg_put);
 
+static int genl_lock_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct genl_ops *ops = cb->data;
+	int rc;
+
+	genl_lock();
+	rc = ops->dumpit(skb, cb);
+	genl_unlock();
+	return rc;
+}
+
+static int genl_lock_done(struct netlink_callback *cb)
+{
+	struct genl_ops *ops = cb->data;
+	int rc;
+
+	genl_lock();
+	rc = ops->done(cb);
+	genl_unlock();
+	return rc;
+}
+
 static int genl_family_rcv_msg(struct genl_family *family,
 			       struct sk_buff *skb,
 			       struct nlmsghdr *nlh)
@@ -572,15 +594,29 @@  static int genl_family_rcv_msg(struct genl_family *family,
 		return -EPERM;
 
 	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
-		struct netlink_dump_control c = {
-			.dump = ops->dumpit,
-			.done = ops->done,
-		};
+		struct netlink_dump_control c;
+		int rc;
 
 		if (ops->dumpit == NULL)
 			return -EOPNOTSUPP;
 
-		return netlink_dump_start(net->genl_sock, skb, nlh, &c);
+		memset(&c, 0, sizeof(c));
+		if (!family->parallel_ops) {
+			genl_unlock();
+			c.data = ops;
+			c.dump = genl_lock_dumpit;
+			if (ops->done)
+				c.done = genl_lock_done;
+		} else {
+			c.dump = ops->dumpit;
+			c.done = ops->done;
+		}
+
+		rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
+		if (!family->parallel_ops)
+			genl_lock();
+		return rc;
+
 	}
 
 	if (ops->doit == NULL)