Search Linux Wireless

Re: [PATCH] cfg80211: use parallel_ops for genl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Johannes,

On 7/26/19 2:16 PM, Johannes Berg wrote:
From: Johannes Berg <johannes.berg@xxxxxxxxx>

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@xxxxxxxxx>
---
  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@xxxxxxxxx>

Regards,
-Denis



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux