diff mbox series

[v3,2/3] P2P: optimize join scan freq

Message ID 20221221031546.1373427-1-matthewmwang@chromium.org
State Changes Requested
Headers show
Series None | expand

Commit Message

Matthew Wang Dec. 21, 2022, 3:15 a.m. UTC 
From: Matthew Wang <matthewmwang@google.com>

Allow clients to force the BSSID of an auto GO. If the auto GO has been
discovered on another interface, optimize scan frequency by performing
a single channel scan first. Android and ChromeOS use this to streamline
auto GO discovery.

Signed-off-by: Matthew Wang <matthewmwang@chromium.org>
---
 wpa_supplicant/bss.c                        |  5 ++--
 wpa_supplicant/ctrl_iface.c                 |  2 +-
 wpa_supplicant/dbus/dbus_new_handlers_p2p.c |  6 ++++-
 wpa_supplicant/events.c                     |  2 +-
 wpa_supplicant/p2p_supplicant.c             | 24 ++++++++++++------
 wpa_supplicant/p2p_supplicant.h             |  3 ++-
 wpa_supplicant/scan.c                       | 27 ++++++++++++++++++++-
 wpa_supplicant/wpa_supplicant_i.h           |  1 +
 8 files changed, 56 insertions(+), 14 deletions(-)

Comments

Jouni Malinen Feb. 21, 2023, 11:47 a.m. UTC | #1 
On Tue, Dec 20, 2022 at 07:15:46PM -0800, Matthew Wang wrote:
> Allow clients to force the BSSID of an auto GO. If the auto GO has been
> discovered on another interface, optimize scan frequency by performing
> a single channel scan first. Android and ChromeOS use this to streamline
> auto GO discovery.

How would the P2P Client know which BSSID the GO is using in this
instance? The BSSID, i.e., the P2P Interface Address, may change between
each invocation of a persistent group.

> diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
> @@ -240,7 +240,7 @@ void wpa_bss_remove(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
> - * @bssid: BSSID
> + * @bssid: BSSID, or %NULL to match any BSSID

> @@ -252,7 +252,8 @@ struct wpa_bss * wpa_bss_get(struct wpa_supplicant *wpa_s, const u8 *bssid,
>  	if (!wpa_supplicant_filter_bssid_match(wpa_s, bssid))
>  		return NULL;

That could result in NULL pointer dereferencing the bssid parameter,
i.e., this call needs to be made conditional on bssid being not-NULL.

> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> +++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> @@ -356,6 +356,7 @@ DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
> +	int force_go_bssid = 0;

That should really be bool/false here.

> @@ -382,6 +383,9 @@ DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
> +		} else if (os_strcmp(entry.key, "force_go_bssid") &&

os_strcmp() returns 0 on match, i.e., this needs " == 0" to be added to
work correctly.

> +			   entry.type == DBUS_TYPE_BOOLEAN) {
> +			force_go_bssid = entry.bool_value;

>  		if (wpas_p2p_group_add_persistent(wpa_s, ssid, 0, freq, 0, 0, 0,
>  						  0, 0, 0, 0, NULL, 0, 0,
> -						  false, retry_limit)) {
> +						  false, retry_limit, true)) {

true? Was that supposed to be force_go_bssid instead?

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c

>  static int wpas_start_p2p_client(struct wpa_supplicant *wpa_s,
>  				 struct wpa_ssid *params, int addr_allocated,
> -				 int freq, int force_scan, int retry_limit)
> +				 int freq, int force_scan, int retry_limit,
> +				 bool force_go_bssid)

> +	if (force_go_bssid && params->bssid_set) {
> +		ssid->bssid_set = 1;
> +		os_memcpy(ssid->bssid, params->bssid, ETH_ALEN);
> +	}

The params->bssid here is the bssid value in the special network profile
that is used to store the persistent group information. That special
case uses the bssid value to store the P2P Device Address of the GO; not
the P2P Interface Address (which would be the BSSID). How is this
supposed to work if the GO uses P2P Interface Addresses that differ from
the P2P Device Address?
Matthew Wang Feb. 22, 2023, 12:47 a.m. UTC | #2 
> How would the P2P Client know which BSSID the GO is using in this
> instance? The BSSID, i.e., the P2P Interface Address, may change between
> each invocation of a persistent group.

The use case in ChromeOS and Android is out-of-band discovery, where
the BSSID is communicated via e.g. Bluetooth.

> The params->bssid here is the bssid value in the special network profile
> that is used to store the persistent group information. That special
> case uses the bssid value to store the P2P Device Address of the GO; not
> the P2P Interface Address (which would be the BSSID). How is this
> supposed to work if the GO uses P2P Interface Addresses that differ from
> the P2P Device Address?

Ah I see. In that case, how about directly passing in a BSSID via
D-Bus? I've sent a new patch, PTAL!

On Tue, Feb 21, 2023 at 3:47 AM Jouni Malinen <j@w1.fi> wrote:
>
> On Tue, Dec 20, 2022 at 07:15:46PM -0800, Matthew Wang wrote:
> > Allow clients to force the BSSID of an auto GO. If the auto GO has been
> > discovered on another interface, optimize scan frequency by performing
> > a single channel scan first. Android and ChromeOS use this to streamline
> > auto GO discovery.
>
> How would the P2P Client know which BSSID the GO is using in this
> instance? The BSSID, i.e., the P2P Interface Address, may change between
> each invocation of a persistent group.
>
> > diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
> > @@ -240,7 +240,7 @@ void wpa_bss_remove(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
> > - * @bssid: BSSID
> > + * @bssid: BSSID, or %NULL to match any BSSID
>
> > @@ -252,7 +252,8 @@ struct wpa_bss * wpa_bss_get(struct wpa_supplicant *wpa_s, const u8 *bssid,
> >       if (!wpa_supplicant_filter_bssid_match(wpa_s, bssid))
> >               return NULL;
>
> That could result in NULL pointer dereferencing the bssid parameter,
> i.e., this call needs to be made conditional on bssid being not-NULL.
>
> > diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> > +++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> > @@ -356,6 +356,7 @@ DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
> > +     int force_go_bssid = 0;
>
> That should really be bool/false here.
>
> > @@ -382,6 +383,9 @@ DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
> > +             } else if (os_strcmp(entry.key, "force_go_bssid") &&
>
> os_strcmp() returns 0 on match, i.e., this needs " == 0" to be added to
> work correctly.
>
> > +                        entry.type == DBUS_TYPE_BOOLEAN) {
> > +                     force_go_bssid = entry.bool_value;
>
> >               if (wpas_p2p_group_add_persistent(wpa_s, ssid, 0, freq, 0, 0, 0,
> >                                                 0, 0, 0, 0, NULL, 0, 0,
> > -                                               false, retry_limit)) {
> > +                                               false, retry_limit, true)) {
>
> true? Was that supposed to be force_go_bssid instead?
>
> > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
>
> >  static int wpas_start_p2p_client(struct wpa_supplicant *wpa_s,
> >                                struct wpa_ssid *params, int addr_allocated,
> > -                              int freq, int force_scan, int retry_limit)
> > +                              int freq, int force_scan, int retry_limit,
> > +                              bool force_go_bssid)
>
> > +     if (force_go_bssid && params->bssid_set) {
> > +             ssid->bssid_set = 1;
> > +             os_memcpy(ssid->bssid, params->bssid, ETH_ALEN);
> > +     }
>
> The params->bssid here is the bssid value in the special network profile
> that is used to store the persistent group information. That special
> case uses the bssid value to store the P2P Device Address of the GO; not
> the P2P Interface Address (which would be the BSSID). How is this
> supposed to work if the GO uses P2P Interface Addresses that differ from
> the P2P Device Address?
>
> --
> Jouni Malinen                                            PGP id EFC895FA
diff mbox series

Patch

diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c
index cd2c164c3b7..0f986bbcad8 100644
--- a/wpa_supplicant/bss.c
+++ b/wpa_supplicant/bss.c
@@ -240,7 +240,7 @@  void wpa_bss_remove(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
 /**
  * wpa_bss_get - Fetch a BSS table entry based on BSSID and SSID
  * @wpa_s: Pointer to wpa_supplicant data
- * @bssid: BSSID
+ * @bssid: BSSID, or %NULL to match any BSSID
  * @ssid: SSID
  * @ssid_len: Length of @ssid
  * Returns: Pointer to the BSS entry or %NULL if not found
@@ -252,7 +252,8 @@  struct wpa_bss * wpa_bss_get(struct wpa_supplicant *wpa_s, const u8 *bssid,
 	if (!wpa_supplicant_filter_bssid_match(wpa_s, bssid))
 		return NULL;
 	dl_list_for_each(bss, &wpa_s->bss, struct wpa_bss, list) {
-		if (os_memcmp(bss->bssid, bssid, ETH_ALEN) == 0 &&
+		if ((!bssid ||
+		     os_memcmp(bss->bssid, bssid, ETH_ALEN) == 0) &&
 		    bss->ssid_len == ssid_len &&
 		    os_memcmp(bss->ssid, ssid, ssid_len) == 0)
 			return bss;
diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
index 6588fd47b4a..27397e99b57 100644
--- a/wpa_supplicant/ctrl_iface.c
+++ b/wpa_supplicant/ctrl_iface.c
@@ -7128,7 +7128,7 @@  static int p2p_ctrl_group_add_persistent(struct wpa_supplicant *wpa_s,
 	return wpas_p2p_group_add_persistent(wpa_s, ssid, 0, freq,
 					     vht_center_freq2, 0, ht40, vht,
 					     vht_chwidth, he, edmg,
-					     NULL, 0, 0, allow_6ghz, 0);
+					     NULL, 0, 0, allow_6ghz, 0, false);
 }
 
 
diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
index 370aee278cf..26e8833e688 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
@@ -356,6 +356,7 @@  DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
 	int persistent_group = 0;
 	int freq = 0;
 	int retry_limit = 0;
+	int force_go_bssid = 0;
 	char *iface = NULL;
 	unsigned int group_id = 0;
 	struct wpa_ssid *ssid;
@@ -382,6 +383,9 @@  DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
 			retry_limit = entry.int32_value;
 			if (retry_limit <= 0)
 				goto inv_args_clear;
+		} else if (os_strcmp(entry.key, "force_go_bssid") &&
+			   entry.type == DBUS_TYPE_BOOLEAN) {
+			force_go_bssid = entry.bool_value;
 		} else if (os_strcmp(entry.key, "persistent_group_object") ==
 			   0 &&
 			   entry.type == DBUS_TYPE_OBJECT_PATH)
@@ -432,7 +436,7 @@  DBusMessage * wpas_dbus_handler_p2p_group_add(DBusMessage *message,
 
 		if (wpas_p2p_group_add_persistent(wpa_s, ssid, 0, freq, 0, 0, 0,
 						  0, 0, 0, 0, NULL, 0, 0,
-						  false, retry_limit)) {
+						  false, retry_limit, true)) {
 			reply = wpas_dbus_error_unknown_error(
 				message,
 				"Failed to reinvoke a persistent group");
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
index 64b2bcd1daf..c8c639a939a 100644
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -1162,7 +1162,7 @@  static void owe_trans_ssid(struct wpa_supplicant *wpa_s, struct wpa_bss *bss,
 }
 
 
-static int disabled_freq(struct wpa_supplicant *wpa_s, int freq)
+int disabled_freq(struct wpa_supplicant *wpa_s, int freq)
 {
 	int i, j;
 
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index 6cf5a485f00..a2b70a6d675 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -3299,7 +3299,8 @@  static void wpas_invitation_received(void *ctx, const u8 *sa, const u8 *bssid,
 				wpa_s->conf->p2p_go_he,
 				wpa_s->conf->p2p_go_edmg, NULL,
 				go ? P2P_MAX_INITIAL_CONN_WAIT_GO_REINVOKE : 0,
-				1, is_p2p_allow_6ghz(wpa_s->global->p2p), 0);
+				1, is_p2p_allow_6ghz(wpa_s->global->p2p), 0,
+				false);
 		} else if (bssid) {
 			wpa_s->user_initiated_pd = 0;
 			wpa_msg_global(wpa_s, MSG_INFO,
@@ -3529,7 +3530,8 @@  static void wpas_invitation_result(void *ctx, int status, const u8 *bssid,
 				      ssid->mode == WPAS_MODE_P2P_GO ?
 				      P2P_MAX_INITIAL_CONN_WAIT_GO_REINVOKE :
 				      0, 1,
-				      is_p2p_allow_6ghz(wpa_s->global->p2p), 0);
+				      is_p2p_allow_6ghz(wpa_s->global->p2p), 0,
+				      false);
 }
 
 
@@ -4615,7 +4617,7 @@  static void wpas_p2ps_prov_complete(void *ctx, u8 status, const u8 *dev,
 					persistent_go->mode ==
 					WPAS_MODE_P2P_GO ?
 					P2P_MAX_INITIAL_CONN_WAIT_GO_REINVOKE :
-					0, 0, false, 0);
+					0, 0, false, 0, false);
 			} else if (response_done) {
 				wpas_p2p_group_add(wpa_s, 1, freq,
 						   0, 0, 0, 0, 0, 0, false);
@@ -4738,7 +4740,7 @@  static int wpas_prov_disc_resp_cb(void *ctx)
 			NULL,
 			persistent_go->mode == WPAS_MODE_P2P_GO ?
 			P2P_MAX_INITIAL_CONN_WAIT_GO_REINVOKE : 0, 0,
-			is_p2p_allow_6ghz(wpa_s->global->p2p), 0);
+			is_p2p_allow_6ghz(wpa_s->global->p2p), 0, false);
 	} else {
 		wpas_p2p_group_add(wpa_s, 1, freq, 0, 0, 0, 0, 0, 0,
 				   is_p2p_allow_6ghz(wpa_s->global->p2p));
@@ -6911,7 +6913,8 @@  int wpas_p2p_group_add(struct wpa_supplicant *wpa_s, int persistent_group,
 
 static int wpas_start_p2p_client(struct wpa_supplicant *wpa_s,
 				 struct wpa_ssid *params, int addr_allocated,
-				 int freq, int force_scan, int retry_limit)
+				 int freq, int force_scan, int retry_limit,
+				 bool force_go_bssid)
 {
 	struct wpa_ssid *ssid;
 	int other_iface_found = 0;
@@ -6954,6 +6957,11 @@  static int wpas_start_p2p_client(struct wpa_supplicant *wpa_s,
 	if (params->passphrase)
 		ssid->passphrase = os_strdup(params->passphrase);
 
+	if (force_go_bssid && params->bssid_set) {
+		ssid->bssid_set = 1;
+		os_memcpy(ssid->bssid, params->bssid, ETH_ALEN);
+	}
+
 	wpa_s->show_group_started = 1;
 	wpa_s->p2p_in_invitation = 1;
 	wpa_s->p2p_retry_limit = retry_limit;
@@ -7000,7 +7008,8 @@  int wpas_p2p_group_add_persistent(struct wpa_supplicant *wpa_s,
 				  int edmg,
 				  const struct p2p_channels *channels,
 				  int connection_timeout, int force_scan,
-				  bool allow_6ghz, int retry_limit)
+				  bool allow_6ghz, int retry_limit,
+				  bool force_go_bssid)
 {
 	struct p2p_go_neg_results params;
 	int go = 0, freq;
@@ -7068,7 +7077,8 @@  int wpas_p2p_group_add_persistent(struct wpa_supplicant *wpa_s,
 		}
 
 		return wpas_start_p2p_client(wpa_s, ssid, addr_allocated, freq,
-					     force_scan, retry_limit);
+					     force_scan, retry_limit,
+					     force_go_bssid);
 	} else {
 		return -1;
 	}
diff --git a/wpa_supplicant/p2p_supplicant.h b/wpa_supplicant/p2p_supplicant.h
index 11cc9c390ea..e113c62b232 100644
--- a/wpa_supplicant/p2p_supplicant.h
+++ b/wpa_supplicant/p2p_supplicant.h
@@ -52,7 +52,8 @@  int wpas_p2p_group_add_persistent(struct wpa_supplicant *wpa_s,
 				  int max_oper_chwidth, int he, int edmg,
 				  const struct p2p_channels *channels,
 				  int connection_timeout, int force_scan,
-				  bool allow_6ghz, int retry_limit);
+				  bool allow_6ghz, int retry_limit,
+				  bool force_go_bssid);
 struct p2p_group * wpas_p2p_group_init(struct wpa_supplicant *wpa_s,
 				       struct wpa_ssid *ssid);
 enum wpas_p2p_prov_disc_use {
diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
index cc9faf2a807..88d7e6e65e9 100644
--- a/wpa_supplicant/scan.c
+++ b/wpa_supplicant/scan.c
@@ -445,11 +445,36 @@  static void wpa_supplicant_optimize_freqs(
 	}
 
 	if (params->freqs == NULL && wpa_s->p2p_in_invitation) {
+		/*
+		 * Perform a single-channel scan if the GO has already been
+		 * discovered on another non-P2P interface. Note that a scan
+		 * initiated by a P2P interface (e.g. the device interface)
+		 * should already have sufficient IEs and scan results will be
+		 * fetched on interface creation in that case.
+		 */
+		if (wpa_s->p2p_in_invitation == 1 && wpa_s->current_ssid) {
+			struct wpa_supplicant *ifs;
+			struct wpa_bss *bss = NULL;
+			struct wpa_ssid *ssid = wpa_s->current_ssid;
+			u8 *bssid = ssid->bssid_set ? ssid->bssid : NULL;
+			dl_list_for_each(ifs, &wpa_s->radio->ifaces,
+					 struct wpa_supplicant, radio_list) {
+				bss = wpa_bss_get(ifs, bssid, ssid->ssid,
+						  ssid->ssid_len);
+				if (bss)
+					break;
+			}
+			if (bss && !disabled_freq(wpa_s, bss->freq)) {
+				params->freqs = os_calloc(2, sizeof(int));
+				if (params->freqs)
+					params->freqs[0] = bss->freq;
+			}
+		}
 		/*
 		 * Optimize scan based on GO information during persistent
 		 * group reinvocation
 		 */
-		if (wpa_s->p2p_in_invitation < 5 &&
+		if (params->freqs == NULL && wpa_s->p2p_in_invitation < 5 &&
 		    wpa_s->p2p_invite_go_freq > 0) {
 			if (wpa_s->p2p_invite_go_freq == 2 ||
 			    wpa_s->p2p_invite_go_freq == 5) {
diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h
index da8152560f8..8491588d6fc 100644
--- a/wpa_supplicant/wpa_supplicant_i.h
+++ b/wpa_supplicant/wpa_supplicant_i.h
@@ -1823,6 +1823,7 @@  int get_shared_radio_freqs_data(struct wpa_supplicant *wpa_s,
 int get_shared_radio_freqs(struct wpa_supplicant *wpa_s,
 			   int *freq_array, unsigned int len,
 			   bool exclude_current);
+int disabled_freq(struct wpa_supplicant *wpa_s, int freq);
 
 void wpas_network_reenabled(void *eloop_ctx, void *timeout_ctx);