diff mbox series

[v9,2/4] mac80211: add multiple bssid support to interface handling

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

Commit Message

Aloka Dixit March 10, 2021, 6:26 p.m. UTC
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.

Signed-off-by: John Crispin <john@phrozen.org>
Co-developed-by: Aloka Dixit <alokad@codeaurora.org>
Signed-off-by: Aloka Dixit <alokad@codeaurora.org>
---
 include/net/mac80211.h | 30 +++++++++++++++++++++++-
 net/mac80211/cfg.c     | 53 ++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs.c |  1 +
 net/mac80211/iface.c   |  6 +++++
 4 files changed, 89 insertions(+), 1 deletion(-)

Comments

Johannes Berg April 8, 2021, 12:06 p.m. UTC | #1
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
Aloka Dixit April 16, 2021, 9:35 p.m. UTC | #2
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.
Johannes Berg April 19, 2021, 11:28 a.m. UTC | #3
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
Aloka Dixit April 19, 2021, 10:35 p.m. UTC | #4
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.
Aloka Dixit April 19, 2021, 10:42 p.m. UTC | #5
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.
Johannes Berg April 20, 2021, 8:25 a.m. UTC | #6
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 mbox series

Patch

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, &params->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;