> +++ b/net/wireless/nl80211.c
> @@ -6077,30 +6078,34 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
> if (rdev->sched_scan_req)
> return -EINPROGRESS;
>
> - rdev->sched_scan_req = nl80211_parse_sched_scan(&rdev->wiphy, wdev,
> - info->attrs);
> - err = PTR_ERR_OR_ZERO(rdev->sched_scan_req);
> + sched_scan_req = nl80211_parse_sched_scan(&rdev->wiphy, wdev,
> + info->attrs);
> +
> + err = PTR_ERR_OR_ZERO(sched_scan_req);
> if (err)
> goto out_err;
>
> - err = rdev_sched_scan_start(rdev, dev, rdev->sched_scan_req);
> + rcu_assign_pointer(rdev->sched_scan_req, sched_scan_req);
> +
> + err = rdev_sched_scan_start(rdev, dev, sched_scan_req);
> if (err)
> goto out_free;
>
> - rdev->sched_scan_req->dev = dev;
> - rdev->sched_scan_req->wiphy = &rdev->wiphy;
> -
> if (info->attrs[NL80211_ATTR_SOCKET_OWNER])
> - rdev->sched_scan_req->owner_nlportid = info->snd_portid;
> + rtnl_dereference(rdev->sched_scan_req)->owner_nlportid =
> + info->snd_portid;
> +
> + rtnl_dereference(rdev->sched_scan_req)->dev = dev;
> + rtnl_dereference(rdev->sched_scan_req)->wiphy = &rdev->wiphy;
This is still all wrong - you need to fully build the local variable and
then assign it after everything is done.
You can *probably* assign it only after calling the driver, in which
case you don't even need to kfree_rcu() below if it was never assigned
in failure cases.
> out_free:
> - kfree(rdev->sched_scan_req);
> + kfree_rcu(sched_scan_req, rcu_head);
> + rcu_assign_pointer(rdev->sched_scan_req, NULL);
use RCU_INIT_POINTER() for NULL values
> out_err:
> - rdev->sched_scan_req = NULL;
Also why did that move into a different label?
> void cfg80211_sched_scan_results(struct wiphy *wiphy)
> {
> + struct cfg80211_sched_scan_request *sched_scan_req;
> +
> trace_cfg80211_sched_scan_results(wiphy);
> /* ignore if we're not scanning */
> - if (wiphy_to_rdev(wiphy)->sched_scan_req)
> +
> + rcu_read_lock();
> + sched_scan_req = rcu_dereference(wiphy_to_rdev(wiphy)->sched_scan_req);
> + rcu_read_unlock();
Umm. No no no. You probably don't want anything but rcu_access_pointer()
here, or do the rcu_read_lock() around all the users ...
> - kfree(rdev->sched_scan_req);
> - rdev->sched_scan_req = NULL;
> + kfree_rcu(sched_scan_req, rcu_head);
> +
> + rcu_assign_pointer(rdev->sched_scan_req, NULL);
You really need to do that the other way around...
Maybe you can find somebody else who has experience with RCU and is
willing to review your patches first? :)
s
Also - this patch really should come *first* in the series. Don't break
the code and fix it in the next patch, do it right once.
johanne
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
