* [PATCH 02/11] netdev: Report RSSI to frame watch callbacks
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 3:34 ` Denis Kenzior
2019-10-21 13:55 ` [PATCH 03/11] netdev: Extend checks for P2P scenarios Andrew Zaborowski
` (9 subsequent siblings)
10 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 6523 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
The main use case is for user-space implemented scanning / device
discovery logic, like in P2P, to be able to report device rssi like
the kernel- or driver-side scanning does, based on the received beacons,
probe requests, probe responses, GO negotiation requests, PD requests
etc.
---
src/ap.c | 13 +++++++------
src/device.c | 2 +-
src/netdev.c | 20 ++++++++++++++------
src/netdev.h | 2 +-
4 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/ap.c b/src/ap.c
index cb7288db..db231167 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -966,7 +966,7 @@ bad_frame:
static void ap_assoc_req_cb(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
struct ap_state *ap = user_data;
struct sta_state *sta;
@@ -1000,7 +1000,7 @@ static void ap_assoc_req_cb(struct netdev *netdev,
static void ap_reassoc_req_cb(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
struct ap_state *ap = user_data;
struct sta_state *sta;
@@ -1054,7 +1054,7 @@ static void ap_probe_resp_cb(struct l_genl_msg *msg, void *user_data)
static void ap_probe_req_cb(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
struct ap_state *ap = user_data;
const struct mmpdu_probe_request *req = body;
@@ -1147,7 +1147,7 @@ static void ap_probe_req_cb(struct netdev *netdev,
static void ap_disassoc_cb(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
struct ap_state *ap = user_data;
struct sta_state *sta;
@@ -1217,7 +1217,8 @@ static void ap_auth_reply(struct ap_state *ap, const uint8_t *dest,
* 802.11-2016 12.3.3.2 (MLME/SME)
*/
static void ap_auth_cb(struct netdev *netdev, const struct mmpdu_header *hdr,
- const void *body, size_t body_len, void *user_data)
+ const void *body, size_t body_len, int rssi,
+ void *user_data)
{
struct ap_state *ap = user_data;
const struct mmpdu_authentication *auth = body;
@@ -1285,7 +1286,7 @@ done:
/* 802.11-2016 9.3.3.13 (frame format), 802.11-2016 11.3.4.5 (MLME/SME) */
static void ap_deauth_cb(struct netdev *netdev, const struct mmpdu_header *hdr,
- const void *body, size_t body_len,
+ const void *body, size_t body_len, int rssi,
void *user_data)
{
struct ap_state *ap = user_data;
diff --git a/src/device.c b/src/device.c
index 4f9d3933..4fd4edbe 100644
--- a/src/device.c
+++ b/src/device.c
@@ -57,7 +57,7 @@ static uint32_t netdev_watch;
static void device_ap_roam_frame_event(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
struct device *device = user_data;
struct station *station = station_find(device->index);
diff --git a/src/netdev.c b/src/netdev.c
index 526480dd..23fd980d 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -36,6 +36,7 @@
#include <linux/filter.h>
#include <sys/socket.h>
#include <errno.h>
+#include <limits.h>
#include <ell/ell.h>
@@ -2895,7 +2896,7 @@ static void netdev_ft_request_cb(struct l_genl_msg *msg, void *user_data)
static void netdev_ft_response_frame_event(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
int ret;
uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
@@ -3171,7 +3172,7 @@ int netdev_neighbor_report_req(struct netdev *netdev,
static void netdev_neighbor_report_frame_event(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
if (body_len < 3) {
l_debug("Neighbor Report frame too short");
@@ -3206,7 +3207,7 @@ static void netdev_sa_query_resp_cb(struct l_genl_msg *msg,
static void netdev_sa_query_req_frame_event(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
uint8_t sa_resp[4];
uint16_t transaction;
@@ -3244,7 +3245,7 @@ static void netdev_sa_query_req_frame_event(struct netdev *netdev,
static void netdev_sa_query_resp_frame_event(struct netdev *netdev,
const struct mmpdu_header *hdr,
const void *body, size_t body_len,
- void *user_data)
+ int rssi, void *user_data)
{
if (body_len < 4) {
l_debug("SA Query frame too short");
@@ -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;
if (!l_genl_attr_init(&attr, msg))
return;
@@ -3527,6 +3529,11 @@ static void netdev_mgmt_frame_event(struct l_genl_msg *msg,
frame_len = len;
break;
+ case NL80211_ATTR_RX_SIGNAL_DBM:
+ if (len != 4)
+ break;
+
+ rssi = *(const int32_t *) data;
}
}
@@ -3550,12 +3557,13 @@ static void netdev_mgmt_frame_event(struct l_genl_msg *msg,
WATCHLIST_NOTIFY_MATCHES(&netdev->frame_watches,
netdev_frame_watch_match_prefix, &info,
netdev_frame_watch_func_t,
- netdev, mpdu, body, info.body_len);
+ netdev, mpdu, body, info.body_len,
+ rssi);
else
WATCHLIST_NOTIFY_MATCHES(&wdev_frame_watches,
netdev_frame_watch_match_prefix, &info,
netdev_frame_watch_func_t,
- NULL, mpdu, body, info.body_len);
+ NULL, mpdu, body, info.body_len, rssi);
}
static void netdev_pae_destroy(void *user_data)
diff --git a/src/netdev.h b/src/netdev.h
index 624811b2..77da52de 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -119,7 +119,7 @@ typedef void (*netdev_preauthenticate_cb_t)(struct netdev *netdev,
typedef void (*netdev_frame_watch_func_t)(struct netdev *netdev,
const struct mmpdu_header *frame,
const void *body, size_t body_len,
- void *user_data);
+ int rssi, void *user_data);
typedef void (*netdev_station_watch_func_t)(struct netdev *netdev,
const uint8_t *mac, bool added,
void *user_data);
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 02/11] netdev: Report RSSI to frame watch callbacks
2019-10-21 13:55 ` [PATCH 02/11] netdev: Report RSSI to frame watch callbacks Andrew Zaborowski
@ 2019-10-22 3:34 ` Denis Kenzior
2019-10-22 13:46 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-22 3:34 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]
Hi Andrew,
<snip>
> diff --git a/src/netdev.c b/src/netdev.c
> index 526480dd..23fd980d 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -36,6 +36,7 @@
> #include <linux/filter.h>
> #include <sys/socket.h>
> #include <errno.h>
> +#include <limits.h>
This would suck to have to use INT_MIN everywhere...
>
> #include <ell/ell.h>
>
<snip>
> @@ -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?
>
> if (!l_genl_attr_init(&attr, msg))
> return;
> @@ -3527,6 +3529,11 @@ static void netdev_mgmt_frame_event(struct l_genl_msg *msg,
>
> frame_len = len;
> break;
> + case NL80211_ATTR_RX_SIGNAL_DBM:
> + if (len != 4)
> + break;
> +
> + rssi = *(const int32_t *) data;
> }
> }
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 02/11] netdev: Report RSSI to frame watch callbacks
2019-10-22 3:34 ` Denis Kenzior
@ 2019-10-22 13:46 ` Andrew Zaborowski
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-22 13:46 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
Hi Denis,
On Tue, 22 Oct 2019 at 05:34, Denis Kenzior <denkenz@gmail.com> wrote:
> > #include <sys/socket.h>
> > #include <errno.h>
> > +#include <limits.h>
>
> This would suck to have to use INT_MIN everywhere...
Because of the #include? Most places don't care about the rssi though.
>
> >
> > #include <ell/ell.h>
> >
>
> <snip>
>
> > @@ -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?
Ok.
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 03/11] netdev: Extend checks for P2P scenarios
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 02/11] netdev: Report RSSI to frame watch callbacks Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 3:36 ` Denis Kenzior
2019-10-21 13:55 ` [PATCH 04/11] eapol: Move the EAP event handler to handshake state Andrew Zaborowski
` (8 subsequent siblings)
10 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2754 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
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(-)
diff --git a/src/netdev.c b/src/netdev.c
index 23fd980d..d9357a22 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -1470,12 +1470,14 @@ void netdev_handshake_failed(struct handshake_state *hs, uint16_t reason_code)
switch (netdev->type) {
case NL80211_IFTYPE_STATION:
+ case NL80211_IFTYPE_P2P_CLIENT:
msg = netdev_build_cmd_disconnect(netdev, reason_code);
netdev->disconnect_cmd_id = l_genl_family_send(nl80211, msg,
netdev_disconnect_cb,
netdev, NULL);
break;
case NL80211_IFTYPE_AP:
+ case NL80211_IFTYPE_P2P_GO:
msg = netdev_build_cmd_del_station(netdev, nhs->super.spa,
reason_code, false);
if (!l_genl_family_send(nl80211, msg, NULL, NULL, NULL))
@@ -2465,7 +2467,8 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
struct eapol_sm *sm = NULL;
bool is_rsn = hs->supplicant_ie != NULL;
- if (netdev->type != NL80211_IFTYPE_STATION)
+ if (netdev->type != NL80211_IFTYPE_STATION &&
+ netdev->type != NL80211_IFTYPE_P2P_CLIENT)
return -ENOTSUP;
if (netdev->connected)
@@ -2521,7 +2524,8 @@ int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
size_t ie_len;
struct eapol_sm *sm;
- if (netdev->type != NL80211_IFTYPE_STATION)
+ if (netdev->type != NL80211_IFTYPE_STATION &&
+ netdev->type != NL80211_IFTYPE_P2P_CLIENT)
return -ENOTSUP;
if (netdev->connected)
@@ -2564,7 +2568,8 @@ int netdev_disconnect(struct netdev *netdev,
{
struct l_genl_msg *disconnect;
- if (netdev->type != NL80211_IFTYPE_STATION)
+ if (netdev->type != NL80211_IFTYPE_STATION &&
+ netdev->type != NL80211_IFTYPE_P2P_CLIENT)
return -ENOTSUP;
if (!netdev->connected)
@@ -3441,10 +3446,8 @@ static void netdev_mlme_notify(struct l_genl_msg *msg, void *user_data)
}
}
- if (!netdev) {
- l_warn("MLME notification is missing ifindex attribute");
+ if (!netdev)
return;
- }
switch (cmd) {
case NL80211_CMD_AUTHENTICATE:
@@ -4395,6 +4398,11 @@ static void netdev_getlink_cb(int error, uint16_t type, const void *data,
netdev_newlink_notify(ifi, bytes);
+ /* Don't do anything automatically for P2P interfaces */
+ if (netdev->type == NL80211_IFTYPE_P2P_CLIENT ||
+ netdev->type == NL80211_IFTYPE_P2P_GO)
+ return;
+
/*
* If the interface is UP, reset it to ensure a clean state.
* Otherwise, if we need to set a random mac, do so. If not, just
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 04/11] eapol: Move the EAP event handler to handshake state
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 02/11] netdev: Report RSSI to frame watch callbacks Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 03/11] netdev: Extend checks for P2P scenarios Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 4:11 ` Denis Kenzior
2019-10-21 13:55 ` [PATCH 05/11] unit: Update test-wsc to use handshake_state_set_eap_event_func Andrew Zaborowski
` (7 subsequent siblings)
10 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5167 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
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(-)
diff --git a/src/eapol.c b/src/eapol.c
index 349e824f..f370cbed 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -901,11 +901,6 @@ void eapol_sm_set_user_data(struct eapol_sm *sm, void *user_data)
sm->user_data = user_data;
}
-void eapol_sm_set_event_func(struct eapol_sm *sm, eapol_sm_event_func_t func)
-{
- sm->event_func = func;
-}
-
static void eapol_sm_write(struct eapol_sm *sm, const struct eapol_frame *ef,
bool noencrypt)
{
@@ -2167,10 +2162,11 @@ static void eapol_eap_event_cb(unsigned int event,
{
struct eapol_sm *sm = user_data;
- if (!sm->event_func)
+ if (!sm->handshake->eap_event_func)
return;
- sm->event_func(event, event_data, sm->user_data);
+ sm->handshake->eap_event_func(event, event_data,
+ sm->handshake->eap_user_data);
}
void eapol_sm_set_use_eapol_start(struct eapol_sm *sm, bool enabled)
diff --git a/src/eapol.h b/src/eapol.h
index 9462b56d..7143b53a 100644
--- a/src/eapol.h
+++ b/src/eapol.h
@@ -117,7 +117,6 @@ void eapol_sm_set_use_eapol_start(struct eapol_sm *sm, bool enabled);
void eapol_sm_set_require_handshake(struct eapol_sm *sm, bool enabled);
void eapol_sm_set_listen_interval(struct eapol_sm *sm, uint16_t interval);
void eapol_sm_set_user_data(struct eapol_sm *sm, void *user_data);
-void eapol_sm_set_event_func(struct eapol_sm *sm, eapol_sm_event_func_t func);
void eapol_register(struct eapol_sm *sm);
bool eapol_start(struct eapol_sm *sm);
diff --git a/src/handshake.c b/src/handshake.c
index 514f47db..d3f2a3ca 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -249,6 +249,14 @@ void handshake_state_set_event_func(struct handshake_state *s,
s->user_data = user_data;
}
+void handshake_state_set_eap_event_func(struct handshake_state *s,
+ handshake_eap_event_func_t func,
+ void *user_data)
+{
+ s->eap_event_func = func;
+ s->eap_user_data = user_data;
+}
+
void handshake_state_set_passphrase(struct handshake_state *s,
const char *passphrase)
{
diff --git a/src/handshake.h b/src/handshake.h
index 4b9d438b..2c144fea 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -69,6 +69,9 @@ typedef void (*handshake_install_igtk_func_t)(struct handshake_state *hs,
const uint8_t *igtk, uint8_t igtk_len,
const uint8_t *ipn, uint8_t ipn_len,
uint32_t cipher);
+typedef void (*handshake_eap_event_func_t)(unsigned int event,
+ const void *event_data,
+ void *user_data);
void __handshake_set_get_nonce_func(handshake_get_nonce_func_t func);
void __handshake_set_install_tk_func(handshake_install_tk_func_t func);
@@ -123,11 +126,13 @@ struct handshake_state {
uint8_t proto_version : 2;
unsigned int gtk_index;
struct erp_cache_entry *erp_cache;
- void *user_data;
void (*free)(struct handshake_state *s);
handshake_event_func_t event_func;
+ void *user_data;
+ handshake_eap_event_func_t eap_event_func;
+ void *eap_user_data;
};
void handshake_state_free(struct handshake_state *s);
@@ -160,6 +165,9 @@ void handshake_state_set_kh_ids(struct handshake_state *s,
void handshake_state_set_event_func(struct handshake_state *s,
handshake_event_func_t func,
void *user_data);
+void handshake_state_set_eap_event_func(struct handshake_state *s,
+ handshake_eap_event_func_t func,
+ void *user_data);
void handshake_state_set_passphrase(struct handshake_state *s,
const char *passphrase);
void handshake_state_set_no_rekey(struct handshake_state *s, bool no_rekey);
diff --git a/src/netdev.c b/src/netdev.c
index d9357a22..48cdb5c1 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2551,9 +2551,11 @@ int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
l_genl_msg_append_attr(cmd_connect, NL80211_ATTR_IE, ie_len, ie);
l_free(ie);
+ handshake_state_set_eap_event_func(hs,
+ (handshake_eap_event_func_t) eapol_cb,
+ user_data);
+
sm = eapol_sm_new(hs);
- eapol_sm_set_user_data(sm, user_data);
- eapol_sm_set_event_func(sm, eapol_cb);
return netdev_connect_common(netdev, cmd_connect, bss, hs, sm,
event_filter, cb, user_data);
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 04/11] eapol: Move the EAP event handler to handshake state
2019-10-21 13:55 ` [PATCH 04/11] eapol: Move the EAP event handler to handshake state Andrew Zaborowski
@ 2019-10-22 4:11 ` Denis Kenzior
2019-10-22 14:00 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-22 4:11 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]
Hi Andrew,
On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> From: Andrew Zaborowski <andrew.zaborowski@intel.com>
>
> 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...
>
> 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.
I'd imagine they can be reworked to act like netdev_set_tk, etc.
Removing set_user_data is a good idea regardless.
> ---
> 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(-)
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/11] eapol: Move the EAP event handler to handshake state
2019-10-22 4:11 ` Denis Kenzior
@ 2019-10-22 14:00 ` Andrew Zaborowski
2019-10-22 14:34 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-22 14:00 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]
Hi Denis,
On Tue, 22 Oct 2019 at 06:11, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> > 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.
>
> >
> > 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.
>
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/11] eapol: Move the EAP event handler to handshake state
2019-10-22 14:00 ` Andrew Zaborowski
@ 2019-10-22 14:34 ` Denis Kenzior
0 siblings, 0 replies; 36+ messages in thread
From: Denis Kenzior @ 2019-10-22 14:34 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]
Hi Andrew,
On 10/22/19 9:00 AM, Andrew Zaborowski wrote:
> Hi Denis,
>
> On Tue, 22 Oct 2019 at 06:11, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
>>> 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 "...".
That is what I'm thinking. Or make event_data part of the argument list
and any 'extra' arguments are part of the variadic argument array. That
way existing callers don't have to worry about invoking va_arg, etc.
>
> The uglier option would be to reserve a range of enum handshake_event
> values for eap events.
I considered that, but this seems less elegant.
>
>>
>>>
>>> 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.
>>
>> 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.
Well, we can't since user_data is used by wsc/station to observe
handshake events.
And set_tk and friends don't look up by ifindex any more. They get
passed in the handshake_state as the first argument.
>
> Best regards
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 05/11] unit: Update test-wsc to use handshake_state_set_eap_event_func
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (2 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 04/11] eapol: Move the EAP event handler to handshake state Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 06/11] wsc: Replace netdev_connect_wsc with netdev_connect usage Andrew Zaborowski
` (6 subsequent siblings)
10 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
unit/test-wsc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/unit/test-wsc.c b/unit/test-wsc.c
index b4814622..76f5e18e 100644
--- a/unit/test-wsc.c
+++ b/unit/test-wsc.c
@@ -1999,11 +1999,10 @@ static void wsc_test_pbc_handshake(const void *data)
handshake_state_set_authenticator_address(hs, ap_address);
handshake_state_set_supplicant_address(hs, sta_address);
handshake_state_set_event_func(hs, verify_handshake_event, &verify);
+ handshake_state_set_eap_event_func(hs, verify_credential, &verify);
__eapol_set_tx_packet_func(verify_8021x);
__eapol_set_tx_user_data(&verify);
- eapol_sm_set_user_data(sm, &verify);
- eapol_sm_set_event_func(sm, verify_credential);
settings = l_settings_new();
l_settings_set_uint(settings, "EAP", "mtu", 1400);
@@ -2109,11 +2108,10 @@ static void wsc_test_retransmission_no_fragmentation(const void *data)
handshake_state_set_authenticator_address(hs, ap_address);
handshake_state_set_supplicant_address(hs, sta_address);
handshake_state_set_event_func(hs, verify_handshake_event, &verify);
+ handshake_state_set_eap_event_func(hs, verify_credential, &verify);
__eapol_set_tx_packet_func(verify_8021x);
__eapol_set_tx_user_data(&verify);
- eapol_sm_set_user_data(sm, &verify);
- eapol_sm_set_event_func(sm, verify_credential);
settings = l_settings_new();
l_settings_set_uint(settings, "EAP", "mtu", 1400);
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 06/11] wsc: Replace netdev_connect_wsc with netdev_connect usage
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (3 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 05/11] unit: Update test-wsc to use handshake_state_set_eap_event_func Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 07/11] netdev: Drop unused netdev_connect_wsc Andrew Zaborowski
` (5 subsequent siblings)
10 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
netdev_connect can achieve the same effect as netdev_connect_wsc but is
more flexible as it allows us to supply additional association IEs. We
will need this capability to make P2P connections. This way we're also
moving the WSC-specific bits to wsc.c from the crowded netdev.c.
Also make sure we free the handshake state on netdev_connect error in
wsc_connect().
---
src/netdev.c | 2 +-
src/wsc.c | 43 +++++++++++++++++++++++++++++++++++--------
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/netdev.c b/src/netdev.c
index 48cdb5c1..93baa6ea 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2501,7 +2501,7 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
if (!cmd_connect)
return -EINVAL;
- if (is_rsn)
+ if (is_rsn || hs->settings_8021x)
sm = eapol_sm_new(hs);
}
diff --git a/src/wsc.c b/src/wsc.c
index 068cf82a..d971797a 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -360,7 +360,7 @@ static void wsc_credential_obtained(struct wsc *wsc,
wsc->n_creds += 1;
}
-static void wsc_eapol_event(uint32_t event, const void *event_data,
+static void wsc_eap_event(uint32_t event, const void *event_data,
void *user_data)
{
struct wsc *wsc = user_data;
@@ -434,6 +434,11 @@ static void wsc_connect(struct wsc *wsc)
struct handshake_state *hs;
struct l_settings *settings = l_settings_new();
struct scan_bss *bss = wsc->target;
+ int r;
+ struct wsc_association_request request;
+ uint8_t *pdu;
+ size_t pdu_len;
+ struct iovec ie_iov;
wsc->target = NULL;
@@ -473,19 +478,41 @@ static void wsc_connect(struct wsc *wsc)
}
handshake_state_set_event_func(hs, wsc_handshake_event, wsc);
+ handshake_state_set_eap_event_func(hs, wsc_eap_event, wsc);
handshake_state_set_8021x_config(hs, settings);
wsc->eap_settings = settings;
- if (netdev_connect_wsc(wsc->netdev, bss, hs,
- wsc_netdev_event, wsc_connect_cb,
- wsc_eapol_event, wsc) < 0) {
- dbus_pending_reply(&wsc->pending,
- dbus_error_failed(wsc->pending));
- handshake_state_free(hs);
- return;
+ request.version2 = true;
+ request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
+
+ pdu = wsc_build_association_request(&request, &pdu_len);
+ if (!pdu) {
+ r = -ENOMEM;
+ goto error;
}
+ ie_iov.iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
+ &ie_iov.iov_len);
+ l_free(pdu);
+
+ if (!ie_iov.iov_base) {
+ r = -ENOMEM;
+ goto error;
+ }
+
+ r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
+ wsc_connect_cb, wsc);
+ l_free(ie_iov.iov_base);
+
+ if (r < 0)
+ goto error;
+
wsc->wsc_association = true;
+ return;
+error:
+ handshake_state_free(hs);
+ dbus_pending_reply(&wsc->pending,
+ dbus_error_failed(wsc->pending));
}
static void station_state_watch(enum station_state state, void *userdata)
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 07/11] netdev: Drop unused netdev_connect_wsc
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (4 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 06/11] wsc: Replace netdev_connect_wsc with netdev_connect usage Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor Andrew Zaborowski
` (4 subsequent siblings)
10 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
src/netdev.c | 57 ----------------------------------------------------
src/netdev.h | 9 ---------
2 files changed, 66 deletions(-)
diff --git a/src/netdev.c b/src/netdev.c
index 93baa6ea..488370ab 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -52,7 +52,6 @@
#include "src/device.h"
#include "src/scan.h"
#include "src/netdev.h"
-#include "src/wscutil.h"
#include "src/ft.h"
#include "src/util.h"
#include "src/watchlist.h"
@@ -2509,62 +2508,6 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
event_filter, cb, user_data);
}
-int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
- struct handshake_state *hs,
- netdev_event_func_t event_filter,
- netdev_connect_cb_t cb,
- netdev_eapol_event_func_t eapol_cb,
- void *user_data)
-{
- struct l_genl_msg *cmd_connect;
- struct wsc_association_request request;
- uint8_t *pdu;
- size_t pdu_len;
- void *ie;
- size_t ie_len;
- struct eapol_sm *sm;
-
- if (netdev->type != NL80211_IFTYPE_STATION &&
- netdev->type != NL80211_IFTYPE_P2P_CLIENT)
- return -ENOTSUP;
-
- if (netdev->connected)
- return -EISCONN;
-
- cmd_connect = netdev_build_cmd_connect(netdev, bss, hs, NULL, NULL, 0);
- if (!cmd_connect)
- return -EINVAL;
-
- request.version2 = true;
- request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
-
- pdu = wsc_build_association_request(&request, &pdu_len);
- if (!pdu)
- goto error;
-
- ie = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len, &ie_len);
- l_free(pdu);
-
- if (!ie)
- goto error;
-
- l_genl_msg_append_attr(cmd_connect, NL80211_ATTR_IE, ie_len, ie);
- l_free(ie);
-
- handshake_state_set_eap_event_func(hs,
- (handshake_eap_event_func_t) eapol_cb,
- user_data);
-
- sm = eapol_sm_new(hs);
-
- return netdev_connect_common(netdev, cmd_connect, bss, hs, sm,
- event_filter, cb, user_data);
-
-error:
- l_genl_msg_unref(cmd_connect);
- return -ENOMEM;
-}
-
int netdev_disconnect(struct netdev *netdev,
netdev_disconnect_cb_t cb, void *user_data)
{
diff --git a/src/netdev.h b/src/netdev.h
index 77da52de..b4da4fc2 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -107,9 +107,6 @@ typedef void (*netdev_watch_func_t)(struct netdev *netdev,
enum netdev_watch_event event,
void *user_data);
typedef void (*netdev_destroy_func_t)(void *user_data);
-typedef void (*netdev_eapol_event_func_t)(unsigned int event,
- const void *event_data,
- void *user_data);
typedef void (*netdev_neighbor_report_cb_t)(struct netdev *netdev, int err,
const uint8_t *reports,
size_t reports_len, void *user_data);
@@ -150,12 +147,6 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
size_t num_vendor_ies,
netdev_event_func_t event_filter,
netdev_connect_cb_t cb, void *user_data);
-int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
- struct handshake_state *hs,
- netdev_event_func_t event_filter,
- netdev_connect_cb_t cb,
- netdev_eapol_event_func_t eapol_cb,
- void *user_data);
int netdev_disconnect(struct netdev *netdev,
netdev_disconnect_cb_t cb, void *user_data);
int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (5 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 07/11] netdev: Drop unused netdev_connect_wsc Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 14:47 ` Denis Kenzior
2019-10-21 13:55 ` [PATCH 09/11] wsc: Accept extra IEs in wsc_new_p2p_enrollee Andrew Zaborowski
` (3 subsequent siblings)
10 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 9759 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
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(-)
diff --git a/Makefile.am b/Makefile.am
index ecd1c228..03be9577 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -199,7 +199,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
src/agent.h src/agent.c \
src/storage.h src/storage.c \
src/network.h src/network.c \
- src/wsc.c \
+ src/wsc.h src/wsc.c \
src/backtrace.h src/backtrace.c \
src/knownnetworks.h \
src/knownnetworks.c \
diff --git a/src/wsc.c b/src/wsc.c
index d971797a..61683e7b 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -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];
- char passphrase[64];
- };
- uint8_t addr[6];
- bool has_passphrase;
- } creds[3];
+ struct wsc_credentials creds[3];
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;
}
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;
}
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) {
- 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));
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));
+ return;
+ } else if (err) {
+ dbus_pending_reply(&wsc->pending,
+ dbus_error_failed(wsc->pending));
+ return;
+ }
+
+ 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,
+ * the caller is expected to watch interfaces going away and there's no
+ * need to handle the event in both places.
+ */
+
+ return wsc;
+}
+
+void wsc_destroy(struct wsc *wsc)
+{
+ wsc_free(wsc);
+}
+
static int wsc_init(void)
{
l_debug("");
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor
2019-10-21 13:55 ` [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor Andrew Zaborowski
@ 2019-10-22 14:47 ` Denis Kenzior
2019-10-22 23:46 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-22 14:47 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 10912 bytes --]
Hi Andrew,
On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> From: Andrew Zaborowski <andrew.zaborowski@intel.com>
>
> 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(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index ecd1c228..03be9577 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -199,7 +199,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
> src/agent.h src/agent.c \
> src/storage.h src/storage.c \
> src/network.h src/network.c \
> - src/wsc.c \
> + src/wsc.h src/wsc.c \
Looks like you forgot to add wsc.h itself?
> src/backtrace.h src/backtrace.c \
> src/knownnetworks.h \
> src/knownnetworks.c \
> diff --git a/src/wsc.c b/src/wsc.c
> index d971797a..61683e7b 100644
> --- a/src/wsc.c
> +++ b/src/wsc.c
> @@ -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];
> - 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
> 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;
> }
>
> 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
> }
>
> 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) {
> - 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?
> 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));
> + return;
> + } else if (err) {
> + dbus_pending_reply(&wsc->pending,
> + dbus_error_failed(wsc->pending));
> + return;
> + }
switch/case might look cleaner
> +
> + 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,
> + * 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
> +
> +void wsc_destroy(struct wsc *wsc)
> +{
> + wsc_free(wsc);
> +}
> +
> static int wsc_init(void)
> {
> l_debug("");
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor
2019-10-22 14:47 ` Denis Kenzior
@ 2019-10-22 23:46 ` Andrew Zaborowski
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-22 23:46 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 13492 bytes --]
Hi Denis,
On Tue, 22 Oct 2019 at 16:47, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> > src/agent.h src/agent.c \
> > src/storage.h src/storage.c \
> > src/network.h src/network.c \
> > - src/wsc.c \
> > + src/wsc.h src/wsc.c \
>
> Looks like you forgot to add wsc.h itself?
Oops. But there wasn't much to see there.
>
> > src/backtrace.h src/backtrace.c \
> > src/knownnetworks.h \
> > src/knownnetworks.c \
> > diff --git a/src/wsc.c b/src/wsc.c
> > index d971797a..61683e7b 100644
> > --- a/src/wsc.c
> > +++ b/src/wsc.c
> > @@ -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];
> > - 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
Ok.
>
> > 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;
> > }
> >
> > 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
So this is part of splitting the wsc core code from the dbus-specific
code. The dbus specific code receives the method call, scans, sets up
wsc->done_cb and calls wsc_connect.
connect_wsc will connect, obtain the credentials, call done_cb and
then wipe the credentials. Since wsc_try_credentials is now part of
dbus-specific done_cb, it's not supposed to clear the credentials
which is done in the common code after calling done_cb.
>
> > }
> >
> > 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) {
> > - 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?
We only call done_cb which, in the DBus case, calls store_credentials
and try_credentials.
>
> > 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));
> > + return;
> > + } else if (err) {
> > + dbus_pending_reply(&wsc->pending,
> > + dbus_error_failed(wsc->pending));
> > + return;
> > + }
>
> switch/case might look cleaner
Ok.
>
> > +
> > + 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,
> > + * 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
Ok.
>
> > +
> > +void wsc_destroy(struct wsc *wsc)
> > +{
> > + wsc_free(wsc);
> > +}
> > +
> > static int wsc_init(void)
> > {
> > l_debug("");
> >
>
> Regards,
> -Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 09/11] wsc: Accept extra IEs in wsc_new_p2p_enrollee
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (6 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 08/11] wsc: Add wsc_new_p2p_enrollee, refactor Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-21 13:55 ` [PATCH 10/11] wiphy: Add wiphy_get_max_roc_duration Andrew Zaborowski
` (2 subsequent siblings)
10 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
src/wsc.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/src/wsc.c b/src/wsc.c
index 61683e7b..6d232139 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -440,7 +440,8 @@ static inline enum wsc_rf_band freq_to_rf_band(uint32_t freq)
return WSC_RF_BAND_2_4_GHZ;
}
-static void wsc_connect(struct wsc *wsc)
+static void wsc_connect(struct wsc *wsc, struct iovec *ies,
+ unsigned int ies_num)
{
struct handshake_state *hs;
struct l_settings *settings = l_settings_new();
@@ -449,7 +450,7 @@ static void wsc_connect(struct wsc *wsc)
struct wsc_association_request request;
uint8_t *pdu;
size_t pdu_len;
- struct iovec ie_iov;
+ struct iovec ie_iov[1 + ies_num];
wsc->target = NULL;
@@ -496,18 +497,20 @@ static void wsc_connect(struct wsc *wsc)
goto error;
}
- ie_iov.iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
- &ie_iov.iov_len);
+ ie_iov[0].iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
+ &ie_iov[0].iov_len);
l_free(pdu);
- if (!ie_iov.iov_base) {
+ if (!ie_iov[0].iov_base) {
r = -ENOMEM;
goto error;
}
- r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
- wsc_connect_cb, wsc);
- l_free(ie_iov.iov_base);
+ memcpy(ie_iov + 1, ies, sizeof(struct iovec) * ies_num);
+
+ r = netdev_connect(wsc->netdev, bss, hs, ie_iov, 1 + ies_num,
+ wsc_netdev_event, wsc_connect_cb, wsc);
+ l_free(ie_iov[0].iov_base);
if (r < 0)
goto error;
@@ -562,7 +565,7 @@ static void station_state_watch(enum station_state state, void *userdata)
station_remove_state_watch(wsc->station, wsc->station_state_watch);
wsc->station_state_watch = 0;
- wsc_connect(wsc);
+ wsc_connect(wsc, NULL, 0);
}
static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
@@ -594,7 +597,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
switch (station_get_state(wsc->station)) {
case STATION_STATE_DISCONNECTED:
- wsc_connect(wsc);
+ wsc_connect(wsc, NULL, 0);
return;
case STATION_STATE_CONNECTING:
case STATION_STATE_CONNECTED:
@@ -1188,8 +1191,9 @@ 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)
+ char *pin,
+ struct iovec *ies, unsigned int ies_num,
+ wsc_done_cb_t done_cb, void *user_data)
{
struct wsc *wsc;
@@ -1202,7 +1206,7 @@ struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
wsc->done_data = user_data;
wsc->pin = l_strdup(pin);
- wsc_connect(wsc);
+ wsc_connect(wsc, ies, ies_num);
/*
* Wsc objects created this way are not handled in wsc_remove_interface,
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 10/11] wiphy: Add wiphy_get_max_roc_duration
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (7 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 09/11] wsc: Accept extra IEs in wsc_new_p2p_enrollee Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 3:26 ` Denis Kenzior
2019-10-21 13:55 ` [PATCH 11/11] wiphy: Add wiphy_get_supported_rates Andrew Zaborowski
2019-10-22 14:53 ` [PATCH 01/11] netdev: Add a wdev_id based frame watch API Denis Kenzior
10 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
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(+)
diff --git a/src/wiphy.c b/src/wiphy.c
index 169631af..9cb9ae66 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -66,6 +66,7 @@ struct wiphy {
uint32_t feature_flags;
uint8_t ext_features[(NUM_NL80211_EXT_FEATURES + 7) / 8];
uint8_t max_num_ssids_per_scan;
+ uint32_t max_roc_duration;
uint16_t supported_iftypes;
uint16_t supported_ciphers;
struct scan_freq_set *supported_freqs;
@@ -355,6 +356,11 @@ uint8_t wiphy_get_max_num_ssids_per_scan(struct wiphy *wiphy)
return wiphy->max_num_ssids_per_scan;
}
+uint32_t wiphy_get_max_roc_duration(struct wiphy *wiphy)
+{
+ return wiphy->max_roc_duration;
+}
+
bool wiphy_supports_adhoc_rsn(struct wiphy *wiphy)
{
return wiphy->support_adhoc_rsn;
@@ -769,6 +775,12 @@ static void wiphy_parse_attributes(struct wiphy *wiphy,
parse_iftype_extended_capabilities(wiphy, &nested);
break;
+ case NL80211_ATTR_MAX_REMAIN_ON_CHANNEL_DURATION:
+ if (len != 4)
+ l_warn("Invalid MAX_ROC_DURATION attribute");
+ else
+ wiphy->max_roc_duration = *((uint32_t *) data);
+ break;
}
}
}
diff --git a/src/wiphy.h b/src/wiphy.h
index 61e1caf8..c109f0a8 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -64,6 +64,7 @@ bool wiphy_rrm_capable(struct wiphy *wiphy);
bool wiphy_has_feature(struct wiphy *wiphy, uint32_t feature);
bool wiphy_has_ext_feature(struct wiphy *wiphy, uint32_t feature);
uint8_t wiphy_get_max_num_ssids_per_scan(struct wiphy *wiphy);
+uint32_t wiphy_get_max_roc_duration(struct wiphy *wiphy);
bool wiphy_supports_iftype(struct wiphy *wiphy, uint32_t iftype);
bool wiphy_supports_adhoc_rsn(struct wiphy *wiphy);
bool wiphy_can_offchannel_tx(struct wiphy *wiphy);
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 11/11] wiphy: Add wiphy_get_supported_rates
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (8 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 10/11] wiphy: Add wiphy_get_max_roc_duration Andrew Zaborowski
@ 2019-10-21 13:55 ` Andrew Zaborowski
2019-10-22 14:53 ` [PATCH 01/11] netdev: Add a wdev_id based frame watch API Denis Kenzior
10 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-21 13:55 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
Add code to parse the supported data rates info from the wiphy dumps and
expose it for P2P's use with a getter function.
---
src/wiphy.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
src/wiphy.h | 2 ++
2 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/src/wiphy.c b/src/wiphy.c
index 9cb9ae66..12ec5d17 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -76,6 +76,7 @@ struct wiphy {
struct watchlist state_watches;
uint8_t extended_capabilities[EXT_CAP_LEN + 2]; /* max bitmap size + IE header */
uint8_t *iftype_extended_capabilities[NUM_NL80211_IFTYPES];
+ uint8_t *supported_rates[NUM_NL80211_BANDS];
uint8_t rm_enabled_capabilities[7]; /* 5 size max + header */
bool support_scheduled_scan:1;
@@ -212,6 +213,9 @@ static void wiphy_free(void *data)
for (i = 0; i < NUM_NL80211_IFTYPES; i++)
l_free(wiphy->iftype_extended_capabilities[i]);
+ for (i = 0; i < NUM_NL80211_BANDS; i++)
+ l_free(wiphy->supported_rates[i]);
+
scan_freq_set_free(wiphy->supported_freqs);
watchlist_destroy(&wiphy->state_watches);
l_free(wiphy->model_str);
@@ -478,6 +482,14 @@ bool wiphy_supports_iftype(struct wiphy *wiphy, uint32_t iftype)
return wiphy->supported_iftypes & (1 << (iftype - 1));
}
+const uint8_t *wiphy_get_supported_rates(struct wiphy *wiphy, unsigned int band)
+{
+ if (band >= L_ARRAY_SIZE(wiphy->supported_rates))
+ return NULL;
+
+ return wiphy->supported_rates[band];
+}
+
uint32_t wiphy_state_watch_add(struct wiphy *wiphy,
wiphy_state_watch_func_t func,
void *user_data, wiphy_destroy_func_t destroy)
@@ -622,20 +634,70 @@ static void parse_supported_frequencies(struct wiphy *wiphy,
}
}
+static uint8_t *parse_supported_rates(struct l_genl_attr *attr)
+{
+ uint16_t type;
+ uint16_t len;
+ const void *data;
+ struct l_genl_attr nested;
+ int count = 0;
+ uint8_t *ret;
+
+ if (!l_genl_attr_recurse(attr, &nested))
+ return NULL;
+
+ while (l_genl_attr_next(&nested, NULL, NULL, NULL))
+ count++;
+
+ if (!l_genl_attr_recurse(attr, &nested))
+ return NULL;
+
+ ret = l_malloc(count + 1);
+ ret[count] = 0;
+
+ count = 0;
+
+ while (l_genl_attr_next(&nested, NULL, NULL, NULL)) {
+ struct l_genl_attr nested2;
+
+ if (!l_genl_attr_recurse(&nested, &nested2)) {
+ l_free(ret);
+ return NULL;
+ }
+
+ while (l_genl_attr_next(&nested2, &type, &len, &data)) {
+ if (type != NL80211_BITRATE_ATTR_RATE || len != 4)
+ continue;
+
+ /*
+ * Convert from the 100kb/s units reported by the
+ * kernel to the 500kb/s used in 802.11 IEs.
+ */
+ ret[count++] = *(const uint32_t *) data / 5;
+ }
+ }
+
+ return ret;
+}
+
static void parse_supported_bands(struct wiphy *wiphy,
struct l_genl_attr *bands)
{
- uint16_t type, len;
- const void *data;
+ uint16_t type;
struct l_genl_attr attr;
l_debug("");
- while (l_genl_attr_next(bands, NULL, NULL, NULL)) {
+ while (l_genl_attr_next(bands, &type, NULL, NULL)) {
+ enum nl80211_band band = type;
+
+ if (band != NL80211_BAND_2GHZ && band != NL80211_BAND_5GHZ)
+ continue;
+
if (!l_genl_attr_recurse(bands, &attr))
continue;
- while (l_genl_attr_next(&attr, &type, &len, &data)) {
+ while (l_genl_attr_next(&attr, &type, NULL, NULL)) {
struct l_genl_attr freqs;
switch (type) {
@@ -645,6 +707,14 @@ static void parse_supported_bands(struct wiphy *wiphy,
parse_supported_frequencies(wiphy, &freqs);
break;
+
+ case NL80211_BAND_ATTR_RATES:
+ if (wiphy->supported_rates[band])
+ continue;
+
+ wiphy->supported_rates[band] =
+ parse_supported_rates(&attr);
+ break;
}
}
}
diff --git a/src/wiphy.h b/src/wiphy.h
index c109f0a8..a5133972 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -66,6 +66,8 @@ bool wiphy_has_ext_feature(struct wiphy *wiphy, uint32_t feature);
uint8_t wiphy_get_max_num_ssids_per_scan(struct wiphy *wiphy);
uint32_t wiphy_get_max_roc_duration(struct wiphy *wiphy);
bool wiphy_supports_iftype(struct wiphy *wiphy, uint32_t iftype);
+const uint8_t *wiphy_get_supported_rates(struct wiphy *wiphy,
+ unsigned int band);
bool wiphy_supports_adhoc_rsn(struct wiphy *wiphy);
bool wiphy_can_offchannel_tx(struct wiphy *wiphy);
bool wiphy_supports_qos_set_map(struct wiphy *wiphy);
--
2.20.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-21 13:55 [PATCH 01/11] netdev: Add a wdev_id based frame watch API Andrew Zaborowski
` (9 preceding siblings ...)
2019-10-21 13:55 ` [PATCH 11/11] wiphy: Add wiphy_get_supported_rates Andrew Zaborowski
@ 2019-10-22 14:53 ` Denis Kenzior
2019-10-22 23:56 ` Andrew Zaborowski
10 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-22 14:53 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]
Hi Andrew,
On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> From: Andrew Zaborowski <andrew.zaborowski@intel.com>
>
> 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(-)
>
<snip>
> diff --git a/src/netdev.h b/src/netdev.h
> index 114a6035..624811b2 100644
> --- a/src/netdev.h
> +++ b/src/netdev.h
> @@ -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.
> void netdev_handshake_failed(struct handshake_state *hs, uint16_t reason_code);
>
> struct netdev *netdev_find(int ifindex);
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-22 14:53 ` [PATCH 01/11] netdev: Add a wdev_id based frame watch API Denis Kenzior
@ 2019-10-22 23:56 ` Andrew Zaborowski
2019-10-23 0:23 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-22 23:56 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]
On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> > diff --git a/src/netdev.h b/src/netdev.h
> > index 114a6035..624811b2 100644
> > --- a/src/netdev.h
> > +++ b/src/netdev.h
> > @@ -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.
Ok, I only put it inside netdev as an extension of the current api.
> 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.
>
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-22 23:56 ` Andrew Zaborowski
@ 2019-10-23 0:23 ` Denis Kenzior
2019-10-23 1:04 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-23 0:23 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]
Hi Andrew,
On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
> On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
>>> diff --git a/src/netdev.h b/src/netdev.h
>>> index 114a6035..624811b2 100644
>>> --- a/src/netdev.h
>>> +++ b/src/netdev.h
>>> @@ -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.
>
> Ok, I only put it inside netdev as an extension of the current api.
I know. But given that this isn't urgent, I'd rather do it right the
first time.
>
>> 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
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.
> 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.
> the kernel have a simple frame unregister command or keep all of the
Well, we tried that and got rejected. See [1]. So as I said, until the
kernel guys start treating nl80211 as a proper API, we have to live with
this.
> 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.
>
>>
>> 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.
>
> Best regards
>
Regards,
-Denis
[1] https://patchwork.kernel.org/patch/11131097/
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-23 0:23 ` Denis Kenzior
@ 2019-10-23 1:04 ` Andrew Zaborowski
2019-10-23 1:32 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-23 1:04 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]
On Wed, 23 Oct 2019 at 02:23, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
> > On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
> >> 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
>
> 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.
>
> > 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.
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?
>
> > 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.
>
> >
> >>
> >> 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.
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
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-23 1:04 ` Andrew Zaborowski
@ 2019-10-23 1:32 ` Denis Kenzior
2019-10-24 0:59 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-23 1:32 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5894 bytes --]
Hi Andrew,
On 10/22/19 8:04 PM, Andrew Zaborowski wrote:
> On Wed, 23 Oct 2019 at 02:23, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
>>> On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> 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
>>
>> 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.
>>
>>> 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.
>
> 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();
>>
>>> 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.
>>
>>>
>>>>
>>>> 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.
>
> 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.
> 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.
>
> 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.
Ah maybe. See what I wrote in the very beginning...
>
> 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
Well, strictly speaking we can. Just use a separate socket. wpa_s uses
>1 per device for example and closes/re-opens these when requested to
change the iftype.
> 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
If we register the frame watch types upfront at module init time, then
this becomes quite easy. And only the 'set' ones might benefit from
this optimization. But then the set ones won't need any of these
optimizations anyway. Don't over-complicate this...
> 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.
>
Exactly.
> Best regards
>
Regards.
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-23 1:32 ` Denis Kenzior
@ 2019-10-24 0:59 ` Andrew Zaborowski
2019-10-24 2:53 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-24 0:59 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5838 bytes --]
Hi Denis,
On Wed, 23 Oct 2019 at 03:32, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/22/19 8:04 PM, Andrew Zaborowski wrote:
> > On Wed, 23 Oct 2019 at 02:23, Denis Kenzior <denkenz@gmail.com> wrote:
> >> On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
> >>> On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
> >>>> 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
> >>
> >> 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.
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.
>
> >>
> >>> 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.
> >
> > 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();
>
> >>
> >>> 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.
Ok, let's do that.
>
> >>
> >>>
> >>>>
> >>>> 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.
> >
> > 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.
>
> > 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.
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().
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 0:59 ` Andrew Zaborowski
@ 2019-10-24 2:53 ` Denis Kenzior
2019-10-24 3:22 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-24 2:53 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4213 bytes --]
Hi Andrew,
On 10/23/19 7:59 PM, Andrew Zaborowski wrote:
> Hi Denis,
>
> On Wed, 23 Oct 2019 at 03:32, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/22/19 8:04 PM, Andrew Zaborowski wrote:
>>> On Wed, 23 Oct 2019 at 02:23, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
>>>>> On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>>> 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
>>>>
>>>> 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.
>
> 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?
>
> 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.
> 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).
So it wouldn't be the direction I'd go, but if you think it can work I'm
okay with giving that a try.
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 2:53 ` Denis Kenzior
@ 2019-10-24 3:22 ` Andrew Zaborowski
2019-10-24 15:29 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-24 3:22 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3577 bytes --]
On Thu, 24 Oct 2019 at 04:54, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/23/19 7:59 PM, Andrew Zaborowski wrote:
> > 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...
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.
>
> 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?
True, we can't because we haven't even set a channel...
>
> >
> > 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.
>
> > 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.
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)
>
> 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,
};
> 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.
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 3:22 ` Andrew Zaborowski
@ 2019-10-24 15:29 ` Denis Kenzior
2019-10-24 21:47 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-24 15:29 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]
Hi Andrew,
On 10/23/19 10:22 PM, Andrew Zaborowski wrote:
> On Thu, 24 Oct 2019 at 04:54, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/23/19 7:59 PM, Andrew Zaborowski wrote:
>>> 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...
>
> 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 ;)
>>
>> 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...
>
>>
>> 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,
> };
>
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.
>> 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?
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 15:29 ` Denis Kenzior
@ 2019-10-24 21:47 ` Andrew Zaborowski
2019-10-24 22:16 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-24 21:47 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]
Hi Denis,
On Thu, 24 Oct 2019 at 17:29, Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Andrew,
>
> On 10/23/19 10:22 PM, Andrew Zaborowski wrote:
> > On Thu, 24 Oct 2019 at 04:54, Denis Kenzior <denkenz@gmail.com> wrote:
> >> 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.
>
> 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.
>
> The kernel API is not ideal. Lets make it work, but don't go crazy ;)
>
> >>
> >> 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...
>
> >
> >>
> >> 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,
> > };
> >
>
> 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.
>
> >> 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.
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 21:47 ` Andrew Zaborowski
@ 2019-10-24 22:16 ` Denis Kenzior
2019-10-24 22:45 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-24 22:16 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 6527 bytes --]
Hi Andrew,
On 10/24/19 4:47 PM, Andrew Zaborowski wrote:
> Hi Denis,
>
> On Thu, 24 Oct 2019 at 17:29, Denis Kenzior <denkenz@gmail.com> wrote:
>>
>> Hi Andrew,
>>
>> On 10/23/19 10:22 PM, Andrew Zaborowski wrote:
>>> On Thu, 24 Oct 2019 at 04:54, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> 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.
>>
>> 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.
>
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.
>>
>> The kernel API is not ideal. Lets make it work, but don't go crazy ;)
>>
>>>>
>>>> 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...
>>
>>>
>>>>
>>>> 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,
>>> };
>>>
>>
>> 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.
>
You already have 3 groups / netdev above where I'd just have one / netdev.
>>
>>>> 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;
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.
> ...
> };
>
> 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.
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.
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 22:16 ` Denis Kenzior
@ 2019-10-24 22:45 ` Andrew Zaborowski
2019-10-25 1:27 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-24 22:45 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 8983 bytes --]
frame_watch_set_add(group, ...);
On Fri, 25 Oct 2019 at 00:16, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/24/19 4:47 PM, Andrew Zaborowski wrote:
> > On Thu, 24 Oct 2019 at 17:29, Denis Kenzior <denkenz@gmail.com> wrote:
> >>
> >> Hi Andrew,
> >>
> >> On 10/23/19 10:22 PM, Andrew Zaborowski wrote:
> >>> On Thu, 24 Oct 2019 at 04:54, Denis Kenzior <denkenz@gmail.com> wrote:
> >>>> 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.
> >>
> >> 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.
> >
>
> This happens already. Look at the mac80211 rx path. I only really care
Right but for some drivers this will happen in the firmware and the
kernel won't be bothered if we haven't registered to those frame
prefixes.
> 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.
>
> >>
> >> The kernel API is not ideal. Lets make it work, but don't go crazy ;)
> >>
> >>>>
> >>>> 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...
> >>
> >>>
> >>>>
> >>>> 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,
> >>> };
> >>>
> >>
> >> 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.
> >
>
> You already have 3 groups / netdev above where I'd just have one / netdev.
You'll probably going to want one for the neighbor report responses,
but in any case we'll have more at some point.
>
> >>
> >>>> 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;
>
> 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.
>
> 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.
>
> > ...
> > };
> >
> > 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.
>
> 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.
>
> 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.
>
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-24 22:45 ` Andrew Zaborowski
@ 2019-10-25 1:27 ` Denis Kenzior
2019-10-25 2:59 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-25 1:27 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5578 bytes --]
Hi Andrew,
>> 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
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.
> 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.
>>
>> 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.
>>
>> 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, ...);
>
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.
> 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.
>
> 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.
>>
>> 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.
>
>>
>> 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)
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);
Regards,
-Denis
>
> Best regards
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-25 1:27 ` Denis Kenzior
@ 2019-10-25 2:59 ` Andrew Zaborowski
2019-10-25 3:56 ` Denis Kenzior
0 siblings, 1 reply; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-25 2:59 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 9960 bytes --]
On Fri, 25 Oct 2019 at 03:27, Denis Kenzior <denkenz@gmail.com> wrote:
> > 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.
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.
>
> > 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.
>
> >>
> >> 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.
Ok. Well, you can always change the uint32 into an opaque pointer to
avoid some lookups, but I understand what you mean.
What I was really suggesting was for the groups to be managed by the
framewatch module but be short lived.
>
> >>
> >> 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, ...);
> >
>
> 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.
>
> > frame_watch_register_set(netdev, group, user_data);
>
> Well this is not what you suggested earlier with an enumeration. So now
Right! I wasn't clear... here I was only replying to the "groups can't
be global" statement but not arguing for doing it this way. I'd
rather have them only exist per wdev/netdev.
> 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.
>
> >
> > 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.
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.
>
> >>
> >> 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.
> >
> >>
> >> 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)
>
> 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);
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.
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-25 2:59 ` Andrew Zaborowski
@ 2019-10-25 3:56 ` Denis Kenzior
2019-10-25 4:42 ` Andrew Zaborowski
0 siblings, 1 reply; 36+ messages in thread
From: Denis Kenzior @ 2019-10-25 3:56 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 7574 bytes --]
Hi Andrew,
> 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.
>>
>> 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.
Right
>
> 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 ;)
> 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.
> 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.
>
> 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.
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.
>>
>> 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);
>
> 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.
> 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.
> 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.
>
> You still would use a new socket for each frame_set_new() call, right?
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
Okay, this is what I understood.
> 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.
> 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.
>
Sure, and I'm less worried about the tear-down than I'm about the setup
at connection time.
Regards,
-Denis
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API
2019-10-25 3:56 ` Denis Kenzior
@ 2019-10-25 4:42 ` Andrew Zaborowski
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Zaborowski @ 2019-10-25 4:42 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 8137 bytes --]
On Fri, 25 Oct 2019 at 05:56, Denis Kenzior <denkenz@gmail.com> wrote:
> > 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.
Makes sense. We can probably think about moving some things to after
signalling the connection is operative (I think we set up the GTK this
way in some scenarios)
Or instead of registering to some of those frames in station mode at
connection time like I was mentioning before we can keep them
registered the whole time like we're going now. Since we're not on
any channel until we call CMD_CONNECT we wouldn't receiving frames
(not sure about during scanning...)
> >
> > 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 ;)
>
> > 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.
>
> > 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.
>
> >
> > 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.
>
> 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.
So that's kind of what I'd like to avoid because this makes for
readable code where in each step of a sequence (and there are a few
different sequences) you say what frames you watch for next. So I
think p2p_peer_frame_xchg can register to those frames only when
called and unregister from them when one is actually received but
without tearing down that group and causing the netlink socket to be
closed.
>
> >>
> >> 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);
> >
> > 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.
>
> > 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.
>
> > 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.
>
> >
> > You still would use a new socket for each frame_set_new() call, right?
>
> 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
>
> Okay, this is what I understood.
>
> > 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.
Right, I'd do that. If we have a group id called
FRAME_GROUP_CONNECTION for all the watches we want during a connection
it's safe to unregister it on disconnect without checking if there
were any actual active watches in that group.
Do you still think it's better to have an explicit call to create the
group and receive a dynamic id or pointer that has to be saved?
Thinking about this, since those groups may end up having no frame
watches it will still be better to defer actually opening the socket
until the first frame prefix is registered by the user.
Best regards
^ permalink raw reply [flat|nested] 36+ messages in thread