| Message ID | 20210310182604.8858-3-alokad@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Multiple BSSID support | expand |
On Wed, 2021-03-10 at 10:26 -0800, Aloka Dixit wrote: > From: John Crispin <john@phrozen.org> > > Add a new helper ieee80211_set_multiple_bssid_options() takes propagating > the cfg80211 data down the stack. > > The patch also makes sure that all members of the bss set will get closed > when either of them is shutdown. s/either/any/ > > static int ieee80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev) > { > + struct ieee80211_sub_if_data *sdata; > + > + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); can be one line > + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { > + if (sdata->vif.multiple_bssid.flags & IEEE80211_VIF_MBSS_TRANSMITTING) { > + struct ieee80211_sub_if_data *child; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) > + if (child->vif.multiple_bssid.parent == &sdata->vif) > + dev_close(child->wdev.netdev); > + rcu_read_unlock(); You never tested this properly, this is wrong. johannes
On 2021-04-08 05:06, Johannes Berg wrote: > On Wed, 2021-03-10 at 10:26 -0800, Aloka Dixit wrote: >> From: John Crispin <john@phrozen.org> >> >> Add a new helper ieee80211_set_multiple_bssid_options() takes >> propagating >> the cfg80211 data down the stack. >> >> The patch also makes sure that all members of the bss set will get >> closed >> when either of them is shutdown. > > s/either/any/ >> >> static int ieee80211_del_iface(struct wiphy *wiphy, struct >> wireless_dev *wdev) >> { >> + struct ieee80211_sub_if_data *sdata; >> + >> + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); > > can be one line > Okay >> + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { >> + if (sdata->vif.multiple_bssid.flags & >> IEEE80211_VIF_MBSS_TRANSMITTING) { >> + struct ieee80211_sub_if_data *child; >> + >> + rcu_read_lock(); >> + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) >> + if (child->vif.multiple_bssid.parent == &sdata->vif) >> + dev_close(child->wdev.netdev); >> + rcu_read_unlock(); > > You never tested this properly, this is wrong. > > johannes This was added for graceful shutdown of non-transmitting interfaces whenever the transmitting one is being brought down. But I see that dev_close() is happening twice now. Inclining towards removing this and just return error to application if it tries to remove transmitting before all non-transmitting are deleted. However, currently the "parent" pointer to indicate the transmitting interface is maintained in mac80211, nothing in cfg80211. Which option would be better, (1) Add new function to mac80211_config_ops to return yes/no to whether cfg80211 can delete a particular interface when MBSSID is in use. (2) Add a new pointer in struct wireless_dev to track the transmitting interface of the set. Then the yes/no decision can be taken in cfg80211 itself. Thanks.
Hi, > > > + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { > > > + if (sdata->vif.multiple_bssid.flags & > > > IEEE80211_VIF_MBSS_TRANSMITTING) { > > > + struct ieee80211_sub_if_data *child; > > > + > > > + rcu_read_lock(); > > > + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) > > > + if (child->vif.multiple_bssid.parent == &sdata->vif) > > > + dev_close(child->wdev.netdev); > > > + rcu_read_unlock(); > This was added for graceful shutdown of non-transmitting interfaces > whenever the transmitting one is being brought down. > I know, I asked you to. > But I see that > dev_close() is happening twice now. > That wouldn't be an issue. The issue is that dev_close() needs to be able to sleep, and it even contains a might_sleep(), so you can't do it under the RCU protection you used here. > Inclining towards removing this and just return error to application if > it tries to remove transmitting before all non-transmitting are deleted. > However, currently the "parent" pointer to indicate the transmitting > interface is maintained in mac80211, nothing in cfg80211. That seems kinda awkward, considering e.g. if hostapd crashes and then a new instance has to clean up, it might not really have much knowledge of the order in which it should be doing that. I think it's better if you just fix the locking here? johannes
On 2021-04-19 04:28, Johannes Berg wrote: > Hi, > >> > > + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { >> > > + if (sdata->vif.multiple_bssid.flags & >> > > IEEE80211_VIF_MBSS_TRANSMITTING) { >> > > + struct ieee80211_sub_if_data *child; >> > > + >> > > + rcu_read_lock(); >> > > + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) >> > > + if (child->vif.multiple_bssid.parent == &sdata->vif) >> > > + dev_close(child->wdev.netdev); >> > > + rcu_read_unlock(); > >> This was added for graceful shutdown of non-transmitting interfaces >> whenever the transmitting one is being brought down. >> > > I know, I asked you to. > >> But I see that >> dev_close() is happening twice now. >> > > That wouldn't be an issue. > > The issue is that dev_close() needs to be able to sleep, and it even > contains a might_sleep(), so you can't do it under the RCU protection > you used here. > >> Inclining towards removing this and just return error to application >> if >> it tries to remove transmitting before all non-transmitting are >> deleted. >> However, currently the "parent" pointer to indicate the transmitting >> interface is maintained in mac80211, nothing in cfg80211. > > That seems kinda awkward, considering e.g. if hostapd crashes and then > a > new instance has to clean up, it might not really have much knowledge > of > the order in which it should be doing that. > > I think it's better if you just fix the locking here? > > johannes Hi Johannes, Thanks for the response, I need more help. Is rcu_read_lock is not allowed around dev_close() because it might block or *ANY* lock? Because both functions with new dev_close() themselves are called with locks held, (1) ieee80211_do_stop() happens inside wiphy_lock(sdata->local->hw.wiphy) (2) ieee80211_del_iface() happens inside mutex_lock(&rdev->wiphy.mtx). Should these be unlocked temporarily too before calling dev_close()? Didn't find any instance of wiphy.mtx lock/unlock in mac80211. Also, in cfg.c, list_for_each_entry(sdata, &local->interfaces, list) is called with two different murexes: (1) local->iflist_mtx (2) local->mtx Which is the correct one for this purpose among above two and rcu_read_lock()? Once that decided, would following be sufficient - lock() list_for_each_entry(sdata, &local->interfaces, list) { get_child_pointer unlock() dev_close() lock() } unlock Thanks.
On 2021-04-19 15:35, Aloka Dixit wrote: > On 2021-04-19 04:28, Johannes Berg wrote: >> Hi, >> >>> > > + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { >>> > > + if (sdata->vif.multiple_bssid.flags & >>> > > IEEE80211_VIF_MBSS_TRANSMITTING) { >>> > > + struct ieee80211_sub_if_data *child; >>> > > + >>> > > + rcu_read_lock(); >>> > > + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) >>> > > + if (child->vif.multiple_bssid.parent == &sdata->vif) >>> > > + dev_close(child->wdev.netdev); >>> > > + rcu_read_unlock(); >> >>> This was added for graceful shutdown of non-transmitting interfaces >>> whenever the transmitting one is being brought down. >>> >> >> I know, I asked you to. >> >>> But I see that >>> dev_close() is happening twice now. >>> >> >> That wouldn't be an issue. >> >> The issue is that dev_close() needs to be able to sleep, and it even >> contains a might_sleep(), so you can't do it under the RCU protection >> you used here. >> >>> Inclining towards removing this and just return error to application >>> if >>> it tries to remove transmitting before all non-transmitting are >>> deleted. >>> However, currently the "parent" pointer to indicate the transmitting >>> interface is maintained in mac80211, nothing in cfg80211. >> >> That seems kinda awkward, considering e.g. if hostapd crashes and then >> a >> new instance has to clean up, it might not really have much knowledge >> of >> the order in which it should be doing that. >> >> I think it's better if you just fix the locking here? >> >> johannes > > Hi Johannes, > Thanks for the response, I need more help. > > Is rcu_read_lock is not allowed around dev_close() because it might > block or *ANY* lock? > Because both functions with new dev_close() themselves are called with > locks held, > (1) ieee80211_do_stop() happens inside > wiphy_lock(sdata->local->hw.wiphy) > (2) ieee80211_del_iface() happens inside mutex_lock(&rdev->wiphy.mtx). > Should these be unlocked temporarily too before calling dev_close()? > Didn't find any instance of wiphy.mtx lock/unlock in mac80211. > My bad, wiphy_lock() internally uses wiphy.mtx so mac80211 does have a way, then should it be unlocked temporarily before dev_close()? > Also, in cfg.c, list_for_each_entry(sdata, &local->interfaces, list) > is called with two different murexes: (1) local->iflist_mtx > (2) local->mtx > > Which is the correct one for this purpose among above two and > rcu_read_lock()? > Once that decided, would following be sufficient - > lock() > list_for_each_entry(sdata, &local->interfaces, list) { > get_child_pointer > unlock() > dev_close() > lock() > } > unlock > > Thanks.
Hi Aloka, > > Is rcu_read_lock is not allowed around dev_close() because it might > block or *ANY* lock? Well, I guess it's more nuanced than that. rcu_read_lock() is not allowed because it enters an atomic context. Similarly not allowed would be spinlocks, or local_bh_disable, or any similar thing that makes the context atomic. From a locking perspective, normal mutexes *would* be allowed. However, you'd have to be extremely careful to not allow recursion, since dev_close() will call back into cfg80211/mac80211. > Because both functions with new dev_close() themselves are called with > locks held, > (1) ieee80211_do_stop() happens inside > wiphy_lock(sdata->local->hw.wiphy) This is probably already problematic. > (2) ieee80211_del_iface() happens inside mutex_lock(&rdev->wiphy.mtx). As you discovered, that's the same lock. > Should these be unlocked temporarily too before calling dev_close()? I don't think temporarily dropping locks is ever a good idea, it makes it really hard to reason about the code. But we already do this for AP-VLAN interfaces, so not sure why this is so different? > > Also, in cfg.c, list_for_each_entry(sdata, &local->interfaces, list) is > called with two different murexes: (1) local->iflist_mtx > (2) local->mtx > > Which is the correct one for this purpose among above two and > rcu_read_lock()? > Once that decided, would following be sufficient - > lock() > list_for_each_entry(sdata, &local->interfaces, list) { > get_child_pointer > unlock() > dev_close() What guarantees you don't lose the device after the unlock()? I think you'd risk list corruption here this way... Look at the other instance of dev_close() here - as long as you can guarantee there won't be recursion (and you do, because non-transmitting interfaces don't have other non-transmitting below them, though they may actually have AP-VLANs!), we should be fine just doing it like that code? johannes
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 2d1d629e5d14..b9f51515c2e8 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -631,6 +631,7 @@ struct ieee80211_fils_discovery { * @s1g: BSS is S1G BSS (affects Association Request format). * @beacon_tx_rate: The configured beacon transmit rate that needs to be passed * to driver when rate control is offloaded to firmware. + * @multiple_bssid: the multiple bssid settings of the AP. */ struct ieee80211_bss_conf { const u8 *bssid; @@ -700,6 +701,7 @@ struct ieee80211_bss_conf { u32 unsol_bcast_probe_resp_interval; bool s1g; struct cfg80211_bitrate_mask beacon_tx_rate; + struct cfg80211_multiple_bssid multiple_bssid; }; /** @@ -1663,6 +1665,19 @@ enum ieee80211_offload_flags { IEEE80211_OFFLOAD_DECAP_ENABLED = BIT(2), }; +/** + * enum ieee80211_vif_multiple_bssid_flags - virtual interface multiple bssid flags + * + * @IEEE80211_VIF_MBSS_TRANSMITTING: this BSS is transmitting beacons + * @IEEE80211_VIF_MBSS_NON_TRANSMITTING: this BSS is not transmitting beacons + * @IEEE80211_VIF_MBSS_EMA_BEACON: beacons should be send out in EMA mode + */ +enum ieee80211_vif_multiple_bssid_flags { + IEEE80211_VIF_MBSS_TRANSMITTING = BIT(1), + IEEE80211_VIF_MBSS_NON_TRANSMITTING = BIT(2), + IEEE80211_VIF_MBSS_EMA_BEACON = BIT(3), +}; + /** * struct ieee80211_vif - per-interface data * @@ -1709,6 +1724,11 @@ enum ieee80211_offload_flags { * protected by fq->lock. * @offload_flags: 802.3 -> 802.11 enapsulation offload flags, see * &enum ieee80211_offload_flags. + * + * @multiple_bssid: Multiple BSSID configurations. + * @multiple_bssid.parent: Interface index of the transmitted BSS. + * @multiple_bssid.flags: multiple bssid flags, see + * enum ieee80211_vif_multiple_bssid_flags. */ struct ieee80211_vif { enum nl80211_iftype type; @@ -1737,6 +1757,11 @@ struct ieee80211_vif { bool txqs_stopped[IEEE80211_NUM_ACS]; + struct { + struct ieee80211_vif *parent; + u32 flags; + } multiple_bssid; + /* must be last */ u8 drv_priv[] __aligned(sizeof(void *)); }; @@ -2384,7 +2409,7 @@ struct ieee80211_txq { * @IEEE80211_HW_TX_STATUS_NO_AMPDU_LEN: Driver does not report accurate A-MPDU * length in tx status information * - * @IEEE80211_HW_SUPPORTS_MULTI_BSSID: Hardware supports multi BSSID + * @IEEE80211_HW_SUPPORTS_MULTI_BSSID: Hardware supports multi BSSID in STA mode * * @IEEE80211_HW_SUPPORTS_ONLY_HE_MULTI_BSSID: Hardware supports multi BSSID * only for HE APs. Applies if @IEEE80211_HW_SUPPORTS_MULTI_BSSID is set. @@ -2399,6 +2424,8 @@ struct ieee80211_txq { * @IEEE80211_HW_SUPPORTS_RX_DECAP_OFFLOAD: Hardware supports rx decapsulation * offload * + * @IEEE80211_HW_SUPPORTS_MULTI_BSSID_AP: Hardware supports multi BSSID in + * AP mode * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays */ enum ieee80211_hw_flags { @@ -2453,6 +2480,7 @@ enum ieee80211_hw_flags { IEEE80211_HW_AMPDU_KEYBORDER_SUPPORT, IEEE80211_HW_SUPPORTS_TX_ENCAP_OFFLOAD, IEEE80211_HW_SUPPORTS_RX_DECAP_OFFLOAD, + IEEE80211_HW_SUPPORTS_MULTI_BSSID_AP, /* keep last, obviously */ NUM_IEEE80211_HW_FLAGS diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index c4c70e30ad7f..91e659a43f67 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -111,6 +111,39 @@ static int ieee80211_set_mon_options(struct ieee80211_sub_if_data *sdata, return 0; } +static void ieee80211_set_multiple_bssid_options(struct ieee80211_sub_if_data *sdata, + struct cfg80211_ap_settings *params) +{ + struct ieee80211_local *local = sdata->local; + struct wiphy *wiphy = local->hw.wiphy; + struct net_device *parent; + struct ieee80211_sub_if_data *psdata; + + if (!ieee80211_hw_check(&local->hw, SUPPORTS_MULTI_BSSID_AP)) + return; + + if (!params->multiple_bssid.count) + return; + + if (params->multiple_bssid.parent) { + parent = __dev_get_by_index(wiphy_net(wiphy), + params->multiple_bssid.parent); + if (!parent || !parent->ieee80211_ptr) + return; + psdata = IEEE80211_WDEV_TO_SUB_IF(parent->ieee80211_ptr); + if (psdata->vif.multiple_bssid.parent) + return; + sdata->vif.multiple_bssid.parent = &psdata->vif; + sdata->vif.multiple_bssid.flags |= IEEE80211_VIF_MBSS_NON_TRANSMITTING; + } else { + sdata->vif.multiple_bssid.flags |= IEEE80211_VIF_MBSS_TRANSMITTING; + } + + if (params->multiple_bssid.ema) + sdata->vif.multiple_bssid.flags |= IEEE80211_VIF_MBSS_EMA_BEACON; + sdata->vif.bss_conf.multiple_bssid = params->multiple_bssid; +} + static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy, const char *name, unsigned char name_assign_type, @@ -141,6 +174,23 @@ static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy, static int ieee80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev) { + struct ieee80211_sub_if_data *sdata; + + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); + if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) { + if (sdata->vif.multiple_bssid.flags & IEEE80211_VIF_MBSS_TRANSMITTING) { + struct ieee80211_sub_if_data *child; + + rcu_read_lock(); + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) + if (child->vif.multiple_bssid.parent == &sdata->vif) + dev_close(child->wdev.netdev); + rcu_read_unlock(); + } else { + sdata->vif.multiple_bssid.parent = NULL; + } + } + ieee80211_if_remove(IEEE80211_WDEV_TO_SUB_IF(wdev)); return 0; @@ -1078,6 +1128,9 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, changed |= BSS_CHANGED_HE_BSS_COLOR; } + if (sdata->vif.type == NL80211_IFTYPE_AP) + ieee80211_set_multiple_bssid_options(sdata, params); + mutex_lock(&local->mtx); err = ieee80211_vif_use_channel(sdata, ¶ms->chandef, IEEE80211_CHANCTX_SHARED); diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c index 5296898875ff..249b8f4e6a54 100644 --- a/net/mac80211/debugfs.c +++ b/net/mac80211/debugfs.c @@ -456,6 +456,7 @@ static const char *hw_flag_names[] = { FLAG(AMPDU_KEYBORDER_SUPPORT), FLAG(SUPPORTS_TX_ENCAP_OFFLOAD), FLAG(SUPPORTS_RX_DECAP_OFFLOAD), + FLAG(SUPPORTS_MULTI_BSSID_AP), #undef FLAG }; diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index b80c9b016b2b..6bb48bf87aca 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -376,6 +376,12 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do bool cancel_scan; struct cfg80211_nan_func *func; + /* make sure the parent is already down */ + if (sdata->vif.type == NL80211_IFTYPE_AP && + sdata->vif.multiple_bssid.parent && + ieee80211_sdata_running(vif_to_sdata(sdata->vif.multiple_bssid.parent))) + dev_close(vif_to_sdata(sdata->vif.multiple_bssid.parent)->wdev.netdev); + clear_bit(SDATA_STATE_RUNNING, &sdata->state); cancel_scan = rcu_access_pointer(local->scan_sdata) == sdata;