| Message ID | 20210916025437.29138-3-alokad@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | MBSSID and EMA support in AP mode | expand |
On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote: > Configure multiple BSSID and enhanced multi-BSSID advertisement (EMA) > parameters in mac80211 for AP mode. > > For each interface, 'mbssid_tx_vif' points to the transmitting interface of > the MBSSID set. The pointer is set to NULL if MBSSID is disabled. > > Function ieee80211_stop() is modified to always bring down all the > non-transmitting interfaces first and the transmitting interface last. This has already been applied, but this has some apparent issues that are now showing up with mac80211_hwsim testing being available.. > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, > + struct cfg80211_mbssid_config params) While that does not really break behavior, why is that params argument passed by value instead of by reference? I see no point in copying struct cfg80211_mbssid_config members for this call since the function is only reading the value. > + sdata->vif.mbssid_tx_vif = NULL; > + sdata->vif.bss_conf.bssid_index = 0; > + sdata->vif.bss_conf.nontransmitted = false; > + sdata->vif.bss_conf.ema_ap = false; This cleanup is important, but it is done only here in this helper function.. > @@ -1105,6 +1135,14 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, > + if (sdata->vif.type == NL80211_IFTYPE_AP && > + params->mbssid_config.tx_wdev) { > + err = ieee80211_set_ap_mbssid_options(sdata, > + params->mbssid_config); > + if (err) > + return err; > + } And that is the only place where the help function is called and this happens only under the params->mbssid_config.tx_wdev condition. In other words, those bssid_index/nontransmitted/ema_ap values are not cleared in all cases. This results in issue when the bss_conf (link_conf in the current kernel snapshot) is left in the previous mbssid configuration. As an example, this will make the following mac80211_hwsim test case sequence fail: hostap/tests/hwsim/vm$ ./vm-run.sh he_ap_ema p2p_group_cli_invalid This happens because ema_ap is set to true in he_ap_ema and then it is left set true for p2p_group_cli_invalid and that test case does not actually end up sending Beacon frames. This can be fixed by clearing something in the !params->mbssid_config.tx_wdev case in ieee80211_start_ap(). I'm not completely sure what is the correct way of doing this, but at least ema_ap needs to be cleared to false and likely some other cleanup needs to be done as well.
On 12/18/2022 7:24 AM, Jouni Malinen wrote: > On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote: >> Configure multiple BSSID and enhanced multi-BSSID advertisement (EMA) >> parameters in mac80211 for AP mode. >> >> For each interface, 'mbssid_tx_vif' points to the transmitting interface of >> the MBSSID set. The pointer is set to NULL if MBSSID is disabled. >> >> Function ieee80211_stop() is modified to always bring down all the >> non-transmitting interfaces first and the transmitting interface last. > > This has already been applied, but this has some apparent issues that > are now showing up with mac80211_hwsim testing being available.. > >> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c >> +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, >> + struct cfg80211_mbssid_config params) > > While that does not really break behavior, why is that params argument > passed by value instead of by reference? I see no point in copying > struct cfg80211_mbssid_config members for this call since the function > is only reading the value. > Hi Jouni, the only reason for value instead of reference is that this function does not need to change anything in 'params'. I didn't understand your question. Are you suggesting moving the assignments to ieee80211_start_ap() instead of this separate function? >> + sdata->vif.mbssid_tx_vif = NULL; >> + sdata->vif.bss_conf.bssid_index = 0; >> + sdata->vif.bss_conf.nontransmitted = false; >> + sdata->vif.bss_conf.ema_ap = false; > > This cleanup is important, but it is done only here in this helper > function.. > And that is the only place where the help function is called and this > happens only under the params->mbssid_config.tx_wdev condition. In other > words, those bssid_index/nontransmitted/ema_ap values are not cleared in > all cases. This results in issue when the bss_conf (link_conf in the > current kernel snapshot) is left in the previous mbssid configuration. > Will send a patch to fix this part. Please let me know regarding the first question above so that I can include that in the same patch. Thanks.
On Mon, Dec 19, 2022 at 10:53:55AM -0800, Aloka Dixit wrote: > On 12/18/2022 7:24 AM, Jouni Malinen wrote: > > On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote: > > > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > > > +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, > > > + struct cfg80211_mbssid_config params) > > > > While that does not really break behavior, why is that params argument > > passed by value instead of by reference? I see no point in copying > > struct cfg80211_mbssid_config members for this call since the function > > is only reading the value. > > Hi Jouni, the only reason for value instead of reference is that this > function does not need to change anything in 'params'. I didn't understand > your question. Are you suggesting moving the assignments to > ieee80211_start_ap() instead of this separate function? I would use a constant pointer (const struct cfg80211_mbssid_config *params) to avoid the need to copy the full contents of that struct whenever calling the function. Maybe the following patch is a clearer way of showing what I was thinking of (and testing with): diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index e30b2bdb8f01..b0abd99f006e 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -138,7 +138,7 @@ static int ieee80211_set_mon_options(struct ieee80211_sub_if_data *sdata, } static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, - struct cfg80211_mbssid_config params, + const struct cfg80211_mbssid_config *params, struct ieee80211_bss_conf *link_conf) { struct ieee80211_sub_if_data *tx_sdata; @@ -148,10 +148,10 @@ static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, link_conf->nontransmitted = false; link_conf->ema_ap = false; - if (sdata->vif.type != NL80211_IFTYPE_AP || !params.tx_wdev) + if (sdata->vif.type != NL80211_IFTYPE_AP || !params->tx_wdev) return -EINVAL; - tx_sdata = IEEE80211_WDEV_TO_SUB_IF(params.tx_wdev); + tx_sdata = IEEE80211_WDEV_TO_SUB_IF(params->tx_wdev); if (!tx_sdata) return -EINVAL; @@ -160,9 +160,9 @@ static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, } else { sdata->vif.mbssid_tx_vif = &tx_sdata->vif; link_conf->nontransmitted = true; - link_conf->bssid_index = params.index; + link_conf->bssid_index = params->index; } - if (params.ema) + if (params->ema) link_conf->ema_ap = true; return 0; @@ -1268,10 +1268,17 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, if (sdata->vif.type == NL80211_IFTYPE_AP && params->mbssid_config.tx_wdev) { err = ieee80211_set_ap_mbssid_options(sdata, - params->mbssid_config, + ¶ms->mbssid_config, link_conf); if (err) return err; + } else { + /* FIX: Is this the correct thing to do here and under which + * conditions? At least ema_ap needs to be cleared for AP mode + * if mbssid_config.tx_wdev is not set. */ + link_conf->bssid_index = 0; + link_conf->nontransmitted = false; + link_conf->ema_ap = false; } mutex_lock(&local->mtx); > > This cleanup is important, but it is done only here in this helper > > function.. > > And that is the only place where the help function is called and this > > happens only under the params->mbssid_config.tx_wdev condition. In other > > words, those bssid_index/nontransmitted/ema_ap values are not cleared in > > all cases. This results in issue when the bss_conf (link_conf in the > > current kernel snapshot) is left in the previous mbssid configuration. > > > > Will send a patch to fix this part. Thanks. > Please let me know regarding the first question above so that I can include > that in the same patch. That should not be in the same patch since it is just cleanup/optimization while the not clearing parameters in some cases is a visible bug that should be fixed on its own first.
On 12/19/2022 11:15 AM, Jouni Malinen wrote: > On Mon, Dec 19, 2022 at 10:53:55AM -0800, Aloka Dixit wrote: >> On 12/18/2022 7:24 AM, Jouni Malinen wrote: >>> On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote: >>>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c >>>> +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, >>>> + struct cfg80211_mbssid_config params) >>> > @@ -1268,10 +1268,17 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, > if (sdata->vif.type == NL80211_IFTYPE_AP && > params->mbssid_config.tx_wdev) { > err = ieee80211_set_ap_mbssid_options(sdata, > - params->mbssid_config, > + ¶ms->mbssid_config, > link_conf); > if (err) > return err; > + } else { > + /* FIX: Is this the correct thing to do here and under which > + * conditions? At least ema_ap needs to be cleared for AP mode > + * if mbssid_config.tx_wdev is not set. */ > + link_conf->bssid_index = 0; > + link_conf->nontransmitted = false; > + link_conf->ema_ap = false; Will need to clean MBSSID elements in stored beacon as well. Would the two HWSIM testcases you mentioned earlier be sufficient for testing? I've never used P2P related tests, and in fact I only even ran one testcase at a time so stale configurations didn't show up. Thanks.
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index af0fc13cea34..0d405e36c213 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1715,6 +1715,7 @@ enum ieee80211_offload_flags { * write-protected by sdata_lock and local->mtx so holding either is fine * for read access. * @color_change_color: the bss color that will be used after the change. + * @mbssid_tx_vif: Pointer to the transmitting interface if MBSSID is enabled. */ struct ieee80211_vif { enum nl80211_iftype type; @@ -1746,6 +1747,8 @@ struct ieee80211_vif { bool color_change_active; u8 color_change_color; + struct ieee80211_vif *mbssid_tx_vif; + /* must be last */ u8 drv_priv[] __aligned(sizeof(void *)); }; diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index d69b31c20fe2..e2b791c37591 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -111,6 +111,36 @@ static int ieee80211_set_mon_options(struct ieee80211_sub_if_data *sdata, return 0; } +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata, + struct cfg80211_mbssid_config params) +{ + struct ieee80211_sub_if_data *tx_sdata; + + sdata->vif.mbssid_tx_vif = NULL; + sdata->vif.bss_conf.bssid_index = 0; + sdata->vif.bss_conf.nontransmitted = false; + sdata->vif.bss_conf.ema_ap = false; + + if (sdata->vif.type != NL80211_IFTYPE_AP || !params.tx_wdev) + return -EINVAL; + + tx_sdata = IEEE80211_WDEV_TO_SUB_IF(params.tx_wdev); + if (!tx_sdata) + return -EINVAL; + + if (tx_sdata == sdata) { + sdata->vif.mbssid_tx_vif = &sdata->vif; + } else { + sdata->vif.mbssid_tx_vif = &tx_sdata->vif; + sdata->vif.bss_conf.nontransmitted = true; + sdata->vif.bss_conf.bssid_index = params.index; + } + if (params.ema) + sdata->vif.bss_conf.ema_ap = true; + + return 0; +} + static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy, const char *name, unsigned char name_assign_type, @@ -1105,6 +1135,14 @@ 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 && + params->mbssid_config.tx_wdev) { + err = ieee80211_set_ap_mbssid_options(sdata, + params->mbssid_config); + if (err) + return err; + } + mutex_lock(&local->mtx); err = ieee80211_vif_use_channel(sdata, ¶ms->chandef, IEEE80211_CHANCTX_SHARED); diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 62c95597704b..7c687434c5e9 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -632,17 +632,44 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do ieee80211_add_virtual_monitor(local); } +static void ieee80211_stop_mbssid(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_sub_if_data *tx_sdata, *non_tx_sdata, *tmp_sdata; + struct ieee80211_vif *tx_vif = sdata->vif.mbssid_tx_vif; + + tx_sdata = vif_to_sdata(tx_vif); + sdata->vif.mbssid_tx_vif = NULL; + + list_for_each_entry_safe(non_tx_sdata, tmp_sdata, + &tx_sdata->local->interfaces, list) { + if (non_tx_sdata != sdata && non_tx_sdata != tx_sdata && + non_tx_sdata->vif.mbssid_tx_vif == tx_vif && + ieee80211_sdata_running(non_tx_sdata)) { + non_tx_sdata->vif.mbssid_tx_vif = NULL; + dev_close(non_tx_sdata->wdev.netdev); + } + } + + if (sdata != tx_sdata && ieee80211_sdata_running(tx_sdata)) { + tx_sdata->vif.mbssid_tx_vif = NULL; + dev_close(tx_sdata->wdev.netdev); + } +} + static int ieee80211_stop(struct net_device *dev) { struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); - /* close all dependent VLAN interfaces before locking wiphy */ + /* close dependent VLAN and MBSSID interfaces before locking wiphy */ if (sdata->vif.type == NL80211_IFTYPE_AP) { struct ieee80211_sub_if_data *vlan, *tmpsdata; list_for_each_entry_safe(vlan, tmpsdata, &sdata->u.ap.vlans, u.vlan.list) dev_close(vlan->dev); + + if (sdata->vif.mbssid_tx_vif) + ieee80211_stop_mbssid(sdata); } wiphy_lock(sdata->local->hw.wiphy);