| Message ID | 1377206736-18357-1-git-send-email-pshelar@nicira.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
> @@ -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
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
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)
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(-)