| Message ID | 20200812150050.2683396-3-john@phrozen.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: add multiple bssid / EMA | expand |
> + struct { > + struct ieee80211_vif *parent; > + struct list_head list; Is there a lot of value in having a separate list, vs. iterating all interfaces and checking the parent pointer? And exposing the list to the driver seems like a bad idea anyway, they can't really get locking right. (to be continued, mail client is acting up) johannes
> + struct { > + struct ieee80211_vif *parent; > + struct list_head list; > + bool non_transmitted; > + } multiple_bssid; Oh, and also - surely parent isn't set for the transmitted BSSID, so the bool non_transmitted is redundant? It's basically the same as !!parent? Which also applies at the cfg80211 level. > +static int ieee80211_set_multiple_bssid_options(struct ieee80211_sub_if_data *sdata, > + struct vif_params *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 0; That was probably meant to be an error? Otherwise the function can be void. > +++ b/net/mac80211/iface.c > @@ -810,6 +810,13 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, > bool cancel_scan; > struct cfg80211_nan_func *func; > > + if (sdata->vif.type == NL80211_IFTYPE_AP && > + sdata->vif.multiple_bssid.non_transmitted) > + /* make sure the parent is already down */ > + if (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); > + > This is nice but somewhere you also need to actually NULL the pointer. As it is now, it seems you could set up two interfaces, say wlan_nontx wlan_tx then ifup both, then delete wlan_tx (forcing wlan_nontx down) and then set wlan_nontx back up and back down or whatever, but the parent pointer wasn't cleared so ... bad things will happen? Maybe this stuff could even happen at the cfg80211 level? Might be useful for others too. johannes
> > static int ieee80211_del_iface(struct wiphy *wiphy, struct > wireless_dev *wdev) > { > + struct ieee80211_sub_if_data *sdata; > + struct ieee80211_vif *child, *tmp; > + > + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); > + if (sdata->vif.type == NL80211_IFTYPE_AP) { Hi John, Observed a NULL ptr dereference error here.. Thanks Pradeep
On 08.10.20 02:33, Pradeep Kumar Chitrapu wrote: >> >> static int ieee80211_del_iface(struct wiphy *wiphy, struct >> wireless_dev *wdev) >> { >> + struct ieee80211_sub_if_data *sdata; >> + struct ieee80211_vif *child, *tmp; >> + >> + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); >> + if (sdata->vif.type == NL80211_IFTYPE_AP) { > Hi John, Observed a NULL ptr dereference error here.. > > Thanks > Pradeep how did you trigger it ? John
On 2020-10-08 01:06, John Crispin wrote: > On 08.10.20 02:33, Pradeep Kumar Chitrapu wrote: >>> >>> static int ieee80211_del_iface(struct wiphy *wiphy, struct >>> wireless_dev *wdev) >>> { >>> + struct ieee80211_sub_if_data *sdata; >>> + struct ieee80211_vif *child, *tmp; >>> + >>> + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); >>> + if (sdata->vif.type == NL80211_IFTYPE_AP) { >> Hi John, Observed a NULL ptr dereference error here.. >> >> Thanks >> Pradeep > > > how did you trigger it ? > > John Hi Deleted the interface and did rmmod and insmod of cfg80211/mac80211/ath modules. [ 883.565933] Unable to handle kernel NULL pointer dereference at virtual address 00000000 [ 883.565970] pgd = b311c000 [ 883.573357] [00000000] *pgd=00000000 [ 883.579021] Internal error: Oops: 5 [#1] PREEMPT SMP ARM [ 883.848257] task: bd1ac600 ti: b027a000 task.ti: b027a000 [ 883.852904] PC is at ieee80211_del_iface+0x34/0x90 [mac80211] [ 883.858333] LR is at extack_doit+0x20/0x6c [compat] [ 884.092936] [<c751fbd8>] (ieee80211_del_iface [mac80211]) from [<7f56181c>] (extack_doit+0x20/0x6c [compat]) [ 884.100991] [<7f56181c>] (extack_doit [compat]) from [<8076a340>] (genl_rcv_msg+0x27c/0x300) [ 884.110854] [<8076a340>] (genl_rcv_msg) from [<807696c0>] (netlink_rcv_skb+0x58/0xb4) Thanks Pradeep
On 08.10.20 19:21, Pradeep Kumar Chitrapu wrote: > On 2020-10-08 01:06, John Crispin wrote: >> On 08.10.20 02:33, Pradeep Kumar Chitrapu wrote: >>>> >>>> static int ieee80211_del_iface(struct wiphy *wiphy, struct >>>> wireless_dev *wdev) >>>> { >>>> + struct ieee80211_sub_if_data *sdata; >>>> + struct ieee80211_vif *child, *tmp; >>>> + >>>> + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); >>>> + if (sdata->vif.type == NL80211_IFTYPE_AP) { >>> Hi John, Observed a NULL ptr dereference error here.. >>> >>> Thanks >>> Pradeep >> >> >> how did you trigger it ? >> >> John > Hi > > Deleted the interface and did rmmod and insmod of > cfg80211/mac80211/ath modules. > > [ 883.565933] Unable to handle kernel NULL pointer dereference at > virtual address 00000000 > [ 883.565970] pgd = b311c000 > [ 883.573357] [00000000] *pgd=00000000 > [ 883.579021] Internal error: Oops: 5 [#1] PREEMPT SMP ARM > [ 883.848257] task: bd1ac600 ti: b027a000 task.ti: b027a000 > [ 883.852904] PC is at ieee80211_del_iface+0x34/0x90 [mac80211] > [ 883.858333] LR is at extack_doit+0x20/0x6c [compat] > [ 884.092936] [<c751fbd8>] (ieee80211_del_iface [mac80211]) from > [<7f56181c>] (extack_doit+0x20/0x6c [compat]) > [ 884.100991] [<7f56181c>] (extack_doit [compat]) from [<8076a340>] > (genl_rcv_msg+0x27c/0x300) > [ 884.110854] [<8076a340>] (genl_rcv_msg) from [<807696c0>] > (netlink_rcv_skb+0x58/0xb4) > > Thanks > Pradeep > last I tested rmmod/insmod will always crash the kernel when using ath11k however i'll ass !NULL guard in the next series John
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index f0ae718633d2..b409a5f1026c 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1649,6 +1649,9 @@ enum ieee80211_vif_flags { * write-protected by sdata_lock and local->mtx so holding either is fine * for read access. * @cca_color: the color that we will have after the change. + * @multiple_bssid.parent: a non-transmitted bssid has a transmitted parent. + * @multiple_bssid.list: linked list for tracking parent - child relations. + * @multiple_bssid.non_transmitted: Is this a non-transmitted bssi */ struct ieee80211_vif { enum nl80211_iftype type; @@ -1675,6 +1678,11 @@ struct ieee80211_vif { bool rx_mcast_action_reg; bool txqs_stopped[IEEE80211_NUM_ACS]; + struct { + struct ieee80211_vif *parent; + struct list_head list; + bool non_transmitted; + } multiple_bssid; bool cca_active; u8 cca_color; @@ -2326,7 +2334,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. @@ -2335,6 +2343,8 @@ struct ieee80211_txq { * aggregating MPDUs with the same keyid, allowing mac80211 to keep Tx * A-MPDU sessions active while rekeying with Extended Key ID. * + * @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 { @@ -2387,6 +2397,7 @@ enum ieee80211_hw_flags { IEEE80211_HW_SUPPORTS_MULTI_BSSID, IEEE80211_HW_SUPPORTS_ONLY_HE_MULTI_BSSID, IEEE80211_HW_AMPDU_KEYBORDER_SUPPORT, + 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 37a218b89c9a..50a219d8a2cc 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_multiple_bssid_options(struct ieee80211_sub_if_data *sdata, + struct vif_params *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 0; + + if (params->multiple_bssid.non_transmitted) { + parent = __dev_get_by_index(wiphy_net(wiphy), + params->multiple_bssid.parent); + if (!parent || !parent->ieee80211_ptr) + return -EINVAL; + psdata = IEEE80211_WDEV_TO_SUB_IF(parent->ieee80211_ptr); + if (psdata->vif.multiple_bssid.non_transmitted) + return -EINVAL; + sdata->vif.multiple_bssid.parent = &psdata->vif; + list_add(&sdata->vif.multiple_bssid.list, + &psdata->vif.multiple_bssid.list); + sdata->vif.multiple_bssid.non_transmitted = true; + } else { + INIT_LIST_HEAD(&sdata->vif.multiple_bssid.list); + } + + return 0; +} + static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy, const char *name, unsigned char name_assign_type, @@ -136,11 +166,35 @@ static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy, } } + if (type == NL80211_IFTYPE_AP) { + err = ieee80211_set_multiple_bssid_options(sdata, params); + if (err) { + ieee80211_if_remove(sdata); + return NULL; + } + } + return wdev; } static int ieee80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev) { + struct ieee80211_sub_if_data *sdata; + struct ieee80211_vif *child, *tmp; + + sdata = IEEE80211_WDEV_TO_SUB_IF(wdev); + if (sdata->vif.type == NL80211_IFTYPE_AP) { + if (!sdata->vif.multiple_bssid.non_transmitted) { + if (!list_empty(&sdata->vif.multiple_bssid.list)) + list_for_each_entry_safe(child, tmp, + &sdata->vif.multiple_bssid.list, + multiple_bssid.list) + dev_close(vif_to_sdata(child)->wdev.netdev); + } else { + list_del(&sdata->vif.multiple_bssid.list); + } + } + ieee80211_if_remove(IEEE80211_WDEV_TO_SUB_IF(wdev)); return 0; diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c index 54080290d6e2..5d5c9185755a 100644 --- a/net/mac80211/debugfs.c +++ b/net/mac80211/debugfs.c @@ -408,6 +408,7 @@ static const char *hw_flag_names[] = { FLAG(SUPPORTS_MULTI_BSSID), FLAG(SUPPORTS_ONLY_HE_MULTI_BSSID), FLAG(AMPDU_KEYBORDER_SUPPORT), + FLAG(SUPPORTS_MULTI_BSSID_AP), #undef FLAG }; diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 8060cdc102d4..e6397d0c788d 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -810,6 +810,13 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool cancel_scan; struct cfg80211_nan_func *func; + if (sdata->vif.type == NL80211_IFTYPE_AP && + sdata->vif.multiple_bssid.non_transmitted) + /* make sure the parent is already down */ + if (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;
When bringing up multi bssid APs we need to track the parent-child relation of non-transmitting and transmitting VAPs. This patch checks the above by using a linked list to track the relations. The patch also ensures that when a non-transmitting interface is closed the transmitting one is also closed. Signed-off-by: John Crispin <john@phrozen.org> --- include/net/mac80211.h | 13 +++++++++- net/mac80211/cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++ net/mac80211/debugfs.c | 1 + net/mac80211/iface.c | 7 ++++++ 4 files changed, 74 insertions(+), 1 deletion(-)