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

[Jouni Malinen]

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

-- 
Jouni Malinen                                            PGP id EFC895FA