wifi: cfg80211: Add short_beacon_tail/head/period
Subsystems (2)
PATCH v7 1/2 wifi: cfg80211: Add short_beacon_tail/head/period
From: Kieran Frewen <[email protected]>
Support variables to handle short beacon period and adding a separate tail/head for them. Also, add short beacon period, head and tail attributes for user configuration.
---
v7:
- move new nl80211 attributes to the end
v6:
- update to latest wireless-next
v5:
- initialise s1g beacon variables to false
v4:
- squash cfg80211 commits together
- add protection for short head/tail with S1G band check
- better validation rule for short_beacon_period and align its types
--- include/net/cfg80211.h | 10 +++- include/uapi/linux/nl80211.h | 10 ++++ net/wireless/nl80211.c | 103 ++++++++++++++++++++++++++--------- 3 files changed, 97 insertions(+), 26 deletions(-)
include/net/cfg80211.h +9 -1
@@ -1211,8 +1211,13 @@ struct cfg80211_rnr_elems {* or %NULL if not changed * @tail: tail portion of beacon (after TIM IE) * or %NULL if not changed +* @short_head: head portion of short beacon or %NULL if not changed +* @short_tail: short tail portion of beacon (after TIM IE) +* or %NULL if not changed * @head_len: length of @head * @tail_len: length of @tail +* @short_head_len: length of @short_head +* @short_tail_len: length of @short_tail * @beacon_ies: extra information element(s) to add into Beacon frames or %NULL * @beacon_ies_len: length of beacon_ies in octets * @proberesp_ies: extra information element(s) to add into Probe Response
@@ -1241,6 +1246,7 @@ struct cfg80211_beacon_data {unsigned int link_id;const u8 *head, *tail; +const u8 *short_head, *short_tail;const u8 *beacon_ies;const u8 *proberesp_ies;const u8 *assocresp_ies;
@@ -1252,6 +1258,7 @@ struct cfg80211_beacon_data {s8 ftm_responder;size_t head_len, tail_len; +size_t short_head_len, short_tail_len;size_t beacon_ies_len;size_t proberesp_ies_len;size_t assocresp_ies_len;
@@ -1324,6 +1331,7 @@ struct cfg80211_unsol_bcast_probe_resp {* @beacon: beacon data * @beacon_interval: beacon interval * @dtim_period: DTIM period +* @short_beacon_period: S1G short beacon period * @ssid: SSID to be used in the BSS (note: may be %NULL if not provided from * user space) * @ssid_len: length of @ssid
@@ -1365,7 +1373,7 @@ struct cfg80211_ap_settings {struct cfg80211_beacon_data beacon; -int beacon_interval, dtim_period; +int beacon_interval, dtim_period, short_beacon_period;const u8 *ssid;size_t ssid_len;enum nl80211_hidden_ssid hidden_ssid;
include/uapi/linux/nl80211.h +10 -0
@@ -2815,6 +2815,12 @@ enum nl80211_commands {* @NL80211_ATTR_MLO_LINK_DISABLED: Flag attribute indicating that the link is * disabled. * +* @NL80211_ATTR_SHORT_BEACON_PERIOD: (u16) period for S1G long beacon +* @NL80211_ATTR_SHORT_BEACON_HEAD: portion of the S1G short beacon before +* the TIM element +* @NL80211_ATTR_SHORT_BEACON_TAIL: portion of the S1G short beacon after +* the TIM element +* * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3353,6 +3359,10 @@ enum nl80211_attrs {NL80211_ATTR_MLO_LINK_DISABLED , +NL80211_ATTR_SHORT_BEACON_PERIOD , +NL80211_ATTR_SHORT_BEACON_HEAD , +NL80211_ATTR_SHORT_BEACON_TAIL , +/* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST,
net/wireless/nl80211.c +78 -25
203 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v7 2/2 wifi: mac80211: S1G beacon/short beacon support
From: Kieran Frewen <[email protected]>
With the kernel able to send both short and long S1G beacons, include the ability for setting the short beacon period. If configured, use the S1G short beacon format. The S1G short beacon format includes a limited set of information elements. It also adds the support for distinguish short beacon head and tail.
---
v7:
- no udpates
v6:
- update to latest wireless-next
v4:
- squash mac80211 commits together
- address hitting short head/tail code path in nl80211.c
- update code style
--- include/net/mac80211.h | 2 ++ net/mac80211/cfg.c | 36 ++++++++++++++++++++++++++++++++++-- net/mac80211/ieee80211_i.h | 3 +++ net/mac80211/tx.c | 22 ++++++++++++++++++++-- 4 files changed, 59 insertions(+), 4 deletions(-)
include/net/mac80211.h +2 -0
@@ -573,6 +573,7 @@ struct ieee80211_fils_discovery {* Note also that this is not used with MLD associations, mac80211 doesn't * know how to track beacons for all of the links for this. * @beacon_int: beacon interval +* @short_beacon_period: S1G short beacon period * @assoc_capability: capabilities taken from assoc resp * @basic_rates: bitmap of basic rates, each bit stands for an * index into the rate table configured by the driver in
@@ -706,6 +707,7 @@ struct ieee80211_bss_conf {bool enable_beacon;u8 dtim_period;u16 beacon_int; +u16 short_beacon_period;u16 assoc_capability;u64 sync_tsf;u32 sync_device_ts;
net/mac80211/cfg.c +34 -2
@@ -1119,6 +1119,7 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,struct cfg80211_rnr_elems *rnr =NULL ;struct beacon_data *new, *old;int new_head_len, new_tail_len; +int new_short_head_len, new_short_tail_len;int size, err;u64 _changed =BSS_CHANGED_BEACON ;struct ieee80211_bss_conf *link_conf = link->conf;
@@ -1135,6 +1136,13 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,else new_head_len = old->head_len; +/* Short head is not a mandatory field */ + new_short_head_len =0 ; +if (params->short_head) + new_short_head_len = params->short_head_len; +else if (old && old->short_head) + new_short_head_len = old->short_head_len; +/* new or old tail? */ if (params->tail || !old)/* params->tail_len will be zero for !params->tail */
@@ -1142,7 +1150,14 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,else new_tail_len = old->tail_len; - size =sizeof (*new) + new_head_len + new_tail_len; +if (params->short_tail || !old) +/* params->short_tail_len will be zero for !params->short_tail */ + new_short_tail_len = params->short_tail_len; +else + new_short_tail_len = old->short_tail_len; + + size =sizeof (*new) + new_head_len + new_tail_len + + new_short_head_len + new_short_tail_len;/* new or old multiple BSSID elements? */ if (params->mbssid_ies) {
@@ -1177,11 +1192,16 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,*/ new->head = ((u8 *) new) +sizeof (*new); new->tail = new->head + new_head_len; + new->short_head = new->tail + new_tail_len; + new->short_tail = new->short_head + new_short_head_len; new->head_len = new_head_len; new->tail_len = new_tail_len; + new->short_head_len = new_short_head_len; + new->short_tail_len = new_short_tail_len; +/* copy in optional mbssid_ies */ if (mbssid) { -u8 *pos = new->tail + new->tail_len; +u8 *pos = new->short_tail + new->short_tail_len; new->mbssid_ies = (void *)pos; pos +=struct_size (new->mbssid_ies, elem, mbssid->cnt);
@@ -1213,6 +1233,11 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,else memcpy (new->head, old->head, new_head_len); +if (params->short_head) +memcpy (new->short_head, params->short_head, new_short_head_len); +else if (old && old->short_head) +memcpy (new->short_head, old->short_head, new_short_head_len); +/* copy in optional tail */ if (params->tail)memcpy (new->tail, params->tail, new_tail_len);
@@ -1220,6 +1245,12 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,if (old)memcpy (new->tail, old->tail, new_tail_len); +/* copy in optional short tail */ +if (params->short_tail) +memcpy (new->short_tail, params->short_tail, new_short_tail_len); +else if (old) +memcpy (new->short_tail, old->short_tail, new_short_tail_len); + err =ieee80211_set_probe_resp (sdata, params->probe_resp, params->probe_resp_len, csa, cca, link);if (err <0 ) {
@@ -1418,6 +1449,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, } link_conf->dtim_period = params->dtim_period; + link_conf->short_beacon_period = params->short_beacon_period; link_conf->enable_beacon = true; link_conf->allow_p2p_go_ps = sdata->vif.p2p; link_conf->twt_responder = params->twt_responder;
net/mac80211/ieee80211_i.h +3 -0
@@ -259,10 +259,13 @@ struct ieee80211_color_change_settings {struct beacon_data {u8 *head, *tail; +u8 *short_head, *short_tail;int head_len, tail_len; +int short_head_len, short_tail_len;struct ieee80211_meshconf_ie *meshconf;u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM ];u8 cntdwn_current_counter; +u8 long_beacon_count;struct cfg80211_mbssid_elems *mbssid_ies;struct cfg80211_rnr_elems *rnr_ies;struct rcu_head rcu_head;
net/mac80211/tx.c +20 -2
@@ -5260,6 +5260,18 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,struct sk_buff *skb =NULL ;u16 csa_off_base =0 ;int mbssid_len; +bool is_short = false; + +if (vif->cfg.s1g) { +if (beacon->long_beacon_count ==0 ) { + is_short = false; + beacon->long_beacon_count = + vif->bss_conf.short_beacon_period -1 ; + }else { + is_short = true; + beacon->long_beacon_count--; + } + }if (beacon->cntdwn_counter_offsets[0 ]) {if (!is_template)
@@ -5282,7 +5294,11 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,return NULL ;skb_reserve (skb, local->tx_headroom); -skb_put_data (skb, beacon->head, beacon->head_len); + +if (is_short) +skb_put_data (skb, beacon->short_head, beacon->short_head_len); +else +skb_put_data (skb, beacon->head, beacon->head_len);ieee80211_beacon_add_tim (sdata, link, &ap->ps, skb, is_template);
@@ -5300,7 +5316,9 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw, csa_off_base = skb->len; } -if (beacon->tail) +if (beacon->short_tail && is_short) +skb_put_data (skb, beacon->short_tail, beacon->short_tail_len); +else if (beacon->tail && !is_short)skb_put_data (skb, beacon->tail, beacon->tail_len);if (ieee80211_beacon_protect (skb, local, sdata, link) <0 )
Signature · 1 line
2.25.1
Johannes Berg
maintainerRe: [PATCH v7 2/2] wifi: mac80211: S1G beacon/short beacon support
Quoted 7 lines · Bassem Dawood
From: Kieran Frewen <[email protected]>
With the kernel able to send both short and long S1G beacons, include the ability for setting the short beacon period. If configured, use the S1G short beacon format. The S1G short beacon format includes a limited set of information elements. It also adds the support for distinguish short beacon head and tail.
Also here, I think that could be more elaborate.
And we like to also have commit logs in imperative voice.
net/mac80211/ieee80211_i.h struct ieee80211_color_change_settings { · line 268
Quoted 12 lines
+++
b/net/mac80211/ieee80211_i.hnet/mac80211/ieee80211_i.h +3
-0@@ -259,10 +259,13 @@ struct ieee80211_color_change_settings {struct beacon_data {u8 *head, *tail; +u8 *short_head, *short_tail;int head_len, tail_len; +int short_head_len, short_tail_len;struct ieee80211_meshconf_ie *meshconf;u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM ];u8 cntdwn_current_counter; +u8 long_beacon_count;
I don't understand why this is called long_beacon_count?
Quoted 5 lines
net/mac80211/tx.c +1
-0+++ b/net/mac80211/tx.c@@ -5260,6 +5260,18 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,struct sk_buff *skb =NULL ;u16 csa_off_base =0 ;int mbssid_len;
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5272
Quoted 10 lines
net/mac80211/tx.c +10
-0+bool is_short = false; + +if (vif->cfg.s1g) { +if (beacon->long_beacon_count ==0 ) { + is_short = false; + beacon->long_beacon_count = + vif->bss_conf.short_beacon_period -1 ; + }else { + is_short = true; + beacon->long_beacon_count--;
It's decremented for every _short_ beacon, and then you get a long one when it reaches 0 ... so it's surely not counting long beacons?
Seems like is_short now needs to be taken into account for the beacon skb allocation, nowhere did you actually ensure that the short beacon is indeed shorter than the long beacon ... you don't want to let userspace crash us with that.
johannes
Johannes Berg
maintainerQuoted 5 lines · Bassem Dawood
From: Kieran Frewen <[email protected]>
Support variables to handle short beacon period and adding a separate tail/head for them. Also, add short beacon period, head and tail attributes for user configuration.
So I'm confused by this commit - maybe the commit log should have more words ;-)
Quoted 16 lines
+++
b/include/net/cfg80211.hinclude/net/cfg80211.h +5
-0@@ -1211,8 +1211,13 @@ struct cfg80211_rnr_elems {* or %NULL if not changed * @tail: tail portion of beacon (after TIM IE) * or %NULL if not changed +* @short_head: head portion of short beacon or %NULL if not changed +* @short_tail: short tail portion of beacon (after TIM IE) +* or %NULL if not changed * @head_len: length of @head * @tail_len: length of @tail +* @short_head_len: length of @short_head +* @short_tail_len: length of @short_tail * @beacon_ies: extra information element(s) to add into Beacon frames or %NULL * @beacon_ies_len: length of beacon_ies in octets * @proberesp_ies: extra information element(s) to add into Probe Response
include/net/cfg80211.h struct cfg80211_beacon_data { · line 1249
Quoted 4 lines
include/net/cfg80211.h +1
-0@@ -1241,6 +1246,7 @@ struct cfg80211_beacon_data {unsigned int link_id;const u8 *head, *tail; +const u8 *short_head, *short_tail;
All of this makes me think that for S1G you need _both_ short and regular beacons. Similar to what FILS discovery frames did elsewhere. Which makes a lot of sense, I guess, though arguably we could go the route that FILS did, with "fils_discovery" in struct cfg80211_ap_settings. See also Aloka's recent commits, which now pass the whole struct on updates, though I'm thinking we might change that again:
However, then we have this:
Quoted 10 lines
net/wireless/nl80211.c +3
-1+++ b/net/wireless/nl80211.c@@ -230,13 +230,19 @@ static int validate_beacon_head(const struct nlattr *attr,const struct element *elem;const struct ieee80211_mgmt *mgmt = (void *)data;unsigned int fixedlen, hdrlen; -bool s1g_bcn; +bool s1g_bcn = false; +bool s1g_short_bcn = false;if (len <offsetofend (typeof (*mgmt), frame_control))
net/wireless/nl80211.c static int validate_beacon_head(const struct nlattr *attr, · line 245
Quoted 10 lines
net/wireless/nl80211.c +6
-1goto err; s1g_bcn =ieee80211_is_s1g_beacon (mgmt->frame_control); -if (s1g_bcn) { + s1g_short_bcn =ieee80211_is_s1g_short_beacon (mgmt->frame_control); +if (s1g_short_bcn) { + fixedlen = offsetof(struct ieee80211_ext , + u.s1g_short_beacon.variable); + hdrlen = offsetof(struct ieee80211_ext, u.s1g_short_beacon); + }else if (s1g_bcn) {
which all makes it look like you can put a short beacon into NL80211_ATTR_BEACON_HEAD, but that _shouldn't_ be true?
So maybe this was just a (bad) shortcut, and you really should refactor this function into two (that can call some common code, of course) that gets put into the policy?
Because detecting which kind of beacon you get passed rather than validating that you get passed the right type isn't really validation, I'd think?
Never mind that the
net/wireless/nl80211.c static int validate_beacon_head(const struct nlattr *attr, · line 236
Quoted 1 line
net/wireless/nl80211.c +0
-0if (len <offsetofend (typeof (*mgmt), frame_control))
check is probably somewhat wrong anyway? Or, well, no it's not wrong because you're checking for 2 bytes anyway, but it reads wrong because you're checking typeof(*mgmt) which is mgmt, not ext ...
Put some kind of comment/BUILD_BUG at least? But if it's split, maybe it won't be needed, depends how you split it.
net/wireless/nl80211.c static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev, · line 5547
Quoted 13 lines
net/wireless/nl80211.c +8
-0if (attrs[NL80211_ATTR_BEACON_HEAD ]) { +struct ieee80211_mgmt *mgmt; + bcn->head =nla_data (attrs[NL80211_ATTR_BEACON_HEAD ]); bcn->head_len =nla_len (attrs[NL80211_ATTR_BEACON_HEAD ]);if (!bcn->head_len)return -EINVAL ; + + mgmt = (void *)bcn->head; +if (ieee80211_is_s1g_beacon (mgmt->frame_control) && !is_s1g_band) +return -EINVAL ; +else if (ieee80211_is_beacon (mgmt->frame_control) && is_s1g_band) +return -EINVAL ;
But then again ... here you _do_ allow an S1G beacon in the NL80211_ATTR_BEACON_HEAD, so I'm even more confused ...
Hmm.
I guess maybe you have long and short beacons, but they're _both_ ext format? Really the comments/commit messages/etc. could make all that clearer for all those who are not intimately familiar with the S1G spec.
net/wireless/nl80211.c static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) · line 6008
Quoted 8 lines
net/wireless/nl80211.c +4
-0@@ -5944,6 +6002,10 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)nla_get_u32 (info->attrs[NL80211_ATTR_BEACON_INTERVAL ]); params->dtim_period =nla_get_u32 (info->attrs[NL80211_ATTR_DTIM_PERIOD ]); + params->short_beacon_period =1 ; +if (info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD ]) + params->short_beacon_period = +nla_get_u16 (info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD ]);
Probably should reject that too on !s1g_band, and why is the default value 1? Your documentation doesn't even say what unit it is:
include/uapi/linux/nl80211.h enum nl80211_commands { · line 2818
Quoted 1 line
include/uapi/linux/nl80211.h +1
-0+* @NL80211_ATTR_SHORT_BEACON_PERIOD: (u16) period for S1G long beacon
(and it also talks about long beacon which is probably not right)
So I'd think it's also in TU, but then 1 doesn't make any sense...
net/wireless/nl80211.c static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info) · line 6288
Quoted 7 lines
net/wireless/nl80211.c +2
-1@@ -6237,7 +6284,8 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)if (!wdev->links[link_id].ap.beacon_interval)return -EINVAL ; - err =nl80211_parse_beacon (rdev, info->attrs, ¶ms, info->extack); + err =nl80211_parse_beacon (rdev, info->attrs, ¶ms, info->extack, + wdev->links[link_id].ap.chandef.chan->band ==NL80211_BAND_S1GHZ );
Unrelated, but somewhere in here I hope we don't allow S1G and MLO to coexist ... :-)
(So link_id should always be 0 for S1G here.)
johannes