diff mbox

[15/26] wpa_supplicant: Fix CSA related IE's order

Message ID 1441705593-1184-16-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

Peer, Ilan Sept. 8, 2015, 9:46 a.m. UTC 
From: Andrei Otcheretianski <andrei.otcheretianski@intel.com>

Fix the order of CSA, eCSA, secondary channel and wide bandwidth
channel switch wrapper elements in beacons and probe responses.

Signed-off-by: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
---
 src/ap/beacon.c         | 90 +++++++++++++++++++++++--------------------------
 src/ap/ieee802_11_ht.c  |  3 +-
 src/ap/ieee802_11_vht.c |  3 +-
 3 files changed, 46 insertions(+), 50 deletions(-)

Comments

Jouni Malinen Oct. 3, 2015, 5:44 p.m. UTC | #1 
On Tue, Sep 08, 2015 at 12:46:22PM +0300, Ilan Peer wrote:
> Fix the order of CSA, eCSA, secondary channel and wide bandwidth
> channel switch wrapper elements in beacons and probe responses.

>  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,

> +	csa_pos = hostapd_eid_csa(hapd, pos);

> +	csa_pos = hostapd_eid_ecsa(hapd, pos);

> +	/* Secondary channel IE */
> +	/* TODO: The spec doesn't specify a position for this IE */
> +	pos = hostapd_eid_secondary_channel(hapd, pos);

I could not find any language in REVmc/D4.2 that would indicate that the
Secondary Channel Offset element would be included in Probe Response or
Beacon frames. Could you please clarify why this element is added here
(and same for ieee802_11_build_ap_params() below)? Please also note that
there is an explicit note stating that Secondary Channel Offset element
is never present with the Extended Channel Switch Announcement element
in a frame.
Andrei Otcheretianski Oct. 6, 2015, 1:16 p.m. UTC | #2 
On Sat, Oct 3, 2015 at 8:44 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Sep 08, 2015 at 12:46:22PM +0300, Ilan Peer wrote:
>> Fix the order of CSA, eCSA, secondary channel and wide bandwidth
>> channel switch wrapper elements in beacons and probe responses.
>
>>  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
>
>> +     csa_pos = hostapd_eid_csa(hapd, pos);
>
>> +     csa_pos = hostapd_eid_ecsa(hapd, pos);
>
>> +     /* Secondary channel IE */
>> +     /* TODO: The spec doesn't specify a position for this IE */
>> +     pos = hostapd_eid_secondary_channel(hapd, pos);
>
> I could not find any language in REVmc/D4.2 that would indicate that the
> Secondary Channel Offset element would be included in Probe Response or
> Beacon frames. Could you please clarify why this element is added here
> (and same for ieee802_11_build_ap_params() below)?

I went over the spec again, and it is indeed looks that Secondary channel offset
shouldn't be added if CSA is done with CSA elements and not action frames.
Anyway, this element is needed for 40mhz channel switch (since we don't have
operating class). If we don't add this element it will break 40 mhz
channel switch -
so I don't understand how it is supposed to work if we comply with the spec.

> Please also note that
> there is an explicit note stating that Secondary Channel Offset element
> is never present with the Extended Channel Switch Announcement element
> in a frame.

You're right, I found this note now :). However, if CSA is used
together with ECSA
for 40 mhz it will break the CSA only clients.

I think the spec is buggy here.. But I guess we need to comply with the spec
anyway, do we?
Though, I'm not sure whether we can do anything other than disabling 40mhz CSA
to fix this.

Andrei

>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
Jouni Malinen Oct. 6, 2015, 1:35 p.m. UTC | #3 
On Tue, Oct 06, 2015 at 04:16:15PM +0300, Andrei Otcheretianski wrote:
> You're right, I found this note now :). However, if CSA is used
> together with ECSA
> for 40 mhz it will break the CSA only clients.
> 
> I think the spec is buggy here.. But I guess we need to comply with the spec
> anyway, do we?
> Though, I'm not sure whether we can do anything other than disabling 40mhz CSA
> to fix this.

If the standard is not correct or complete, the standard can be
fixed/extended.. REVmc is in the sponsor ballot and comments can be
filed against the latest draft to request changes. I can do that in the
next round if you can identify the proposed changes that would be needed
to make this functionality work properly.

I'm fine with wpa_supplicant/hostapd not following the current standard
if there is reason to believe the standard is not correct. As such, I
don't think we have to remove the current behavior. Once TGmc has
reviewed the comments, we may need to change the implementation to match
the outcome from that review, though (if nothing else, to move the IE
into the correct place which is unlikely to be the current location).
Andrei Otcheretianski Oct. 7, 2015, 11:20 a.m. UTC | #4 
On Tue, Oct 6, 2015 at 4:35 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Oct 06, 2015 at 04:16:15PM +0300, Andrei Otcheretianski wrote:
>> You're right, I found this note now :). However, if CSA is used
>> together with ECSA
>> for 40 mhz it will break the CSA only clients.
>>
>> I think the spec is buggy here.. But I guess we need to comply with the spec
>> anyway, do we?
>> Though, I'm not sure whether we can do anything other than disabling 40mhz CSA
>> to fix this.
>
> If the standard is not correct or complete, the standard can be
> fixed/extended.. REVmc is in the sponsor ballot and comments can be
> filed against the latest draft to request changes. I can do that in the
> next round if you can identify the proposed changes that would be needed
> to make this functionality work properly.
>
> I'm fine with wpa_supplicant/hostapd not following the current standard
> if there is reason to believe the standard is not correct. As such, I
> don't think we have to remove the current behavior. Once TGmc has
> reviewed the comments, we may need to change the implementation to match
> the outcome from that review, though (if nothing else, to move the IE
> into the correct place which is unlikely to be the current location).

After an internal discussion we had, looks that the spec is fine.
The 11n spec requires support for eCSA, so secondary channel offset
element isn't really needed.
We'll remove it.

Andrei

>
> --
> Jouni Malinen                                            PGP id EFC895FA
diff mbox

Patch

diff --git a/src/ap/beacon.c b/src/ap/beacon.c
index 5afaf59..32e054e 100644
--- a/src/ap/beacon.c
+++ b/src/ap/beacon.c
@@ -328,51 +328,12 @@  static u8 *hostapd_eid_ecsa(struct hostapd_data *hapd, u8 *eid)
 }
 
 
-static u8 *hostapd_add_csa_elems(struct hostapd_data *hapd, u8 *pos,
-				 u8 *start, unsigned int *csa_counter_off,
-				 unsigned int *ecsa_counter_off)
-{
-	u8 *curr_pos = pos;
-	u8 *csa_pos = pos;
-
-	if (!csa_counter_off || !ecsa_counter_off)
-		return pos;
-
-	*csa_counter_off = 0;
-	*ecsa_counter_off = 0;
-
-	curr_pos = hostapd_eid_csa(hapd, curr_pos);
-
-	/* save an offset to the csa counter - should be last byte */
-	if (curr_pos != pos)
-		*csa_counter_off = curr_pos - start - 1;
-
-	csa_pos = curr_pos;
-	curr_pos = hostapd_eid_ecsa(hapd, curr_pos);
-
-	/* save an offset to the eCSA counter - should be last byte */
-	if (curr_pos != csa_pos)
-		*ecsa_counter_off = curr_pos - start - 1;
-
-	/* at least one of ies is added */
-	if (pos != curr_pos) {
-#ifdef CONFIG_IEEE80211N
-		curr_pos = hostapd_eid_secondary_channel(hapd, curr_pos);
-#endif
-#ifdef CONFIG_IEEE80211AC
-		curr_pos = hostapd_eid_wb_chsw_wrapper(hapd, curr_pos);
-#endif
-	}
-	return curr_pos;
-}
-
-
 static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
 				   const struct ieee80211_mgmt *req,
 				   int is_p2p, size_t *resp_len)
 {
 	struct ieee80211_mgmt *resp;
-	u8 *pos, *epos;
+	u8 *pos, *epos, *csa_pos;
 	size_t buflen;
 
 #define MAX_PROBERESP_LEN 768
@@ -432,6 +393,13 @@  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
 	/* Power Constraint element */
 	pos = hostapd_eid_pwr_constraint(hapd, pos);
 
+	/* CSA IE */
+	csa_pos = hostapd_eid_csa(hapd, pos);
+	if (csa_pos != pos)
+		hapd->cs_c_off_proberesp =  csa_pos - (u8 *)resp - 1;
+
+	pos = csa_pos;
+
 	/* ERP Information element */
 	pos = hostapd_eid_erp_info(hapd, pos);
 
@@ -445,7 +413,18 @@  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
 
 	pos = hostapd_eid_rm_enabled_capab(hapd, pos, epos - pos);
 
+	/* eCSA IE */
+	csa_pos = hostapd_eid_ecsa(hapd, pos);
+	if (csa_pos != pos)
+		hapd->cs_c_off_ecsa_proberesp = csa_pos - (u8 *)resp - 1;
+
+	pos = csa_pos;
+
 #ifdef CONFIG_IEEE80211N
+	/* Secondary channel IE */
+	/* TODO: The spec doesn't specify a position for this IE */
+	pos = hostapd_eid_secondary_channel(hapd, pos);
+
 	pos = hostapd_eid_ht_capabilities(hapd, pos);
 	pos = hostapd_eid_ht_operation(hapd, pos);
 #endif /* CONFIG_IEEE80211N */
@@ -459,10 +438,6 @@  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
 	pos = hostapd_eid_adv_proto(hapd, pos);
 	pos = hostapd_eid_roaming_consortium(hapd, pos);
 
-	pos = hostapd_add_csa_elems(hapd, pos, (u8 *)resp,
-				    &hapd->cs_c_off_proberesp,
-				    &hapd->cs_c_off_ecsa_proberesp);
-
 #ifdef CONFIG_FST
 	if (hapd->iface->fst_ies) {
 		os_memcpy(pos, wpabuf_head(hapd->iface->fst_ies),
@@ -475,6 +450,9 @@  static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd,
 	if (hapd->iconf->ieee80211ac && !hapd->conf->disable_11ac) {
 		pos = hostapd_eid_vht_capabilities(hapd, pos);
 		pos = hostapd_eid_vht_operation(hapd, pos);
+
+		/* Channel Switch Wrapper IE */
+		pos = hostapd_eid_wb_chsw_wrapper(hapd, pos);
 	}
 	if (hapd->conf->vendor_vht)
 		pos = hostapd_eid_vendor_vht(hapd, pos);
@@ -915,7 +893,7 @@  int ieee802_11_build_ap_params(struct hostapd_data *hapd,
 	size_t resp_len = 0;
 #ifdef NEED_AP_MLME
 	u16 capab_info;
-	u8 *pos, *tailpos;
+	u8 *pos, *tailpos, *csa_pos;
 
 #define BEACON_HEAD_BUF_SIZE 256
 #define BEACON_TAIL_BUF_SIZE 512
@@ -996,6 +974,13 @@  int ieee802_11_build_ap_params(struct hostapd_data *hapd,
 	/* Power Constraint element */
 	tailpos = hostapd_eid_pwr_constraint(hapd, tailpos);
 
+	/* CSA IE */
+	csa_pos = hostapd_eid_csa(hapd, tailpos);
+	if (csa_pos != tailpos)
+		hapd->cs_c_off_beacon = csa_pos - tail - 1;
+
+	tailpos = csa_pos;
+
 	/* ERP Information element */
 	tailpos = hostapd_eid_erp_info(hapd, tailpos);
 
@@ -1013,7 +998,18 @@  int ieee802_11_build_ap_params(struct hostapd_data *hapd,
 	tailpos = hostapd_eid_bss_load(hapd, tailpos,
 				       tail + BEACON_TAIL_BUF_SIZE - tailpos);
 
+	/* eCSA IE */
+	csa_pos = hostapd_eid_ecsa(hapd, tailpos);
+	if (csa_pos != tailpos)
+		hapd->cs_c_off_ecsa_beacon = csa_pos - tail - 1;
+
+	tailpos = csa_pos;
+
 #ifdef CONFIG_IEEE80211N
+	/* Secondary Channel IE */
+	/* TODO: The spec doesn't specify secondary channel IE position */
+	tailpos = hostapd_eid_secondary_channel(hapd, tailpos);
+
 	tailpos = hostapd_eid_ht_capabilities(hapd, tailpos);
 	tailpos = hostapd_eid_ht_operation(hapd, tailpos);
 #endif /* CONFIG_IEEE80211N */
@@ -1029,9 +1025,6 @@  int ieee802_11_build_ap_params(struct hostapd_data *hapd,
 	tailpos = hostapd_eid_interworking(hapd, tailpos);
 	tailpos = hostapd_eid_adv_proto(hapd, tailpos);
 	tailpos = hostapd_eid_roaming_consortium(hapd, tailpos);
-	tailpos = hostapd_add_csa_elems(hapd, tailpos, tail,
-					&hapd->cs_c_off_beacon,
-					&hapd->cs_c_off_ecsa_beacon);
 
 #ifdef CONFIG_FST
 	if (hapd->iface->fst_ies) {
@@ -1045,6 +1038,7 @@  int ieee802_11_build_ap_params(struct hostapd_data *hapd,
 	if (hapd->iconf->ieee80211ac && !hapd->conf->disable_11ac) {
 		tailpos = hostapd_eid_vht_capabilities(hapd, tailpos);
 		tailpos = hostapd_eid_vht_operation(hapd, tailpos);
+		tailpos = hostapd_eid_wb_chsw_wrapper(hapd, tailpos);
 	}
 	if (hapd->conf->vendor_vht)
 		tailpos = hostapd_eid_vendor_vht(hapd, tailpos);
diff --git a/src/ap/ieee802_11_ht.c b/src/ap/ieee802_11_ht.c
index 3422269..975f018 100644
--- a/src/ap/ieee802_11_ht.c
+++ b/src/ap/ieee802_11_ht.c
@@ -112,7 +112,8 @@  u8 *hostapd_eid_secondary_channel(struct hostapd_data *hapd, u8 *eid)
 {
 	u8 sec_ch;
 
-	if (!hapd->cs_freq_params.sec_channel_offset)
+	if (!hapd->cs_freq_params.channel ||
+	    !hapd->cs_freq_params.sec_channel_offset)
 		return eid;
 
 	if (hapd->cs_freq_params.sec_channel_offset == -1)
diff --git a/src/ap/ieee802_11_vht.c b/src/ap/ieee802_11_vht.c
index 214adf7..6c72e54 100644
--- a/src/ap/ieee802_11_vht.c
+++ b/src/ap/ieee802_11_vht.c
@@ -136,7 +136,8 @@  u8 *hostapd_eid_wb_chsw_wrapper(struct hostapd_data *hapd, u8 *eid)
 	u8 bw, chan1, chan2 = 0;
 	int freq1;
 
-	if (!hapd->cs_freq_params.vht_enabled)
+	if (!hapd->cs_freq_params.channel ||
+	    !hapd->cs_freq_params.vht_enabled)
 		return eid;
 
 	/* bandwidth: 0: 40, 1: 80, 2: 160, 3: 80+80 */