netdev: Add a wdev_id based frame watch API

src/netdev.c +0 -0

  #include <ell/ell.h>
 

src/netdev.c +0 -0

  	if (!l_genl_attr_init(&attr, msg))
  		return;

@@ -3527,6 +3529,11 @@ static void netdev_mgmt_frame_event(struct l_genl_msg *msg,
 

Quoted 3 lines

#include <sys/socket.h>
#include <errno.h>
+#include <limits.h>

This would suck to have to use INT_MIN everywhere...

Quoted 4 lines

  #include <ell/ell.h>

<snip>

src/netdev.c static void netdev_mgmt_frame_event(struct l_genl_msg *msg, · line 3515

Quoted 5 lines

src/netdev.c +1 -0

@@ -3511,6 +3512,7 @@ static void netdev_mgmt_frame_event(struct l_genl_msg *msg,
      const struct mmpdu_header *mpdu = NULL;
      const uint8_t *body;
      struct frame_prefix_info info;
+     int rssi = INT_MIN;

The kernel uses 0 as a flag value, so can't we do the same?

From: Andrew Zaborowski <[email protected]>

Extend the iftype-based checks to handle the P2P iftypes and remove a warning that may be triggered in normal situations in the P2P scenarios.

---
  src/netdev.c | 20 ++++++++++++++------
  1 file changed, 14 insertions(+), 6 deletions(-)

From: Andrew Zaborowski <[email protected]>

Move the storage of the eapol event callback from the eapol_sm struct to the handshake_state struct. Rename from eapol event to eap event as the callback is only used to relay eap-specific event in eapol.c.

This is allows the handler to be set before calling netdev_connect/netdev_connect_wsc. It's also in theory more type-safe because don't need the cast in netdev_connect_wsc anymore.

Note that eapol_sm_set_user_data is now unused. I'm not sure if it should be removed on its own, the user_data that it sets will also affect rekey_offload callbacks. However rekey_offload's user_data parameter is not used by the only implementation of that callback, so both could be removed together.

---
  src/eapol.c     | 10 +++-------
  src/eapol.h     |  1 -
  src/handshake.c |  8 ++++++++
  src/handshake.h | 10 +++++++++-
  src/netdev.c    |  6 ++++--
  5 files changed, 24 insertions(+), 11 deletions(-)

Quoted 3 lines · Andrew Zaborowski

Move the storage of the eapol event callback from the eapol_sm struct to the handshake_state struct. Rename from eapol event to eap event as the callback is only used to relay eap-specific event in eapol.c.

Hmm, I'd be tempted to use handshake_state_set_event_func() for this as well. Especially given that wsc.c already subscribes to handshake events. Only problem is getting the eap event type and data to the event func. I wonder if variadic functions work through pointers...

Quoted 6 lines

This is allows the handler to be set before calling netdev_connect/netdev_connect_wsc. It's also in theory more type-safe because don't need the cast in netdev_connect_wsc anymore.

Quoted 5 lines

Note that eapol_sm_set_user_data is now unused. I'm not sure if it should be removed on its own, the user_data that it sets will also affect rekey_offload callbacks. However rekey_offload's user_data parameter is not used by the only implementation of that callback, so both could be removed together.

I'd imagine they can be reworked to act like netdev_set_tk, etc.

Hi Denis,

Quoted 9 lines · Denis Kenzior

Quoted 3 lines · Andrew Zaborowski

Move the storage of the eapol event callback from the eapol_sm struct to the handshake_state struct. Rename from eapol event to eap event as the callback is only used to relay eap-specific event in eapol.c.

Hmm, I'd be tempted to use handshake_state_set_event_func() for this as well. Especially given that wsc.c already subscribes to handshake events. Only problem is getting the eap event type and data to the event func. I wonder if variadic functions work through pointers...

I think they should and this may be the best option. So I guess we would want handshake_event_func_t to take:

struct handshake_state *hs,
enum handshake_event event,
void *user_data,
...

while event_data and the eap event subtype would already be part of the "...".

The uglier option would be to reserve a range of enum handshake_event values for eap events.

Quoted 13 lines

Quoted 12 lines

This is allows the handler to be set before calling netdev_connect/netdev_connect_wsc. It's also in theory more type-safe because don't need the cast in netdev_connect_wsc anymore.

Note that eapol_sm_set_user_data is now unused. I'm not sure if it should be removed on its own, the user_data that it sets will also affect rekey_offload callbacks. However rekey_offload's user_data parameter is not used by the only implementation of that callback, so both could be removed together.

Quoted 1 line

I'd imagine they can be reworked to act like netdev_set_tk, etc.

Or those could be reworked to take the user_data so we don't have to look up by ifindex.

Best regards

From: Andrew Zaborowski <[email protected]>

Until now WSC could only be triggered by calling a DBus method and success and error paths would end with a reply to the DBus method. Instead use a wsc done callback pointer so that the WSC connection logic can be triggered through DBus or a C API. The DBus method reply is sent from inside the callback when WSC is triggered through DBus.

Export a function that adds a new entry point to start the WSC connection for the P2P provisioning phase. The function doesn't have anything specific to P2P at this point.

---
  Makefile.am |   2 +-
  src/wsc.c   | 181 +++++++++++++++++++++++++++++++++++++---------------
  2 files changed, 131 insertions(+), 52 deletions(-)

Makefile.am +0 -0

index ecd1c228..03be9577 100644

Makefile.am +0 -0

  					src/backtrace.h src/backtrace.c \
  					src/knownnetworks.h \
  					src/knownnetworks.c \

src/wsc.c +1 -1

index d971797a..61683e7b 100644
@@ -44,6 +44,7 @@
  #include "src/storage.h"
  #include "src/iwd.h"
  #include "src/network.h"
+#include "src/wsc.h"
 
  #define WALK_TIME 120
 

@@ -60,18 +61,14 @@ struct wsc {
  	uint32_t scan_id;
  	struct scan_bss *target;
  	uint32_t station_state_watch;
-	struct {

src/wsc.c +5 -0

  	uint32_t n_creds;
  	struct l_settings *eap_settings;
+	char *pin;
+
+	wsc_done_cb_t done_cb;
+	void *done_data;
+	uint16_t config_method;
 
  	bool wsc_association : 1;
  };

@@ -160,15 +157,12 @@ static void wsc_try_credentials(struct wsc *wsc)
  		l_dbus_message_unref(wsc->pending);
  		wsc->pending = NULL;
 

src/wsc.c +17 -4

  }
 
  static void wsc_store_credentials(struct wsc *wsc)

@@ -224,6 +218,22 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
  	station_set_autoconnect(wsc->station, true);
  }
 
+static void wsc_connect_cleanup(struct wsc *wsc)
+{
+	wsc->wsc_association = false;
+
+	l_settings_free(wsc->eap_settings);
+	wsc->eap_settings = NULL;
+
+	if (wsc->pin) {
+		explicit_bzero(wsc->pin, strlen(wsc->pin));
+		l_free(wsc->pin);
+		wsc->pin = NULL;
+	}
+
+	wsc->target = NULL;
+}
+
  static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
  					void *event_data, void *user_data)
  {

@@ -231,33 +241,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 
  	l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
 
-	wsc->wsc_association = false;
-
-	l_settings_free(wsc->eap_settings);
-	wsc->eap_settings = NULL;
+	wsc_connect_cleanup(wsc);
 
  	if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {

src/wsc.c +35 -26

  		return;
  	}
 
  	switch (result) {
  	case NETDEV_RESULT_ABORTED:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_aborted(wsc->pending));
+		wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
  		return;
  	case NETDEV_RESULT_HANDSHAKE_FAILED:
-		dbus_pending_reply(&wsc->pending,
-					wsc_error_no_credentials(wsc->pending));
+		wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
  		break;
  	default:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_failed(wsc->pending));
+		wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
  		break;
  	}
 
-	station_set_autoconnect(wsc->station, true);
+	if (wsc->station)
+		station_set_autoconnect(wsc->station, true);
  }
 
  static void wsc_credential_obtained(struct wsc *wsc,

@@ -451,30 +462,24 @@ static void wsc_connect(struct wsc *wsc)
  	l_settings_set_uint(settings, "WSC", "RFBand",
  					freq_to_rf_band(bss->frequency));
  	l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
-				WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
-				WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
-				WSC_CONFIGURATION_METHOD_KEYPAD);
+					wsc->config_method);
  	l_settings_set_string(settings, "WSC", "PrimaryDeviceType",
  					"0-00000000-0");
  	l_settings_set_string(settings, "WSC", "EnrolleeMAC",
  		util_address_to_string(netdev_get_address(wsc->netdev)));
 
-	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
-		const char *pin;
-
-		if (l_dbus_message_get_arguments(wsc->pending, "s", &pin)) {
-			enum wsc_device_password_id dpid;
+	if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+		enum wsc_device_password_id dpid;
 
-			if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
-				dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
-			else
-				dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+		if (strlen(wsc->pin) == 4 ||
+				wsc_pin_is_checksum_valid(wsc->pin))
+			dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+		else
+			dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
 
-			l_settings_set_uint(settings, "WSC",
-						"DevicePasswordId", dpid);
-			l_settings_set_string(settings, "WSC",
-						"DevicePassword", pin);
-		}
+		l_settings_set_uint(settings, "WSC", "DevicePasswordId", dpid);
+		l_settings_set_string(settings, "WSC", "DevicePassword",
+					wsc->pin);
  	}
 
  	handshake_state_set_event_func(hs, wsc_handshake_event, wsc);

@@ -511,8 +516,38 @@ static void wsc_connect(struct wsc *wsc)
  	return;
  error:
  	handshake_state_free(hs);
-	dbus_pending_reply(&wsc->pending,
-				dbus_error_failed(wsc->pending));
+	wsc_connect_cleanup(wsc);
+	wsc->done_cb(r, NULL, 0, wsc->done_data);
+}
+
+/* Done callback for when WSC is triggered by DBus methods */
+static void wsc_dbus_done_cb(int err, struct wsc_credentials *creds,
+				unsigned int n_creds, void *user_data)
+{
+	struct wsc *wsc = user_data;
+
+	l_debug("err=%i", err);
+
+	if (err == -ECANCELED) {
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_aborted(wsc->pending));
+		return;
+	} else if (err == -ENOKEY) {
+		dbus_pending_reply(&wsc->pending,
+					wsc_error_no_credentials(wsc->pending));

src/wsc.c +36 -5

+
+	wsc_store_credentials(wsc);
+	wsc_try_credentials(wsc);
  }
 
  static void station_state_watch(enum station_state state, void *userdata)

@@ -541,6 +576,22 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
  	wsc->target = target;
  	station_set_autoconnect(wsc->station, false);
 
+	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
+		char *pin;
+
+		wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
+
+		if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
+			goto error;
+
+		wsc->pin = l_strdup(pin);
+	} else
+		wsc->config_method =
+			WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
+
+	wsc->done_cb = wsc_dbus_done_cb;
+	wsc->done_data = wsc;
+
  	switch (station_get_state(wsc->station)) {
  	case STATION_STATE_DISCONNECTED:
  		wsc_connect(wsc);

@@ -564,7 +615,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
  		break;
  	}
  error:
-	wsc->target = NULL;
+	wsc_connect_cleanup(wsc);
  	dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
  }
 

@@ -1074,12 +1125,12 @@ static void wsc_free(void *userdata)
  	struct wsc *wsc = userdata;
 
  	wsc_cancel_scan(wsc);
+	wsc_connect_cleanup(wsc);
 
  	if (wsc->station_state_watch) {
  		station_remove_state_watch(wsc->station,
  						wsc->station_state_watch);
  		wsc->station_state_watch = 0;
-		wsc->target = NULL;
  	}
 
  	if (wsc->pending)

@@ -1090,9 +1141,6 @@ static void wsc_free(void *userdata)
  		dbus_pending_reply(&wsc->pending_cancel,
  				dbus_error_aborted(wsc->pending_cancel));
 
-	if (wsc->eap_settings)
-		l_settings_free(wsc->eap_settings);
-
  	l_free(wsc);
  }
 

@@ -1139,6 +1187,37 @@ static void wsc_netdev_watch(struct netdev *netdev,
  	}
  }
 
+struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
+					char *pin, wsc_done_cb_t done_cb,
+					void *user_data)
+{
+	struct wsc *wsc;
+
+	wsc = l_new(struct wsc, 1);
+	wsc->netdev = netdev;
+	wsc->target = target;
+	wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
+		WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+	wsc->done_cb = done_cb;
+	wsc->done_data = user_data;
+	wsc->pin = l_strdup(pin);
+

Makefile.am src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \ · line 202

Quoted 5 lines · Andrew Zaborowski

                                      src/agent.h src/agent.c \
                                      src/storage.h src/storage.c \
                                      src/network.h src/network.c \

Makefile.am +1 -1

-                                     src/wsc.c \
+                                     src/wsc.h src/wsc.c \

Looks like you forgot to add wsc.h itself?

Quoted 25 lines

                                      src/backtrace.h src/backtrace.c \
                                      src/knownnetworks.h \
                                      src/knownnetworks.c \

src/wsc.c +1 -5

index d971797a..61683e7b 100644
@@ -44,6 +44,7 @@
  #include "src/storage.h"
  #include "src/iwd.h"
  #include "src/network.h"
+#include "src/wsc.h"
 
  #define WALK_TIME 120
 

@@ -60,18 +61,14 @@ struct wsc {
      uint32_t scan_id;
      struct scan_bss *target;
      uint32_t station_state_watch;
-     struct {
-             char ssid[33];
-             enum security security;
-             union {
-                     uint8_t psk[32];

src/wsc.c struct wsc { · line 64

Quoted 6 lines

src/wsc.c +1 -5

-                     char passphrase[64];
-             };
-             uint8_t addr[6];
-             bool has_passphrase;
-     } creds[3];
+     struct wsc_credentials creds[3];

This seems like it belongs in a separate patch

Quoted 19 lines

src/wsc.c +6 -1

      uint32_t n_creds;
      struct l_settings *eap_settings;
+     char *pin;
+
+     wsc_done_cb_t done_cb;
+     void *done_data;
+     uint16_t config_method;
 
      bool wsc_association : 1;
  };

@@ -160,15 +157,12 @@ static void wsc_try_credentials(struct wsc *wsc)
              l_dbus_message_unref(wsc->pending);
              wsc->pending = NULL;
 
-             goto done;
+             return;
      }
 

src/wsc.c static void wsc_try_credentials(struct wsc *wsc) · line 171

Quoted 6 lines

src/wsc.c +0 -3

      dbus_pending_reply(&wsc->pending,
                                      wsc_error_not_reachable(wsc->pending));
      station_set_autoconnect(wsc->station, true);
-done:
-     memset(wsc->creds, 0, sizeof(wsc->creds));
-     wsc->n_creds = 0;

This again looks like a separate issue and deserves a separate commit

Quoted 37 lines

src/wsc.c +17 -4

  }
 
  static void wsc_store_credentials(struct wsc *wsc)

@@ -224,6 +218,22 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
      station_set_autoconnect(wsc->station, true);
  }
 
+static void wsc_connect_cleanup(struct wsc *wsc)
+{
+     wsc->wsc_association = false;
+
+     l_settings_free(wsc->eap_settings);
+     wsc->eap_settings = NULL;
+
+     if (wsc->pin) {
+             explicit_bzero(wsc->pin, strlen(wsc->pin));
+             l_free(wsc->pin);
+             wsc->pin = NULL;
+     }
+
+     wsc->target = NULL;
+}
+
  static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
                                      void *event_data, void *user_data)
  {

@@ -231,33 +241,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 
      l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
 
-     wsc->wsc_association = false;
-
-     l_settings_free(wsc->eap_settings);
-     wsc->eap_settings = NULL;
+     wsc_connect_cleanup(wsc);
 

src/wsc.c static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result, · line 254

Quoted 11 lines

src/wsc.c +8 -2

      if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
-             wsc_store_credentials(wsc);
-             wsc_try_credentials(wsc);
+             struct wsc_credentials creds[L_ARRAY_SIZE(wsc->creds)];
+             uint32_t n_creds = wsc->n_creds;
+
+             memcpy(creds, wsc->creds, sizeof(creds));
+             explicit_bzero(wsc->creds, sizeof(creds));
+             wsc->n_creds = 0;
+             wsc->done_cb(0, creds, n_creds, wsc->done_data);
+             explicit_bzero(creds, sizeof(creds));

So we don't try credentials now?

Quoted 99 lines

src/wsc.c +39 -26

              return;
      }
 
      switch (result) {
      case NETDEV_RESULT_ABORTED:
-             dbus_pending_reply(&wsc->pending,
-                                     dbus_error_aborted(wsc->pending));
+             wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
              return;
      case NETDEV_RESULT_HANDSHAKE_FAILED:
-             dbus_pending_reply(&wsc->pending,
-                                     wsc_error_no_credentials(wsc->pending));
+             wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
              break;
      default:
-             dbus_pending_reply(&wsc->pending,
-                                     dbus_error_failed(wsc->pending));
+             wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
              break;
      }
 
-     station_set_autoconnect(wsc->station, true);
+     if (wsc->station)
+             station_set_autoconnect(wsc->station, true);
  }
 
  static void wsc_credential_obtained(struct wsc *wsc,

@@ -451,30 +462,24 @@ static void wsc_connect(struct wsc *wsc)
      l_settings_set_uint(settings, "WSC", "RFBand",
                                      freq_to_rf_band(bss->frequency));
      l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
-                             WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
-                             WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
-                             WSC_CONFIGURATION_METHOD_KEYPAD);
+                                     wsc->config_method);
      l_settings_set_string(settings, "WSC", "PrimaryDeviceType",
                                      "0-00000000-0");
      l_settings_set_string(settings, "WSC", "EnrolleeMAC",
              util_address_to_string(netdev_get_address(wsc->netdev)));
 
-     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
-             const char *pin;
-
-             if (l_dbus_message_get_arguments(wsc->pending, "s", &pin)) {
-                     enum wsc_device_password_id dpid;
+     if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+             enum wsc_device_password_id dpid;
 
-                     if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
-                             dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
-                     else
-                             dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+             if (strlen(wsc->pin) == 4 ||
+                             wsc_pin_is_checksum_valid(wsc->pin))
+                     dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+             else
+                     dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
 
-                     l_settings_set_uint(settings, "WSC",
-                                             "DevicePasswordId", dpid);
-                     l_settings_set_string(settings, "WSC",
-                                             "DevicePassword", pin);
-             }
+             l_settings_set_uint(settings, "WSC", "DevicePasswordId", dpid);
+             l_settings_set_string(settings, "WSC", "DevicePassword",
+                                     wsc->pin);
      }
 
      handshake_state_set_event_func(hs, wsc_handshake_event, wsc);

@@ -511,8 +516,38 @@ static void wsc_connect(struct wsc *wsc)
      return;
  error:
      handshake_state_free(hs);
-     dbus_pending_reply(&wsc->pending,
-                             dbus_error_failed(wsc->pending));
+     wsc_connect_cleanup(wsc);
+     wsc->done_cb(r, NULL, 0, wsc->done_data);
+}
+
+/* Done callback for when WSC is triggered by DBus methods */
+static void wsc_dbus_done_cb(int err, struct wsc_credentials *creds,
+                             unsigned int n_creds, void *user_data)
+{
+     struct wsc *wsc = user_data;
+
+     l_debug("err=%i", err);
+
+     if (err == -ECANCELED) {
+             dbus_pending_reply(&wsc->pending,
+                                     dbus_error_aborted(wsc->pending));
+             return;
+     } else if (err == -ENOKEY) {
+             dbus_pending_reply(&wsc->pending,
+                                     wsc_error_no_credentials(wsc->pending));
+             return;
+     } else if (err == -EBUSY) {
+             dbus_pending_reply(&wsc->pending,
+                                     dbus_error_busy(wsc->pending));

src/wsc.c static void wsc_connect(struct wsc *wsc) · line 547

Quoted 6 lines

src/wsc.c +6 -0

+             return;
+     } else if (err) {
+             dbus_pending_reply(&wsc->pending,
+                                     dbus_error_failed(wsc->pending));
+             return;
+     }

switch/case might look cleaner

Quoted 86 lines

src/wsc.c +40 -5

+
+     wsc_store_credentials(wsc);
+     wsc_try_credentials(wsc);
  }
 
  static void station_state_watch(enum station_state state, void *userdata)

@@ -541,6 +576,22 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
      wsc->target = target;
      station_set_autoconnect(wsc->station, false);
 
+     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
+             char *pin;
+
+             wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
+
+             if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
+                     goto error;
+
+             wsc->pin = l_strdup(pin);
+     } else
+             wsc->config_method =
+                     WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
+
+     wsc->done_cb = wsc_dbus_done_cb;
+     wsc->done_data = wsc;
+
      switch (station_get_state(wsc->station)) {
      case STATION_STATE_DISCONNECTED:
              wsc_connect(wsc);

@@ -564,7 +615,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
              break;
      }
  error:
-     wsc->target = NULL;
+     wsc_connect_cleanup(wsc);
      dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
  }
 

@@ -1074,12 +1125,12 @@ static void wsc_free(void *userdata)
      struct wsc *wsc = userdata;
 
      wsc_cancel_scan(wsc);
+     wsc_connect_cleanup(wsc);
 
      if (wsc->station_state_watch) {
              station_remove_state_watch(wsc->station,
                                              wsc->station_state_watch);
              wsc->station_state_watch = 0;
-             wsc->target = NULL;
      }
 
      if (wsc->pending)

@@ -1090,9 +1141,6 @@ static void wsc_free(void *userdata)
              dbus_pending_reply(&wsc->pending_cancel,
                              dbus_error_aborted(wsc->pending_cancel));
 
-     if (wsc->eap_settings)
-             l_settings_free(wsc->eap_settings);
-
      l_free(wsc);
  }
 

@@ -1139,6 +1187,37 @@ static void wsc_netdev_watch(struct netdev *netdev,
      }
  }
 
+struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
+                                     char *pin, wsc_done_cb_t done_cb,
+                                     void *user_data)
+{
+     struct wsc *wsc;
+
+     wsc = l_new(struct wsc, 1);
+     wsc->netdev = netdev;
+     wsc->target = target;
+     wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
+             WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+     wsc->done_cb = done_cb;
+     wsc->done_data = user_data;
+     wsc->pin = l_strdup(pin);
+
+     wsc_connect(wsc);
+
+     /*
+      * Wsc objects created this way are not handled in wsc_remove_interface,

src/wsc.c static void wsc_netdev_watch(struct netdev *netdev, · line 1214

Quoted 6 lines

src/wsc.c +6 -0

+      * the caller is expected to watch interfaces going away and there's no
+      * need to handle the event in both places.
+      */
+
+     return wsc;
+}

This again looks like it should be a separate commit

src/wsc.c static void wsc_netdev_watch(struct netdev *netdev, · line 1223

Quoted 11 lines

src/wsc.c +6 -0

+
+void wsc_destroy(struct wsc *wsc)
+{
+     wsc_free(wsc);
+}
+
  static int wsc_init(void)
  {
      l_debug("");

Regards,
-Denis

From: Andrew Zaborowski <[email protected]>

Add a function to retrieve the maximum Remain On Channel listen duration supported by the wiphy's driver.

---
  src/wiphy.c | 12 ++++++++++++
  src/wiphy.h |  1 +
  2 files changed, 13 insertions(+)

From: Andrew Zaborowski <[email protected]>

Allow watching for frames on interfaces that have no netdev in the kernel and can only be referenced through the wdev_id instead of the ifindex.

---
  src/netdev.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-----
  src/netdev.h |  6 ++++
  2 files changed, 84 insertions(+), 8 deletions(-)

src/netdev.h +0 -0

index 114a6035..624811b2 100644

Quoted 5 lines · Andrew Zaborowski

src/netdev.h +0 -0

index 114a6035..624811b2 100644

src/netdev.h uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type, · line 205

Quoted 9 lines

src/netdev.h +6 -0

@@ -197,6 +197,12 @@ uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type,
                              void *user_data);
  bool netdev_frame_watch_remove(struct netdev *netdev, uint32_t id);
 
+uint32_t netdev_wdev_frame_watch_add(uint64_t wdev_id, uint16_t frame_type,
+                             const uint8_t *prefix, size_t prefix_len,
+                             netdev_frame_watch_func_t handler,
+                             void *user_data);
+bool netdev_wdev_frame_watch_remove(uint32_t id);
+

So this really seems like it is jammed in there and it doesn't fit well.

I think that we should make frame watches into a separate module now that they no longer are only associated with a netdev.

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

We could also implement 'transient' frame watch sets by using a genl socket dedicated to each set. At least until the kernel people fix their subsystem.

Quoted 18 lines · Denis Kenzior

Quoted 5 lines · Andrew Zaborowski

src/netdev.h +0 -0

index 114a6035..624811b2 100644

src/netdev.h uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type, · line 205

Quoted 9 lines

src/netdev.h +6 -0

@@ -197,6 +197,12 @@ uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type,
                               void *user_data);
   bool netdev_frame_watch_remove(struct netdev *netdev, uint32_t id);
 
+uint32_t netdev_wdev_frame_watch_add(uint64_t wdev_id, uint16_t frame_type,
+                             const uint8_t *prefix, size_t prefix_len,
+                             netdev_frame_watch_func_t handler,
+                             void *user_data);
+bool netdev_wdev_frame_watch_remove(uint32_t id);
+

So this really seems like it is jammed in there and it doesn't fit well.

Quoted 2 lines

I think that we should make frame watches into a separate module now that they no longer are only associated with a netdev.

Ok, I only put it inside netdev as an extension of the current api.

Quoted 6 lines

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

The problem is how we will know what logic is implemented in the running kernel version. We also watch iftype changes inside netdev so

we can more comfortably do it here but I'm not sure it helps us. In P2P I'm registering and unregistering the watches for specific operations and we never switch iftypes. I think we should either have

the kernel have a simple frame unregister command or keep all of the

watches for simplicity.

Quoted 5 lines

We could also implement 'transient' frame watch sets by using a genl socket dedicated to each set. At least until the kernel people fix their subsystem.

This sounds like it's much more costly in terms of syscalls and kernel work (and code) than occasionally getting the EAGAIN for a register command.

Best regards

Quoted 7 lines · Andrew Zaborowski

Quoted 5 lines · Denis Kenzior

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

Quoted 2 lines

The problem is how we will know what logic is implemented in the running kernel version. We also watch iftype changes inside netdev so

Well, the particular fix from me got backported into every single LTS kernel. So we just assume the fix is included. Unless you want to go full wpa_s and use a separate socket for each frame watch.

Quoted 4 lines

we can more comfortably do it here but I'm not sure it helps us. In P2P I'm registering and unregistering the watches for specific operations and we never switch iftypes. I think we should either have

Yeah, I think you have to bite the bullet and implement a separate socket for these.

Quoted 2 lines

watches for simplicity.

I don't think this is a good idea. It will cause unnecessary wakeups and round trips into the kernel. And any one of these is one too many.

Quoted 10 lines

Quoted 6 lines

We could also implement 'transient' frame watch sets by using a genl socket dedicated to each set. At least until the kernel people fix their subsystem.

This sounds like it's much more costly in terms of syscalls and kernel work (and code) than occasionally getting the EAGAIN for a register command.

If you assume that frame watches are purged on iftype change, then most of the EAGAINs are only caused by netdev ifup/ifdown. So those we can take care of easily.

Unnecessary wakeups should be avoided completely. So if you have p2p frame watches and you can group them into a set to enable / disable, then they should be registered for on a separate socket.

Quoted 11 lines · Denis Kenzior

Quoted 9 lines · Andrew Zaborowski

Quoted 5 lines · Denis Kenzior

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

The problem is how we will know what logic is implemented in the running kernel version. We also watch iftype changes inside netdev so

Quoted 3 lines

Well, the particular fix from me got backported into every single LTS kernel. So we just assume the fix is included. Unless you want to go full wpa_s and use a separate socket for each frame watch.

Ok, so we add a framewatch.c or similar, it'll keep a list of all registered frame types+prefixes even after our local users have unregistered from them. It will listen for SET_INTERFACE events and drop the watches matching the wdev_id from the list.

Quoted 5 lines

Quoted 4 lines

we can more comfortably do it here but I'm not sure it helps us. In P2P I'm registering and unregistering the watches for specific operations and we never switch iftypes. I think we should either have

Quoted 2 lines

Yeah, I think you have to bite the bullet and implement a separate socket for these.

Ok, then we also open a new genl socket for some frame watches, should we do this depending on the iftype? I.e. for any iftype other than managed, ad-hoc and AP?

Quoted 5 lines

Quoted 2 lines

watches for simplicity.

I don't think this is a good idea. It will cause unnecessary wakeups and round trips into the kernel. And any one of these is one too many.

Right but opening and closing sockets also causes a lot of round trips into the kernel so I'm not clear that we're making progress by doing this.

Quoted 15 lines

Quoted 10 lines

Quoted 6 lines

We could also implement 'transient' frame watch sets by using a genl socket dedicated to each set. At least until the kernel people fix their subsystem.

This sounds like it's much more costly in terms of syscalls and kernel work (and code) than occasionally getting the EAGAIN for a register command.

If you assume that frame watches are purged on iftype change, then most of the EAGAINs are only caused by netdev ifup/ifdown. So those we can take care of easily.

Quoted 3 lines

Unnecessary wakeups should be avoided completely. So if you have p2p frame watches and you can group them into a set to enable / disable, then they should be registered for on a separate socket.

The kernel api under our framewatch API may change again (and hopefully will) so I'm thinking it would be good to not clutter our

API very visibly with those details. Maybe a simple group_id integer parameter when registering a new frame watch would be enough to do the grouping. The users of the API can treat it as informational, or a "suggestion" on how to group those frame watches (if needed at all). Underneath the API would open one socket for each group and would automatically close the socket only if all of the watches from that group have been removed, so that even if the grouping is wrong or unnecessary, the user sees exactly the same results.

For the managed, ad-hoc and AP interface types the user would still be expected to unregister their watches when cleaning up, even though they would be NOPs since those would be purged by the iftype change.

Since we know that we can't unregister from frames at the kernel, if the user adds a new prefix that matches a shorter existing prefix we

1 per device for example and closes/re-opens these when requested to

can skip the registration, like we do in netdev. If the user adds a watch for a shorter prefix then we register that one with the kernel and just let both live in the kernel until an iftype switch or until

the socket closes. I guess it's not worth the effort to consider matching prefixes in other "groups" on the same interface because we won't have many groups.

Best regards

Quoted 12 lines · Andrew Zaborowski

Quoted 11 lines · Denis Kenzior

Quoted 9 lines · Andrew Zaborowski

Quoted 5 lines · Denis Kenzior

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

The problem is how we will know what logic is implemented in the running kernel version. We also watch iftype changes inside netdev so

Quoted 9 lines

Quoted 3 lines

Well, the particular fix from me got backported into every single LTS kernel. So we just assume the fix is included. Unless you want to go full wpa_s and use a separate socket for each frame watch.

Ok, so we add a framewatch.c or similar, it'll keep a list of all registered frame types+prefixes even after our local users have unregistered from them. It will listen for SET_INTERFACE events and drop the watches matching the wdev_id from the list.

Yes, something like this. One thing we might want to do is simply have station, adhoc, ap, etc modules simply tell framewatch which events they care about. And framewatch will register to these whenever iftype changes.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

we can more comfortably do it here but I'm not sure it helps us. In P2P I'm registering and unregistering the watches for specific operations and we never switch iftypes. I think we should either have

Quoted 7 lines

Quoted 2 lines

Yeah, I think you have to bite the bullet and implement a separate socket for these.

Ok, then we also open a new genl socket for some frame watches, should we do this depending on the iftype? I.e. for any iftype other than managed, ad-hoc and AP?

No, I think it should be an explicit api. E.g. frame_watch_set_new, frame_watch_set_add, frame_watch_add_set();

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

watches for simplicity.

I don't think this is a good idea. It will cause unnecessary wakeups and round trips into the kernel. And any one of these is one too many.

Right but opening and closing sockets also causes a lot of round trips into the kernel so I'm not clear that we're making progress by doing this.

Tell this to the kernel people ;)

In the case of P2P these would only be relevant when you initiate P2P discovery. And closing the socket when your discovery finishes. However, stray beacons, probe requests, responses, etc can wake us up for quite some time afterwards. You're also wasting battery time having the hardware wakeup the kernel for these since frequently they're filtered at the hardware when no frame watch is registered. So overall I'd say opening a socket is still much less expensive.

Quoted 16 lines

Quoted 16 lines

Quoted 11 lines

Quoted 7 lines

We could also implement 'transient' frame watch sets by using a genl socket dedicated to each set. At least until the kernel people fix their subsystem.

This sounds like it's much more costly in terms of syscalls and kernel work (and code) than occasionally getting the EAGAIN for a register command.

If you assume that frame watches are purged on iftype change, then most of the EAGAINs are only caused by netdev ifup/ifdown. So those we can take care of easily.

Quoted 6 lines

Quoted 3 lines

Unnecessary wakeups should be avoided completely. So if you have p2p frame watches and you can group them into a set to enable / disable, then they should be registered for on a separate socket.

The kernel api under our framewatch API may change again (and hopefully will) so I'm thinking it would be good to not clutter our

I doubt it at this point, but if we can make our API be able to take advantage of future unregister functionality (if available), then that would be nice. I wouldn't focus on that at this point though.

Quoted 8 lines

API very visibly with those details. Maybe a simple group_id integer parameter when registering a new frame watch would be enough to do the grouping. The users of the API can treat it as informational, or a "suggestion" on how to group those frame watches (if needed at all). Underneath the API would open one socket for each group and would automatically close the socket only if all of the watches from that group have been removed, so that even if the grouping is wrong or unnecessary, the user sees exactly the same results.

Sounds reasonable.

Hi Denis,

Quoted 23 lines · Denis Kenzior

Quoted 12 lines · Andrew Zaborowski

Quoted 11 lines · Denis Kenzior

Quoted 9 lines · Andrew Zaborowski

Quoted 5 lines · Denis Kenzior

This would allow
us to also track iftype changes and wipe out those frame watches that are no longer registered with the kernel. And also allow us not to try and register frame watches that are already live (and bounce with an -EAGAIN from the kernel).

The problem is how we will know what logic is implemented in the running kernel version. We also watch iftype changes inside netdev so

Quoted 9 lines

Quoted 3 lines

Well, the particular fix from me got backported into every single LTS kernel. So we just assume the fix is included. Unless you want to go full wpa_s and use a separate socket for each frame watch.

Ok, so we add a framewatch.c or similar, it'll keep a list of all registered frame types+prefixes even after our local users have unregistered from them. It will listen for SET_INTERFACE events and drop the watches matching the wdev_id from the list.

Quoted 3 lines

Yes, something like this. One thing we might want to do is simply have station, adhoc, ap, etc modules simply tell framewatch which events they care about. And framewatch will register to these whenever iftype changes.

I took a look at netdev and ap.c and we don't really have frames that we need to be unconditionally listening to. In ap.c we only register the watches when starting the AP and in netdev some of the watches should be registered for either when we connect, when we send a Neighbor Report Request or SA Query Request. So in the end it looks like we'll be using the separate socket per set/group everywhere.

Even if we had frames that we cared about for the whole time a specific iftype is active, we'd still need the netdev/adhoc/ap modules to update the user_data for the callbacks everytime we switch to that iftype so it doesn't seem useful to automatically register to any frames.

So I take it you're fine with the whole group/set concept being limited to the group_id (or set_id) parameter, in the user's view, instead of the user having to issue a separate call to create or destroy a "set". So I wouldn't add calls like frame_watch_set_new().

Quoted 6 lines · Andrew Zaborowski

I took a look at netdev and ap.c and we don't really have frames that we need to be unconditionally listening to. In ap.c we only register the watches when starting the AP and in netdev some of the watches should be registered for either when we connect, when we send a Neighbor Report Request or SA Query Request. So in the end it looks like we'll be using the separate socket per set/group everywhere.

Perhaps you're right. Might make sense from a consistency point of view anyway. So we only implement one way and be done with it. Do note:
there are frames which are protected, (SA Query family for example). So the kernel would simply never forward them to us if the management encryption key is not setup. These do not need to be part of a socket set... There may be others that fit this pattern. So maybe we can cheat for at least some of these...

For AP, you know more than I. I imagine that AP only needs a single set for the most part? Can we even receive frames if AP hasn't been started yet?

Quoted 7 lines

Even if we had frames that we cared about for the whole time a specific iftype is active, we'd still need the netdev/adhoc/ap modules to update the user_data for the callbacks everytime we switch to that iftype so it doesn't seem useful to automatically register to any frames.

Depends... It can always fire off a callback with just the wdev/ifindex and let the station/adhoc/ap module figure it out. Yes we'd need to run a find by ifindex/wdev, but that's not really a big deal in the common case.

Quoted 5 lines

So I take it you're fine with the whole group/set concept being limited to the group_id (or set_id) parameter, in the user's view, instead of the user having to issue a separate call to create or destroy a "set". So I wouldn't add calls like frame_watch_set_new().

What I'm a bit worried about is that I'm not sure you're really winning anything by trying to hide the set behind an id. You probably end up having to look up the 'set' object every time based on the id, which is sort of wasteful.

Also, how would one pick IDs anyway? You can't or shouldn't share the id between different wdevs/netdevs anyway since their lifetimes are
distinct. So you end up trying to have these unique gids which might
as well be proper objects.

Also, the set is likely to be setup once and
destroyed when the interface is cleaned up (e.g. on iftype change or ifdown).

Quoted 8 lines · Denis Kenzior

Quoted 6 lines · Andrew Zaborowski

I took a look at netdev and ap.c and we don't really have frames that we need to be unconditionally listening to. In ap.c we only register the watches when starting the AP and in netdev some of the watches should be registered for either when we connect, when we send a Neighbor Report Request or SA Query Request. So in the end it looks like we'll be using the separate socket per set/group everywhere.

Quoted 7 lines

Perhaps you're right. Might make sense from a consistency point of view anyway. So we only implement one way and be done with it. Do note:
there are frames which are protected, (SA Query family for example). So the kernel would simply never forward them to us if the management encryption key is not setup. These do not need to be part of a socket set... There may be others that fit this pattern. So maybe we can cheat for at least some of these...

Good point for the SA Query Request but for SA Query Responses we can still use a separate group because we only want to be woken up if we've sent the Request before.

Quoted 5 lines

What I'm a bit worried about is that I'm not sure you're really winning anything by trying to hide the set behind an id. You probably end up having to look up the 'set' object every time based on the id, which is sort of wasteful.

Hmmm, I think we would only need a single look up when adding a new frame registration. We would have the groups in an l_queue for each wdev (if we expect there to be few groups, or a hashmap if we expect many)

Quoted 6 lines

Also, how would one pick IDs anyway? You can't or shouldn't share the id between different wdevs/netdevs anyway since their lifetimes are
distinct. So you end up trying to have these unique gids which might
as well be proper objects.

I was thinking of the group_id being an additional argument and shared between wdevs/netdevs because the wdev would still be passed in the arguments.

So netdev.c would have one group per watch lifetime:

enum {
FRAME_GROUP_CONNECTION,
FRAME_GROUP_NR,
FRAME_GROUP_SA_QUERY,
};

Quoted 3 lines

Also, the set is likely to be setup once and
destroyed when the interface is cleaned up (e.g. on iftype change or ifdown).

Yes but those are three additional pointers to store, for the three groups in netdev, also three additional create calls.

Hi Andrew,

Quoted 9 lines · Andrew Zaborowski

Quoted 7 lines · Denis Kenzior

Perhaps you're right. Might make sense from a consistency point of view anyway. So we only implement one way and be done with it. Do note:
there are frames which are protected, (SA Query family for example). So the kernel would simply never forward them to us if the management encryption key is not setup. These do not need to be part of a socket set... There may be others that fit this pattern. So maybe we can cheat for at least some of these...

Quoted 3 lines

Good point for the SA Query Request but for SA Query Responses we can still use a separate group because we only want to be woken up if we've sent the Request before.

But do you want to pay the cost? Since we established an encryption key to the AP, and a well behaved AP will send these to us only as a response, I don't see the point.

The kernel API is not ideal. Lets make it work, but don't go crazy ;)

Quoted 10 lines

Quoted 5 lines

What I'm a bit worried about is that I'm not sure you're really winning anything by trying to hide the set behind an id. You probably end up having to look up the 'set' object every time based on the id, which is sort of wasteful.

Hmmm, I think we would only need a single look up when adding a new frame registration. We would have the groups in an l_queue for each wdev (if we expect there to be few groups, or a hashmap if we expect many)

Exactly. So you do something like:

frame_watch_add(..);
frame_watch_add(..);

if (ap_has_ie())

        frame_watch_add(..);

if (ap_has_another_ie())

        frame_watch_add(..);

For each call you're looking up the group. Instead of:

frame_watch_set_new();
frame_watch_set_add(..);
frame_watch_set_add(..);

if (ap_has_ie())

        frame_watch_set_add();

frame_watch_add_set();

Something like this anyway...

Quoted 11 lines

Quoted 6 lines

Also, how would one pick IDs anyway? You can't or shouldn't share the id between different wdevs/netdevs anyway since their lifetimes are
distinct. So you end up trying to have these unique gids which might
as well be proper objects.

I was thinking of the group_id being an additional argument and shared between wdevs/netdevs because the wdev would still be passed in the arguments.

Quoted 8 lines

So netdev.c would have one group per watch lifetime:

enum {
FRAME_GROUP_CONNECTION,
FRAME_GROUP_NR,
FRAME_GROUP_SA_QUERY,
};

I think this will break down once we start supporting more action frames. These will depend on the presence of IEs in the AP, and I would expect the number of combinations will quickly overwhelm such an approach.

Quoted 8 lines

Quoted 4 lines

Also, the set is likely to be setup once and
destroyed when the interface is cleaned up (e.g. on iftype change or ifdown).

Yes but those are three additional pointers to store, for the three groups in netdev, also three additional create calls.

Not sure I understand this last point?

Hi Denis,

Quoted 17 lines · Denis Kenzior

Hi Andrew,

Quoted 12 lines · Andrew Zaborowski

Quoted 7 lines · Denis Kenzior

Perhaps you're right. Might make sense from a consistency point of view anyway. So we only implement one way and be done with it. Do note:
there are frames which are protected, (SA Query family for example). So the kernel would simply never forward them to us if the management encryption key is not setup. These do not need to be part of a socket set... There may be others that fit this pattern. So maybe we can cheat for at least some of these...

Good point for the SA Query Request but for SA Query Responses we can still use a separate group because we only want to be woken up if we've sent the Request before.

Quoted 3 lines

But do you want to pay the cost? Since we established an encryption key to the AP, and a well behaved AP will send these to us only as a response, I don't see the point.

Ok, maybe we can save that one group then, but depending on how the tasks are divided between the firmware and the driver, the kernel may still be getting woken up on spoofed frames if someone registered for them.

Quoted 58 lines

The kernel API is not ideal. Lets make it work, but don't go crazy ;)

Quoted 10 lines

Quoted 5 lines

What I'm a bit worried about is that I'm not sure you're really winning anything by trying to hide the set behind an id. You probably end up having to look up the 'set' object every time based on the id, which is sort of wasteful.

Hmmm, I think we would only need a single look up when adding a new frame registration. We would have the groups in an l_queue for each wdev (if we expect there to be few groups, or a hashmap if we expect many)

Exactly. So you do something like:

frame_watch_add(..);
frame_watch_add(..);

if (ap_has_ie())

         frame_watch_add(..);

if (ap_has_another_ie())

         frame_watch_add(..);

For each call you're looking up the group. Instead of:

frame_watch_set_new();
frame_watch_set_add(..);
frame_watch_set_add(..);

if (ap_has_ie())

         frame_watch_set_add();

frame_watch_add_set();

Something like this anyway...

Quoted 19 lines

Quoted 6 lines

Also, how would one pick IDs anyway? You can't or shouldn't share the id between different wdevs/netdevs anyway since their lifetimes are
distinct. So you end up trying to have these unique gids which might
as well be proper objects.

I was thinking of the group_id being an additional argument and shared between wdevs/netdevs because the wdev would still be passed in the arguments.

So netdev.c would have one group per watch lifetime:

enum {
FRAME_GROUP_CONNECTION,
FRAME_GROUP_NR,
FRAME_GROUP_SA_QUERY,
};

Quoted 3 lines

I think this will break down once we start supporting more action frames. These will depend on the presence of IEs in the AP, and I would expect the number of combinations will quickly overwhelm such an approach.

We don't need separate groups for frames that we are interested in for the entire time we're connected, just because they depend on some IE so I don't see why this would be a problem.

Quoted 10 lines

Quoted 8 lines

Quoted 4 lines

Also, the set is likely to be setup once and
destroyed when the interface is cleaned up (e.g. on iftype change or ifdown).

Yes but those are three additional pointers to store, for the three groups in netdev, also three additional create calls.

Not sure I understand this last point?

So in the integer group id approach we have this in netdev:

enum {
FRAME_GROUP_XXX,
FRAME_GROUP_YYY,
FRAME_GROUP_ZZZ,
};

frame_watch_add(netdev, FRAME_GROUP_XXX, ...);
frame_watch_add(netdev, FRAME_GROUP_XXX, ...);

if (ap_has_ie())

        frame_watch_add(netdev, FRAME_GROUP_XXX, ...);

frame_watch_add(netdev, FRAME_GROUP_YYY, ...);

In your approach we will have

struct netdev {
        ...
        struct frame_watch_group *group_xxx;
        struct frame_watch_group *group_yyy;
        struct frame_watch_group *group_zzz;

        ...
};

netdev->group_xxx = frame_watch_set_new(netdev, ...); netdev->group_yyy = frame_watch_set_new(netdev, ...); netdev->group_zzz = frame_watch_set_new(netdev, ...);

frame_watch_set_add(netdev->group_xxx, ...);
frame_watch_set_add(netdev->group_xxx, ...);

if (ap_has_ie())

        frame_watch_set_add(netdev->group_xxx, ...);

frame_watch_set_add(netdev->group_yyy, ...);

frame_watch_add_set(netdev->group_xxx);
frame_watch_add_set(netdev->group_yyy);
frame_watch_add_set(netdev->group_zzz);

Or similar... you can have those groups be global but in any case that's gonna be 6 extra calls for nothing, assuming 3 groups/sets in netdev. With possible error checking it adds up to a lot of burden.

Quoted 22 lines · Andrew Zaborowski

Quoted 20 lines · Denis Kenzior

Hi Andrew,

Quoted 9 lines · Andrew Zaborowski

Quoted 7 lines · Denis Kenzior

Perhaps you're right. Might make sense from a consistency point of view anyway. So we only implement one way and be done with it. Do note:
there are frames which are protected, (SA Query family for example). So the kernel would simply never forward them to us if the management encryption key is not setup. These do not need to be part of a socket set... There may be others that fit this pattern. So maybe we can cheat for at least some of these...

Quoted 3 lines

Good point for the SA Query Request but for SA Query Responses we can still use a separate group because we only want to be woken up if we've sent the Request before.

But do you want to pay the cost? Since we established an encryption key to the AP, and a well behaved AP will send these to us only as a response, I don't see the point.

Quoted 5 lines

Ok, maybe we can save that one group then, but depending on how the tasks are divided between the firmware and the driver, the kernel may still be getting woken up on spoofed frames if someone registered for them.

This happens already. Look at the mac80211 rx path. I only really care

about saving this for beacons, probe requests and probe responses since there is an insane amount of these and the hardware can usually filter them.

Quoted 63 lines

Quoted 62 lines

The kernel API is not ideal. Lets make it work, but don't go crazy ;)

Quoted 10 lines

Quoted 5 lines

What I'm a bit worried about is that I'm not sure you're really winning anything by trying to hide the set behind an id. You probably end up having to look up the 'set' object every time based on the id, which is sort of wasteful.

Hmmm, I think we would only need a single look up when adding a new frame registration. We would have the groups in an l_queue for each wdev (if we expect there to be few groups, or a hashmap if we expect many)

Exactly. So you do something like:

frame_watch_add(..);
frame_watch_add(..);

if (ap_has_ie())

         frame_watch_add(..);

if (ap_has_another_ie())

         frame_watch_add(..);

For each call you're looking up the group. Instead of:

frame_watch_set_new();
frame_watch_set_add(..);
frame_watch_set_add(..);

if (ap_has_ie())

         frame_watch_set_add();

frame_watch_add_set();

Something like this anyway...

Quoted 11 lines

Quoted 6 lines

Also, how would one pick IDs anyway? You can't or shouldn't share the id between different wdevs/netdevs anyway since their lifetimes are
distinct. So you end up trying to have these unique gids which might
as well be proper objects.

I was thinking of the group_id being an additional argument and shared between wdevs/netdevs because the wdev would still be passed in the arguments.

Quoted 8 lines

So netdev.c would have one group per watch lifetime:

enum {
FRAME_GROUP_CONNECTION,
FRAME_GROUP_NR,
FRAME_GROUP_SA_QUERY,
};

I think this will break down once we start supporting more action frames. These will depend on the presence of IEs in the AP, and I would expect the number of combinations will quickly overwhelm such an approach.

Quoted 4 lines

We don't need separate groups for frames that we are interested in for the entire time we're connected, just because they depend on some IE so I don't see why this would be a problem.

You already have 3 groups / netdev above where I'd just have one / netdev.

Quoted 30 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

Also, the set is likely to be setup once and
destroyed when the interface is cleaned up (e.g. on iftype change or ifdown).

Yes but those are three additional pointers to store, for the three groups in netdev, also three additional create calls.

Not sure I understand this last point?

So in the integer group id approach we have this in netdev:

enum {
FRAME_GROUP_XXX,
FRAME_GROUP_YYY,
FRAME_GROUP_ZZZ,
};

frame_watch_add(netdev, FRAME_GROUP_XXX, ...);
frame_watch_add(netdev, FRAME_GROUP_XXX, ...);

if (ap_has_ie())

        frame_watch_add(netdev, FRAME_GROUP_XXX, ...);

frame_watch_add(netdev, FRAME_GROUP_YYY, ...);

In your approach we will have

Quoted 5 lines

struct netdev {
        ...
        struct frame_watch_group *group_xxx;
        struct frame_watch_group *group_yyy;
        struct frame_watch_group *group_zzz;

No, more like a single watch id:

        uint32_t frame_watch;

Which is a result of us adding the entire set to framewatch.c, associated with the set of groups this netdev connection needs to handle.

And maybe in the weird p2p cases you'd have two frame watches or something.

Quoted 19 lines

        ...
};

netdev->group_xxx = frame_watch_set_new(netdev, ...); netdev->group_yyy = frame_watch_set_new(netdev, ...); netdev->group_zzz = frame_watch_set_new(netdev, ...);

frame_watch_set_add(netdev->group_xxx, ...);
frame_watch_set_add(netdev->group_xxx, ...);

if (ap_has_ie())

        frame_watch_set_add(netdev->group_xxx, ...);

frame_watch_set_add(netdev->group_yyy, ...);

frame_watch_add_set(netdev->group_xxx);
frame_watch_add_set(netdev->group_yyy);
frame_watch_add_set(netdev->group_zzz);

Quoted 3 lines

Or similar... you can have those groups be global but in any case that's gonna be 6 extra calls for nothing, assuming 3 groups/sets in netdev. With possible error checking it adds up to a lot of burden.

We can't have them be global since they're per wdev/netdev. I'm probably missing something?

Anyway, my point here is that submitting full sets by setting some attribute on the set object directly is still way less expensive compared to multiple l_queue_find / l_hashmap_find calls that you're proposing.

You might want to create a concrete scenario with say 3 devices and explain how many sockets we get to open. What I'm saying is that we should be doing 1 socket / wdev/netdev with an active connection. Maybe more in special circumstances, but they should be pretty special.

Quoted 2 lines

about saving this for beacons, probe requests and probe responses since there is an insane amount of these and the hardware can usually filter them.

Ok, you'd said "any one of these is one too many" so I assumed it was your priority to avoid any unneeded wakeups.

In any case the frame registration never happens in hot paths so the overhead is going to be theoretical, but I think we should internally

come up with an API where the users don't need to care what the implementation looks like... by doing the sets/groups thing we're already pushing some of the complication that the kernel is creating for us, into the users of the api.

Quoted 3 lines

No, more like a single watch id:

         uint32_t frame_watch;

Ok, can you give a more complete code example then? Because I had understood you wanted to avoid those lookups by group id by using pointers, but now we're back to ints.

Quoted 3 lines

We can't have them be global since they're per wdev/netdev. I'm probably missing something?

Well you could do:

group = frame_watch_set_new();

frame_watch_set_add(group, ...);
frame_watch_set_add(group, ...);

frame_watch_register_set(netdev, group, user_data);

or similar, in this case only the last of those calls would be done per netdev... sort of similar to how you're creating dbus interfaces and only use one call per actual dbus object to activate that interface.

Quoted 5 lines

Anyway, my point here is that submitting full sets by setting some attribute on the set object directly is still way less expensive compared to multiple l_queue_find / l_hashmap_find calls that you're proposing.

I will need a more complete example of the api you're proposing because I can't see how in my case I'd be adding more lookups... and in any case having usually only about 2-3 groups of frames, that look up boils down to two integer comparisons when registering a new frame prefix.

No lookup would be necessary when the frame is received.

Quoted 5 lines

You might want to create a concrete scenario with say 3 devices and explain how many sockets we get to open. What I'm saying is that we should be doing 1 socket / wdev/netdev with an active connection. Maybe more in special circumstances, but they should be pretty special.

Right, we may just need 1 or 2 sockets in station mode, the three groups was just an example. But let's assume a scenario where we only listen for one specific frame type. It seems like a lot of code overhead and poor design to require three method calls to register that one frame prefix (set_new, set_add and add_set, vs. just register_frame)

Best regards

Quoted 2 lines

In any case the frame registration never happens in hot paths so the overhead is going to be theoretical, but I think we should internally

How do you figure? You're the one arguing that frame registrations should only be done when connected. So when exactly do you want to register the frames then? Just prior to connection? Just after connection? Still seems like a hot path to me.

Quoted 5 lines

come up with an API where the users don't need to care what the implementation looks like... by doing the sets/groups thing we're already pushing some of the complication that the kernel is creating for us, into the users of the api.

iwd is close to the metal, so I don't really consider such a goal to be a primary one.

Quoted 8 lines

Quoted 3 lines

No, more like a single watch id:

         uint32_t frame_watch;

Ok, can you give a more complete code example then? Because I had understood you wanted to avoid those lookups by group id by using pointers, but now we're back to ints.

It depends on how we want to design the frame watch set.

Case 1: It is a standalone class, sort of like genl, that just opens a socket on frame_watch_set_new() and closes it on frame_watch_set_destroy(). Then any frame_watch_set_add() would just register for a frame watch on a given wdev. The caller is responsible for lifetime management. Since you didn't think that paying attention to set_interface events was worthwhile, it doesn't need to know about any of those details.

Case 2: the set is managed by framewatch module. In which case the parameters are stored in the frame_watch_set, then passed to framewatch module which performs the necessary registrations. uint32 is returned as a handle to destroy / cancel the set when the caller determines it should be. Case 2 assumes that some frame watches can be handled as they are today, and we can take advantage of paying attention to set_interface events, etc.

Quoted 5 lines

Quoted 4 lines

We can't have them be global since they're per wdev/netdev. I'm probably missing something?

Quoted 7 lines

Well you could do:

group = frame_watch_set_new();

frame_watch_set_add(group, ...);
frame_watch_set_add(group, ...);

So if I understand correctly your groups are semi static. And then you have netdevs register to a set of groups. Where each group instance is per netdev. But again, I think this runs into combinatorial problem I mentioned earlier where each optional IE results in a different group being required.

Quoted 1 line

frame_watch_register_set(netdev, group, user_data);

Well this is not what you suggested earlier with an enumeration. So now

you're trying to optimize away the l_queue/l_hashmap lookup by making the group global. I doubt you get this to work without using an actual global variable, but okay, I'll admit this might negate my speed argument.

However, each frame_watch_register_set results in a socket being created. So for 3 groups you get 3 sockets. I doubt this is what we want.

Quoted 6 lines

or similar, in this case only the last of those calls would be done per netdev... sort of similar to how you're creating dbus interfaces and only use one call per actual dbus object to activate that interface.

I rather just have one socket / wdev and the wdev decides at connection time or shortly after what it really cares about watching. So yes, we need to minimize kernel wakeups as much as possible, but lets not go overboard creating a socket for each frame type & wdev pair here. We need to find a balance that makes sense.

Quoted 21 lines

Quoted 6 lines

Anyway, my point here is that submitting full sets by setting some attribute on the set object directly is still way less expensive compared to multiple l_queue_find / l_hashmap_find calls that you're proposing.

I will need a more complete example of the api you're proposing because I can't see how in my case I'd be adding more lookups... and in any case having usually only about 2-3 groups of frames, that look up boils down to two integer comparisons when registering a new frame prefix.

No lookup would be necessary when the frame is received.

Quoted 5 lines

You might want to create a concrete scenario with say 3 devices and explain how many sockets we get to open. What I'm saying is that we should be doing 1 socket / wdev/netdev with an active connection. Maybe more in special circumstances, but they should be pretty special.

Quoted 6 lines

Right, we may just need 1 or 2 sockets in station mode, the three groups was just an example. But let's assume a scenario where we only listen for one specific frame type. It seems like a lot of code overhead and poor design to require three method calls to register that one frame prefix (set_new, set_add and add_set, vs. just register_frame)

No, I think you misunderstood. It literally would be one set that holds all frames for the current connection.

i.e.

set = frame_set_new(..);

if (rrm_enabled)

        frame_set_watch(set, NeighborReport);

if (mfp_enabled)

        frame_set_watch(set, SA Query Request);
        frame_set_watch(set, SA Query Response);

if (using_ft)

        frame_set_watch(set, FT Response);

By hot path I mean one that may be executed many times a second, or as a dependency for every other operation (like malloc)... if something is done once per AP connection then I wouldn't call that a hot path and would try to optimize for nice code rather than shaving off a single l_queue_find.

Quoted 6 lines

I rather just have one socket / wdev and the wdev decides at connection time or shortly after what it really cares about watching. So yes, we need to minimize kernel wakeups as much as possible, but lets not go overboard creating a socket for each frame type & wdev pair here. We need to find a balance that makes sense.

Right, so that's a question for each module to resolve (netdev/station, ap, adhoc, p2p). But for framewatch, each new group is going to result in a new genl socket, right? And the users just need to be careful to use few groups.

Honestly I'd rather not have the modules bother considering how expensive are those groups. For example if we had a NL80211_CMD_UNREGISTER_FRAME similar to REGISTER_FRAME, we would be less worried about those details I imagine. And this shouldn't leak

into modules outside the framewatch module. For example currently we never unregister frames and our netdev_frame_watch_remove is a no-op at the syscall level so we can use it as often as we want. Maybe we

should be doing something similar inside the framewatch module: keep some registrations active in the kernel but ignore the frames if no callback is currently registered for a given prefix... but if that same prefix is registered for again, we won't need to send any more commands to the kernel. At the same time if the requested prefix is for a frame that may generate a lot of wakeups we would then use separate sockets for it but the user wouldn't need to care.

One reason for this is that in my p2p.c I currently have a small framework for frame exchange sequences which is invoked by a single function and pass a list of response frames you're interested in, and corresponing callbacks:

p2p_peer_frame_xchg(...,

         tx_frame_iov,
         final_callback,
         &frame1_data, frame1_rx_callback,
         &frame2_data, frame2_rx_callback,
         NULL);

The prefixes will be passed to the framewatch api, and each callback may result in a new call to p2p_peer_frame_xchg so we want to forget these frame registrations quickly but not necessarily create genl sockets for each frame exchange.

Quoted 10 lines

set = frame_set_new(..);

if (rrm_enabled)

         frame_set_watch(set, NeighborReport);

if (mfp_enabled)

         frame_set_watch(set, SA Query Request);
         frame_set_watch(set, SA Query Response);

Quoted 2 lines

if (using_ft)

         frame_set_watch(set, FT Response);

Ok, so you don't care that we get woken up to receive some frames that we have to ignore, except for beacons and probe requests/responses.

However I wasn't really asking about this, but rather what you meant by a set, which specific calls would you include in the frame watch api, and where do you see the wdev_id being passed. In this last

example you don't have an framewatch_add_set call at all, so I understand that the frames watches go live as soon as you call frame_set_watch...

You still would use a new socket for each frame_set_new() call, right?

I.e. if we had three groups per netdev for whatever reason, then we
would have 3 sockets per netdev.

So my proposal was to use an enum like

enum {
FRAME_GROUP_XXX,
FRAME_GROUP_YYY,
FRAME_GROUP_ZZZ,
};

(obviously you might just need one single group, or two...)

and then skip the frame_set_new(...) call. Then you'd call frame_watch_register(wdev_id, FRAME_GROUP_XXX, prefix, prefix_len, callback, user_data) inside netdev_connect_common or netdev_create_from_genl or wherever you need to, and those registrations would immediately become live. However you could then

either destroy the whole group by using the same (wdev_id, FRAME_GROUP_XXX) values -- note how you don't need to bother saving any uint32 set ID or struct frame_watch_set pointer. Or alternatively

you could unregister a specific prefix you're no longer interested in, which would only close that socket if it happened to be the last active prefix in that group/set. Yes, those calls would need to use an l_queue_find... but what you were proposing with uint32_t frame set IDs has exactly the same downside and assuming we are conservative with those groups, the overhead can be smaller than some random compiler optimizations that we have no control over.

Quoted 5 lines

By hot path I mean one that may be executed many times a second, or as a dependency for every other operation (like malloc)... if something is done once per AP connection then I wouldn't call that a hot path and would try to optimize for nice code rather than shaving off a single l_queue_find.

Sure, but really you need to consider anything that affects latency of connections to be a hot path as well. So while what you say is all true, etc, shaving off one or several instances of l_queue_find if you can is still a goal. Just keep that in mind.

Quoted 5 lines

Honestly I'd rather not have the modules bother considering how expensive are those groups. For example if we had a NL80211_CMD_UNREGISTER_FRAME similar to REGISTER_FRAME, we would be less worried about those details I imagine. And this shouldn't leak

Well, this is the problem here. If we had CMD_UNREGISTER_FRAME, then none of this design would be necessary. We wouldn't be talking about sets or opening sockets or any of this. And by extension our APIs would look and act completely different. So I would argue the opposite: we should not be hiding these details from the user. In fact we want these details to be quite visible so that the costs are properly evaluated.

And yes, I get that maybe in the future this command becomes available. But its too late at this point. We go with the kernel we have, not the kernel we wish we would have ;)

Quoted 3 lines

into modules outside the framewatch module. For example currently we never unregister frames and our netdev_frame_watch_remove is a no-op at the syscall level so we can use it as often as we want. Maybe we

And this created a false sense of security. So unless you really know what is happening, the API makes you feel all warm and fuzzy.

Quoted 7 lines

should be doing something similar inside the framewatch module: keep some registrations active in the kernel but ignore the frames if no callback is currently registered for a given prefix... but if that same prefix is registered for again, we won't need to send any more commands to the kernel. At the same time if the requested prefix is for a frame that may generate a lot of wakeups we would then use separate sockets for it but the user wouldn't need to care.

But then you push this decision making onto framewatch which now has to be aware of all the details from other modules. Not saying this can't be made to work, but I'm not sure that's the approach I'd start with.

Quoted 13 lines

One reason for this is that in my p2p.c I currently have a small framework for frame exchange sequences which is invoked by a single function and pass a list of response frames you're interested in, and corresponing callbacks:

p2p_peer_frame_xchg(...,

         tx_frame_iov,
         final_callback,
         &frame1_data, frame1_rx_callback,
         &frame2_data, frame2_rx_callback,
         NULL);

Quoted 4 lines

The prefixes will be passed to the framewatch api, and each callback may result in a new call to p2p_peer_frame_xchg so we want to forget these frame registrations quickly but not necessarily create genl sockets for each frame exchange.

I'd handle this by having a set of frames you register for when P2P discovery is started and then your frame exchange runs as is. Then you can either close this set when you go into the connect phase or keep it for the next discovery procedure.

Quoted 11 lines

Quoted 11 lines

set = frame_set_new(..);

if (rrm_enabled)

         frame_set_watch(set, NeighborReport);

if (mfp_enabled)

         frame_set_watch(set, SA Query Request);
         frame_set_watch(set, SA Query Response);

Quoted 6 lines

Quoted 2 lines

if (using_ft)

         frame_set_watch(set, FT Response);

Ok, so you don't care that we get woken up to receive some frames that we have to ignore, except for beacons and probe requests/responses.

With the exception of Neighbor Reports all these are encrypted. So in the "AP is not going Rogue" case, none of these would result in unnecessary wakeups. (Reminds me, should we be doing FT-over-DS only if MFP is enabled?) Things get a little nasty once we get into ANQP and other Public Action frames, but there's little we can do about that. Creating a socket every time we send an ANQP request is just overkill for example.

Quoted 3 lines

However I wasn't really asking about this, but rather what you meant by a set, which specific calls would you include in the frame watch api, and where do you see the wdev_id being passed. In this last

the wdev would just go into the _new call. And each frame_set_watch would have the prefix, callback, userdata, destroy, etc. Or perhaps userdata and destroy can go into the _new call as well. Implementation details.

Quoted 3 lines

example you don't have an framewatch_add_set call at all, so I understand that the frames watches go live as soon as you call frame_set_watch...

Those are really implementation details. If we use Case 1 from upthread, then what you say above is true. If we use Case 2, then framewatch might get to massage the data first.

Quoted 2 lines

You still would use a new socket for each frame_set_new() call, right?

Right

Quoted 11 lines

I.e. if we had three groups per netdev for whatever reason, then we
would have 3 sockets per netdev.

So my proposal was to use an enum like

enum {
FRAME_GROUP_XXX,
FRAME_GROUP_YYY,
FRAME_GROUP_ZZZ,
};

Quoted 7 lines

(obviously you might just need one single group, or two...)

and then skip the frame_set_new(...) call. Then you'd call frame_watch_register(wdev_id, FRAME_GROUP_XXX, prefix, prefix_len, callback, user_data) inside netdev_connect_common or netdev_create_from_genl or wherever you need to, and those registrations would immediately become live. However you could then

Okay, this is what I understood.

Quoted 3 lines

either destroy the whole group by using the same (wdev_id, FRAME_GROUP_XXX) values -- note how you don't need to bother saving any uint32 set ID or struct frame_watch_set pointer. Or alternatively

Right, but need to duplicate the logic used to register to the groups (e.g. if (using_ft) unregister FT). Or always unregister from groups you might not be even registered to.