Commands for FILS discovery and unsolicited probe response

PATCH In review Aloka Dixit 3 participants 7 messages Jan 20, 2021 – Feb 16, 2021

Subsystems (2)

mac80211 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

Aloka Dixit

author

2021-01-20 00:53 UTC #0 lore raw

cover Commands for FILS discovery and unsolicited probe response

This patchset removes the dependency of FILS discovery and unsolicited probe response configurations on beacon setup command and instead adds new netlink commands which allow setting parameters dynamically.

Aloka Dixit (2):
nl80211: Commands for FILS discovery and unsolicited broadcast probe
response
mac80211: Commands for FILS discovery and unsolicited broadcast probe
response

 include/net/cfg80211.h       |  13 +++--
 include/uapi/linux/nl80211.h |  12 +++++
 net/mac80211/cfg.c           |  33 +++++-------
 net/wireless/nl80211.c       | 102 ++++++++++++++++++++---------------
 net/wireless/rdev-ops.h      |  24 +++++++++
 net/wireless/trace.h         |  43 +++++++++++++++
 6 files changed, 159 insertions(+), 68 deletions(-)

base-commit: 0ae5b43d6dde6003070106e97cd0d41bace2eeb2

Signature · 1 line

2.25.0

Aloka Dixit

author

2021-01-20 00:53 UTC #1 lore raw

PATCH 1/2 nl80211: Commands for FILS discovery and unsolicited broadcast probe response

This change moves the FILS discovery and unsolicited probe response configurations to new netlink commands instead of during beacon set up. It allows modification of transmission parameters such as time interval dynamically.

Signed-off-by Aloka Dixit <[email protected]>

---
 include/net/cfg80211.h       |  13 +++--
 include/uapi/linux/nl80211.h |  12 +++++
 net/wireless/nl80211.c       | 102 ++++++++++++++++++++---------------
 net/wireless/rdev-ops.h      |  24 +++++++++
 net/wireless/trace.h         |  43 +++++++++++++++
 5 files changed, 147 insertions(+), 47 deletions(-)

include/net/cfg80211.h +9 -4

index 9a4bbccddc7f..b88d7e69a1e4 100644
@@ -1179,8 +1179,6 @@ enum cfg80211_ap_settings_flags {
  * @he_obss_pd: OBSS Packet Detection settings
  * @he_bss_color: BSS Color settings
  * @he_oper: HE operation IE (or %NULL if HE isn't enabled)
- * @fils_discovery: FILS discovery transmission parameters
- * @unsol_bcast_probe_resp: Unsolicited broadcast probe response parameters
  */
 struct cfg80211_ap_settings {
 	struct cfg80211_chan_def chandef;

@@ -1211,8 +1209,6 @@ struct cfg80211_ap_settings {
 	u32 flags;
 	struct ieee80211_he_obss_pd he_obss_pd;
 	struct cfg80211_he_bss_color he_bss_color;
-	struct cfg80211_fils_discovery fils_discovery;
-	struct cfg80211_unsol_bcast_probe_resp unsol_bcast_probe_resp;
 };
 
 /**

@@ -3972,6 +3968,9 @@ struct mgmt_frame_regs {
  *	This callback may sleep.
  * @reset_tid_config: Reset TID specific configuration for the peer, for the
  *	given TIDs. This callback may sleep.
+ * @set_fils_discovery: Set FILS discovery transmission parameters.
+ * @set_unsol_bcast_probe_resp: Set unsolicited broadcast probe response
+ *	transmission parameters.
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);

@@ -4299,6 +4298,12 @@ struct cfg80211_ops {
 				    const u8 *peer, u8 tids);
 	int	(*set_sar_specs)(struct wiphy *wiphy,
 				 struct cfg80211_sar_specs *sar);
+	int     (*set_fils_discovery)(struct wiphy *wiphy,
+				      struct net_device *dev,
+				      struct cfg80211_fils_discovery *params);
+	int	(*set_unsol_bcast_probe_resp)(struct wiphy *wiphy,
+					      struct net_device *dev,
+					      struct cfg80211_unsol_bcast_probe_resp *params);
 };
 
 /*

include/uapi/linux/nl80211.h +12 -0

index 40832d13c2f1..f78ba0e7e9a1 100644
@@ -1182,6 +1182,14 @@
  *	passed using %NL80211_ATTR_SAR_SPEC. %NL80211_ATTR_WIPHY is used to
  *	specify the wiphy index to be applied to.
  *
+ * @NL80211_CMD_SET_FILS_DISCOVERY: Set FILS discovery transmission parameters.
+ *	Inputs to the command are provided using the attribute
+ *	%NL80211_ATTR_FILS_DISCOVERY.
+ *
+ * @NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP: Set unsolicited broadcast
+ *	probe response transmission parameters. Inputs to the command are
+ *	provided using the attribute %NL80211_ATTR_UNSOL_BCAST_PROBE_RESP.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */

@@ -1414,6 +1422,10 @@ enum nl80211_commands {
 
 	NL80211_CMD_SET_SAR_SPECS,
 
+	NL80211_CMD_SET_FILS_DISCOVERY,
+
+	NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */

net/wireless/nl80211.c +59 ~~-43~~

154 lines not on this page · raw message

net/wireless/rdev-ops.h +24 -0

index 8b1358d04ca2..ed9bfbebe695 100644
@@ -1358,4 +1358,28 @@ static inline int rdev_set_sar_specs(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline int rdev_set_fils_discovery(struct cfg80211_registered_device *rdev,
+					  struct net_device *dev,
+					  struct cfg80211_fils_discovery *params)
+{
+	int ret;
+
+	trace_rdev_set_fils_discovery(&rdev->wiphy, dev, params);
+	ret = rdev->ops->set_fils_discovery(&rdev->wiphy, dev, params);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
+static inline int rdev_set_unsol_bcast_probe_resp(struct cfg80211_registered_device *rdev,
+						  struct net_device *dev,
+						  struct cfg80211_unsol_bcast_probe_resp *params)
+{
+	int ret;
+
+	trace_rdev_set_unsol_bcast_probe_resp(&rdev->wiphy, dev, params);
+	ret = rdev->ops->set_unsol_bcast_probe_resp(&rdev->wiphy, dev, params);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
 #endif /* __CFG80211_RDEV_OPS */

net/wireless/trace.h +43 -0

49 lines not on this page · raw message

Signature · 1 line

2.25.0

Arend van Spriel

2021-01-20 11:30 UTC #2 lore raw

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

Quoted 4 lines · Aloka Dixit

This change moves the FILS discovery and unsolicited probe response configurations to new netlink commands instead of during beacon set up. It allows modification of transmission parameters such as time interval dynamically.

Why not have both, ie. support initial setup through START_AP and allow modifications with new command or an existing one. Maybe we should consolidate to what has been discussed in thread about AP power save [1].

Regards,
Arend

[1] https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

Signature · 1 line

This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

Aloka Dixit

author

2021-01-25 21:53 UTC #3 lore raw

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

Quoted 11 lines · Arend van Spriel

Quoted 4 lines · Aloka Dixit
This change moves the FILS discovery and unsolicited probe response configurations to new netlink commands instead of during beacon set up. It allows modification of transmission parameters such as time interval dynamically.
Why not have both, ie. support initial setup through START_AP and allow modifications with new command or an existing one. Maybe we should consolidate to what has been discussed in thread about AP power save [1].

Quoted 5 lines

Regards,
Arend
[1]
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

Hi Arend,
FILS discovery and especially unsolicited probe response templates are big. Sometimes send_and_recv() returns error due to memory unavailability during wpa_driver_nl80211_set_ap() depending on how many interfaces, which elements are added. Moving these to separate commands resolves this issue along with more control over the time interval during run-time. Thanks.

Johannes Berg

maintainer

2021-02-12 09:34 UTC #4 lore raw

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

Quoted 5 lines · Aloka Dixit

FILS discovery and especially unsolicited probe response templates are big. Sometimes send_and_recv() returns error due to memory unavailability during wpa_driver_nl80211_set_ap() depending on how many interfaces, which elements are added.

What? Where do you get errors from? Netlink even supports vmalloc now, I believe, so the kernel really shouldn't care?

Quoted 3 lines

Moving these to separate commands
resolves this issue along with more control over the time interval during run-time.

I tend to agree with Arend though, we have NL80211_CMD_SET_BEACON and since NL80211_CMD_NEW_BEACON was renamed to NL80211_CMD_START_AP to more accurately say what it does, I think generalizing "AP modifications" by renaming NL80211_CMD_SET_BEACON to NL80211_CMD_UPDATE_AP (or such) would make a lot of sense.

Let's not conflate the two issues here.

1) memory issues - need to understand better

2) update is needed - I'd say SET_BEACON/UPDATE_AP would be a better way than pulling everything into separate commands. Updates can be partial too, after all, if you include only the changed attributes, and that might even address case 1? I.e. why wouldn't userspace be able to do UPDATE_AP multiple times, just like with your patch it would do NL80211_CMD_SET_FILS_DISCOVERY
and NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP?

Also, technically this constitutes an API break. One that perhaps nobody cares about yet, but surely somebody already has hostapd versions that use NL80211_ATTR_FILS_DISCOVERY or NL80211_ATTR_UNSOL_BCAST_PROBE_RESP? After all, you don't want to tell me you never tested this code ;-)

johannes

Aloka Dixit

author

2021-02-16 22:42 UTC #5 lore raw

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

Quoted 10 lines · Johannes Berg

Quoted 5 lines · Aloka Dixit
FILS discovery and especially unsolicited probe response templates are big. Sometimes send_and_recv() returns error due to memory unavailability during wpa_driver_nl80211_set_ap() depending on how many interfaces, which elements are added.
What? Where do you get errors from? Netlink even supports vmalloc now, I believe, so the kernel really shouldn't care?

The error was shown on hostapd side during attribute building when 16 interfaces were added on one radio and unsolicited broadcast probe response feature was enabled.
It was resolved with a separate command for this feature instead of being a part of NL80211_CMD_START_AP.

Quoted 23 lines

Quoted 3 lines
Moving these to separate commands
resolves this issue along with more control over the time interval during run-time.
I tend to agree with Arend though, we have NL80211_CMD_SET_BEACON and since NL80211_CMD_NEW_BEACON was renamed to NL80211_CMD_START_AP to more accurately say what it does, I think generalizing "AP modifications" by renaming NL80211_CMD_SET_BEACON to NL80211_CMD_UPDATE_AP (or such) would make a lot of sense.
Let's not conflate the two issues here.
1) memory issues - need to understand better
2) update is needed - I'd say SET_BEACON/UPDATE_AP would be a better way than pulling everything into separate commands. Updates can be partial too, after all, if you include only the changed attributes, and that might even address case 1? I.e. why wouldn't userspace be able to do UPDATE_AP multiple times, just like with your patch it would do NL80211_CMD_SET_FILS_DISCOVERY
and NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP?

Quoted 6 lines

Also, technically this constitutes an API break. One that perhaps nobody cares about yet, but surely somebody already has hostapd versions that use NL80211_ATTR_FILS_DISCOVERY or NL80211_ATTR_UNSOL_BCAST_PROBE_RESP? After all, you don't want to tell me you never tested this code ;-)
johannes

I myself added the hostapd changes corresponding to the original kernel code, This patch-set doesn't remove these attributes so, if accepted, current hostapd calls will become no-op but won't break any other functionality until I send the next version for separate commands for hostapd.

On a different note, a line was missed in nl80211_fils_discovery_policy definition from my side in the original change so included it in this patch-set.
Should I send a separate patch for that while we discuss regarding the separate command?
Thanks.

Aloka Dixit

author

2021-01-20 00:53 UTC #6 lore raw

PATCH 2/2 mac80211: Commands for FILS discovery and unsolicited broadcast probe response

In reply to Aloka Dixit

This patch registers ieee80211_set_fils_discovery() and ieee80211_set_unsol_bcast_probe_resp() as callback functions with mac80211_config_ops() to be used for configuring FILS discovery transmission.

Changes also include new calls to ieee80211_bss_info_change_notify() which were not needed earlier as configurations were completed through ieee80211_start_ap().

Signed-off-by Aloka Dixit <[email protected]>

---
 net/mac80211/cfg.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

net/mac80211/cfg.c +12 ~~-21~~

index c4c70e30ad7f..27c97fd88e76 100644
@@ -839,11 +839,13 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
+static int ieee80211_set_fils_discovery(struct wiphy *wiphy,
+					struct net_device *dev,
 					struct cfg80211_fils_discovery *params)
 {
 	struct fils_discovery_data *new, *old = NULL;
 	struct ieee80211_fils_discovery *fd;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
 	if (!params->tmpl || !params->tmpl_len)
 		return -EINVAL;

@@ -863,18 +865,23 @@ static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
 	if (old)
 		kfree_rcu(old, rcu_head);
 
+	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_FILS_DISCOVERY);
 	return 0;
 }
 
 static int
-ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
+ieee80211_set_unsol_bcast_probe_resp(struct wiphy *wiphy,
+				     struct net_device *dev,
 				     struct cfg80211_unsol_bcast_probe_resp *params)
 {
 	struct unsol_bcast_probe_resp_data *new, *old = NULL;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
 	if (!params->tmpl || !params->tmpl_len)
 		return -EINVAL;
 
+	sdata->vif.bss_conf.unsol_bcast_probe_resp_interval = params->interval;
+
 	old = sdata_dereference(sdata->u.ap.unsol_bcast_probe_resp, sdata);
 	new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
 	if (!new)

@@ -886,9 +893,7 @@ ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
 	if (old)
 		kfree_rcu(old, rcu_head);
 
-	sdata->vif.bss_conf.unsol_bcast_probe_resp_interval =
-							params->interval;
-
+	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_UNSOL_BCAST_PROBE_RESP);
 	return 0;
 }

@@ -1161,22 +1166,6 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 		goto error;
 	changed |= err;
 
-	if (params->fils_discovery.max_interval) {
-		err = ieee80211_set_fils_discovery(sdata,
-						   &params->fils_discovery);
-		if (err < 0)
-			goto error;
-		changed |= BSS_CHANGED_FILS_DISCOVERY;
-	}
-
-	if (params->unsol_bcast_probe_resp.interval) {
-		err = ieee80211_set_unsol_bcast_probe_resp(sdata,
-							   &params->unsol_bcast_probe_resp);
-		if (err < 0)
-			goto error;
-		changed |= BSS_CHANGED_UNSOL_BCAST_PROBE_RESP;
-	}
-
 	err = drv_start_ap(sdata->local, sdata);
 	if (err) {
 		old = sdata_dereference(sdata->u.ap.beacon, sdata);

@@ -4187,4 +4176,6 @@ const struct cfg80211_ops mac80211_config_ops = {
 	.set_tid_config = ieee80211_set_tid_config,
 	.reset_tid_config = ieee80211_reset_tid_config,
 	.set_sar_specs = ieee80211_set_sar_specs,
+	.set_fils_discovery = ieee80211_set_fils_discovery,
+	.set_unsol_bcast_probe_resp = ieee80211_set_unsol_bcast_probe_resp,
 };

Signature · 1 line

2.25.0