diff mbox series

[03/13] mbssid: configure all BSSes before beacon setup

Message ID 20220302222634.22185-4-quic_alokad@quicinc.com
State Changes Requested
Headers show
Series hostapd: MBSSID and EMA support | expand

Commit Message

Aloka Dixit March 2, 2022, 10:26 p.m. UTC
With existing design, the transmitted beacon is set before RSN
information element is formed for any nontransmitted profile hence the
beacon has these profiles with open encryption.
It also sets wrong DTIM period, profile periodicity values until all
non-transmitting BSSes are up.

Retrieve configurations for all nontransmitted profiles before setting
the beacon to ensures that beacons reflect correct information.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
---
 src/ap/hostapd.c | 90 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 26 deletions(-)

Comments

Jouni Malinen April 7, 2022, 8:06 p.m. UTC | #1
On Wed, Mar 02, 2022 at 02:26:24PM -0800, Aloka Dixit wrote:
> With existing design, the transmitted beacon is set before RSN
> information element is formed for any nontransmitted profile hence the
> beacon has these profiles with open encryption.
> It also sets wrong DTIM period, profile periodicity values until all
> non-transmitting BSSes are up.

That seems to imply that there is something wrong in the current
implementation which is not really the case here. This should be
reworded to describe how these changes are needed for MBSSID instead of
claiming that something is set incorrectly.

> Retrieve configurations for all nontransmitted profiles before setting
> the beacon to ensures that beacons reflect correct information.

It would be good to explicitly note that this does not change behavior
for the case of multiple BSSIDs where each BSS is beaconing.

> diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
> +static int hostapd_set_beacon(struct hostapd_data *hapd)
...
> +	if (hapd->wpa_auth && wpa_init_keys(hapd->wpa_auth) < 0)
> +		return -1;
...

That wpa_init_keys() looks really strange in a function called
hostapd_set_beacon().. Why would it be moved there?

> + * @set_beacon: Whether beacon should be set. When MBSSID IE is enabled,
> + *	information regarding all BSSes should be retrieved before setting
> + *	beacons.

"should"? Isn't that a requirement rather than recommendation? In other
words, "is set" for the first "should" and "need" for the second one.

> -static int hostapd_setup_bss(struct hostapd_data *hapd, int first)
> +static int hostapd_setup_bss(struct hostapd_data *hapd, int first,
> +			     bool set_beacon)

> @@ -1379,29 +1419,8 @@ static int hostapd_setup_bss(struct hostapd_data *hapd, int first)
> -	if (!conf->start_disabled && ieee802_11_set_beacon(hapd) < 0)
> -		return -1;

So moving this into hostapd_set_beacon() looks appropriate.

> -	if (flush_old_stations && !conf->start_disabled &&
> -	    conf->broadcast_deauth) {
> -		u8 addr[ETH_ALEN];
> -
> -		/* Should any previously associated STA not have noticed that
> -		 * the AP had stopped and restarted, send one more
> -		 * deauthentication notification now that the AP is ready to
> -		 * operate. */
> -		wpa_dbg(hapd->msg_ctx, MSG_DEBUG,
> -			"Deauthenticate all stations at BSS start");
> -		os_memset(addr, 0xff, ETH_ALEN);
> -		hostapd_drv_sta_deauth(hapd, addr,
> -				       WLAN_REASON_PREV_AUTH_NOT_VALID);
> -	}

And I guess I could see why this is moved as well even though it is not
really about setting Beacon frame contents.

> -	if (hapd->wpa_auth && wpa_init_keys(hapd->wpa_auth) < 0)
> -		return -1;

But this does not really have much, if anything, to do with Beacon
frames.

> -	if (hapd->driver && hapd->driver->set_operstate)
> -		hapd->driver->set_operstate(hapd->drv_priv, 1);

And this is kind of related to starting beaconing even if not really
about setting a Beacon frame contents.

> +	if (set_beacon)
> +		return hostapd_set_beacon(hapd);

Maybe this should be more about starting beaconing than setting a
beacon..

> @@ -2112,7 +2131,8 @@ static int hostapd_setup_interface_complete_sync(struct hostapd_iface *iface,

> -		if (hostapd_setup_bss(hapd, j == 0)) {
> +		if (hostapd_setup_bss(hapd, j == 0,
> +				      (hapd->iconf->mbssid ? 0 : 1))) {

0/1 are not valid bool values; false/true should be used instead.
Though, !hapd->iconf->mbssid would be clearer here (or
hapd->iconf->mbssid == MBSSID_DISABLED (or something like that) if my
comment about using a single config option with an enum were
implemented.

> @@ -3007,7 +3045,7 @@ int hostapd_add_iface(struct hapd_interfaces *interfaces, char *buf)
> -			     hostapd_setup_bss(hapd, -1))) {
> +			     hostapd_setup_bss(hapd, -1, 1))) {

1 --> true
diff mbox series

Patch

diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
index 4b88641a2dde..14b608e67c23 100644
--- a/src/ap/hostapd.c
+++ b/src/ap/hostapd.c
@@ -1095,19 +1095,59 @@  static int db_table_create_radius_attributes(sqlite3 *db)
 
 #endif /* CONFIG_NO_RADIUS */
 
+static int hostapd_set_beacon(struct hostapd_data *hapd)
+{
+	struct hostapd_bss_config *conf = hapd->conf;
+	int flush_old_stations = 1;
+
+#ifdef CONFIG_MESH
+	if ((hapd->conf->mesh & MESH_ENABLED) && hapd->iface->mconf == NULL)
+		flush_old_stations = 0;
+#endif /* CONFIG_MESH */
+
+	if (!conf->start_disabled && ieee802_11_set_beacon(hapd) < 0)
+		return -1;
+
+	if (flush_old_stations && !conf->start_disabled &&
+	    conf->broadcast_deauth) {
+		u8 addr[ETH_ALEN];
+
+		/* Should any previously associated STA not have noticed that
+		 * the AP had stopped and restarted, send one more
+		 * deauthentication notification now that the AP is ready to
+		 * operate. */
+		wpa_dbg(hapd->msg_ctx, MSG_DEBUG,
+			"Deauthenticate all stations at BSS start");
+		os_memset(addr, 0xff, ETH_ALEN);
+		hostapd_drv_sta_deauth(hapd, addr,
+				       WLAN_REASON_PREV_AUTH_NOT_VALID);
+	}
+
+	if (hapd->wpa_auth && wpa_init_keys(hapd->wpa_auth) < 0)
+		return -1;
+
+	if (hapd->driver && hapd->driver->set_operstate)
+		hapd->driver->set_operstate(hapd->drv_priv, 1);
+
+	return 0;
+}
 
 /**
  * hostapd_setup_bss - Per-BSS setup (initialization)
  * @hapd: Pointer to BSS data
  * @first: Whether this BSS is the first BSS of an interface; -1 = not first,
  *	but interface may exist
+ * @set_beacon: Whether beacon should be set. When MBSSID IE is enabled,
+ *	information regarding all BSSes should be retrieved before setting
+ *	beacons.
  *
  * This function is used to initialize all per-BSS data structures and
  * resources. This gets called in a loop for each BSS when an interface is
  * initialized. Most of the modules that are initialized here will be
  * deinitialized in hostapd_cleanup().
  */
-static int hostapd_setup_bss(struct hostapd_data *hapd, int first)
+static int hostapd_setup_bss(struct hostapd_data *hapd, int first,
+			     bool set_beacon)
 {
 	struct hostapd_bss_config *conf = hapd->conf;
 	u8 ssid[SSID_MAX_LEN + 1];
@@ -1379,29 +1419,8 @@  static int hostapd_setup_bss(struct hostapd_data *hapd, int first)
 		return -1;
 	}
 
-	if (!conf->start_disabled && ieee802_11_set_beacon(hapd) < 0)
-		return -1;
-
-	if (flush_old_stations && !conf->start_disabled &&
-	    conf->broadcast_deauth) {
-		u8 addr[ETH_ALEN];
-
-		/* Should any previously associated STA not have noticed that
-		 * the AP had stopped and restarted, send one more
-		 * deauthentication notification now that the AP is ready to
-		 * operate. */
-		wpa_dbg(hapd->msg_ctx, MSG_DEBUG,
-			"Deauthenticate all stations at BSS start");
-		os_memset(addr, 0xff, ETH_ALEN);
-		hostapd_drv_sta_deauth(hapd, addr,
-				       WLAN_REASON_PREV_AUTH_NOT_VALID);
-	}
-
-	if (hapd->wpa_auth && wpa_init_keys(hapd->wpa_auth) < 0)
-		return -1;
-
-	if (hapd->driver && hapd->driver->set_operstate)
-		hapd->driver->set_operstate(hapd->drv_priv, 1);
+	if (set_beacon)
+		return hostapd_set_beacon(hapd);
 
 	return 0;
 }
@@ -2112,7 +2131,8 @@  static int hostapd_setup_interface_complete_sync(struct hostapd_iface *iface,
 		hapd = iface->bss[j];
 		if (j)
 			os_memcpy(hapd->own_addr, prev_addr, ETH_ALEN);
-		if (hostapd_setup_bss(hapd, j == 0)) {
+		if (hostapd_setup_bss(hapd, j == 0,
+				      (hapd->iconf->mbssid ? 0 : 1))) {
 			for (;;) {
 				hapd = iface->bss[j];
 				hostapd_bss_deinit_no_free(hapd);
@@ -2126,6 +2146,24 @@  static int hostapd_setup_interface_complete_sync(struct hostapd_iface *iface,
 		if (is_zero_ether_addr(hapd->conf->bssid))
 			prev_addr = hapd->own_addr;
 	}
+
+	if (hapd->iconf->mbssid) {
+		for (j = 0; j < iface->num_bss; j++) {
+			hapd = iface->bss[j];
+			if (hostapd_set_beacon(hapd)) {
+				for (;;) {
+					hapd = iface->bss[j];
+					hostapd_bss_deinit_no_free(hapd);
+					hostapd_free_hapd_data(hapd);
+					if (j == 0)
+						break;
+					j--;
+				}
+				goto fail;
+			}
+		}
+	}
+
 	hapd = iface->bss[0];
 
 	hostapd_tx_queue_params(iface);
@@ -3007,7 +3045,7 @@  int hostapd_add_iface(struct hapd_interfaces *interfaces, char *buf)
 
 			if (start_ctrl_iface_bss(hapd) < 0 ||
 			    (hapd_iface->state == HAPD_IFACE_ENABLED &&
-			     hostapd_setup_bss(hapd, -1))) {
+			     hostapd_setup_bss(hapd, -1, 1))) {
 				hostapd_cleanup(hapd);
 				hapd_iface->bss[hapd_iface->num_bss - 1] = NULL;
 				hapd_iface->conf->num_bss--;