2023-08-10 10:01:11

by Bassem Dawood

[permalink] [raw]
Subject: [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.

Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Kieran Frewen <[email protected]>
Co-developed-by: Gilad Itzkovitch <[email protected]>
Signed-off-by: Gilad Itzkovitch <[email protected]>
---
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(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7c7d03aa9d06..482f455305aa 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -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;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 88eb85c63029..d543d1b83072 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -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,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0da2e6a2a7ea..eedc6f47d74d 100644
--- a/net/wireless/nl80211.c
+++ 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))
goto 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) {
fixedlen = offsetof(struct ieee80211_ext,
u.s1g_beacon.variable);
hdrlen = offsetof(struct ieee80211_ext, u.s1g_beacon);
@@ -263,7 +269,12 @@ static int validate_beacon_head(const struct nlattr *attr,
return 0;

err:
- NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head");
+ if (s1g_short_bcn)
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed S1G short beacon head");
+ else if (s1g_bcn)
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed S1G beacon head");
+ else
+ NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head");
return -EINVAL;
}

@@ -817,6 +828,12 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_HW_TIMESTAMP_ENABLED] = { .type = NLA_FLAG },
[NL80211_ATTR_EMA_RNR_ELEMS] = { .type = NLA_NESTED },
[NL80211_ATTR_MLO_LINK_DISABLED] = { .type = NLA_FLAG },
+
+ [NL80211_ATTR_SHORT_BEACON_PERIOD] = NLA_POLICY_MIN(NLA_U16, 1),
+ [NL80211_ATTR_SHORT_BEACON_HEAD] =
+ NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_beacon_head, IEEE80211_MAX_DATA_LEN),
+ [NL80211_ATTR_SHORT_BEACON_TAIL] =
+ NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_ie_attr, IEEE80211_MAX_DATA_LEN),
};

/* policy for the key attributes */
@@ -5505,7 +5522,8 @@ static int nl80211_parse_he_bss_color(struct nlattr *attrs,
static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
struct nlattr *attrs[],
struct cfg80211_beacon_data *bcn,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack,
+ bool is_s1g_band)
{
bool haveinfo = false;
int err;
@@ -5515,10 +5533,18 @@ static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
bcn->link_id = nl80211_link_id(attrs);

if (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;
haveinfo = true;
}

@@ -5528,6 +5554,22 @@ static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
haveinfo = true;
}

+ if (attrs[NL80211_ATTR_SHORT_BEACON_HEAD]) {
+ if (!is_s1g_band)
+ return -EINVAL;
+ bcn->short_head = nla_data(attrs[NL80211_ATTR_SHORT_BEACON_HEAD]);
+ bcn->short_head_len = nla_len(attrs[NL80211_ATTR_SHORT_BEACON_HEAD]);
+ haveinfo = true;
+ }
+
+ if (attrs[NL80211_ATTR_SHORT_BEACON_TAIL]) {
+ if (!is_s1g_band)
+ return -EINVAL;
+ bcn->short_tail = nla_data(attrs[NL80211_ATTR_SHORT_BEACON_TAIL]);
+ bcn->short_tail_len = nla_len(attrs[NL80211_ATTR_SHORT_BEACON_TAIL]);
+ haveinfo = true;
+ }
+
if (!haveinfo)
return -EINVAL;

@@ -5935,8 +5977,24 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
if (!params)
return -ENOMEM;

+ if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
+ err = nl80211_parse_chandef(rdev, info, &params->chandef);
+ if (err)
+ goto out;
+ } else if (wdev->valid_links) {
+ /* with MLD need to specify the channel configuration */
+ err = -EINVAL;
+ goto out;
+ } else if (wdev->u.ap.preset_chandef.chan) {
+ params->chandef = wdev->u.ap.preset_chandef;
+ } else if (!nl80211_get_ap_channel(rdev, params)) {
+ err = -EINVAL;
+ goto out;
+ }
+
err = nl80211_parse_beacon(rdev, info->attrs, &params->beacon,
- info->extack);
+ info->extack,
+ params->chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
goto out;

@@ -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]);

err = cfg80211_validate_beacon_int(rdev, dev->ieee80211_ptr->iftype,
params->beacon_interval);
@@ -6040,21 +6102,6 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
}
}

- if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
- err = nl80211_parse_chandef(rdev, info, &params->chandef);
- if (err)
- goto out;
- } else if (wdev->valid_links) {
- /* with MLD need to specify the channel configuration */
- err = -EINVAL;
- goto out;
- } else if (wdev->u.ap.preset_chandef.chan) {
- params->chandef = wdev->u.ap.preset_chandef;
- } else if (!nl80211_get_ap_channel(rdev, params)) {
- err = -EINVAL;
- goto out;
- }
-
if (info->attrs[NL80211_ATTR_PUNCT_BITMAP]) {
err = nl80211_parse_punct_bitmap(rdev, info,
&params->chandef,
@@ -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, &params, info->extack);
+ err = nl80211_parse_beacon(rdev, info->attrs, &params, info->extack,
+ wdev->links[link_id].ap.chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
goto out;

@@ -10131,7 +10179,8 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
goto skip_beacons;

err = nl80211_parse_beacon(rdev, info->attrs, &params.beacon_after,
- info->extack);
+ info->extack,
+ wdev->links[link_id].ap.chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
goto free;

@@ -10149,7 +10198,8 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
goto free;

err = nl80211_parse_beacon(rdev, csa_attrs, &params.beacon_csa,
- info->extack);
+ info->extack,
+ wdev->links[link_id].ap.chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
goto free;

@@ -15976,6 +16026,7 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
struct cfg80211_color_change_settings params = {};
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
+ unsigned int link_id = nl80211_link_id(info->attrs);
struct nlattr **tb;
u16 offset;
int err;
@@ -15999,7 +16050,8 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
params.color = nla_get_u8(info->attrs[NL80211_ATTR_COLOR_CHANGE_COLOR]);

err = nl80211_parse_beacon(rdev, info->attrs, &params.beacon_next,
- info->extack);
+ info->extack,
+ wdev->links[link_id].ap.chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
return err;

@@ -16014,7 +16066,8 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
goto out;

err = nl80211_parse_beacon(rdev, tb, &params.beacon_color_change,
- info->extack);
+ info->extack,
+ wdev->links[link_id].ap.chandef.chan->band == NL80211_BAND_S1GHZ);
if (err)
goto out;

--
2.25.1



2023-09-13 14:38:44

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v7 1/2] wifi: cfg80211: Add short_beacon_tail/head/period

On Thu, 2023-08-10 at 19:35 +1000, Bassem Dawood wrote:
> 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 ;-)

> +++ b/include/net/cfg80211.h
> @@ -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;

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:

https://lore.kernel.org/linux-wireless/[email protected]/T/#m86511f184d40ab36221f4ceae066900233ceb84e

However, then we have this:

> +++ 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))
> goto 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

> if (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.

> if (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.

> @@ -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:

> + * @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...

> @@ -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, &params, info->extack);
> + err = nl80211_parse_beacon(rdev, info->attrs, &params, 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