From: Johannes Berg Updating beacon and related templates with NL80211_CMD_SET_BEACON requires the wiphy mutex, which can get contended, delaying the updates. Add a NL80211_CMD_UPDATE_BEACON operation that can only do beacon template updates, not other configuration changes, but is called without the wiphy mutex. If something requires the mutex then the driver can return -EAGAIN, to go through the normal change_beacon with wiphy mutex held. Also with this support updating multiple beacons for an MLD at the same time, just to further reduce round-trips if desired. Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 5 + include/uapi/linux/nl80211.h | 16 ++ net/wireless/nl80211.c | 312 +++++++++++++++++++++++++++++------ net/wireless/rdev-ops.h | 11 ++ net/wireless/trace.h | 14 +- 5 files changed, 307 insertions(+), 51 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 45a96c92fec3..7de6dbdae930 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -4949,6 +4949,9 @@ struct mgmt_frame_regs { * @start_ap: Start acting in AP mode defined by the parameters. * @change_beacon: Change the beacon parameters for an access point mode * interface. This should reject the call when AP mode wasn't started. + * @update_beacon: Update the beacon and other templates like @change_beacon, + * but called without the wiphy mutex held. May return -EAGAIN to have + * it fall back to @change_beacon with the wiphy mutex held. * @stop_ap: Stop being an AP, including stopping beaconing. * * @add_station: Add a new station. @@ -5364,6 +5367,8 @@ struct cfg80211_ops { struct cfg80211_ap_settings *settings); int (*change_beacon)(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_ap_update *info); + int (*update_beacon)(struct wiphy *wiphy, struct net_device *dev, + struct cfg80211_ap_update *info); int (*stop_ap)(struct wiphy *wiphy, struct net_device *dev, unsigned int link_id); diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 341f8395d1af..b56a96f57bec 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -1458,6 +1458,16 @@ * keep their non-evacuable marking, removed channels lose it, and newly * added channels are evacuable by default; issue this command again to * change the non-evacuable set. + * @NL80211_CMD_UPDATE_BEACON: Update the beacon and other templates of an AP + * interface, but only that. Same attributes as %NL80211_CMD_SET_BEACON, + * %NL80211_ATTR_BEACON_HEAD and %NL80211_ATTR_BEACON_TAIL are required, + * configuration changes such as %NL80211_ATTR_FTM_RESPONDER rejected. + * This doesn't acquire the wiphy mutex internally, so it's faster, but + * cannot update any other configuration. + * For MLO a single link is set with %NL80211_ATTR_MLO_LINK_ID as usual + * and multiple can be nested in %NL80211_ATTR_MLO_LINKS but then the + * update isn't atomic if any failures happen. + * Only supported with %NL80211_EXT_FEATURE_UPDATE_BEACON. * @NL80211_CMD_MAX: highest used command number * @__NL80211_CMD_AFTER_LAST: internal use */ @@ -1738,6 +1748,8 @@ enum nl80211_commands { NL80211_CMD_NAN_SET_NON_EVAC_CHANNELS, + NL80211_CMD_UPDATE_BEACON, + /* add new commands above here */ /* used to define NL80211_CMD_MAX below */ @@ -7178,6 +7190,9 @@ enum nl80211_feature_flags { * offload in station mode, including Fast Transition or Opportunistic * Key Caching. * + * @NL80211_EXT_FEATURE_UPDATE_BEACON: Driver supports + * %NL80211_CMD_UPDATE_BEACON. + * * @NUM_NL80211_EXT_FEATURES: number of extended features. * @MAX_NL80211_EXT_FEATURES: highest extended feature index. */ @@ -7261,6 +7276,7 @@ enum nl80211_ext_feature_index { NL80211_EXT_FEATURE_SET_KEY_LTF_SEED, NL80211_EXT_FEATURE_PROBE_AP, NL80211_EXT_FEATURE_FAST_ROAM_OFFLOAD, + NL80211_EXT_FEATURE_UPDATE_BEACON, /* add new features before the definition below */ NUM_NL80211_EXT_FEATURES, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index a48bd4e4f4c7..6387a312b35e 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -7578,15 +7578,87 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) return err; } +static int nl80211_parse_ap_update(struct cfg80211_registered_device *rdev, + struct nlattr *attrs[], + struct ieee80211_channel *chan, + struct cfg80211_ap_update *params, + struct netlink_ext_ack *extack) +{ + struct nlattr *attr; + int err; + + err = nl80211_parse_beacon(rdev, attrs, ¶ms->beacon, chan, extack); + if (err) + return err; + + attr = attrs[NL80211_ATTR_FILS_DISCOVERY]; + if (attr) { + err = nl80211_parse_fils_discovery(rdev, attr, + ¶ms->fils_discovery); + if (err) + return err; + } + + attr = attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]; + if (attr) { + err = nl80211_parse_unsol_bcast_probe_resp(rdev, attr, + ¶ms->unsol_bcast_probe_resp); + if (err) + return err; + } + + attr = attrs[NL80211_ATTR_S1G_SHORT_BEACON]; + if (attr) { + err = nl80211_parse_s1g_short_beacon(rdev, attr, + ¶ms->s1g_short_beacon); + if (err) + return err; + } + + return 0; +} + +static int nl80211_change_beacon(struct cfg80211_registered_device *rdev, + struct net_device *dev, + struct cfg80211_ap_update *params) +{ + struct cfg80211_beaconing_check_config beacon_check = {}; + struct wireless_dev *wdev = dev->ieee80211_ptr; + unsigned int link_id = params->beacon.link_id; + int err; + + /* recheck beaconing is permitted with possibly changed power type */ + beacon_check.iftype = wdev->iftype; + beacon_check.relax = true; + beacon_check.reg_power = + cfg80211_get_6ghz_power_type(params->beacon.tail, + params->beacon.tail_len, 0); + if (!cfg80211_reg_check_beaconing(&rdev->wiphy, + &wdev->links[link_id].ap.chandef, + &beacon_check)) + return -EINVAL; + + err = rdev_change_beacon(rdev, dev, params); + if (!err) + wdev->links[link_id].ap.reg_power = beacon_check.reg_power; + + return err; +} + +static void nl80211_free_ap_update(struct cfg80211_ap_update *params) +{ + kfree(params->beacon.mbssid_ies); + kfree(params->beacon.rnr_ies); + kfree(params); +} + static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; - struct cfg80211_beaconing_check_config beacon_check = {}; unsigned int link_id = nl80211_link_id(info->attrs); struct net_device *dev = info->user_ptr[1]; struct wireless_dev *wdev = dev->ieee80211_ptr; struct cfg80211_ap_update *params; - struct nlattr *attr; int err; if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP && @@ -7603,57 +7675,190 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info) if (!params) return -ENOMEM; - err = nl80211_parse_beacon(rdev, info->attrs, ¶ms->beacon, - wdev->links[link_id].ap.chandef.chan, - info->extack); - if (err) - goto out; - - /* recheck beaconing is permitted with possibly changed power type */ - beacon_check.iftype = wdev->iftype; - beacon_check.relax = true; - beacon_check.reg_power = - cfg80211_get_6ghz_power_type(params->beacon.tail, - params->beacon.tail_len, 0); - if (!cfg80211_reg_check_beaconing(&rdev->wiphy, - &wdev->links[link_id].ap.chandef, - &beacon_check)) { - err = -EINVAL; - goto out; - } - - attr = info->attrs[NL80211_ATTR_FILS_DISCOVERY]; - if (attr) { - err = nl80211_parse_fils_discovery(rdev, attr, - ¶ms->fils_discovery); - if (err) - goto out; - } - - attr = info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]; - if (attr) { - err = nl80211_parse_unsol_bcast_probe_resp(rdev, attr, - ¶ms->unsol_bcast_probe_resp); - if (err) - goto out; - } - - attr = info->attrs[NL80211_ATTR_S1G_SHORT_BEACON]; - if (attr) { - err = nl80211_parse_s1g_short_beacon(rdev, attr, - ¶ms->s1g_short_beacon); - if (err) - goto out; - } - - err = rdev_change_beacon(rdev, dev, params); + err = nl80211_parse_ap_update(rdev, info->attrs, + wdev->links[link_id].ap.chandef.chan, + params, info->extack); if (!err) - wdev->links[link_id].ap.reg_power = beacon_check.reg_power; + err = nl80211_change_beacon(rdev, dev, params); + nl80211_free_ap_update(params); + return err; +} + +static struct cfg80211_ap_update * +nl80211_parse_link_update(struct cfg80211_registered_device *rdev, + struct net_device *dev, struct nlattr *attrs[], + struct netlink_ext_ack *extack) +{ + struct nlattr *link_attr = attrs[NL80211_ATTR_MLO_LINK_ID]; + struct wireless_dev *wdev = dev->ieee80211_ptr; + unsigned int link_id = nl80211_link_id(attrs); + u16 valid_links = READ_ONCE(wdev->valid_links); + struct cfg80211_ap_update *params; + struct ieee80211_channel *chan; + int err; + + /* without locks this is racy, the driver needs to check again */ + if (valid_links ? !link_attr || !(valid_links & BIT(link_id)) : + !!link_attr) + return ERR_PTR(-EINVAL); + + if (attrs[NL80211_ATTR_FTM_RESPONDER] || + attrs[NL80211_ATTR_HE_BSS_COLOR] || + attrs[NL80211_ATTR_S1G_SHORT_BEACON]) { + NL_SET_ERR_MSG(extack, "cannot change configuration in update"); + return ERR_PTR(-EINVAL); + } + + if (!attrs[NL80211_ATTR_BEACON_HEAD] || + !attrs[NL80211_ATTR_BEACON_TAIL]) { + NL_SET_ERR_MSG(extack, "beacon head and tail are required"); + return ERR_PTR(-EINVAL); + } + + chan = READ_ONCE(wdev->links[link_id].ap.chandef.chan); + if (!chan || !READ_ONCE(wdev->links[link_id].ap.beacon_interval)) + return ERR_PTR(-EINVAL); + + params = kzalloc_obj(*params); + if (!params) + return ERR_PTR(-ENOMEM); + + err = nl80211_parse_ap_update(rdev, attrs, chan, params, extack); + if (err) { + nl80211_free_ap_update(params); + return ERR_PTR(err); + } + + return params; +} + +static int nl80211_update_link_beacon(struct cfg80211_registered_device *rdev, + struct net_device *dev, + struct cfg80211_ap_update *params) +{ + struct wireless_dev *wdev = dev->ieee80211_ptr; + unsigned int link_id = params->beacon.link_id; + + /* a power type change needs regulatory checks under the wiphy mutex */ + if (cfg80211_get_6ghz_power_type(params->beacon.tail, + params->beacon.tail_len, 0) != + READ_ONCE(wdev->links[link_id].ap.reg_power)) + return -EAGAIN; + + return rdev_update_beacon(rdev, dev, params); +} + +static int +nl80211_update_link_beacon_locked(struct cfg80211_registered_device *rdev, + struct net_device *dev, + struct cfg80211_ap_update *params) +{ + struct wireless_dev *wdev = dev->ieee80211_ptr; + unsigned int link_id = params->beacon.link_id; + + lockdep_assert_wiphy(&rdev->wiphy); + + if (!rdev->wiphy.registered || !wdev_running(wdev)) + return -ENETDOWN; + + if (wdev->valid_links ? !(wdev->valid_links & BIT(link_id)) : link_id) + return -ENOLINK; + + if (!wdev->links[link_id].ap.beacon_interval) + return -EINVAL; + + return nl80211_change_beacon(rdev, dev, params); +} + +static int nl80211_update_beacon(struct sk_buff *skb, struct genl_info *info) +{ + struct cfg80211_ap_update *params[IEEE80211_MLD_MAX_NUM_LINKS] = {}; + struct cfg80211_registered_device *rdev = info->user_ptr[0]; + struct net_device *dev = info->user_ptr[1]; + unsigned long need_lock = 0; + unsigned int n = 0, i; + struct nlattr *link; + u16 links = 0; + int rem, err = 0; + + if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP && + dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO) + return -EOPNOTSUPP; + + if (!rdev->ops->update_beacon || !rdev->ops->change_beacon || + !wiphy_ext_feature_isset(&rdev->wiphy, + NL80211_EXT_FEATURE_UPDATE_BEACON)) + return -EOPNOTSUPP; + + if (!info->attrs[NL80211_ATTR_MLO_LINKS]) { + params[0] = nl80211_parse_link_update(rdev, dev, info->attrs, + info->extack); + if (IS_ERR(params[0])) + return PTR_ERR(params[0]); + n = 1; + } else { + struct nlattr **attrs = kzalloc_objs(*attrs, NUM_NL80211_ATTR); + + if (!attrs) + return -ENOMEM; + + nla_for_each_nested(link, info->attrs[NL80211_ATTR_MLO_LINKS], + rem) { + struct cfg80211_ap_update *p; + + err = nla_parse_nested(attrs, NL80211_ATTR_MAX, link, + NULL, info->extack); + if (err) + break; + + p = nl80211_parse_link_update(rdev, dev, attrs, + info->extack); + if (IS_ERR(p)) { + err = PTR_ERR(p); + break; + } + + /* also ensures we don't overflow params[] */ + if (links & BIT(p->beacon.link_id)) { + nl80211_free_ap_update(p); + err = -EINVAL; + break; + } + links |= BIT(p->beacon.link_id); + params[n++] = p; + } + + kfree(attrs); + if (!err && !n) + err = -EINVAL; + if (err) + goto out; + } + + /* try updates without wiphy mutex first */ + for (i = 0; i < n; i++) { + err = nl80211_update_link_beacon(rdev, dev, params[i]); + if (err == -EAGAIN) + __set_bit(i, &need_lock); + else if (err) + goto out; + } + + err = 0; + if (need_lock) { + wiphy_lock(&rdev->wiphy); + for_each_set_bit(i, &need_lock, n) { + err = nl80211_update_link_beacon_locked(rdev, dev, + params[i]); + if (err) + break; + } + wiphy_unlock(&rdev->wiphy); + } out: - kfree(params->beacon.mbssid_ies); - kfree(params->beacon.rnr_ies); - kfree(params); + for (i = 0; i < n; i++) + nl80211_free_ap_update(params[i]); return err; } @@ -20927,6 +21132,13 @@ static const struct genl_small_ops nl80211_small_ops[] = { .flags = GENL_ADMIN_PERM, .internal_flags = IFLAGS(NL80211_FLAG_NEED_WDEV_UP), }, + { + .cmd = NL80211_CMD_UPDATE_BEACON, + .doit = nl80211_update_beacon, + .flags = GENL_UNS_ADMIN_PERM, + .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_NO_WIPHY_MTX), + }, }; static struct genl_family nl80211_fam __ro_after_init = { diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 1f1311c20e94..95882c8972d2 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -183,6 +183,17 @@ static inline int rdev_change_beacon(struct cfg80211_registered_device *rdev, return ret; } +static inline int rdev_update_beacon(struct cfg80211_registered_device *rdev, + struct net_device *dev, + struct cfg80211_ap_update *info) +{ + int ret; + trace_rdev_update_beacon(&rdev->wiphy, dev, info); + ret = rdev->ops->update_beacon(&rdev->wiphy, dev, info); + trace_rdev_return_int(&rdev->wiphy, ret); + return ret; +} + static inline int rdev_stop_ap(struct cfg80211_registered_device *rdev, struct net_device *dev, unsigned int link_id) { diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 83007a824010..f5fa2fb76474 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -740,7 +740,7 @@ TRACE_EVENT(rdev_start_ap, __entry->inactivity_timeout, __entry->link_id) ); -TRACE_EVENT(rdev_change_beacon, +DECLARE_EVENT_CLASS(rdev_ap_update, TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, struct cfg80211_ap_update *info), TP_ARGS(wiphy, netdev, info), @@ -788,6 +788,18 @@ TRACE_EVENT(rdev_change_beacon, WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->link_id) ); +DEFINE_EVENT(rdev_ap_update, rdev_change_beacon, + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, + struct cfg80211_ap_update *info), + TP_ARGS(wiphy, netdev, info) +); + +DEFINE_EVENT(rdev_ap_update, rdev_update_beacon, + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, + struct cfg80211_ap_update *info), + TP_ARGS(wiphy, netdev, info) +); + TRACE_EVENT(rdev_stop_ap, TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, unsigned int link_id), -- 2.56.0 From: Johannes Berg The AP's beacon, presp etc. templates are only updated under wiphy mutex right now. Prepare for being able to update them without the wiphy mutex by using a (per-link) spinlock for them. Signed-off-by: Johannes Berg --- net/mac80211/cfg.c | 66 +++++++++++++------------------------- net/mac80211/ieee80211_i.h | 18 +++++++++++ net/mac80211/link.c | 1 + 3 files changed, 42 insertions(+), 43 deletions(-) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index d0d14f621588..08088a2656c7 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1123,13 +1123,11 @@ ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, const struct ieee80211_color_change_settings *cca, struct ieee80211_link_data *link) { - struct probe_resp *new, *old; + struct probe_resp *new; if (!resp || !resp_len) return 1; - old = sdata_dereference(link->u.ap.probe_resp, sdata); - new = kzalloc(sizeof(struct probe_resp) + resp_len, GFP_KERNEL); if (!new) return -ENOMEM; @@ -1144,9 +1142,7 @@ ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, else if (cca) new->cntdwn_counter_offsets[0] = cca->counter_offset_presp; - rcu_assign_pointer(link->u.ap.probe_resp, new); - if (old) - kfree_rcu(old, rcu_head); + ap_tmpl_replace(link, link->u.ap.probe_resp, new); return 0; } @@ -1157,7 +1153,7 @@ static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata, struct ieee80211_bss_conf *link_conf, u64 *changed) { - struct fils_discovery_data *new, *old = NULL; + struct fils_discovery_data *new = NULL; struct ieee80211_fils_discovery *fd; /* @@ -1175,20 +1171,15 @@ static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata, fd->min_interval = params->min_interval; fd->max_interval = params->max_interval; - old = sdata_dereference(link->u.ap.fils_discovery, sdata); if (params->tmpl && params->tmpl_len) { new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL); if (!new) return -ENOMEM; new->len = params->tmpl_len; memcpy(new->data, params->tmpl, params->tmpl_len); - rcu_assign_pointer(link->u.ap.fils_discovery, new); - } else { - RCU_INIT_POINTER(link->u.ap.fils_discovery, NULL); } - if (old) - kfree_rcu(old, rcu_head); + ap_tmpl_replace(link, link->u.ap.fils_discovery, new); *changed |= BSS_CHANGED_FILS_DISCOVERY; return 0; @@ -1201,7 +1192,7 @@ ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata, struct ieee80211_bss_conf *link_conf, u64 *changed) { - struct unsol_bcast_probe_resp_data *new, *old = NULL; + struct unsol_bcast_probe_resp_data *new = NULL; /* * This configuration is only applicable to transmitting BSSes. @@ -1216,21 +1207,15 @@ ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata, link_conf->unsol_bcast_probe_resp_interval = params->interval; - old = sdata_dereference(link->u.ap.unsol_bcast_probe_resp, sdata); - if (params->tmpl && params->tmpl_len) { new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL); if (!new) return -ENOMEM; new->len = params->tmpl_len; memcpy(new->data, params->tmpl, params->tmpl_len); - rcu_assign_pointer(link->u.ap.unsol_bcast_probe_resp, new); - } else { - RCU_INIT_POINTER(link->u.ap.unsol_bcast_probe_resp, NULL); } - if (old) - kfree_rcu(old, rcu_head); + ap_tmpl_replace(link, link->u.ap.unsol_bcast_probe_resp, new); *changed |= BSS_CHANGED_UNSOL_BCAST_PROBE_RESP; return 0; @@ -1649,12 +1634,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, _changed |= BSS_CHANGED_FTM_RESPONDER; } - rcu_assign_pointer(link->u.ap.beacon, new); + ap_tmpl_replace(link, link->u.ap.beacon, new); sdata->u.ap.active = true; - if (old) - kfree_rcu(old, rcu_head); - ieee80211_update_ap_bandwidth(link, params); *changed |= _changed; @@ -1956,11 +1938,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, err = drv_start_ap(sdata->local, sdata, link_conf); if (err) { - old = sdata_dereference(link->u.ap.beacon, sdata); - - if (old) - kfree_rcu(old, rcu_head); - RCU_INIT_POINTER(link->u.ap.beacon, NULL); + ap_tmpl_replace(link, link->u.ap.beacon, NULL); if (ieee80211_num_beaconing_links(sdata) == 0) sdata->u.ap.active = false; @@ -2088,16 +2066,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev, lockdep_assert_wiphy(local->hw.wiphy); - old_beacon = sdata_dereference(link->u.ap.beacon, sdata); - if (!old_beacon) + if (!sdata_dereference(link->u.ap.beacon, sdata)) return -ENOENT; - old_probe_resp = sdata_dereference(link->u.ap.probe_resp, - sdata); - old_fils_discovery = sdata_dereference(link->u.ap.fils_discovery, - sdata); - old_unsol_bcast_probe_resp = - sdata_dereference(link->u.ap.unsol_bcast_probe_resp, - sdata); old_s1g_short_beacon = sdata_dereference(link->u.ap.s1g_short_beacon, sdata); @@ -2118,10 +2088,20 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev, } /* remove beacon and probe response */ - RCU_INIT_POINTER(link->u.ap.beacon, NULL); - RCU_INIT_POINTER(link->u.ap.probe_resp, NULL); - RCU_INIT_POINTER(link->u.ap.fils_discovery, NULL); - RCU_INIT_POINTER(link->u.ap.unsol_bcast_probe_resp, NULL); + scoped_guard(spinlock, &link->ap_tmpl_lock) { + old_beacon = ap_tmpl_dereference(link, link->u.ap.beacon); + old_probe_resp = ap_tmpl_dereference(link, + link->u.ap.probe_resp); + old_fils_discovery = + ap_tmpl_dereference(link, link->u.ap.fils_discovery); + old_unsol_bcast_probe_resp = + ap_tmpl_dereference(link, + link->u.ap.unsol_bcast_probe_resp); + RCU_INIT_POINTER(link->u.ap.beacon, NULL); + RCU_INIT_POINTER(link->u.ap.probe_resp, NULL); + RCU_INIT_POINTER(link->u.ap.fils_discovery, NULL); + RCU_INIT_POINTER(link->u.ap.unsol_bcast_probe_resp, NULL); + } RCU_INIT_POINTER(link->u.ap.s1g_short_beacon, NULL); kfree_rcu(old_beacon, rcu_head); if (old_probe_resp) diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index b571019bd05a..b16ff6a4aacf 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -1152,6 +1152,9 @@ struct ieee80211_link_data { struct ieee80211_link_data_ap ap; } u; + /* protects replacing the u.ap template pointers, also without wiphy mutex */ + spinlock_t ap_tmpl_lock; + struct ieee80211_tx_queue_params tx_conf[IEEE80211_NUM_ACS]; struct ieee80211_bss_conf *conf; @@ -1166,6 +1169,21 @@ struct ieee80211_link_data { #endif }; +#define ap_tmpl_dereference(link, p) \ + rcu_dereference_protected(p, lockdep_is_held(&(link)->ap_tmpl_lock)) + +#define ap_tmpl_replace(link, p, new) \ +do { \ + typeof(ap_tmpl_dereference(link, p)) __old; \ + \ + spin_lock(&(link)->ap_tmpl_lock); \ + __old = rcu_replace_pointer(p, new, \ + lockdep_is_held(&(link)->ap_tmpl_lock));\ + spin_unlock(&(link)->ap_tmpl_lock); \ + if (__old) \ + kfree_rcu(__old, rcu_head); \ +} while (0) + struct ieee80211_sub_if_data { struct list_head list; diff --git a/net/mac80211/link.c b/net/mac80211/link.c index 931950a10508..ce1c95b40cd9 100644 --- a/net/mac80211/link.c +++ b/net/mac80211/link.c @@ -126,6 +126,7 @@ void ieee80211_link_init(struct ieee80211_sub_if_data *sdata, link->ap_power_level = IEEE80211_UNSET_POWER_LEVEL; link->user_power_level = sdata->local->user_power_level; link_conf->txpower = INT_MIN; + spin_lock_init(&link->ap_tmpl_lock); wiphy_work_init(&link->csa.finalize_work, ieee80211_csa_finalize_work); -- 2.56.0 From: Johannes Berg Refactor allocating/filling beacon data to a new function, so we can reuse it for lockless beacon updates. Also split out the bssid_indicator calculation, lockless updates will need to ensure it didn't change. Assisted-by: LLM Signed-off-by: Johannes Berg --- net/mac80211/cfg.c | 127 +++++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 56 deletions(-) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 08088a2656c7..0b124b04003e 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1483,52 +1483,16 @@ static void ieee80211_update_ap_bandwidth(struct ieee80211_link_data *link, ieee80211_recalc_chanctx_min_def(local, chanctx); } -static int -ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, - struct ieee80211_link_data *link, - struct cfg80211_beacon_data *params, - const struct ieee80211_csa_settings *csa, - const struct ieee80211_color_change_settings *cca, - u64 *changed) +static struct beacon_data * +ieee80211_alloc_beacon(struct cfg80211_mbssid_elems *mbssid, + struct cfg80211_rnr_elems *rnr, + int head_len, int tail_len) { - struct cfg80211_mbssid_elems *mbssid = NULL; - struct cfg80211_rnr_elems *rnr = NULL; - struct beacon_data *new, *old; - int new_head_len, new_tail_len; - int size, err; + struct beacon_data *new; + int size; u8 *pos; - u64 _changed = BSS_CHANGED_BEACON; - struct ieee80211_bss_conf *link_conf = link->conf; - old = sdata_dereference(link->u.ap.beacon, sdata); - - /* Need to have a beacon head if we don't have one yet */ - if (!params->head && !old) - return -EINVAL; - - /* new or old head? */ - if (params->head) - new_head_len = params->head_len; - else - new_head_len = old->head_len; - - /* new or old tail? */ - if (params->tail || !old) - /* params->tail_len will be zero for !params->tail */ - new_tail_len = params->tail_len; - else - new_tail_len = old->tail_len; - - size = sizeof(*new) + new_head_len + new_tail_len; - - /* new or old multiple BSSID elements? */ - if (params->mbssid_ies) { - mbssid = params->mbssid_ies; - rnr = params->rnr_ies; - } else if (old && old->mbssid_ies) { - mbssid = old->mbssid_ies; - rnr = old->rnr_ies; - } + size = sizeof(*new) + head_len + tail_len; if (mbssid) { size += struct_size(new->mbssid_ies, elem, mbssid->cnt); @@ -1540,9 +1504,7 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, new = kzalloc(size, GFP_KERNEL); if (!new) - return -ENOMEM; - - /* start filling the new info now */ + return NULL; /* * pointers go into the block we allocated, the order in memory is @@ -1565,11 +1527,11 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, } new->head = pos; - new->head_len = new_head_len; - pos += new_head_len; + new->head_len = head_len; + pos += head_len; new->tail = pos; - new->tail_len = new_tail_len; - pos += new_tail_len; + new->tail_len = tail_len; + pos += tail_len; /* copy in optional mbssid_ies */ if (mbssid) { @@ -1577,14 +1539,67 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, mbssid); if (rnr) ieee80211_copy_rnr_beacon(pos, new->rnr_ies, rnr); - /* update bssid_indicator */ - if (new->mbssid_ies->cnt && new->mbssid_ies->elem[0].len > 2) - link_conf->bssid_indicator = - *(new->mbssid_ies->elem[0].data + 2); - else - link_conf->bssid_indicator = 0; } + return new; +} + +static u8 ieee80211_beacon_bssid_indicator(struct beacon_data *beacon) +{ + if (beacon->mbssid_ies->cnt && beacon->mbssid_ies->elem[0].len > 2) + return *(beacon->mbssid_ies->elem[0].data + 2); + return 0; +} + +static int +ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, + struct ieee80211_link_data *link, + struct cfg80211_beacon_data *params, + const struct ieee80211_csa_settings *csa, + const struct ieee80211_color_change_settings *cca, + u64 *changed) +{ + struct cfg80211_mbssid_elems *mbssid = params->mbssid_ies; + struct cfg80211_rnr_elems *rnr = mbssid ? params->rnr_ies : NULL; + struct beacon_data *new, *old; + int new_head_len, new_tail_len; + int err; + u64 _changed = BSS_CHANGED_BEACON; + struct ieee80211_bss_conf *link_conf = link->conf; + + old = sdata_dereference(link->u.ap.beacon, sdata); + + /* Need to have a beacon head if we don't have one yet */ + if (!params->head && !old) + return -EINVAL; + + /* new or old head? */ + if (params->head) + new_head_len = params->head_len; + else + new_head_len = old->head_len; + + /* new or old tail? */ + if (params->tail || !old) + /* params->tail_len will be zero for !params->tail */ + new_tail_len = params->tail_len; + else + new_tail_len = old->tail_len; + + /* new or old multiple BSSID elements? */ + if (!mbssid && old && old->mbssid_ies) { + mbssid = old->mbssid_ies; + rnr = old->rnr_ies; + } + + new = ieee80211_alloc_beacon(mbssid, rnr, new_head_len, new_tail_len); + if (!new) + return -ENOMEM; + + if (new->mbssid_ies) + link_conf->bssid_indicator = + ieee80211_beacon_bssid_indicator(new); + if (csa) { new->cntdwn_current_counter = csa->count; memcpy(new->cntdwn_counter_offsets, csa->counter_offsets_beacon, -- 2.56.0 From: Johannes Berg Implement the new update_beacon() method to update beacon and related templates without the wiphy mutex. If anything else were to change return -EAGAIN, same if there are any active countdowns, and during HW restart. Eventually, the idea is to replace countdowns by beacon update requests so hostapd can handle everything instead. This doesn't enable it for any drivers since it depends on the driver behaviour, if it fetches each beacon and doesn't use the other templates it can just set the feature flag (NL80211_EXT_FEATURE_UPDATE_BEACON), otherwise it must implement the new update_beacon() mac80211 method. The regular beacon update may need to take head, tail and MBSSID/RNR elements from the old beacon, so while that's going on mark "bss_update_in_progress" and make lockless updates return -EAGAIN during that time, so that the old data remains valid/identical. For MBSSID, the lockless path also can't copy old data, so that also has to return -EAGAIN. All those -EAGAIN cases fall back to cfg80211 and do the update with the lock held, where it can then be handled. Userspace should ideally ensure it doesn't hit that path. Assisted-by: LLM Signed-off-by: Johannes Berg --- v2: - split - handle concurrent locked/unlocked updates differently to avoid mbssid problems --- include/net/mac80211.h | 7 ++ net/mac80211/cfg.c | 225 ++++++++++++++++++++++++++++++++++--- net/mac80211/driver-ops.h | 16 +++ net/mac80211/ieee80211_i.h | 2 + net/mac80211/trace.h | 28 +++++ 5 files changed, 263 insertions(+), 15 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index a24d849b8f9c..2a61079e3975 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -4586,6 +4586,9 @@ struct ieee80211_prep_tx_info { * just "paused" for scanning/ROC, which is indicated by the beacon being * disabled/enabled via @bss_info_changed. * @stop_ap: Stop operation on the AP interface. + * @update_beacon: Update the templates indicated by @changed (beacon, probe + * response, FILS discovery and/or unsolicitated bcast probe response) + * without the wiphy mutex held. This callback must not sleep. * * @reconfig_complete: Called after a call to ieee80211_restart_hw() and * during resume, when the reconfiguration has completed. @@ -4783,6 +4786,10 @@ struct ieee80211_ops { struct ieee80211_bss_conf *link_conf); void (*stop_ap)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_bss_conf *link_conf); + void (*update_beacon)(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct ieee80211_bss_conf *link_conf, + u64 changed); u64 (*prepare_multicast)(struct ieee80211_hw *hw, struct netdev_hw_addr_list *mc_list); diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 0b124b04003e..30fe29f25928 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1561,17 +1561,23 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, { struct cfg80211_mbssid_elems *mbssid = params->mbssid_ies; struct cfg80211_rnr_elems *rnr = mbssid ? params->rnr_ies : NULL; - struct beacon_data *new, *old; + struct beacon_data *new = NULL, *old; int new_head_len, new_tail_len; int err; u64 _changed = BSS_CHANGED_BEACON; struct ieee80211_bss_conf *link_conf = link->conf; - old = sdata_dereference(link->u.ap.beacon, sdata); + /* grab old and make lockless updates return -EAGAIN */ + spin_lock(&link->ap_tmpl_lock); + old = ap_tmpl_dereference(link, link->u.ap.beacon); + link->u.ap.bss_update_in_progress = true; + spin_unlock(&link->ap_tmpl_lock); /* Need to have a beacon head if we don't have one yet */ - if (!params->head && !old) - return -EINVAL; + if (!params->head && !old) { + err = -EINVAL; + goto error; + } /* new or old head? */ if (params->head) @@ -1593,8 +1599,10 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, } new = ieee80211_alloc_beacon(mbssid, rnr, new_head_len, new_tail_len); - if (!new) - return -ENOMEM; + if (!new) { + err = -ENOMEM; + goto error; + } if (new->mbssid_ies) link_conf->bssid_indicator = @@ -1625,10 +1633,8 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, err = ieee80211_set_probe_resp(sdata, params->probe_resp, params->probe_resp_len, csa, cca, link); - if (err < 0) { - kfree(new); - return err; - } + if (err < 0) + goto error; if (err == 0) _changed |= BSS_CHANGED_AP_PROBE_RESP; @@ -1641,21 +1647,31 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, params->civicloc_len, link_conf); - if (err < 0) { - kfree(new); - return err; - } + if (err < 0) + goto error; _changed |= BSS_CHANGED_FTM_RESPONDER; } - ap_tmpl_replace(link, link->u.ap.beacon, new); + spin_lock(&link->ap_tmpl_lock); + rcu_assign_pointer(link->u.ap.beacon, new); + link->u.ap.bss_update_in_progress = false; + spin_unlock(&link->ap_tmpl_lock); sdata->u.ap.active = true; + if (old) + kfree_rcu(old, rcu_head); + ieee80211_update_ap_bandwidth(link, params); *changed |= _changed; return 0; +error: + spin_lock(&link->ap_tmpl_lock); + link->u.ap.bss_update_in_progress = false; + spin_unlock(&link->ap_tmpl_lock); + kfree(new); + return err; } static u8 ieee80211_num_beaconing_links(struct ieee80211_sub_if_data *sdata) @@ -2050,6 +2066,184 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, return 0; } +static int ieee80211_update_beacon(struct wiphy *wiphy, struct net_device *dev, + struct cfg80211_ap_update *params) +{ + struct cfg80211_unsol_bcast_probe_resp *ubpr = + ¶ms->unsol_bcast_probe_resp; + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); + struct cfg80211_fils_discovery *fd = ¶ms->fils_discovery; + struct unsol_bcast_probe_resp_data *new_ubpr = NULL, *old_ubpr = NULL; + struct cfg80211_beacon_data *beacon = ¶ms->beacon; + struct fils_discovery_data *new_fd = NULL, *old_fd = NULL; + struct probe_resp *new_presp = NULL, *old_presp = NULL; + struct ieee80211_local *local = sdata->local; + struct beacon_data *new, *old = NULL; + struct ieee80211_bss_conf *link_conf; + struct ieee80211_link_data *link; + struct ieee80211_channel *chan; + u64 changed = BSS_CHANGED_BEACON; + int err = 0; + + if (READ_ONCE(local->in_reconfig) || READ_ONCE(local->quiescing)) + return -EAGAIN; + + new = ieee80211_alloc_beacon(beacon->mbssid_ies, + beacon->mbssid_ies ? beacon->rnr_ies : NULL, + beacon->head_len, beacon->tail_len); + if (!new) + return -ENOMEM; + memcpy(new->head, beacon->head, beacon->head_len); + memcpy(new->tail, beacon->tail, beacon->tail_len); + + if (beacon->probe_resp && beacon->probe_resp_len) { + new_presp = kzalloc(sizeof(*new_presp) + beacon->probe_resp_len, + GFP_KERNEL); + if (!new_presp) { + err = -ENOMEM; + goto free; + } + new_presp->len = beacon->probe_resp_len; + memcpy(new_presp->data, beacon->probe_resp, + beacon->probe_resp_len); + changed |= BSS_CHANGED_AP_PROBE_RESP; + } + + if (fd->update && fd->tmpl && fd->tmpl_len) { + new_fd = kzalloc(sizeof(*new_fd) + fd->tmpl_len, GFP_KERNEL); + if (!new_fd) { + err = -ENOMEM; + goto free; + } + new_fd->len = fd->tmpl_len; + memcpy(new_fd->data, fd->tmpl, fd->tmpl_len); + } + + if (ubpr->update && ubpr->tmpl && ubpr->tmpl_len) { + new_ubpr = kzalloc(sizeof(*new_ubpr) + ubpr->tmpl_len, + GFP_KERNEL); + if (!new_ubpr) { + err = -ENOMEM; + goto free; + } + new_ubpr->len = ubpr->tmpl_len; + memcpy(new_ubpr->data, ubpr->tmpl, ubpr->tmpl_len); + } + + rcu_read_lock(); + + /* do_stop() synchronizes RCU before the interface type can change */ + if (!ieee80211_sdata_running(sdata) || + (sdata->vif.type != NL80211_IFTYPE_AP && + sdata->vif.type != NL80211_IFTYPE_P2P_GO)) { + err = -ENETDOWN; + goto unlock; + } + + link = rcu_dereference(sdata->link[beacon->link_id]); + if (!link) { + err = -ENOLINK; + goto unlock; + } + link_conf = link->conf; + + /* configuration changes need the wiphy mutex, let cfg80211 retry */ + err = -EAGAIN; + + if (READ_ONCE(link_conf->csa_active) || + READ_ONCE(link_conf->color_change_active)) + goto unlock; + + if (new->mbssid_ies && + ieee80211_beacon_bssid_indicator(new) != + READ_ONCE(link_conf->bssid_indicator)) + goto unlock; + + if (fd->update && + (fd->min_interval != READ_ONCE(link_conf->fils_discovery.min_interval) || + fd->max_interval != READ_ONCE(link_conf->fils_discovery.max_interval))) + goto unlock; + + if (ubpr->update && + ubpr->interval != + READ_ONCE(link_conf->unsol_bcast_probe_resp_interval)) + goto unlock; + + chan = READ_ONCE(link_conf->chanreq.oper.chan); + if (!chan) + goto unlock; + + if (chan->band != NL80211_BAND_S1GHZ) { + enum ieee80211_sta_rx_bandwidth he_and_lower; + + he_and_lower = ieee80211_calc_ap_he_and_lower(beacon); + if (he_and_lower != READ_ONCE(link->bss_bw.he_and_lower) || + ieee80211_calc_ap_eht_bw(beacon, he_and_lower) != + READ_ONCE(link->bss_bw.eht)) + goto unlock; + } + + spin_lock(&link->ap_tmpl_lock); + old = ap_tmpl_dereference(link, link->u.ap.beacon); + if (!old) { + err = -ENOENT; + } else if (link->u.ap.bss_update_in_progress) { + /* a locked update is in progress */ + old = NULL; + } else if (READ_ONCE(old->cntdwn_current_counter)) { + /* a countdown just started, must keep the offsets */ + old = NULL; + } else if (old->mbssid_ies && !new->mbssid_ies) { + /* only locked path copies old MBSSID data */ + old = NULL; + } else { + rcu_assign_pointer(link->u.ap.beacon, new); + new = NULL; + + if (new_presp) + old_presp = rcu_replace_pointer(link->u.ap.probe_resp, + new_presp, + lockdep_is_held(&link->ap_tmpl_lock)); + new_presp = NULL; + + if (fd->update) { + old_fd = rcu_replace_pointer(link->u.ap.fils_discovery, + new_fd, + lockdep_is_held(&link->ap_tmpl_lock)); + changed |= BSS_CHANGED_FILS_DISCOVERY; + } + new_fd = NULL; + + if (ubpr->update) { + old_ubpr = rcu_replace_pointer(link->u.ap.unsol_bcast_probe_resp, + new_ubpr, + lockdep_is_held(&link->ap_tmpl_lock)); + changed |= BSS_CHANGED_UNSOL_BCAST_PROBE_RESP; + } + new_ubpr = NULL; + + drv_update_beacon(local, sdata, link_conf, changed); + err = 0; + } + spin_unlock(&link->ap_tmpl_lock); +unlock: + rcu_read_unlock(); +free: + kfree(new); + kfree(new_presp); + kfree(new_fd); + kfree(new_ubpr); + if (old) + kfree_rcu(old, rcu_head); + if (old_presp) + kfree_rcu(old_presp, rcu_head); + if (old_fd) + kfree_rcu(old_fd, rcu_head); + if (old_ubpr) + kfree_rcu(old_ubpr, rcu_head); + return err; +} + static void ieee80211_free_next_beacon(struct ieee80211_link_data *link) { if (!link->u.ap.next_beacon) @@ -6071,6 +6265,7 @@ const struct cfg80211_ops mac80211_config_ops = { .set_default_beacon_key = ieee80211_config_default_beacon_key, .start_ap = ieee80211_start_ap, .change_beacon = ieee80211_change_beacon, + .update_beacon = ieee80211_update_beacon, .stop_ap = ieee80211_stop_ap, .add_station = ieee80211_add_station, .del_station = ieee80211_del_station, diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index f1c0b87fddd5..accd89bbc1fb 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -1107,6 +1107,22 @@ static inline void drv_stop_ap(struct ieee80211_local *local, trace_drv_return_void(local); } +static inline void drv_update_beacon(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + u64 changed) +{ + /* can race with HW restart, so don't warn */ + if (!(sdata->flags & IEEE80211_SDATA_IN_DRIVER)) + return; + + trace_drv_update_beacon(local, sdata, link_conf, changed); + if (local->ops->update_beacon) + local->ops->update_beacon(&local->hw, &sdata->vif, link_conf, + changed); + trace_drv_return_void(local); +} + static inline void drv_reconfig_complete(struct ieee80211_local *local, enum ieee80211_reconfig_type reconfig_type) diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index b16ff6a4aacf..16c1e0337f3a 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -1103,6 +1103,8 @@ struct ieee80211_link_data_ap { struct unsol_bcast_probe_resp_data __rcu *unsol_bcast_probe_resp; struct s1g_short_beacon_data __rcu *s1g_short_beacon; + bool bss_update_in_progress; + /* to be used after channel switch. */ struct cfg80211_beacon_data *next_beacon; }; diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h index 562a4964afa3..ea46c3cca21a 100644 --- a/net/mac80211/trace.h +++ b/net/mac80211/trace.h @@ -1924,6 +1924,34 @@ TRACE_EVENT(drv_stop_ap, ) ); +TRACE_EVENT(drv_update_beacon, + TP_PROTO(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + u64 changed), + + TP_ARGS(local, sdata, link_conf, changed), + + TP_STRUCT__entry( + LOCAL_ENTRY + VIF_ENTRY + __field(u32, link_id) + __field(u64, changed) + ), + + TP_fast_assign( + LOCAL_ASSIGN; + VIF_ASSIGN; + __entry->link_id = link_conf->link_id; + __entry->changed = changed; + ), + + TP_printk( + LOCAL_PR_FMT VIF_PR_FMT " link id %u changed:%#llx", + LOCAL_PR_ARG, VIF_PR_ARG, __entry->link_id, __entry->changed + ) +); + TRACE_EVENT(drv_reconfig_complete, TP_PROTO(struct ieee80211_local *local, enum ieee80211_reconfig_type reconfig_type), -- 2.56.0 From: Johannes Berg We just use ieee80211_beacon_get_template_ema_list() or ieee80211_beacon_get() here to transmit the beacons, so there's no need to implement update_beacon() and we can just set NL80211_EXT_FEATURE_UPDATE_BEACON. Signed-off-by: Johannes Berg --- drivers/net/wireless/virtual/mac80211_hwsim_main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/wireless/virtual/mac80211_hwsim_main.c b/drivers/net/wireless/virtual/mac80211_hwsim_main.c index ce06867ff01e..87f20d5ff0ee 100644 --- a/drivers/net/wireless/virtual/mac80211_hwsim_main.c +++ b/drivers/net/wireless/virtual/mac80211_hwsim_main.c @@ -6017,6 +6017,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, NL80211_EXT_FEATURE_EXT_KEY_ID); wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_ASSOC_FRAME_ENCRYPTION); + wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_UPDATE_BEACON); hw->wiphy->interface_modes = param->iftypes; -- 2.56.0 From: Johannes Berg The update-beacon operation currently can fairly easily work without extra care and without the wiphy mutex as it holds the netdev. However, we can't do the same now for wdevs, they could be unregistered (and destroyed) while the operation is running. Allow for such operations as well by giving the wdevs a refcount as well, and waiting for that refcount to drop to zero during wdev unregistration. This requires some special care in nl80211, especially for a "re-do the operation locked" implementation, but that's contained here so drivers need not worry about it (except if they request the operation to be retried under lock, that might never happen). Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 8 +++++ net/wireless/core.c | 11 ++++++ net/wireless/core.h | 21 +++++++++++ net/wireless/nl80211.c | 82 +++++++++++++++++++++++++++++++++--------- 4 files changed, 106 insertions(+), 16 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 7de6dbdae930..16f6b96e0d88 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -7226,6 +7227,9 @@ enum ieee80211_ap_reg_power { * @netdev: (private) Used to reference back to the netdev, may be %NULL * @identifier: (private) Identifier used in nl80211 to identify this * wireless device if it has no netdev + * @refcnt: (private) references held by nl80211 if there's no netdev, + * to ensure cfg80211_unregister_wdev() cannot return while + * unlocked ops are running * @u: union containing data specific to @iftype * @connected: indicates if connected or not (STA mode) * @wext: (private) Used by the internal wireless extensions compat code @@ -7291,6 +7295,7 @@ struct wireless_dev { struct net_device *netdev; u32 identifier; + refcount_t refcnt; struct list_head mgmt_registrations; u8 mgmt_registrations_need_update:1; @@ -10174,6 +10179,9 @@ u32 cfg80211_calculate_bitrate(struct rate_info *rate); * when the driver wishes to unregister the wdev, e.g. when the hardware device * is unbound from the driver. * + * Note that unlocked operations can run concurrently until this returns, so be + * careful about ordering. + * * Context: Requires the RTNL and wiphy mutex to be held. */ void cfg80211_unregister_wdev(struct wireless_dev *wdev); diff --git a/net/wireless/core.c b/net/wireless/core.c index 01b077909179..777f48a01101 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -1514,6 +1514,16 @@ static void _cfg80211_unregister_wdev(struct wireless_dev *wdev, list_del_rcu(&wdev->list); synchronize_net(); + + /* + * After synchronize_net() any lookups under RCU either have + * references (must wdev_hold() inside the critical section) + * or can't find this wdev anymore. Wait for any wdev_hold() + * users on a pure wdev to get to dev_put(). + */ + if (!wdev->netdev && !refcount_dec_and_test(&wdev->refcnt)) + wait_var_event(&wdev->refcnt, !refcount_read(&wdev->refcnt)); + rdev->devlist_generation++; wiphy_work_cancel(wdev->wiphy, &wdev->disconnect_wk); @@ -1700,6 +1710,7 @@ EXPORT_SYMBOL(cfg80211_stop_link); void cfg80211_init_wdev(struct wireless_dev *wdev) { + refcount_set(&wdev->refcnt, 1); INIT_LIST_HEAD(&wdev->event_list); spin_lock_init(&wdev->event_lock); INIT_LIST_HEAD(&wdev->mgmt_registrations); diff --git a/net/wireless/core.h b/net/wireless/core.h index dfcb9ed5035b..220544a6bb83 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include "reg.h" @@ -259,6 +260,26 @@ void cfg80211_init_wdev(struct wireless_dev *wdev); void cfg80211_register_wdev(struct cfg80211_registered_device *rdev, struct wireless_dev *wdev); +static inline void wdev_hold(struct wireless_dev *wdev) +{ + if (wdev->netdev) { + dev_hold(wdev->netdev); + return; + } + + /* also not RTNL, but can't check that */ + lockdep_assert_not_held(&wdev->wiphy->mtx); + refcount_inc(&wdev->refcnt); +} + +static inline void wdev_put(struct wireless_dev *wdev) +{ + if (wdev->netdev) + dev_put(wdev->netdev); + else if (refcount_dec_and_test(&wdev->refcnt)) + wake_up_var(&wdev->refcnt); +} + static inline bool cfg80211_has_monitors_only(struct cfg80211_registered_device *rdev) { lockdep_assert_held(&rdev->wiphy.mtx); diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 6387a312b35e..eb91937a7238 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -266,6 +266,34 @@ nl80211_lock_and_recheck(struct cfg80211_registered_device *rdev, return ERR_PTR(-ENODEV); } +/* + * Must have wdev_hold(), acquire the wiphy mutex. This has to + * look up the wdev again (otherwise it could deadlock against + * wdev removal), so it can return an error pointer. + */ +static struct wireless_dev * +nl80211_lock_held_wdev(struct cfg80211_registered_device *rdev, + struct genl_info *info, struct wireless_dev *wdev) +{ + u32 wdev_id = wdev->identifier; + + /* the wdev reference was keeping the wiphy alive */ + get_device(&rdev->wiphy.dev); + info->user_ptr[1] = NULL; + wdev_put(wdev); + + wdev = nl80211_lock_and_recheck(rdev, genl_info_net(info), wdev_id); + if (IS_ERR(wdev)) + return wdev; + + if (!wdev_running(wdev)) { + wiphy_unlock(&rdev->wiphy); + return ERR_PTR(-ENETDOWN); + } + + return wdev; +} + static int validate_beacon_head(const struct nlattr *attr, struct netlink_ext_ack *extack) { @@ -19906,14 +19934,10 @@ nl80211_epcs_cfg(struct sk_buff *skb, struct genl_info *info) /* * Note: a - * SELECTOR(..., - * NL80211_FLAG_NO_WIPHY_MTX | - * NL80211_FLAG_NEED_WDEV) - * or * SELECTOR(..., NL80211_FLAG_NO_WIPHY_MTX) * - * isn't valid - nothing would ensure the device stays around, - * need at least a netdev, the wiphy mutex, or RTNL. + * isn't valid - nothing would ensure the device stays around, need + * at least a wdev/netdev reference, the wiphy mutex, or RTNL. */ enum nl80211_internal_flags_selector { @@ -19928,6 +19952,13 @@ static u32 nl80211_internal_flags[] = { #undef SELECTOR }; +/* without RTNL or wiphy mutex, a wdev reference keeps everything alive */ +static bool nl80211_need_wdev_ref(u32 internal_flags) +{ + return !(internal_flags & NL80211_FLAG_NEED_RTNL) && + internal_flags & NL80211_FLAG_NO_WIPHY_MTX; +} + static int nl80211_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info) @@ -19936,7 +19967,7 @@ static int nl80211_pre_doit(const struct genl_split_ops *ops, struct net *netns = genl_info_net(info); struct wireless_dev *wdev = NULL; struct net_device *dev = NULL; - bool need_rtnl, locked = false; + bool need_rtnl, wdev_ref, locked = false; u32 internal_flags, wdev_id = 0; int err; @@ -19945,6 +19976,7 @@ static int nl80211_pre_doit(const struct genl_split_ops *ops, internal_flags = nl80211_internal_flags[ops->internal_flags]; need_rtnl = internal_flags & NL80211_FLAG_NEED_RTNL; + wdev_ref = nl80211_need_wdev_ref(internal_flags); if (need_rtnl) rtnl_lock(); @@ -19966,7 +19998,16 @@ static int nl80211_pre_doit(const struct genl_split_ops *ops, } dev = wdev->netdev; - dev_hold(dev); + /* + * Take the wdev ref only if we need it (wdev_ref is true when + * the op is unlocked), we must not try to acquire this with + * a wdev pointer protected by locking, it'd deadlock against + * unregistration. + */ + if (wdev_ref) + wdev_hold(wdev); + else + dev_hold(dev); rdev = wiphy_to_rdev(wdev->wiphy); } @@ -19999,12 +20040,12 @@ static int nl80211_pre_doit(const struct genl_split_ops *ops, locked = true; } else if (rdev && !need_rtnl) { /* - * Without any mutex need a netdev to keep it alive, see - * also the note about selectors - the WARN can't happen + * Without any mutex need a wdev reference to keep it alive, + * see also the note about selectors - the WARN can't happen * without wrong selectors and those don't exist. */ put_device(&rdev->wiphy.dev); - if (WARN_ON(!dev)) { + if (WARN_ON(!wdev)) { err = -EINVAL; goto out; } @@ -20069,7 +20110,10 @@ static int nl80211_pre_doit(const struct genl_split_ops *ops, out: if (locked) wiphy_unlock(&rdev->wiphy); - dev_put(dev); + if (wdev_ref && wdev) + wdev_put(wdev); + else + dev_put(dev); if (need_rtnl) rtnl_unlock(); return err; @@ -20086,17 +20130,23 @@ static void nl80211_post_doit(const struct genl_split_ops *ops, struct genl_info *info) { u32 internal_flags = nl80211_internal_flags[ops->internal_flags]; + struct wireless_dev *wdev = NULL; + struct net_device *dev = NULL; if (info->user_ptr[1]) { if (internal_flags & NL80211_FLAG_NEED_WDEV) { - struct wireless_dev *wdev = info->user_ptr[1]; - - dev_put(wdev->netdev); + wdev = info->user_ptr[1]; + dev = wdev->netdev; } else { - dev_put(info->user_ptr[1]); + dev = info->user_ptr[1]; } } + if (wdev && nl80211_need_wdev_ref(internal_flags)) + wdev_put(wdev); + else + dev_put(dev); + if (info->user_ptr[0] && !(internal_flags & NL80211_FLAG_NO_WIPHY_MTX)) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; -- 2.56.0 From: Johannes Berg Management frame transmissions need the wiphy mutex, but the normal transmit path doesn't, so most cases shouldn't need it here either. Due to checks, limit it to AP/GO for now. To support it, drivers must implement the new method mgmt_tx_unlocked(), and return -EAGAIN from that for requests that cannot be done without the wiphy mutex (e.g. off-channel in mac80211), then the operation will be retried with the wiphy mutex held. Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 11 ++++++++ net/wireless/core.h | 4 +++ net/wireless/mlme.c | 58 ++++++++++++++++++++++++++++++++++++----- net/wireless/nl80211.c | 50 +++++++++++++++++++++++++++-------- net/wireless/rdev-ops.h | 12 +++++++++ net/wireless/trace.h | 14 +++++++++- 6 files changed, 130 insertions(+), 19 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 16f6b96e0d88..0eee48bdc937 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -5087,6 +5087,13 @@ struct mgmt_frame_regs { * This allows the operation to be terminated prior to timeout based on * the duration value. * @mgmt_tx: Transmit a management frame. + * @mgmt_tx_unlocked: Like @mgmt_tx, but called without the wiphy mutex, + * currently limited to AP/GO/NAN. The driver may -EAGAIN to get + * @mgmt_tx with the wiphy mutex held for the same operation, + * although if the device is removed concurrently that might not + * happen and userspace sees -ENODEV. + * Note this cannot acquire the wiphy mutex or RTNL, that would + * deadlock against unregistering the wdev. * @mgmt_tx_cancel_wait: Cancel the wait time from transmitting a management * frame on another channel * @@ -5510,6 +5517,10 @@ struct cfg80211_ops { int (*mgmt_tx)(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params, u64 cookie); + int (*mgmt_tx_unlocked)(struct wiphy *wiphy, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie); int (*mgmt_tx_cancel_wait)(struct wiphy *wiphy, struct wireless_dev *wdev, u64 cookie); diff --git a/net/wireless/core.h b/net/wireless/core.h index 220544a6bb83..0e05d4247bea 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -432,6 +432,10 @@ int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params, u64 cookie); +int cfg80211_mlme_mgmt_tx_unlocked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie); void cfg80211_oper_and_ht_capa(struct ieee80211_ht_cap *ht_capa, const struct ieee80211_ht_cap *ht_capa_mask); void cfg80211_oper_and_vht_capa(struct ieee80211_vht_cap *vht_capa, diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c index 96fd14f2a305..80ca9031d29c 100644 --- a/net/wireless/mlme.c +++ b/net/wireless/mlme.c @@ -892,15 +892,14 @@ static bool cfg80211_allowed_random_address(struct wireless_dev *wdev, return false; } -int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev, - struct wireless_dev *wdev, - struct cfg80211_mgmt_tx_params *params, u64 cookie) +static int cfg80211_mgmt_tx_check(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + enum nl80211_iftype iftype, + struct cfg80211_mgmt_tx_params *params) { const struct ieee80211_mgmt *mgmt; u16 stype; - lockdep_assert_wiphy(&rdev->wiphy); - if (!wdev->wiphy->mgmt_stypes) return -EOPNOTSUPP; @@ -917,14 +916,14 @@ int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev, return -EINVAL; stype = le16_to_cpu(mgmt->frame_control) & IEEE80211_FCTL_STYPE; - if (!(wdev->wiphy->mgmt_stypes[wdev->iftype].tx & BIT(stype >> 4))) + if (!(wdev->wiphy->mgmt_stypes[iftype].tx & BIT(stype >> 4))) return -EINVAL; if (ieee80211_is_action(mgmt->frame_control) && mgmt->u.action.category != WLAN_CATEGORY_PUBLIC) { int err = 0; - switch (wdev->iftype) { + switch (iftype) { case NL80211_IFTYPE_ADHOC: /* * check for IBSS DA must be done by driver as @@ -1003,10 +1002,55 @@ int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev, !cfg80211_allowed_random_address(wdev, mgmt)) return -EINVAL; + return 0; +} + +int cfg80211_mlme_mgmt_tx(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, u64 cookie) +{ + int err; + + lockdep_assert_wiphy(&rdev->wiphy); + + err = cfg80211_mgmt_tx_check(rdev, wdev, wdev->iftype, params); + if (err) + return err; + /* Transmit the management frame as requested by user space */ return rdev_mgmt_tx(rdev, wdev, params, cookie); } +int cfg80211_mlme_mgmt_tx_unlocked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie) +{ + /* can change concurrently, so read only once */ + enum nl80211_iftype iftype = READ_ONCE(wdev->iftype); + int err; + + if (!rdev->ops->mgmt_tx_unlocked) + return -EAGAIN; + + switch (iftype) { + case NL80211_IFTYPE_AP: + case NL80211_IFTYPE_P2P_GO: + case NL80211_IFTYPE_NAN: + case NL80211_IFTYPE_NAN_DATA: + break; + default: + /* the checks for other interface types need the wiphy mutex */ + return -EAGAIN; + } + + err = cfg80211_mgmt_tx_check(rdev, wdev, iftype, params); + if (err) + return err; + + return rdev_mgmt_tx_unlocked(rdev, wdev, params, cookie); +} + bool cfg80211_rx_mgmt_ext(struct wireless_dev *wdev, struct cfg80211_rx_info *info) { diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index eb91937a7238..7cb3bb9438d7 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -15152,6 +15152,22 @@ static int nl80211_register_mgmt(struct sk_buff *skb, struct genl_info *info) info->extack); } +static int nl80211_tx_mgmt_locked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie) +{ + if (params->link_id >= 0 && + !(wdev->valid_links & BIT(params->link_id))) + return -EINVAL; + + if (params->offchan && + !cfg80211_off_channel_oper_allowed(wdev, params->chan)) + return -EBUSY; + + return cfg80211_mlme_mgmt_tx(rdev, wdev, params, cookie); +} + static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; @@ -15237,18 +15253,14 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info) if (!chandef.chan && params.offchan) return -EINVAL; - if (params.offchan && - !cfg80211_off_channel_oper_allowed(wdev, chandef.chan)) - return -EBUSY; - params.link_id = nl80211_link_id_or_invalid(info->attrs); /* - * This now races due to the unlock, but we cannot check - * the valid links for the _station_ anyway, so that's up - * to the driver. + * This races (even with the wiphy mutex, since it's not held for + * netdevs here), but we cannot check the valid links for the + * _station_ anyway, so that's up to the driver. */ if (params.link_id >= 0 && - !(wdev->valid_links & BIT(params.link_id))) + !(READ_ONCE(wdev->valid_links) & BIT(params.link_id))) return -EINVAL; params.no_sta = @@ -15279,7 +15291,19 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info) params.chan = chandef.chan; cookie = cfg80211_assign_cookie(rdev); - err = cfg80211_mlme_mgmt_tx(rdev, wdev, ¶ms, cookie); + + /* pre_doit didn't lock the wiphy mutex, try without */ + err = cfg80211_mlme_mgmt_tx_unlocked(rdev, wdev, ¶ms, cookie); + if (err == -EAGAIN) { + wdev = nl80211_lock_held_wdev(rdev, info, wdev); + if (IS_ERR(wdev)) { + err = PTR_ERR(wdev); + } else { + err = nl80211_tx_mgmt_locked(rdev, wdev, ¶ms, + cookie); + wiphy_unlock(&rdev->wiphy); + } + } if (err) goto free_msg; @@ -19930,7 +19954,10 @@ nl80211_epcs_cfg(struct sk_buff *skb, struct genl_info *info) SELECTOR(__sel, WDEV_UP_RTNL_NOMTX, \ NL80211_FLAG_NEED_WDEV_UP | \ NL80211_FLAG_NO_WIPHY_MTX | \ - NL80211_FLAG_NEED_RTNL) + NL80211_FLAG_NEED_RTNL) \ + SELECTOR(__sel, WDEV_UP_NOMTX, \ + NL80211_FLAG_NEED_WDEV_UP | \ + NL80211_FLAG_NO_WIPHY_MTX) /* * Note: a @@ -20690,7 +20717,8 @@ static const struct genl_small_ops nl80211_small_ops[] = { .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, .doit = nl80211_tx_mgmt, .flags = GENL_UNS_ADMIN_PERM, - .internal_flags = IFLAGS(NL80211_FLAG_NEED_WDEV_UP), + .internal_flags = IFLAGS(NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_NO_WIPHY_MTX), }, { .cmd = NL80211_CMD_FRAME_WAIT_CANCEL, diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 95882c8972d2..5502cd947844 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -785,6 +785,18 @@ static inline int rdev_mgmt_tx(struct cfg80211_registered_device *rdev, return ret; } +static inline int +rdev_mgmt_tx_unlocked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, u64 cookie) +{ + int ret; + trace_rdev_mgmt_tx_unlocked(&rdev->wiphy, wdev, params); + ret = rdev->ops->mgmt_tx_unlocked(&rdev->wiphy, wdev, params, cookie); + trace_rdev_return_int_cookie(&rdev->wiphy, ret, cookie); + return ret; +} + static inline int rdev_tx_control_port(struct cfg80211_registered_device *rdev, struct net_device *dev, const void *buf, size_t len, diff --git a/net/wireless/trace.h b/net/wireless/trace.h index f5fa2fb76474..1f3c1e0eb26a 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -2236,7 +2236,7 @@ TRACE_EVENT(rdev_cancel_remain_on_channel, WIPHY_PR_ARG, WDEV_PR_ARG, __entry->cookie) ); -TRACE_EVENT(rdev_mgmt_tx, +DECLARE_EVENT_CLASS(rdev_mgmt_tx_evt, TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params), TP_ARGS(wiphy, wdev, params), @@ -2266,6 +2266,18 @@ TRACE_EVENT(rdev_mgmt_tx, BOOL_TO_STR(__entry->dont_wait_for_ack)) ); +DEFINE_EVENT(rdev_mgmt_tx_evt, rdev_mgmt_tx, + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params), + TP_ARGS(wiphy, wdev, params) +); + +DEFINE_EVENT(rdev_mgmt_tx_evt, rdev_mgmt_tx_unlocked, + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params), + TP_ARGS(wiphy, wdev, params) +); + TRACE_EVENT(rdev_tx_control_port, TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, const u8 *buf, size_t len, const u8 *dest, __be16 proto, -- 2.56.0 From: Johannes Berg The cookie counter is currently protected by the wiphy mutex, but in order to be able to transmit management frames without holding that, we need to assign cookies atomically. Signed-off-by: Johannes Berg --- net/wireless/core.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/net/wireless/core.h b/net/wireless/core.h index 0e05d4247bea..07d7a68a7cfc 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -83,7 +83,9 @@ struct cfg80211_registered_device { /* protected by RTNL only */ int num_running_ifaces; int num_running_monitor_ifaces; - u64 cookie_counter; + + /* atomic for unlocked users */ + atomic64_t cookie_counter; /* BSSes/scanning */ spinlock_t bss_lock; @@ -171,10 +173,10 @@ cfg80211_rdev_free_wowlan(struct cfg80211_registered_device *rdev) static inline u64 cfg80211_assign_cookie(struct cfg80211_registered_device *rdev) { - u64 r = ++rdev->cookie_counter; + u64 r = atomic64_inc_return(&rdev->cookie_counter); if (WARN_ON(r == 0)) - r = ++rdev->cookie_counter; + r = atomic64_inc_return(&rdev->cookie_counter); return r; } -- 2.56.0 From: Johannes Berg Refactor ieee80211_mgmt_tx() into on-channel and frame building helper functions so we can later implement the new mgmt_tx_unlocked() method. Signed-off-by: Johannes Berg --- net/mac80211/offchannel.c | 225 +++++++++++++++++++++----------------- 1 file changed, 122 insertions(+), 103 deletions(-) diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c index c46cb81fab22..27ca2984826f 100644 --- a/net/mac80211/offchannel.c +++ b/net/mac80211/offchannel.c @@ -814,6 +814,124 @@ int ieee80211_cancel_remain_on_channel(struct wiphy *wiphy, return ieee80211_cancel_roc(local, cookie, false); } +static bool +ieee80211_mgmt_tx_need_offchan(struct ieee80211_sub_if_data *sdata, + struct cfg80211_mgmt_tx_params *params, + const struct ieee80211_mgmt *mgmt, + bool mlo_sta, int *link_id) +{ + struct ieee80211_chanctx_conf *chanctx_conf = NULL; + int i; + + guard(rcu)(); + + /* Check all the links first */ + for (i = 0; i < ARRAY_SIZE(sdata->vif.link_conf); i++) { + struct ieee80211_bss_conf *conf; + + conf = rcu_dereference(sdata->vif.link_conf[i]); + if (!conf) + continue; + + chanctx_conf = rcu_dereference(conf->chanctx_conf); + if (!chanctx_conf) + continue; + + if (mlo_sta && params->chan == chanctx_conf->def.chan && + ether_addr_equal(sdata->vif.addr, mgmt->sa)) { + *link_id = i; + break; + } + + if (ether_addr_equal(conf->addr, mgmt->sa)) { + /* If userspace requested Tx on a specific link + * use the same link id if the link bss is matching + * the requested chan. + */ + if (sdata->vif.valid_links && + params->link_id >= 0 && params->link_id == i && + params->chan == chanctx_conf->def.chan) + *link_id = i; + + break; + } + + chanctx_conf = NULL; + } + + if (!chanctx_conf) + return true; + + return params->chan && params->chan != chanctx_conf->def.chan; +} + +static struct sk_buff * +ieee80211_mgmt_tx_skb(struct ieee80211_sub_if_data *sdata, + struct cfg80211_mgmt_tx_params *params, + u64 cookie, gfp_t gfp) +{ + struct ieee80211_local *local = sdata->local; + struct sk_buff *skb; + u32 flags; + u8 *data; + + if (params->dont_wait_for_ack) + flags = IEEE80211_TX_CTL_NO_ACK; + else + flags = IEEE80211_TX_INTFL_NL80211_FRAME_TX | + IEEE80211_TX_CTL_REQ_TX_STATUS; + + if (params->no_cck) + flags |= IEEE80211_TX_CTL_NO_CCK_RATE; + + skb = dev_alloc_skb(local->hw.extra_tx_headroom + params->len); + if (!skb) + return NULL; + skb_reserve(skb, local->hw.extra_tx_headroom); + + data = skb_put_data(skb, params->buf, params->len); + + /* Update CSA counters */ + if (sdata->vif.bss_conf.csa_active && + (sdata->vif.type == NL80211_IFTYPE_AP || + sdata->vif.type == NL80211_IFTYPE_MESH_POINT || + sdata->vif.type == NL80211_IFTYPE_ADHOC) && + params->n_csa_offsets) { + int i; + struct beacon_data *beacon = NULL; + + rcu_read_lock(); + + if (sdata->vif.type == NL80211_IFTYPE_AP) + beacon = rcu_dereference(sdata->deflink.u.ap.beacon); + else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) + beacon = rcu_dereference(sdata->u.ibss.presp); + else if (ieee80211_vif_is_mesh(&sdata->vif)) + beacon = rcu_dereference(sdata->u.mesh.beacon); + + if (beacon) + for (i = 0; i < params->n_csa_offsets; i++) + data[params->csa_offsets[i]] = + beacon->cntdwn_current_counter; + + rcu_read_unlock(); + } + + IEEE80211_SKB_CB(skb)->flags = flags; + IEEE80211_SKB_CB(skb)->control.flags |= IEEE80211_TX_CTRL_DONT_USE_RATE_MASK; + + skb->dev = sdata->dev; + + /* make a copy to preserve the frame contents in case of encryption */ + if (!params->dont_wait_for_ack && + ieee80211_attach_ack_skb(local, skb, &cookie, gfp)) { + kfree_skb(skb); + return NULL; + } + + return skb; +} + int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params, u64 cookie) { @@ -825,21 +943,10 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, bool need_offchan = false; bool mlo_sta = false; int link_id = -1; - u32 flags; int ret; - u8 *data; lockdep_assert_wiphy(local->hw.wiphy); - if (params->dont_wait_for_ack) - flags = IEEE80211_TX_CTL_NO_ACK; - else - flags = IEEE80211_TX_INTFL_NL80211_FRAME_TX | - IEEE80211_TX_CTL_REQ_TX_STATUS; - - if (params->no_cck) - flags |= IEEE80211_TX_CTL_NO_CCK_RATE; - switch (sdata->vif.type) { case NL80211_IFTYPE_ADHOC: if (!sdata->vif.cfg.ibss_joined) @@ -927,52 +1034,9 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, sdata->vif.type == NL80211_IFTYPE_NAN_DATA) { /* Frames can be sent during NAN schedule */ } else if (!need_offchan) { - struct ieee80211_chanctx_conf *chanctx_conf = NULL; - int i; - - rcu_read_lock(); - /* Check all the links first */ - for (i = 0; i < ARRAY_SIZE(sdata->vif.link_conf); i++) { - struct ieee80211_bss_conf *conf; - - conf = rcu_dereference(sdata->vif.link_conf[i]); - if (!conf) - continue; - - chanctx_conf = rcu_dereference(conf->chanctx_conf); - if (!chanctx_conf) - continue; - - if (mlo_sta && params->chan == chanctx_conf->def.chan && - ether_addr_equal(sdata->vif.addr, mgmt->sa)) { - link_id = i; - break; - } - - if (ether_addr_equal(conf->addr, mgmt->sa)) { - /* If userspace requested Tx on a specific link - * use the same link id if the link bss is matching - * the requested chan. - */ - if (sdata->vif.valid_links && - params->link_id >= 0 && params->link_id == i && - params->chan == chanctx_conf->def.chan) - link_id = i; - - break; - } - - chanctx_conf = NULL; - } - - if (chanctx_conf) { - need_offchan = params->chan && - (params->chan != - chanctx_conf->def.chan); - } else { - need_offchan = true; - } - rcu_read_unlock(); + need_offchan = ieee80211_mgmt_tx_need_offchan(sdata, params, + mgmt, mlo_sta, + &link_id); } if (need_offchan && !params->offchan) { @@ -980,56 +1044,11 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, goto out_unlock; } - skb = dev_alloc_skb(local->hw.extra_tx_headroom + params->len); + skb = ieee80211_mgmt_tx_skb(sdata, params, cookie, GFP_KERNEL); if (!skb) { ret = -ENOMEM; goto out_unlock; } - skb_reserve(skb, local->hw.extra_tx_headroom); - - data = skb_put_data(skb, params->buf, params->len); - - /* Update CSA counters */ - if (sdata->vif.bss_conf.csa_active && - (sdata->vif.type == NL80211_IFTYPE_AP || - sdata->vif.type == NL80211_IFTYPE_MESH_POINT || - sdata->vif.type == NL80211_IFTYPE_ADHOC) && - params->n_csa_offsets) { - int i; - struct beacon_data *beacon = NULL; - - rcu_read_lock(); - - if (sdata->vif.type == NL80211_IFTYPE_AP) - beacon = rcu_dereference(sdata->deflink.u.ap.beacon); - else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) - beacon = rcu_dereference(sdata->u.ibss.presp); - else if (ieee80211_vif_is_mesh(&sdata->vif)) - beacon = rcu_dereference(sdata->u.mesh.beacon); - - if (beacon) - for (i = 0; i < params->n_csa_offsets; i++) - data[params->csa_offsets[i]] = - beacon->cntdwn_current_counter; - - rcu_read_unlock(); - } - - IEEE80211_SKB_CB(skb)->flags = flags; - IEEE80211_SKB_CB(skb)->control.flags |= IEEE80211_TX_CTRL_DONT_USE_RATE_MASK; - - skb->dev = sdata->dev; - - if (!params->dont_wait_for_ack) { - /* make a copy to preserve the frame contents - * in case of encryption. - */ - ret = ieee80211_attach_ack_skb(local, skb, &cookie, GFP_KERNEL); - if (ret) { - kfree_skb(skb); - goto out_unlock; - } - } if (!need_offchan) { ieee80211_tx_skb_tid(sdata, skb, -- 2.56.0 From: Johannes Berg Implement the mgmt_tx_unlocked() method and accept frames that are transmitted on the operating channel with it, as they don't need further handling and can go out directly. For off-channel etc. just return -EAGAIN to fall back to the locked version. Signed-off-by: Johannes Berg --- net/mac80211/cfg.c | 1 + net/mac80211/ieee80211_i.h | 3 + net/mac80211/offchannel.c | 109 +++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 30fe29f25928..daabac9fc6e6 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -6316,6 +6316,7 @@ const struct cfg80211_ops mac80211_config_ops = { .remain_on_channel = ieee80211_remain_on_channel, .cancel_remain_on_channel = ieee80211_cancel_remain_on_channel, .mgmt_tx = ieee80211_mgmt_tx, + .mgmt_tx_unlocked = ieee80211_mgmt_tx_unlocked, .mgmt_tx_cancel_wait = ieee80211_mgmt_tx_cancel_wait, .set_cqm_rssi_config = ieee80211_set_cqm_rssi_config, .set_cqm_rssi_range_config = ieee80211_set_cqm_rssi_range_config, diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 16c1e0337f3a..bf9f26234662 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -2192,6 +2192,9 @@ int ieee80211_cancel_remain_on_channel(struct wiphy *wiphy, struct wireless_dev *wdev, u64 cookie); int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params, u64 cookie); +int ieee80211_mgmt_tx_unlocked(struct wiphy *wiphy, struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie); int ieee80211_mgmt_tx_cancel_wait(struct wiphy *wiphy, struct wireless_dev *wdev, u64 cookie); diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c index 27ca2984826f..44808120ff51 100644 --- a/net/mac80211/offchannel.c +++ b/net/mac80211/offchannel.c @@ -1074,6 +1074,115 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, return ret; } +int ieee80211_mgmt_tx_unlocked(struct wiphy *wiphy, struct wireless_dev *wdev, + struct cfg80211_mgmt_tx_params *params, + u64 cookie) +{ + struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); + const struct ieee80211_mgmt *mgmt = (void *)params->buf; + struct ieee80211_local *local = sdata->local; + struct sta_info *sta = NULL; + enum nl80211_band band; + struct sk_buff *skb; + int link_id = -1, i; + + if (READ_ONCE(local->in_reconfig) || READ_ONCE(local->quiescing)) + return -EAGAIN; + + guard(rcu)(); + + /* do_stop() synchronizes RCU before the interface type can change */ + if (!ieee80211_sdata_running(sdata)) + return -EAGAIN; + + switch (sdata->vif.type) { + case NL80211_IFTYPE_NAN_DATA: + if (is_multicast_ether_addr(mgmt->da)) + return -EOPNOTSUPP; + fallthrough; + case NL80211_IFTYPE_NAN: + /* Frames can be sent during NAN schedule */ + skb = ieee80211_mgmt_tx_skb(sdata, params, cookie, GFP_ATOMIC); + if (!skb) + return -ENOMEM; + + __ieee80211_tx_skb_tid_band(sdata, skb, + params->no_sta ? ERR_PTR(-ENOENT) : + NULL, + 7, -1, NUM_NL80211_BANDS); + return 0; + case NL80211_IFTYPE_AP: + case NL80211_IFTYPE_P2P_GO: + if (sdata->bss->active) + break; + fallthrough; + default: + return -EAGAIN; + } + + if (!params->no_sta) + sta = sta_info_get_bss(sdata, mgmt->da); + + if (ieee80211_is_action(mgmt->frame_control) && + mgmt->u.action.category != WLAN_CATEGORY_PUBLIC && + mgmt->u.action.category != WLAN_CATEGORY_SELF_PROTECTED && + mgmt->u.action.category != WLAN_CATEGORY_SPECTRUM_MGMT) { + if (!sta) + return -ENOLINK; + if (params->link_id >= 0 && + !(sta->sta.valid_links & BIT(params->link_id))) + return -ENOLINK; + link_id = params->link_id; + } + + if ((params->chan || !sta || !sta->sta.mlo) && + ieee80211_mgmt_tx_need_offchan(sdata, params, mgmt, + sta && sta->sta.mlo, &link_id)) + return -EAGAIN; + + /* things may have changed, avoid WARNs in ieee80211_tx_skb_tid() */ + if (ieee80211_vif_is_mld(&sdata->vif)) { + band = 0; + + if (link_id < 0 && + !ether_addr_equal(sdata->vif.addr, mgmt->sa)) { + for (i = 0; i < ARRAY_SIZE(sdata->vif.link_conf); i++) { + struct ieee80211_bss_conf *conf; + + conf = rcu_dereference(sdata->vif.link_conf[i]); + if (conf && + ether_addr_equal(conf->addr, mgmt->sa)) { + link_id = i; + break; + } + } + if (link_id < 0) + return -EAGAIN; + } + + if (link_id >= 0 && + !(READ_ONCE(sdata->vif.active_links) & BIT(link_id))) + return -EAGAIN; + } else { + struct ieee80211_chanctx_conf *chanctx_conf; + + chanctx_conf = + rcu_dereference(sdata->vif.bss_conf.chanctx_conf); + if (!chanctx_conf) + return -EAGAIN; + band = chanctx_conf->def.chan->band; + } + + skb = ieee80211_mgmt_tx_skb(sdata, params, cookie, GFP_ATOMIC); + if (!skb) + return -ENOMEM; + + __ieee80211_tx_skb_tid_band(sdata, skb, + params->no_sta ? ERR_PTR(-ENOENT) : sta, + 7, link_id, band); + return 0; +} + int ieee80211_mgmt_tx_cancel_wait(struct wiphy *wiphy, struct wireless_dev *wdev, u64 cookie) { -- 2.56.0 From: Johannes Berg The comment about wiphy mutex is stale since the cookie cleanup, and everything else can be under RCU, we just have to drop the warning in case it races AP stop. Signed-off-by: Johannes Berg --- net/mac80211/cfg.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index daabac9fc6e6..0acbb2415a02 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -5223,11 +5223,17 @@ static int ieee80211_probe_peer(struct wiphy *wiphy, struct net_device *dev, int size; int ret; - /* the lock is needed to assign the cookie later */ - lockdep_assert_wiphy(local->hw.wiphy); + guard(rcu)(); + + /* do_stop() synchronizes RCU before the interface type can change */ + if (!ieee80211_sdata_running(sdata)) + return -ENETDOWN; switch (ieee80211_vif_type_p2p(&sdata->vif)) { case NL80211_IFTYPE_AP: + /* NULL if the interface was a station when checked */ + if (!peer) + return -EINVAL; fromds = true; break; case NL80211_IFTYPE_STATION: @@ -5253,8 +5259,7 @@ static int ieee80211_probe_peer(struct wiphy *wiphy, struct net_device *dev, * per-link address for the client's link. */ link_id = sta->deflink.link_id; - conf = wiphy_dereference(local->hw.wiphy, - sdata->vif.link_conf[link_id]); + conf = rcu_dereference(sdata->vif.link_conf[link_id]); if (!conf) return -ENOLINK; src_addr = conf->addr; @@ -5269,9 +5274,8 @@ static int ieee80211_probe_peer(struct wiphy *wiphy, struct net_device *dev, /* MLD transmissions must not rely on the band */ band = 0; } else { - chanctx_conf = wiphy_dereference(local->hw.wiphy, - sdata->vif.bss_conf.chanctx_conf); - if (WARN_ON(!chanctx_conf)) + chanctx_conf = rcu_dereference(sdata->vif.bss_conf.chanctx_conf); + if (!chanctx_conf) return -EINVAL; band = chanctx_conf->def.chan->band; link_id = 0; @@ -5318,9 +5322,7 @@ static int ieee80211_probe_peer(struct wiphy *wiphy, struct net_device *dev, } local_bh_disable(); - rcu_read_lock(); ieee80211_xmit(sdata, sta, skb); - rcu_read_unlock(); local_bh_enable(); return 0; -- 2.56.0 From: Johannes Berg None of the drivers implementing probe_peer (mac80211, wil6210, nxpwifi) need the wiphy mutex for it, so we can just not take it. Document that as well. Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 5 ++++- net/wireless/nl80211.c | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 0eee48bdc937..32b458c9a7ee 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -5149,7 +5149,10 @@ struct mgmt_frame_regs { * * @probe_peer: probe a connected peer (AP: STA MAC required; STA: no MAC), * must use the @cookie as provided which is later passed to - * cfg80211_probe_status(). + * cfg80211_probe_status(). Called without the wiphy mutex, so the + * interface type may have changed/be changing and the MAC may be + * %NULL even in AP mode (if runtime type changes are supported), + * drivers must check for that. * * @set_noack_map: Set the NoAck Map for the TIDs. * diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 7cb3bb9438d7..b4ff8c811dc1 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -20835,7 +20835,8 @@ static const struct genl_small_ops nl80211_small_ops[] = { .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, .doit = nl80211_probe_peer, .flags = GENL_UNS_ADMIN_PERM, - .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), + .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_NO_WIPHY_MTX), }, { .cmd = NL80211_CMD_REGISTER_BEACONS, -- 2.56.0 From: Johannes Berg The wiphy mutex was only required for the cookie counter, but that's been in cfg80211 for a while. The rest is just datapath, so can work under RCU, just need to be careful to handle interface type changes. Signed-off-by: Johannes Berg --- net/mac80211/tx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 4b0ffd0a6ac7..ef97d6d3bdcd 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -6640,9 +6640,6 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev, u32 flags = 0; int err; - /* mutex lock is only needed for incrementing the cookie counter */ - lockdep_assert_wiphy(local->hw.wiphy); - /* Only accept CONTROL_PORT_PROTOCOL configured in CONNECT/ASSOCIATE * or Pre-Authentication */ @@ -6701,6 +6698,14 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev, * AF_PACKET */ rcu_read_lock(); + + /* do_stop() synchronizes RCU before the interface type can change */ + if (!ieee80211_sdata_running(sdata)) { + dev_kfree_skb(skb); + rcu_read_unlock(); + return -ENETDOWN; + } + err = ieee80211_lookup_ra_sta(sdata, skb, &sta, true); if (err) { dev_kfree_skb(skb); -- 2.56.0 From: Johannes Berg Only mac80211 currently implements tx_control_port, and it no longer needs the wiphy mutex, so don't take it here. Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 1 + net/wireless/nl80211.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 32b458c9a7ee..3fa310b30139 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -5271,6 +5271,7 @@ struct mgmt_frame_regs { * @tx_control_port: TX a control port frame (EAPoL). The noencrypt parameter * tells the driver that the frame should not be encrypted. A @cookie * value of 0 means the caller does not want TX status reporting. + * Called without the wiphy mutex. * * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available. * Statistics should be cumulative, currently no way to reset is provided. diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index b4ff8c811dc1..4044ea7d344a 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -21070,7 +21070,8 @@ static const struct genl_small_ops nl80211_small_ops[] = { .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, .doit = nl80211_tx_control_port, .flags = GENL_UNS_ADMIN_PERM, - .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), + .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_NO_WIPHY_MTX), }, { .cmd = NL80211_CMD_GET_FTM_RESPONDER_STATS, -- 2.56.0 From: Johannes Berg Qualcomm reported that there can be a lot of lock contention on the wiphy mutex due to background processes dumping the station statistics. In many drivers (e.g. all mac80211 drivers without their own station statistics) it doesn't need to be locked, the data is never guaranteed to be fully consistent anyway given it's collected by concurrent TX/RX. Support unlocked get/dump station methods so that drivers can reduce lock contention by implementing these methods. As in the other cases, it's allowed to return -EAGAIN if a locked call is needed. Signed-off-by: Johannes Berg --- include/net/cfg80211.h | 14 +++ net/wireless/nl80211.c | 201 ++++++++++++++++++++++++++++++++-------- net/wireless/rdev-ops.h | 27 ++++++ net/wireless/trace.h | 19 +++- 4 files changed, 219 insertions(+), 42 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 3fa310b30139..f773856c1818 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -4963,7 +4963,13 @@ struct mgmt_frame_regs { * them, also against the existing state! Drivers must call * cfg80211_check_station_change() to validate the information. * @get_station: get station information for the station identified by @mac + * @get_station_unlocked: Like @get_station, but called without the wiphy + * mutex. Return -EAGAIN to get @get_station called with the wiphy mutex + * held instead. * @dump_station: dump station callback -- resume dump at index @idx + * @dump_station_unlocked: Like @dump_station, but called without the wiphy + * mutex. Return -EAGAIN to get @dump_station called with the wiphy mutex + * held instead. * * @add_mpath: add a fixed mesh path * @del_mpath: delete a given mesh path @@ -5395,8 +5401,16 @@ struct cfg80211_ops { struct station_parameters *params); int (*get_station)(struct wiphy *wiphy, struct wireless_dev *wdev, const u8 *mac, struct station_info *sinfo); + int (*get_station_unlocked)(struct wiphy *wiphy, + struct wireless_dev *wdev, + const u8 *mac, + struct station_info *sinfo); int (*dump_station)(struct wiphy *wiphy, struct wireless_dev *wdev, int idx, u8 *mac, struct station_info *sinfo); + int (*dump_station_unlocked)(struct wiphy *wiphy, + struct wireless_dev *wdev, + int idx, u8 *mac, + struct station_info *sinfo); int (*add_mpath)(struct wiphy *wiphy, struct net_device *dev, const u8 *dst, const u8 *next_hop); diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 4044ea7d344a..948113050b50 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -266,23 +266,51 @@ nl80211_lock_and_recheck(struct cfg80211_registered_device *rdev, return ERR_PTR(-ENODEV); } +/* like nl80211_lock_and_recheck() but under RCU, for wdev_hold() */ +static struct wireless_dev * +nl80211_wdev_by_id_rcu(struct cfg80211_registered_device *rdev, + struct net *netns, u32 wdev_id) +{ + struct wireless_dev *wdev; + + if (!net_eq(wiphy_net(&rdev->wiphy), netns)) + return NULL; + + list_for_each_entry_rcu(wdev, &rdev->wiphy.wdev_list, list) { + if (wdev->identifier != wdev_id) + continue; + if (wdev->netdev && !net_eq(dev_net(wdev->netdev), netns)) + return NULL; + return wdev; + } + + return NULL; +} + /* * Must have wdev_hold(), acquire the wiphy mutex. This has to * look up the wdev again (otherwise it could deadlock against * wdev removal), so it can return an error pointer. */ static struct wireless_dev * -nl80211_lock_held_wdev(struct cfg80211_registered_device *rdev, - struct genl_info *info, struct wireless_dev *wdev) +__nl80211_lock_held_wdev(struct cfg80211_registered_device *rdev, + struct net *netns, struct wireless_dev *wdev) { u32 wdev_id = wdev->identifier; /* the wdev reference was keeping the wiphy alive */ get_device(&rdev->wiphy.dev); - info->user_ptr[1] = NULL; wdev_put(wdev); - wdev = nl80211_lock_and_recheck(rdev, genl_info_net(info), wdev_id); + return nl80211_lock_and_recheck(rdev, netns, wdev_id); +} + +static struct wireless_dev * +nl80211_lock_held_wdev(struct cfg80211_registered_device *rdev, + struct genl_info *info, struct wireless_dev *wdev) +{ + info->user_ptr[1] = NULL; + wdev = __nl80211_lock_held_wdev(rdev, genl_info_net(info), wdev); if (IS_ERR(wdev)) return wdev; @@ -1326,10 +1354,10 @@ nl80211_packet_pattern_policy[MAX_NL80211_PKTPAT + 1] = { [NL80211_PKTPAT_OFFSET] = { .type = NLA_U32 }, }; -static int nl80211_prepare_wdev_dump(struct netlink_callback *cb, - struct cfg80211_registered_device **rdev, - struct wireless_dev **wdev, - struct nlattr **attrbuf) +static int __nl80211_prepare_wdev_dump(struct netlink_callback *cb, + struct cfg80211_registered_device **rdev, + struct wireless_dev **wdev, + struct nlattr **attrbuf, bool lock) { struct net *netns = sock_net(cb->skb->sk); u32 wdev_id; @@ -1374,13 +1402,22 @@ static int nl80211_prepare_wdev_dump(struct netlink_callback *cb, wdev_id = cb->args[1]; } - get_device(&(*rdev)->wiphy.dev); - rcu_read_unlock(); + if (!lock) { + *wdev = nl80211_wdev_by_id_rcu(*rdev, netns, wdev_id); + if (*wdev) + wdev_hold(*wdev); + rcu_read_unlock(); + if (!*wdev) + return -ENODEV; + } else { + get_device(&(*rdev)->wiphy.dev); + rcu_read_unlock(); - /* things may have changed since the lookup or the last dumpit call */ - *wdev = nl80211_lock_and_recheck(*rdev, netns, wdev_id); - if (IS_ERR(*wdev)) - return PTR_ERR(*wdev); + /* things may have changed since the lookup or last dumpit */ + *wdev = nl80211_lock_and_recheck(*rdev, netns, wdev_id); + if (IS_ERR(*wdev)) + return PTR_ERR(*wdev); + } /* 0 is the first index - add 1 to parse only once */ cb->args[0] = (*rdev)->wiphy_idx + 1; @@ -1389,6 +1426,14 @@ static int nl80211_prepare_wdev_dump(struct netlink_callback *cb, return 0; } +static int nl80211_prepare_wdev_dump(struct netlink_callback *cb, + struct cfg80211_registered_device **rdev, + struct wireless_dev **wdev, + struct nlattr **attrbuf) +{ + return __nl80211_prepare_wdev_dump(cb, rdev, wdev, attrbuf, true); +} + /* message building helper */ void *nl80211hdr_put(struct sk_buff *skb, u32 portid, u32 seq, int flags, u8 cmd) @@ -8843,13 +8888,46 @@ static int nl80211_put_link_station_payload(struct sk_buff *msg, return -EMSGSIZE; } +static int nl80211_fill_station_info(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, + struct nl80211_dump_station_ctx *ctx, + bool locked) +{ + int err; + + if (!ctx->filter_mac) { + if (locked) + return rdev_dump_station(rdev, wdev, ctx->sta_idx, + ctx->mac_addr, &ctx->sinfo); + if (!rdev->ops->dump_station_unlocked) + return -EAGAIN; + return rdev_dump_station_unlocked(rdev, wdev, ctx->sta_idx, + ctx->mac_addr, &ctx->sinfo); + } + + if (locked) + err = rdev_get_station(rdev, wdev, ctx->filter_mac_addr, + &ctx->sinfo); + else if (rdev->ops->get_station_unlocked) + err = rdev_get_station_unlocked(rdev, wdev, ctx->filter_mac_addr, + &ctx->sinfo); + else + return -EAGAIN; + + if (!err) + memcpy(ctx->mac_addr, ctx->filter_mac_addr, ETH_ALEN); + return err; +} + static int nl80211_dump_station(struct sk_buff *skb, struct netlink_callback *cb) { struct cfg80211_registered_device *rdev; struct wireless_dev *wdev; struct nl80211_dump_station_ctx *ctx = (void *)cb->args[2]; + struct net *netns = sock_net(cb->skb->sk); struct nlattr **attrbuf __free(kfree) = NULL; + bool locked = false; int err; if (!ctx) { @@ -8858,11 +8936,9 @@ static int nl80211_dump_station(struct sk_buff *skb, return -ENOMEM; } - err = nl80211_prepare_wdev_dump(cb, &rdev, &wdev, attrbuf); + err = __nl80211_prepare_wdev_dump(cb, &rdev, &wdev, attrbuf, false); if (err) return err; - /* nl80211_prepare_wdev_dump acquired it in the successful case */ - __acquire(&rdev->wiphy.mtx); if (!ctx) { ctx = kzalloc_obj(*ctx); @@ -8919,21 +8995,28 @@ static int nl80211_dump_station(struct sk_buff *skb, } } - if (ctx->filter_mac) { - if (ctx->sta_idx > 0) { - err = skb->len; - goto out_err_release; + if (ctx->filter_mac && ctx->sta_idx > 0) { + err = skb->len; + goto out_err_release; + } + + err = nl80211_fill_station_info(rdev, wdev, ctx, + locked); + if (err == -EAGAIN && !locked) { + cfg80211_sinfo_release_content(&ctx->sinfo); + memset(&ctx->sinfo, 0, sizeof(ctx->sinfo)); + + /* this fails if the wdev is being removed */ + wdev = __nl80211_lock_held_wdev(rdev, netns, + wdev); + if (IS_ERR(wdev)) { + err = PTR_ERR(wdev); + goto out_err; } - err = rdev_get_station(rdev, wdev, - ctx->filter_mac_addr, - &ctx->sinfo); - if (!err) - memcpy(ctx->mac_addr, - ctx->filter_mac_addr, ETH_ALEN); - } else { - err = rdev_dump_station(rdev, wdev, ctx->sta_idx, - ctx->mac_addr, - &ctx->sinfo); + + /* try again locked now */ + locked = true; + continue; } if (err == -ENOENT) { err = skb->len; @@ -9028,7 +9111,10 @@ static int nl80211_dump_station(struct sk_buff *skb, cfg80211_sinfo_release_content(&ctx->sinfo); memset(&ctx->sinfo, 0, sizeof(ctx->sinfo)); out_err: - wiphy_unlock(&rdev->wiphy); + if (locked) + wiphy_unlock(&rdev->wiphy); + else if (wdev) + wdev_put(wdev); return err; } @@ -9049,6 +9135,7 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info) struct cfg80211_registered_device *rdev = info->user_ptr[0]; struct wireless_dev *wdev = info->user_ptr[1]; struct station_info sinfo; + bool locked = false; struct sk_buff *msg; u8 *mac_addr = NULL; int err, i; @@ -9074,26 +9161,54 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info) } } - err = rdev_get_station(rdev, wdev, mac_addr, &sinfo); - if (err) { - cfg80211_sinfo_release_content(&sinfo); - return err; - } - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); if (!msg) { cfg80211_sinfo_release_content(&sinfo); return -ENOMEM; } + err = -EAGAIN; + if (rdev->ops->get_station_unlocked) { + err = rdev_get_station_unlocked(rdev, wdev, mac_addr, &sinfo); + + if (err == -EAGAIN) { + cfg80211_sinfo_release_content(&sinfo); + memset(&sinfo, 0, sizeof(sinfo)); + } + } + + if (err == -EAGAIN) { + /* the wdev pointer is only valid until unlocking after this */ + info->user_ptr[1] = NULL; + /* can fail if the wdev is being removed */ + wdev = __nl80211_lock_held_wdev(rdev, genl_info_net(info), + wdev); + if (IS_ERR(wdev)) { + err = PTR_ERR(wdev); + } else { + locked = true; + err = rdev_get_station(rdev, wdev, mac_addr, &sinfo); + } + } + + if (err) { + cfg80211_sinfo_release_content(&sinfo); + goto out; + } + if (sinfo.valid_links) cfg80211_sta_set_mld_sinfo(&sinfo); if (nl80211_send_station(msg, NL80211_CMD_NEW_STATION, info->snd_portid, info->snd_seq, 0, - rdev, wdev, mac_addr, &sinfo) < 0) { + rdev, wdev, mac_addr, &sinfo) < 0) + err = -ENOBUFS; +out: + if (locked) + wiphy_unlock(&rdev->wiphy); + if (err) { nlmsg_free(msg); - return -ENOBUFS; + return err; } return genlmsg_reply(msg, info); @@ -19894,6 +20009,9 @@ nl80211_epcs_cfg(struct sk_buff *skb, struct genl_info *info) NL80211_FLAG_NEED_WIPHY) \ SELECTOR(__sel, WDEV, \ NL80211_FLAG_NEED_WDEV) \ + SELECTOR(__sel, WDEV_NOMTX, \ + NL80211_FLAG_NEED_WDEV | \ + NL80211_FLAG_NO_WIPHY_MTX) \ SELECTOR(__sel, NETDEV, \ NL80211_FLAG_NEED_NETDEV) \ SELECTOR(__sel, NETDEV_LINK, \ @@ -20323,7 +20441,8 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_get_station, .dumpit = nl80211_dump_station, .done = nl80211_dump_station_done, - .internal_flags = IFLAGS(NL80211_FLAG_NEED_WDEV), + .internal_flags = IFLAGS(NL80211_FLAG_NEED_WDEV | + NL80211_FLAG_NO_WIPHY_MTX), }, }; diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 5502cd947844..ae15cec9bf8b 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -248,6 +248,19 @@ static inline int rdev_get_station(struct cfg80211_registered_device *rdev, return ret; } +static inline int +rdev_get_station_unlocked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, const u8 *mac, + struct station_info *sinfo) +{ + int ret; + + trace_rdev_get_station_unlocked(&rdev->wiphy, wdev, mac); + ret = rdev->ops->get_station_unlocked(&rdev->wiphy, wdev, mac, sinfo); + trace_rdev_return_int_station_info(&rdev->wiphy, ret, sinfo); + return ret; +} + static inline int rdev_dump_station(struct cfg80211_registered_device *rdev, struct wireless_dev *wdev, int idx, u8 *mac, struct station_info *sinfo) @@ -259,6 +272,20 @@ static inline int rdev_dump_station(struct cfg80211_registered_device *rdev, return ret; } +static inline int +rdev_dump_station_unlocked(struct cfg80211_registered_device *rdev, + struct wireless_dev *wdev, int idx, u8 *mac, + struct station_info *sinfo) +{ + int ret; + + trace_rdev_dump_station_unlocked(&rdev->wiphy, wdev, idx, mac); + ret = rdev->ops->dump_station_unlocked(&rdev->wiphy, wdev, idx, mac, + sinfo); + trace_rdev_return_int_station_info(&rdev->wiphy, ret, sinfo); + return ret; +} + static inline int rdev_add_mpath(struct cfg80211_registered_device *rdev, struct net_device *dev, u8 *dst, u8 *next_hop) { diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 1f3c1e0eb26a..31000ffebe43 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -1056,12 +1056,17 @@ DEFINE_EVENT(wiphy_wdev_mac_evt, rdev_get_station, TP_ARGS(wiphy, wdev, mac) ); +DEFINE_EVENT(wiphy_wdev_mac_evt, rdev_get_station_unlocked, + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, const u8 *mac), + TP_ARGS(wiphy, wdev, mac) +); + DEFINE_EVENT(wiphy_netdev_mac_evt, rdev_del_mpath, TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, const u8 *mac), TP_ARGS(wiphy, netdev, mac) ); -TRACE_EVENT(rdev_dump_station, +DECLARE_EVENT_CLASS(rdev_dump_station_evt, TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, int _idx, u8 *mac), TP_ARGS(wiphy, wdev, _idx, mac), @@ -1082,6 +1087,18 @@ TRACE_EVENT(rdev_dump_station, __entry->idx) ); +DEFINE_EVENT(rdev_dump_station_evt, rdev_dump_station, + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, int _idx, + u8 *mac), + TP_ARGS(wiphy, wdev, _idx, mac) +); + +DEFINE_EVENT(rdev_dump_station_evt, rdev_dump_station_unlocked, + TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, int _idx, + u8 *mac), + TP_ARGS(wiphy, wdev, _idx, mac) +); + TRACE_EVENT(rdev_return_int_station_info, TP_PROTO(struct wiphy *wiphy, int ret, struct station_info *sinfo), TP_ARGS(wiphy, ret, sinfo), -- 2.56.0 From: Johannes Berg Refactor the code a bit to pass around link-STA pointers instead of looking them up again and again. This will simplify the next changes, since otherwise all of these lookups would have to be converted to RCU-safe ones. It also simplifies the code generally. Signed-off-by: Johannes Berg --- net/mac80211/sta_info.c | 87 ++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 48 deletions(-) diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 9252aaa39895..d761847b70b3 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -2547,20 +2547,11 @@ void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local, } static struct ieee80211_sta_rx_stats * -sta_get_last_rx_stats(struct sta_info *sta, int link_id) +sta_get_last_rx_stats(struct link_sta_info *link_sta_info) { - struct ieee80211_sta_rx_stats *stats; - struct link_sta_info *link_sta_info; + struct ieee80211_sta_rx_stats *stats = &link_sta_info->rx_stats; int cpu; - if (link_id < 0) - link_sta_info = &sta->deflink; - else - link_sta_info = wiphy_dereference(sta->local->hw.wiphy, - sta->link[link_id]); - - stats = &link_sta_info->rx_stats; - if (!link_sta_info->pcpu_rx_stats) return stats; @@ -2576,6 +2567,19 @@ sta_get_last_rx_stats(struct sta_info *sta, int link_id) return stats; } +static unsigned long sta_link_last_active(struct link_sta_info *link_sta_info) +{ + struct ieee80211_sta_rx_stats *stats; + + stats = sta_get_last_rx_stats(link_sta_info); + + if (!link_sta_info->status_stats.last_ack || + time_after(stats->last_rx, link_sta_info->status_stats.last_ack)) + return stats->last_rx; + + return link_sta_info->status_stats.last_ack; +} + static void sta_stats_decode_rate(struct ieee80211_local *local, u32 rate, struct rate_info *rinfo) { @@ -2653,10 +2657,11 @@ static void sta_stats_decode_rate(struct ieee80211_local *local, u32 rate, } } -static int sta_set_rate_info_rx(struct sta_info *sta, struct rate_info *rinfo, - int link_id) +static int sta_set_rate_info_rx(struct sta_info *sta, + struct link_sta_info *link_sta_info, + struct rate_info *rinfo) { - u32 rate = READ_ONCE(sta_get_last_rx_stats(sta, link_id)->last_rate); + u32 rate = READ_ONCE(sta_get_last_rx_stats(link_sta_info)->last_rate); if (rate == STA_STATS_RATE_INVALID) return -EINVAL; @@ -2680,19 +2685,13 @@ static inline u64 sta_get_tidstats_msdu(struct ieee80211_sta_rx_stats *rxstats, } static void sta_set_tidstats(struct sta_info *sta, + struct link_sta_info *link_sta_info, struct cfg80211_tid_stats *tidstats, - int tid, int link_id) + int tid, bool txq_stats) { struct ieee80211_local *local = sta->local; - struct link_sta_info *link_sta_info; int cpu; - if (link_id < 0) - link_sta_info = &sta->deflink; - else - link_sta_info = wiphy_dereference(sta->local->hw.wiphy, - sta->link[link_id]); - if (!(tidstats->filled & BIT(NL80211_TID_STATS_RX_MSDU))) { tidstats->rx_msdu += sta_get_tidstats_msdu(&link_sta_info->rx_stats, @@ -2731,7 +2730,7 @@ static void sta_set_tidstats(struct sta_info *sta, link_sta_info->status_stats.msdu_failed[tid]; } - if (link_id < 0 && tid < IEEE80211_NUM_TIDS) { + if (txq_stats && tid < IEEE80211_NUM_TIDS) { spin_lock_bh(&local->fq.lock); tidstats->filled |= BIT(NL80211_TID_STATS_TXQ_STATS); @@ -2837,20 +2836,17 @@ static u32 sta_estimate_expected_throughput(struct sta_info *sta, } static void sta_set_link_sinfo(struct sta_info *sta, + struct link_sta_info *link_sta_info, struct link_station_info *link_sinfo, struct ieee80211_link_data *link, bool tidstats) { struct ieee80211_sub_if_data *sdata = sta->sdata; struct ieee80211_sta_rx_stats *last_rxstats; - int i, ac, cpu, link_id = link->link_id; - struct link_sta_info *link_sta_info; + int i, ac, cpu; u32 thr = 0; - last_rxstats = sta_get_last_rx_stats(sta, link_id); - - link_sta_info = wiphy_dereference(sta->local->hw.wiphy, - sta->link[link_id]); + last_rxstats = sta_get_last_rx_stats(link_sta_info); /* do before driver, so beacon filtering drivers have a * chance to e.g. just add the number of filtered beacons @@ -2876,8 +2872,7 @@ static void sta_set_link_sinfo(struct sta_info *sta, link_sinfo->inactive_time = jiffies_delta_to_msecs(jiffies - - ieee80211_sta_last_active(sta, - link_id)); + sta_link_last_active(link_sta_info)); if (!(link_sinfo->filled & (BIT_ULL(NL80211_STA_INFO_TX_BYTES64) | BIT_ULL(NL80211_STA_INFO_TX_BYTES)))) { @@ -3025,8 +3020,8 @@ static void sta_set_link_sinfo(struct sta_info *sta, } if (!(link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_RX_BITRATE))) { - if (sta_set_rate_info_rx(sta, &link_sinfo->rxrate, - link_id) == 0) + if (sta_set_rate_info_rx(sta, link_sta_info, + &link_sinfo->rxrate) == 0) link_sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_BITRATE); } @@ -3034,8 +3029,8 @@ static void sta_set_link_sinfo(struct sta_info *sta, if (tidstats && !cfg80211_link_sinfo_alloc_tid_stats(link_sinfo, GFP_KERNEL)) { for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) - sta_set_tidstats(sta, &link_sinfo->pertid[i], i, - link_id); + sta_set_tidstats(sta, link_sta_info, + &link_sinfo->pertid[i], i, false); } if (sdata->vif.type == NL80211_IFTYPE_STATION) { @@ -3091,7 +3086,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, int i, ac, cpu; struct ieee80211_sta_rx_stats *last_rxstats; - last_rxstats = sta_get_last_rx_stats(sta, -1); + last_rxstats = sta_get_last_rx_stats(&sta->deflink); sinfo->generation = sdata->local->sta_generation; @@ -3119,7 +3114,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, sinfo->assoc_at = sta->assoc_at; sinfo->inactive_time = jiffies_delta_to_msecs(jiffies - - ieee80211_sta_last_active(sta, -1)); + sta_link_last_active(&sta->deflink)); if (!(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_TX_BYTES64) | BIT_ULL(NL80211_STA_INFO_TX_BYTES)))) { @@ -3258,13 +3253,15 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_RX_BITRATE)) && !sta->sta.valid_links) { - if (sta_set_rate_info_rx(sta, &sinfo->rxrate, -1) == 0) + if (sta_set_rate_info_rx(sta, &sta->deflink, + &sinfo->rxrate) == 0) sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_BITRATE); } if (tidstats && !cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_KERNEL)) { for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) - sta_set_tidstats(sta, &sinfo->pertid[i], i, -1); + sta_set_tidstats(sta, &sta->deflink, + &sinfo->pertid[i], i, true); } #ifdef CONFIG_MAC80211_MESH @@ -3367,7 +3364,8 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, sinfo->valid_links &= ~BIT(link_id); continue; } - sta_set_link_sinfo(sta, link_sinfo, link, tidstats); + sta_set_link_sinfo(sta, link_sta, link_sinfo, link, + tidstats); if (!thr && (link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_EXPECTED_THROUGHPUT))) est_thr += link_sinfo->expected_throughput; @@ -3400,22 +3398,15 @@ u32 sta_get_expected_throughput(struct sta_info *sta) unsigned long ieee80211_sta_last_active(struct sta_info *sta, int link_id) { - struct ieee80211_sta_rx_stats *stats; struct link_sta_info *link_sta_info; - stats = sta_get_last_rx_stats(sta, link_id); - if (link_id < 0) link_sta_info = &sta->deflink; else link_sta_info = wiphy_dereference(sta->local->hw.wiphy, sta->link[link_id]); - if (!link_sta_info->status_stats.last_ack || - time_after(stats->last_rx, link_sta_info->status_stats.last_ack)) - return stats->last_rx; - - return link_sta_info->status_stats.last_ack; + return sta_link_last_active(link_sta_info); } int ieee80211_sta_allocate_link(struct sta_info *sta, unsigned int link_id) -- 2.56.0 From: Johannes Berg Implement the new cfg80211 unlocked STA statistics. If the driver has an operation, then we need to always do the locked again, so return -EAGAIN unconditionally if it has a locked but not atomic operation, but drivers can implement the new atomic methods to provide needed data atomically (and can also return -EAGAIN). This should, given a driver without any ops or adding the atomic ops, reduce wiphy mutex contention. Signed-off-by: Johannes Berg --- include/net/mac80211.h | 18 ++++++ net/mac80211/cfg.c | 120 ++++++++++++++++++++++++++++---------- net/mac80211/driver-ops.c | 76 ++++++++++++++++++++++++ net/mac80211/driver-ops.h | 46 ++++----------- net/mac80211/sta_info.c | 54 ++++++++++------- net/mac80211/sta_info.h | 10 +++- 6 files changed, 233 insertions(+), 91 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 2a61079e3975..a1a6409b44c1 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -4369,6 +4369,13 @@ struct ieee80211_prep_tx_info { * Statistics that the driver doesn't fill will be filled by mac80211. * The callback can sleep. * + * @sta_statistics_atomic: Like @sta_statistics, but called under RCU, so it + * must be atomic. This is called instead of @sta_statistics if available, + * but may return -EAGAIN to fall back to @sta_statistics with the wiphy + * mutex held. + * Note that it's not guaranteed this will always be called first, some + * (internal) calls may call @sta_statistics directly. + * * @link_sta_statistics: Get link statistics for this station. For example with * beacon filtering, the statistics kept by mac80211 might not be * accurate, so let the driver pre-fill the statistics. The driver can @@ -4378,6 +4385,9 @@ struct ieee80211_prep_tx_info { * Statistics that the driver doesn't fill will be filled by mac80211. * The callback can sleep. * + * @link_sta_statistics_atomic: Like @link_sta_statistics, with the same + * locking/retry semantics as @sta_statistics_atomic. Must be atomic. + * * @conf_tx: Configure TX queue parameters (EDCF (aifs, cw_min, cw_max), * bursting) for a hardware TX queue. * Returns a negative error code on failure. @@ -4883,6 +4893,10 @@ struct ieee80211_ops { struct ieee80211_vif *vif, struct ieee80211_sta *sta, struct station_info *sinfo); + int (*sta_statistics_atomic)(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct ieee80211_sta *sta, + struct station_info *sinfo); int (*conf_tx)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, unsigned int link_id, u16 ac, @@ -4898,6 +4912,10 @@ struct ieee80211_ops { struct ieee80211_vif *vif, struct ieee80211_link_sta *link_sta, struct link_station_info *link_sinfo); + int (*link_sta_statistics_atomic)(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct ieee80211_link_sta *link_sta, + struct link_station_info *link_sinfo); /** * @ampdu_action: diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 0acbb2415a02..9608c07bcea7 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1018,31 +1018,63 @@ void sta_set_rate_info_tx(struct sta_info *sta, rinfo->flags |= RATE_INFO_FLAGS_SHORT_GI; } +static int __ieee80211_dump_station(struct ieee80211_sub_if_data *sdata, + int idx, u8 *mac, + struct station_info *sinfo, bool locked) +{ + struct sta_info *sta; + int ret; + + sta = sta_info_get_by_idx(sdata, idx); + if (!sta) + return -ENOENT; + + memcpy(mac, sta->sta.addr, ETH_ALEN); + ret = __sta_set_sinfo(sta, sinfo, true, locked); + if (ret) + return ret; + + /* + * Add accumulated removed link data to sinfo data for + * consistency for MLO + */ + if (sinfo->valid_links) + sta_set_accumulated_removed_links_sinfo(sta, sinfo); + + return 0; +} + static int ieee80211_dump_station(struct wiphy *wiphy, struct wireless_dev *wdev, int idx, u8 *mac, struct station_info *sinfo) { struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); - struct ieee80211_local *local = sdata->local; - struct sta_info *sta; - int ret = -ENOENT; - lockdep_assert_wiphy(local->hw.wiphy); + lockdep_assert_wiphy(sdata->local->hw.wiphy); - sta = sta_info_get_by_idx(sdata, idx); - if (sta) { - ret = 0; - memcpy(mac, sta->sta.addr, ETH_ALEN); - sta_set_sinfo(sta, sinfo, true); + return __ieee80211_dump_station(sdata, idx, mac, sinfo, true); +} - /* Add accumulated removed link data to sinfo data for - * consistency for MLO - */ - if (sinfo->valid_links) - sta_set_accumulated_removed_links_sinfo(sta, sinfo); +/* driver must have atomic ops (or none), otherwise we need the mutex */ +static bool ieee80211_sta_stats_need_mtx(struct ieee80211_local *local) +{ + const struct ieee80211_ops *ops = local->ops; - } + return (ops->sta_statistics && !ops->sta_statistics_atomic) || + (ops->link_sta_statistics && !ops->link_sta_statistics_atomic); +} - return ret; +static int ieee80211_dump_station_unlocked(struct wiphy *wiphy, + struct wireless_dev *wdev, + int idx, u8 *mac, + struct station_info *sinfo) +{ + struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); + + if (ieee80211_sta_stats_need_mtx(sdata->local)) + return -EAGAIN; + + guard(rcu)(); + return __ieee80211_dump_station(sdata, idx, mac, sinfo, false); } static int ieee80211_dump_survey(struct wiphy *wiphy, struct net_device *dev, @@ -1053,30 +1085,54 @@ static int ieee80211_dump_survey(struct wiphy *wiphy, struct net_device *dev, return drv_get_survey(local, idx, survey); } +static int __ieee80211_get_station(struct ieee80211_sub_if_data *sdata, + const u8 *mac, struct station_info *sinfo, + bool locked) +{ + struct sta_info *sta; + int ret; + + sta = sta_info_get_bss(sdata, mac); + if (!sta) + return -ENOENT; + + ret = __sta_set_sinfo(sta, sinfo, true, locked); + if (ret) + return ret; + + /* + * Add accumulated removed link data to sinfo data for + * consistency for MLO + */ + if (sinfo->valid_links) + sta_set_accumulated_removed_links_sinfo(sta, sinfo); + + return 0; +} + static int ieee80211_get_station(struct wiphy *wiphy, struct wireless_dev *wdev, const u8 *mac, struct station_info *sinfo) { struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); - struct ieee80211_local *local = sdata->local; - struct sta_info *sta; - int ret = -ENOENT; - lockdep_assert_wiphy(local->hw.wiphy); + lockdep_assert_wiphy(sdata->local->hw.wiphy); - sta = sta_info_get_bss(sdata, mac); - if (sta) { - ret = 0; - sta_set_sinfo(sta, sinfo, true); + return __ieee80211_get_station(sdata, mac, sinfo, true); +} - /* Add accumulated removed link data to sinfo data for - * consistency for MLO - */ - if (sinfo->valid_links) - sta_set_accumulated_removed_links_sinfo(sta, sinfo); - } +static int ieee80211_get_station_unlocked(struct wiphy *wiphy, + struct wireless_dev *wdev, + const u8 *mac, + struct station_info *sinfo) +{ + struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); - return ret; + if (ieee80211_sta_stats_need_mtx(sdata->local)) + return -EAGAIN; + + guard(rcu)(); + return __ieee80211_get_station(sdata, mac, sinfo, false); } static int ieee80211_set_monitor_channel(struct wiphy *wiphy, @@ -6273,7 +6329,9 @@ const struct cfg80211_ops mac80211_config_ops = { .del_station = ieee80211_del_station, .change_station = ieee80211_change_station, .get_station = ieee80211_get_station, + .get_station_unlocked = ieee80211_get_station_unlocked, .dump_station = ieee80211_dump_station, + .dump_station_unlocked = ieee80211_dump_station_unlocked, .dump_survey = ieee80211_dump_survey, #ifdef CONFIG_MAC80211_MESH .add_mpath = ieee80211_add_mpath, diff --git a/net/mac80211/driver-ops.c b/net/mac80211/driver-ops.c index 49753b73aba2..1f3e0765a8ab 100644 --- a/net/mac80211/driver-ops.c +++ b/net/mac80211/driver-ops.c @@ -633,3 +633,79 @@ int drv_change_sta_links(struct ieee80211_local *local, return 0; } + +int drv_sta_statistics(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + struct station_info *sinfo, + bool locked) +{ + int ret; + + sdata = get_bss_sdata(sdata); + if (!check_sdata_in_driver(sdata)) + return 0; + + trace_drv_sta_statistics(local, sdata, sta); + + if (locked) { + might_sleep(); + lockdep_assert_wiphy(local->hw.wiphy); + + ret = 0; + + if (local->ops->sta_statistics) + local->ops->sta_statistics(&local->hw, + &sdata->vif, + sta, sinfo); + } else { + ret = -EAGAIN; + + if (local->ops->sta_statistics_atomic) + ret = local->ops->sta_statistics_atomic(&local->hw, + &sdata->vif, + sta, sinfo); + } + + trace_drv_return_int(local, ret); + + return ret; +} + +int drv_link_sta_statistics(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_link_sta *link_sta, + struct link_station_info *link_sinfo, + bool locked) +{ + int ret; + + sdata = get_bss_sdata(sdata); + if (!check_sdata_in_driver(sdata)) + return 0; + + if (locked) { + might_sleep(); + lockdep_assert_wiphy(local->hw.wiphy); + + ret = 0; + + if (local->ops->link_sta_statistics) + local->ops->link_sta_statistics(&local->hw, + &sdata->vif, + link_sta, + link_sinfo); + } else { + ret = -EAGAIN; + + if (local->ops->sta_statistics_atomic) + ret = local->ops->link_sta_statistics_atomic(&local->hw, + &sdata->vif, + link_sta, + link_sinfo); + } + + trace_drv_return_int(local, ret); + + return 0; +} diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index accd89bbc1fb..83dd15f2daf6 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -617,42 +617,16 @@ static inline void drv_sta_rate_tbl_update(struct ieee80211_local *local, trace_drv_return_void(local); } -static inline void drv_sta_statistics(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_sta *sta, - struct station_info *sinfo) -{ - might_sleep(); - lockdep_assert_wiphy(local->hw.wiphy); - - sdata = get_bss_sdata(sdata); - if (!check_sdata_in_driver(sdata)) - return; - - trace_drv_sta_statistics(local, sdata, sta); - if (local->ops->sta_statistics) - local->ops->sta_statistics(&local->hw, &sdata->vif, sta, sinfo); - trace_drv_return_void(local); -} - -static inline void drv_link_sta_statistics(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_link_sta *link_sta, - struct link_station_info *link_sinfo) -{ - might_sleep(); - lockdep_assert_wiphy(local->hw.wiphy); - - sdata = get_bss_sdata(sdata); - if (!check_sdata_in_driver(sdata)) - return; - - trace_drv_link_sta_statistics(local, sdata, link_sta); - if (local->ops->link_sta_statistics) - local->ops->link_sta_statistics(&local->hw, &sdata->vif, - link_sta, link_sinfo); - trace_drv_return_void(local); -} +int drv_sta_statistics(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + struct station_info *sinfo, + bool locked); +int drv_link_sta_statistics(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_link_sta *link_sta, + struct link_station_info *link_sinfo, + bool locked); int drv_conf_tx(struct ieee80211_local *local, struct ieee80211_link_data *link, u16 ac, diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index d761847b70b3..5c95ffee257d 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -2822,7 +2822,7 @@ static u32 sta_estimate_expected_throughput(struct sta_info *sta, u32 duration; u8 band; - conf = sdata_dereference(bss_conf->chanctx_conf, sta->sdata); + conf = rcu_dereference_wiphy(hw->wiphy, bss_conf->chanctx_conf); if (!conf) return 0; band = conf->def.chan->band; @@ -2835,15 +2835,15 @@ static u32 sta_estimate_expected_throughput(struct sta_info *sta, return ((1024 * USEC_PER_SEC) / duration) * 8; } -static void sta_set_link_sinfo(struct sta_info *sta, - struct link_sta_info *link_sta_info, - struct link_station_info *link_sinfo, - struct ieee80211_link_data *link, - bool tidstats) +static int sta_set_link_sinfo(struct sta_info *sta, + struct link_sta_info *link_sta_info, + struct link_station_info *link_sinfo, + struct ieee80211_link_data *link, + bool tidstats, bool locked) { struct ieee80211_sub_if_data *sdata = sta->sdata; struct ieee80211_sta_rx_stats *last_rxstats; - int i, ac, cpu; + int i, ac, cpu, ret; u32 thr = 0; last_rxstats = sta_get_last_rx_stats(link_sta_info); @@ -2857,9 +2857,10 @@ static void sta_set_link_sinfo(struct sta_info *sta, ether_addr_copy(link_sinfo->addr, link_sta_info->addr); - drv_link_sta_statistics(sta->local, sdata, - link_sta_info->pub, - link_sinfo); + ret = drv_link_sta_statistics(sta->local, sdata, link_sta_info->pub, + link_sinfo, locked); + if (ret) + return ret; link_sinfo->filled |= BIT_ULL(NL80211_STA_INFO_INACTIVE_TIME) | BIT_ULL(NL80211_STA_INFO_RX_DROP_MISC); @@ -3027,7 +3028,7 @@ static void sta_set_link_sinfo(struct sta_info *sta, } if (tidstats && !cfg80211_link_sinfo_alloc_tid_stats(link_sinfo, - GFP_KERNEL)) { + GFP_ATOMIC)) { for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) sta_set_tidstats(sta, link_sta_info, &link_sinfo->pertid[i], i, false); @@ -3075,15 +3076,17 @@ static void sta_set_link_sinfo(struct sta_info *sta, link_sinfo->filled |= BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG); } + + return 0; } -void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, - bool tidstats) +int __sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, + bool tidstats, bool locked) { struct ieee80211_sub_if_data *sdata = sta->sdata; struct ieee80211_local *local = sdata->local; u32 thr = 0; - int i, ac, cpu; + int i, ac, cpu, ret; struct ieee80211_sta_rx_stats *last_rxstats; last_rxstats = sta_get_last_rx_stats(&sta->deflink); @@ -3097,7 +3100,10 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, if (sdata->vif.type == NL80211_IFTYPE_STATION) sinfo->rx_beacon = sdata->deflink.u.mgd.count_beacon_signal; - drv_sta_statistics(local, sdata, &sta->sta, sinfo); + ret = drv_sta_statistics(local, sdata, &sta->sta, sinfo, locked); + if (ret) + return ret; + sinfo->filled |= BIT_ULL(NL80211_STA_INFO_INACTIVE_TIME) | BIT_ULL(NL80211_STA_INFO_STA_FLAGS) | BIT_ULL(NL80211_STA_INFO_CONNECTED_TIME) | @@ -3258,7 +3264,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_BITRATE); } - if (tidstats && !cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_KERNEL)) { + if (tidstats && !cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_ATOMIC)) { for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) sta_set_tidstats(sta, &sta->deflink, &sinfo->pertid[i], i, true); @@ -3355,17 +3361,19 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, for_each_valid_link(sinfo, link_id) { struct link_station_info *link_sinfo = sinfo->links[link_id]; - link_sta = wiphy_dereference(sta->local->hw.wiphy, - sta->link[link_id]); - link = wiphy_dereference(sdata->local->hw.wiphy, - sdata->link[link_id]); + link_sta = rcu_dereference_wiphy(sta->local->hw.wiphy, + sta->link[link_id]); + link = rcu_dereference_wiphy(sdata->local->hw.wiphy, + sdata->link[link_id]); if (!link_sta || !link_sinfo || !link) { sinfo->valid_links &= ~BIT(link_id); continue; } - sta_set_link_sinfo(sta, link_sta, link_sinfo, link, - tidstats); + ret = sta_set_link_sinfo(sta, link_sta, link_sinfo, link, + tidstats, locked); + if (ret) + return ret; if (!thr && (link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_EXPECTED_THROUGHPUT))) est_thr += link_sinfo->expected_throughput; @@ -3375,6 +3383,8 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, sinfo->expected_throughput = est_thr; } } + + return 0; } u32 sta_get_expected_throughput(struct sta_info *sta) diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index f409f6a3afa1..8942541375b2 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -989,8 +989,14 @@ static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata, void sta_set_rate_info_tx(struct sta_info *sta, const struct ieee80211_tx_rate *rate, struct rate_info *rinfo); -void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, - bool tidstats); +int __sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, + bool tidstats, bool locked); + +static inline void sta_set_sinfo(struct sta_info *sta, + struct station_info *sinfo, bool tidstats) +{ + __sta_set_sinfo(sta, sinfo, tidstats, true); +} void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, struct station_info *sinfo); -- 2.56.0