2022-05-06 16:59:17

by Aloka Dixit

Subject: [PATCH v2 0/2] Additional processing in NL80211_CMD_SET_BEACON

FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP, however both stop
after userspace issues NL80211_CMD_SET_BEACON command as these
attributes are not processed as part of this command.
Add the missing implementation in nl80211 and mac80211 to fix the issue.

This series addresses the comments for the following series:
https://patchwork.kernel.org/project/linux-wireless/list/?series=417807&state=%2A&archive=both

Command NL80211_CMD_SET_BEACON is not renamed to NL80211_CMD_UPDATE_AP
as part of this series to keep the changes to a minimum but it can be
done if necessary.

Aloka Dixit (2):
nl80211: process additional attributes in NL80211_CMD_SET_BEACON
nl80211: process additional data in ieee80211_change_beacon()

include/net/cfg80211.h | 2 +-
net/mac80211/cfg.c | 25 +++++++++++++++++---
net/wireless/nl80211.c | 24 ++++++++++++++++---
net/wireless/rdev-ops.h | 2 +-
net/wireless/trace.h | 52 +++++++++++++++++++++++------------------
5 files changed, 74 insertions(+), 31 deletions(-)


base-commit: fc20106d6e2086dd37bf286605c28b28b4f2492c
--
2.31.1

2022-05-08 09:24:46

by Aloka Dixit

Subject: [PATCH v2 1/2] nl80211: process additional attributes in NL80211_CMD_SET_BEACON

FILS discovery and unsolicited broadcast probe response transmissions
are configured as part of NL80211_CMD_START_AP, however both stop
after userspace issues NL80211_CMD_SET_BEACON command as these
attributes are not processed as part of this command.
Add the missing implementation.

Modify the local variable in nl80211_set_beacon() and input parameter
to rdev_change_beacon() from type struct cfg80211_beacon_data to
type struct cfg80211_ap_settings to support the new processing.

Signed-off-by: Aloka Dixit <[email protected]>
---
v1:
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

include/net/cfg80211.h | 2 +-
net/wireless/nl80211.c | 24 ++++++++++++++++---
net/wireless/rdev-ops.h | 2 +-
net/wireless/trace.h | 52 +++++++++++++++++++++++------------------
4 files changed, 52 insertions(+), 28 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 68713388b617..b388e5c9beb8 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4195,7 +4195,7 @@ struct cfg80211_ops {
int (*start_ap)(struct wiphy *wiphy, struct net_device *dev,
struct cfg80211_ap_settings *settings);
int (*change_beacon)(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_beacon_data *info);
+ struct cfg80211_ap_settings *info);
int (*stop_ap)(struct wiphy *wiphy, struct net_device *dev);


diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 945ed87d12e0..ee94ad8cd619 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5799,9 +5799,11 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
- struct cfg80211_beacon_data params;
+ struct cfg80211_ap_settings params;
int err;

+ memset(&params, 0, sizeof(params));
+
if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
return -EOPNOTSUPP;
@@ -5812,16 +5814,32 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
if (!wdev->beacon_interval)
return -EINVAL;

- err = nl80211_parse_beacon(rdev, info->attrs, &params);
+ err = nl80211_parse_beacon(rdev, info->attrs, &params.beacon);
if (err)
goto out;

+ if (info->attrs[NL80211_ATTR_FILS_DISCOVERY]) {
+ err = nl80211_parse_fils_discovery(rdev,
+ info->attrs[NL80211_ATTR_FILS_DISCOVERY],
+ &params);
+ if (err)
+ goto out;
+ }
+
+ if (info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]) {
+ err = nl80211_parse_unsol_bcast_probe_resp(
+ rdev, info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP],
+ &params);
+ if (err)
+ goto out;
+ }
+
wdev_lock(wdev);
err = rdev_change_beacon(rdev, dev, &params);
wdev_unlock(wdev);

out:
- kfree(params.mbssid_ies);
+ kfree(params.beacon.mbssid_ies);
return err;
}

diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 439bcf52369c..131fbe9c3199 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -162,7 +162,7 @@ static inline int rdev_start_ap(struct cfg80211_registered_device *rdev,

static inline int rdev_change_beacon(struct cfg80211_registered_device *rdev,
struct net_device *dev,
- struct cfg80211_beacon_data *info)
+ struct cfg80211_ap_settings *info)
{
int ret;
trace_rdev_change_beacon(&rdev->wiphy, dev, info);
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 228079d7690a..97ca10cbbfee 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -597,44 +597,50 @@ TRACE_EVENT(rdev_start_ap,

TRACE_EVENT(rdev_change_beacon,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
- struct cfg80211_beacon_data *info),
+ struct cfg80211_ap_settings *info),
TP_ARGS(wiphy, netdev, info),
TP_STRUCT__entry(
WIPHY_ENTRY
NETDEV_ENTRY
- __dynamic_array(u8, head, info ? info->head_len : 0)
- __dynamic_array(u8, tail, info ? info->tail_len : 0)
- __dynamic_array(u8, beacon_ies, info ? info->beacon_ies_len : 0)
+ __dynamic_array(u8, head, info ? info->beacon.head_len : 0)
+ __dynamic_array(u8, tail, info ? info->beacon.tail_len : 0)
+ __dynamic_array(u8, beacon_ies,
+ info ? info->beacon.beacon_ies_len : 0)
__dynamic_array(u8, proberesp_ies,
- info ? info->proberesp_ies_len : 0)
+ info ? info->beacon.proberesp_ies_len : 0)
__dynamic_array(u8, assocresp_ies,
- info ? info->assocresp_ies_len : 0)
- __dynamic_array(u8, probe_resp, info ? info->probe_resp_len : 0)
+ info ? info->beacon.assocresp_ies_len : 0)
+ __dynamic_array(u8, probe_resp,
+ info ? info->beacon.probe_resp_len : 0)
),
TP_fast_assign(
WIPHY_ASSIGN;
NETDEV_ASSIGN;
if (info) {
- if (info->head)
- memcpy(__get_dynamic_array(head), info->head,
- info->head_len);
- if (info->tail)
- memcpy(__get_dynamic_array(tail), info->tail,
- info->tail_len);
- if (info->beacon_ies)
+ if (info->beacon.head)
+ memcpy(__get_dynamic_array(head),
+ info->beacon.head,
+ info->beacon.head_len);
+ if (info->beacon.tail)
+ memcpy(__get_dynamic_array(tail),
+ info->beacon.tail,
+ info->beacon.tail_len);
+ if (info->beacon.beacon_ies)
memcpy(__get_dynamic_array(beacon_ies),
- info->beacon_ies, info->beacon_ies_len);
- if (info->proberesp_ies)
+ info->beacon.beacon_ies,
+ info->beacon.beacon_ies_len);
+ if (info->beacon.proberesp_ies)
memcpy(__get_dynamic_array(proberesp_ies),
- info->proberesp_ies,
- info->proberesp_ies_len);
- if (info->assocresp_ies)
+ info->beacon.proberesp_ies,
+ info->beacon.proberesp_ies_len);
+ if (info->beacon.assocresp_ies)
memcpy(__get_dynamic_array(assocresp_ies),
- info->assocresp_ies,
- info->assocresp_ies_len);
- if (info->probe_resp)
+ info->beacon.assocresp_ies,
+ info->beacon.assocresp_ies_len);
+ if (info->beacon.probe_resp)
memcpy(__get_dynamic_array(probe_resp),
- info->probe_resp, info->probe_resp_len);
+ info->beacon.probe_resp,
+ info->beacon.probe_resp_len);
}
),
TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT, WIPHY_PR_ARG, NETDEV_PR_ARG)
--
2.31.1

2022-05-09 06:25:28

by Jeff Johnson

Subject: Re: [PATCH v2 1/2] nl80211: process additional attributes in NL80211_CMD_SET_BEACON

On 5/5/2022 10:34 AM, Aloka Dixit wrote:
> FILS discovery and unsolicited broadcast probe response transmissions
> are configured as part of NL80211_CMD_START_AP, however both stop
> after userspace issues NL80211_CMD_SET_BEACON command as these
> attributes are not processed as part of this command.
> Add the missing implementation.
>
> Modify the local variable in nl80211_set_beacon() and input parameter
> to rdev_change_beacon() from type struct cfg80211_beacon_data to
> type struct cfg80211_ap_settings to support the new processing.
>
> Signed-off-by: Aloka Dixit <[email protected]>
> ---
> v1:
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> include/net/cfg80211.h | 2 +-
> net/wireless/nl80211.c | 24 ++++++++++++++++---
> net/wireless/rdev-ops.h | 2 +-
> net/wireless/trace.h | 52 +++++++++++++++++++++++------------------
> 4 files changed, 52 insertions(+), 28 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 68713388b617..b388e5c9beb8 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4195,7 +4195,7 @@ struct cfg80211_ops {
> int (*start_ap)(struct wiphy *wiphy, struct net_device *dev,
> struct cfg80211_ap_settings *settings);
> int (*change_beacon)(struct wiphy *wiphy, struct net_device *dev,
> - struct cfg80211_beacon_data *info);
> + struct cfg80211_ap_settings *info);
> int (*stop_ap)(struct wiphy *wiphy, struct net_device *dev);
>
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 945ed87d12e0..ee94ad8cd619 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -5799,9 +5799,11 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
> struct cfg80211_registered_device *rdev = info->user_ptr[0];
> struct net_device *dev = info->user_ptr[1];
> struct wireless_dev *wdev = dev->ieee80211_ptr;
> - struct cfg80211_beacon_data params;
> + struct cfg80211_ap_settings params;
> int err;
>
> + memset(&params, 0, sizeof(params));
> +

is adding an = {} initializer preferable?

> if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
> dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
> return -EOPNOTSUPP;
> @@ -5812,16 +5814,32 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
> if (!wdev->beacon_interval)
> return -EINVAL;
>
> - err = nl80211_parse_beacon(rdev, info->attrs, &params);
> + err = nl80211_parse_beacon(rdev, info->attrs, &params.beacon);
> if (err)
> goto out;
>
> + if (info->attrs[NL80211_ATTR_FILS_DISCOVERY]) {
> + err = nl80211_parse_fils_discovery(rdev,
> + info->attrs[NL80211_ATTR_FILS_DISCOVERY],
> + &params);
> + if (err)
> + goto out;
> + }
> +
> + if (info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]) {
> + err = nl80211_parse_unsol_bcast_probe_resp(
> + rdev, info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP],
> + &params);
> + if (err)
> + goto out;
> + }
> +

would adding a local variable 'attr' make the code more readable?
attr = info->attrs[NL80211_ATTR_{foo}];
if (attr) {
err = nl80211_parse_{foo}(rdev, attr, &params);

> wdev_lock(wdev);
> err = rdev_change_beacon(rdev, dev, &params);
> wdev_unlock(wdev);
>
> out:
> - kfree(params.mbssid_ies);
> + kfree(params.beacon.mbssid_ies);
> return err;
> }
[...snip...]

2022-05-09 08:34:50

by Aloka Dixit

Subject: Re: [PATCH v2 1/2] nl80211: process additional attributes in NL80211_CMD_SET_BEACON

On 5/6/2022 11:22 AM, Jeff Johnson wrote:
> On 5/5/2022 10:34 AM, Aloka Dixit wrote:
>> -    struct cfg80211_beacon_data params;
>> +    struct cfg80211_ap_settings params;
>>       int err;
>> +    memset(&params, 0, sizeof(params));
>> +
>
> is adding an = {} initializer preferable?
>

I will change this from static to dynamic allocation in the next version
instead because noticed following error with some compiler:
error: the frame size of 1032 bytes is larger than 1024 bytes
[-Werror=frame-larger-than=]

>> +    if (info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]) {
>> +        err = nl80211_parse_unsol_bcast_probe_resp(
>> +                rdev, info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP],
>> +                &params);
>> +        if (err)
>> +            goto out;
>> +    }
>> +
>
> would adding a local variable 'attr' make the code more readable?
>     attr = info->attrs[NL80211_ATTR_{foo}];
>     if (attr) {
>         err = nl80211_parse_{foo}(rdev, attr, &params);
>

Sure, thanks.