linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] cfg80211: use parallel_ops for genl
  2019-07-26 19:16 [PATCH] cfg80211: use parallel_ops for genl Johannes Berg
@ 2019-07-26  0:16 ` Denis Kenzior
  2019-07-27 18:41   ` Johannes Berg
  0 siblings, 1 reply; 3+ messages in thread
From: Denis Kenzior @ 2019-07-26  0:16 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless; +Cc: Johannes Berg

Hi Johannes,

On 7/26/19 2:16 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Over time, we really need to get rid of all of our global locking.
> One of the things needed is to use parallel_ops. This isn't really
> the most important (RTNL is much more important) but OTOH we just
> keep adding uses of genl_family_attrbuf() now. Use .parallel_ops to
> disallow this.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   net/wireless/nl80211.c | 112 +++++++++++++++++++++++++++++------------
>   1 file changed, 81 insertions(+), 31 deletions(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 10b57aa10227..59aefcd7ccb6 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -749,19 +749,29 @@ int nl80211_prepare_wdev_dump(struct netlink_callback *cb,
>   	int err;
>   
>   	if (!cb->args[0]) {
> +		struct nlattr **attrbuf;
> +
> +		attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf),
> +				  GFP_KERNEL);
> +		if (!attrbuf)
> +			return -ENOMEM;
> +
>   		err = nlmsg_parse_deprecated(cb->nlh,
>   					     GENL_HDRLEN + nl80211_fam.hdrsize,
> -					     genl_family_attrbuf(&nl80211_fam),
> -					     nl80211_fam.maxattr,
> +					     attrbuf, nl80211_fam.maxattr,
>   					     nl80211_policy, NULL);
> -		if (err)
> +		if (err) {
> +			kfree(attrbuf);
>   			return err;
> +		}
>   
> -		*wdev = __cfg80211_wdev_from_attrs(
> -					sock_net(cb->skb->sk),
> -					genl_family_attrbuf(&nl80211_fam));
> -		if (IS_ERR(*wdev))
> +		*wdev = __cfg80211_wdev_from_attrs(sock_net(cb->skb->sk),
> +						   attrbuf);
> +		kfree(attrbuf);
> +		if (IS_ERR(*wdev)) {
> +			kfree(attrbuf);

Hmm, you just freed attrbuf above?

>   			return PTR_ERR(*wdev);
> +		}
>   		*rdev = wiphy_to_rdev((*wdev)->wiphy);
>   		/* 0 is the first index - add 1 to parse only once */
>   		cb->args[0] = (*rdev)->wiphy_idx + 1;

<snip>

> @@ -12846,24 +12880,32 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
>   		return 0;
>   	}
>   
> +	attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf), GFP_KERNEL);
> +	if (!attrbuf)
> +		return -ENOMEM;
> +
>   	err = nlmsg_parse_deprecated(cb->nlh,
>   				     GENL_HDRLEN + nl80211_fam.hdrsize,
>   				     attrbuf, nl80211_fam.maxattr,
>   				     nl80211_policy, NULL);
>   	if (err)
> -		return err;
> +		goto out;
>   
>   	if (!attrbuf[NL80211_ATTR_VENDOR_ID] ||
> -	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD])
> -		return -EINVAL;
> +	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD]) {
> +		err = -EINVAL;
> +		goto out;
> +	}

Might be nicer to just set err = -EINVAL before the if instead of using 
{} here

>   
>   	*wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk), attrbuf);
>   	if (IS_ERR(*wdev))
>   		*wdev = NULL;
>   
>   	*rdev = __cfg80211_rdev_from_attrs(sock_net(skb->sk), attrbuf);
> -	if (IS_ERR(*rdev))
> -		return PTR_ERR(*rdev);
> +	if (IS_ERR(*rdev)) {
> +		err = PTR_ERR(*rdev);
> +		goto out;
> +	}
>   
>   	vid = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_ID]);
>   	subcmd = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_SUBCMD]);
> @@ -12876,15 +12918,19 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
>   		if (vcmd->info.vendor_id != vid || vcmd->info.subcmd != subcmd)
>   			continue;
>   
> -		if (!vcmd->dumpit)
> -			return -EOPNOTSUPP;
> +		if (!vcmd->dumpit) {
> +			err = -EOPNOTSUPP;
> +			goto out;
> +		}

Same thing here, setting err = -EOPNOTSUPP before the for...

>   
>   		vcmd_idx = i;
>   		break;
>   	}
>   
> -	if (vcmd_idx < 0)
> -		return -EOPNOTSUPP;
> +	if (vcmd_idx < 0) {
> +		err = -EOPNOTSUPP;
> +		goto out;
> +	}
>   
>   	if (attrbuf[NL80211_ATTR_VENDOR_DATA]) {
>   		data = nla_data(attrbuf[NL80211_ATTR_VENDOR_DATA]);

<snip>

Otherwise LGTM.

Feel free to add: Reviewed-by: Denis Kenzior <denkenz@gmail.com>

Regards,
-Denis

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH] cfg80211: use parallel_ops for genl
@ 2019-07-26 19:16 Johannes Berg
  2019-07-26  0:16 ` Denis Kenzior
  0 siblings, 1 reply; 3+ messages in thread
From: Johannes Berg @ 2019-07-26 19:16 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Over time, we really need to get rid of all of our global locking.
One of the things needed is to use parallel_ops. This isn't really
the most important (RTNL is much more important) but OTOH we just
keep adding uses of genl_family_attrbuf() now. Use .parallel_ops to
disallow this.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c | 112 +++++++++++++++++++++++++++++------------
 1 file changed, 81 insertions(+), 31 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 10b57aa10227..59aefcd7ccb6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -749,19 +749,29 @@ int nl80211_prepare_wdev_dump(struct netlink_callback *cb,
 	int err;
 
 	if (!cb->args[0]) {
+		struct nlattr **attrbuf;
+
+		attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf),
+				  GFP_KERNEL);
+		if (!attrbuf)
+			return -ENOMEM;
+
 		err = nlmsg_parse_deprecated(cb->nlh,
 					     GENL_HDRLEN + nl80211_fam.hdrsize,
-					     genl_family_attrbuf(&nl80211_fam),
-					     nl80211_fam.maxattr,
+					     attrbuf, nl80211_fam.maxattr,
 					     nl80211_policy, NULL);
-		if (err)
+		if (err) {
+			kfree(attrbuf);
 			return err;
+		}
 
-		*wdev = __cfg80211_wdev_from_attrs(
-					sock_net(cb->skb->sk),
-					genl_family_attrbuf(&nl80211_fam));
-		if (IS_ERR(*wdev))
+		*wdev = __cfg80211_wdev_from_attrs(sock_net(cb->skb->sk),
+						   attrbuf);
+		kfree(attrbuf);
+		if (IS_ERR(*wdev)) {
+			kfree(attrbuf);
 			return PTR_ERR(*wdev);
+		}
 		*rdev = wiphy_to_rdev((*wdev)->wiphy);
 		/* 0 is the first index - add 1 to parse only once */
 		cb->args[0] = (*rdev)->wiphy_idx + 1;
@@ -2390,14 +2400,21 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 				    struct netlink_callback *cb,
 				    struct nl80211_dump_wiphy_state *state)
 {
-	struct nlattr **tb = genl_family_attrbuf(&nl80211_fam);
-	int ret = nlmsg_parse_deprecated(cb->nlh,
-					 GENL_HDRLEN + nl80211_fam.hdrsize,
-					 tb, nl80211_fam.maxattr,
-					 nl80211_policy, NULL);
+	struct nlattr **tb = kcalloc(NUM_NL80211_ATTR, sizeof(*tb), GFP_KERNEL);
+	int ret;
+
+	if (!tb)
+		return -ENOMEM;
+
+	ret = nlmsg_parse_deprecated(cb->nlh,
+				     GENL_HDRLEN + nl80211_fam.hdrsize,
+				     tb, nl80211_fam.maxattr,
+				     nl80211_policy, NULL);
 	/* ignore parse errors for backward compatibility */
-	if (ret)
-		return 0;
+	if (ret) {
+		ret = 0;
+		goto out;
+	}
 
 	state->split = tb[NL80211_ATTR_SPLIT_WIPHY_DUMP];
 	if (tb[NL80211_ATTR_WIPHY])
@@ -2410,8 +2427,10 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 		int ifidx = nla_get_u32(tb[NL80211_ATTR_IFINDEX]);
 
 		netdev = __dev_get_by_index(sock_net(skb->sk), ifidx);
-		if (!netdev)
-			return -ENODEV;
+		if (!netdev) {
+			ret = -ENODEV;
+			goto out;
+		}
 		if (netdev->ieee80211_ptr) {
 			rdev = wiphy_to_rdev(
 				netdev->ieee80211_ptr->wiphy);
@@ -2419,7 +2438,10 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 		}
 	}
 
-	return 0;
+	ret = 0;
+out:
+	kfree(tb);
+	return ret;
 }
 
 static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
@@ -8724,7 +8746,7 @@ static int nl80211_send_survey(struct sk_buff *msg, u32 portid, u32 seq,
 
 static int nl80211_dump_survey(struct sk_buff *skb, struct netlink_callback *cb)
 {
-	struct nlattr **attrbuf = genl_family_attrbuf(&nl80211_fam);
+	struct nlattr **attrbuf;
 	struct survey_info survey;
 	struct cfg80211_registered_device *rdev;
 	struct wireless_dev *wdev;
@@ -8732,6 +8754,10 @@ static int nl80211_dump_survey(struct sk_buff *skb, struct netlink_callback *cb)
 	int res;
 	bool radio_stats;
 
+	attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf), GFP_KERNEL);
+	if (!attrbuf)
+		return -ENOMEM;
+
 	rtnl_lock();
 	res = nl80211_prepare_wdev_dump(cb, &rdev, &wdev);
 	if (res)
@@ -8776,6 +8802,7 @@ static int nl80211_dump_survey(struct sk_buff *skb, struct netlink_callback *cb)
 	cb->args[2] = survey_idx;
 	res = skb->len;
  out_err:
+	kfree(attrbuf);
 	rtnl_unlock();
 	return res;
 }
@@ -9635,6 +9662,7 @@ static int nl80211_testmode_dump(struct sk_buff *skb,
 				 struct netlink_callback *cb)
 {
 	struct cfg80211_registered_device *rdev;
+	struct nlattr **attrbuf = NULL;
 	int err;
 	long phy_idx;
 	void *data = NULL;
@@ -9655,7 +9683,12 @@ static int nl80211_testmode_dump(struct sk_buff *skb,
 			goto out_err;
 		}
 	} else {
-		struct nlattr **attrbuf = genl_family_attrbuf(&nl80211_fam);
+		attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf),
+				  GFP_KERNEL);
+		if (!attrbuf) {
+			err = -ENOMEM;
+			goto out_err;
+		}
 
 		err = nlmsg_parse_deprecated(cb->nlh,
 					     GENL_HDRLEN + nl80211_fam.hdrsize,
@@ -9722,6 +9755,7 @@ static int nl80211_testmode_dump(struct sk_buff *skb,
 	/* see above */
 	cb->args[0] = phy_idx + 1;
  out_err:
+	kfree(attrbuf);
 	rtnl_unlock();
 	return err;
 }
@@ -12815,7 +12849,7 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
 				       struct cfg80211_registered_device **rdev,
 				       struct wireless_dev **wdev)
 {
-	struct nlattr **attrbuf = genl_family_attrbuf(&nl80211_fam);
+	struct nlattr **attrbuf;
 	u32 vid, subcmd;
 	unsigned int i;
 	int vcmd_idx = -1;
@@ -12846,24 +12880,32 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
 		return 0;
 	}
 
+	attrbuf = kcalloc(NUM_NL80211_ATTR, sizeof(*attrbuf), GFP_KERNEL);
+	if (!attrbuf)
+		return -ENOMEM;
+
 	err = nlmsg_parse_deprecated(cb->nlh,
 				     GENL_HDRLEN + nl80211_fam.hdrsize,
 				     attrbuf, nl80211_fam.maxattr,
 				     nl80211_policy, NULL);
 	if (err)
-		return err;
+		goto out;
 
 	if (!attrbuf[NL80211_ATTR_VENDOR_ID] ||
-	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD])
-		return -EINVAL;
+	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD]) {
+		err = -EINVAL;
+		goto out;
+	}
 
 	*wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk), attrbuf);
 	if (IS_ERR(*wdev))
 		*wdev = NULL;
 
 	*rdev = __cfg80211_rdev_from_attrs(sock_net(skb->sk), attrbuf);
-	if (IS_ERR(*rdev))
-		return PTR_ERR(*rdev);
+	if (IS_ERR(*rdev)) {
+		err = PTR_ERR(*rdev);
+		goto out;
+	}
 
 	vid = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_ID]);
 	subcmd = nla_get_u32(attrbuf[NL80211_ATTR_VENDOR_SUBCMD]);
@@ -12876,15 +12918,19 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
 		if (vcmd->info.vendor_id != vid || vcmd->info.subcmd != subcmd)
 			continue;
 
-		if (!vcmd->dumpit)
-			return -EOPNOTSUPP;
+		if (!vcmd->dumpit) {
+			err = -EOPNOTSUPP;
+			goto out;
+		}
 
 		vcmd_idx = i;
 		break;
 	}
 
-	if (vcmd_idx < 0)
-		return -EOPNOTSUPP;
+	if (vcmd_idx < 0) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 
 	if (attrbuf[NL80211_ATTR_VENDOR_DATA]) {
 		data = nla_data(attrbuf[NL80211_ATTR_VENDOR_DATA]);
@@ -12895,7 +12941,7 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
 				attrbuf[NL80211_ATTR_VENDOR_DATA],
 				cb->extack);
 		if (err)
-			return err;
+			goto out;
 	}
 
 	/* 0 is the first index - add 1 to parse only once */
@@ -12907,7 +12953,10 @@ static int nl80211_prepare_vendor_dump(struct sk_buff *skb,
 	cb->args[4] = data_len;
 
 	/* keep rtnl locked in successful case */
-	return 0;
+	err = 0;
+out:
+	kfree(attrbuf);
+	return err;
 }
 
 static int nl80211_vendor_cmd_dump(struct sk_buff *skb,
@@ -14585,6 +14634,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
 	.n_ops = ARRAY_SIZE(nl80211_ops),
 	.mcgrps = nl80211_mcgrps,
 	.n_mcgrps = ARRAY_SIZE(nl80211_mcgrps),
+	.parallel_ops = true,
 };
 
 /* notification functions */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] cfg80211: use parallel_ops for genl
  2019-07-26  0:16 ` Denis Kenzior
@ 2019-07-27 18:41   ` Johannes Berg
  0 siblings, 0 replies; 3+ messages in thread
From: Johannes Berg @ 2019-07-27 18:41 UTC (permalink / raw)
  To: Denis Kenzior, linux-wireless

Hi Denis,

(huh, why did your mail make it to my inbox 3 hours ago ...? oh well)

> > +		kfree(attrbuf);
> > +		if (IS_ERR(*wdev)) {
> > +			kfree(attrbuf);
> 
> Hmm, you just freed attrbuf above?

Good catch.

I was being stupid, wrote the patch on one machine, then tested & fixed
it on another, and then sent out the original ...

> >   	if (!attrbuf[NL80211_ATTR_VENDOR_ID] ||
> > -	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD])
> > -		return -EINVAL;
> > +	    !attrbuf[NL80211_ATTR_VENDOR_SUBCMD]) {
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> 
> Might be nicer to just set err = -EINVAL before the if instead of using 
> {} here

Dunno. I don't generally like the values "leaking" out of where they're
intended, tends to hide compiler warnings when you forget to assign or
something ... I guess doing -EINVAL at least would fail safely :-)

I'll revise this then.

johannes


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-27 18:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 19:16 [PATCH] cfg80211: use parallel_ops for genl Johannes Berg
2019-07-26  0:16 ` Denis Kenzior
2019-07-27 18:41   ` Johannes Berg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).