* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-13 13:32 [PATCH 1/3] wsc: Split out enrollee state machine to own object Andrew Zaborowski
@ 2020-01-13 12:56 ` Denis Kenzior
2020-01-13 18:38 ` Andrew Zaborowski
2020-01-13 13:32 ` [PATCH 2/3] wsc: Refactor WSC D-Bus interface logic Andrew Zaborowski
1 sibling, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2020-01-13 12:56 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]
Hi Andrew,
On 1/13/20 7:32 AM, Andrew Zaborowski wrote:
> Create struct wsc_enrollee which is allocated with wsc_enrollee_new,
> taking a done callback as a parameter. The callback is always
> called so there's no need for a separate destroy callback. The object
> only lives until the done callback happens so wsc_enrollee_cancel can
> only be used before this.
>
> Looks like the rest of the file is much simplified thanks to this.
Yes, this is looking really nice now. Just a couple of small things:
> ---
> src/wsc.c | 456 ++++++++++++++++++++++++++----------------------------
> src/wsc.h | 8 +
> 2 files changed, 228 insertions(+), 236 deletions(-)
>
<snip>
> +void wsc_enrollee_cancel(struct wsc_enrollee *wsce)
> +{
> + wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
> + wsce->done_cb = NULL;
> + /* Results in a call to wsc_enrollee_free */
> + netdev_disconnect(wsce->netdev, NULL, NULL);
Are you sure you don't want to wait for netdev_disconnect to finish
prior to signaling the request as aborted & canceled? In theory this
could lead to station bouncing against an -EBUSY error once
station_set_autoconnect is called.
> +
> + /*
> + * TODO: Mark this network as known. We might be getting
> + * multiple credentials from WSC, so there is a possibility
> + * that the network is not known and / or not in scan results.
> + * In both cases, the network should be considered for
> + * auto-connect. Note, since we sync the settings, the next
> + * reboot will put the network on the known list.
> + */
This TODO comment is now likely stale and can be removed (in a separate
commit).
> + }
> }
>
> /* Done callback for when WSC is triggered by DBus methods */
> @@ -541,14 +530,22 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
> {
> struct wsc *wsc = user_data;
>
> + wsc->enrollee = NULL;
> + wsc->target = NULL;
> +
> l_debug("err=%i", err);
>
> + if (err && wsc->station)
> + station_set_autoconnect(wsc->station, true);
> +
> switch (err) {
> case 0:
> break;
> case -ECANCELED:
> - dbus_pending_reply(&wsc->pending,
> + if (wsc->pending)
> + dbus_pending_reply(&wsc->pending,
> dbus_error_aborted(wsc->pending));
Hmm, why the if ()?
Also, if you wait until netdev_disconnect_cb to signal done_cb with
-ECANCELED, you can reply to both the pending and cancel messages here.
> +
> return;
> case -ENOKEY:
> dbus_pending_reply(&wsc->pending,
<snip>
> diff --git a/src/wsc.h b/src/wsc.h
> index ada08c89..48a3297a 100644
> --- a/src/wsc.h
> +++ b/src/wsc.h
> @@ -31,5 +31,13 @@ struct wsc_credentials_info {
> bool has_passphrase;
> };
>
> +struct wsc_enrollee;
> +
> typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
> unsigned int n_creds, void *user_data);
> +
> +struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
> + struct scan_bss *target,
> + const char *pin,
> + wsc_done_cb_t done_cb, void *user_data);
> +void wsc_enrollee_cancel(struct wsc_enrollee *wsce);
>
You may still want an explicit wsc_enrollee_free for cases where
netdev_disconnect shouldn't be used / attempted.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] wsc: Split out enrollee state machine to own object
@ 2020-01-13 13:32 Andrew Zaborowski
2020-01-13 12:56 ` Denis Kenzior
2020-01-13 13:32 ` [PATCH 2/3] wsc: Refactor WSC D-Bus interface logic Andrew Zaborowski
0 siblings, 2 replies; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-13 13:32 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 21209 bytes --]
Create struct wsc_enrollee which is allocated with wsc_enrollee_new,
taking a done callback as a parameter. The callback is always
called so there's no need for a separate destroy callback. The object
only lives until the done callback happens so wsc_enrollee_cancel can
only be used before this.
Looks like the rest of the file is much simplified thanks to this.
---
src/wsc.c | 456 ++++++++++++++++++++++++++----------------------------
src/wsc.h | 8 +
2 files changed, 228 insertions(+), 236 deletions(-)
diff --git a/src/wsc.c b/src/wsc.c
index d50f240f..33379b81 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -51,27 +51,26 @@
static uint32_t netdev_watch = 0;
+struct wsc_enrollee {
+ struct netdev *netdev;
+ struct wsc_credentials_info creds[3];
+ uint32_t n_creds;
+ struct l_settings *eap_settings;
+ wsc_done_cb_t done_cb;
+ void *done_data;
+};
+
struct wsc {
struct netdev *netdev;
struct station *station;
+ struct wsc_enrollee *enrollee;
struct l_dbus_message *pending;
- struct l_dbus_message *pending_cancel;
uint8_t *wsc_ies;
size_t wsc_ies_size;
struct l_timeout *walk_timer;
uint32_t scan_id;
struct scan_bss *target;
uint32_t station_state_watch;
- struct wsc_credentials_info 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;
};
static struct l_dbus_message *wsc_error_session_overlap(
@@ -113,171 +112,47 @@ static struct l_dbus_message *wsc_error_time_expired(struct l_dbus_message *msg)
"No APs in PIN mode found in "
"the alloted time");
}
-static void wsc_try_credentials(struct wsc *wsc,
- struct wsc_credentials_info *creds,
- unsigned int n_creds)
-{
- unsigned int i;
- struct network *network;
- struct scan_bss *bss;
-
- for (i = 0; i < n_creds; i++) {
- network = station_network_find(wsc->station, creds[i].ssid,
- creds[i].security);
- if (!network)
- continue;
-
- bss = network_bss_find_by_addr(network, creds[i].addr);
-
- if (!bss)
- bss = network_bss_select(network, true);
- if (!bss)
- continue;
-
- if (creds[i].security == SECURITY_PSK) {
- bool ret;
-
- /*
- * Prefer setting passphrase, this will work for both
- * WPA2 and WPA3 since the PSK can always be generated
- * if needed
- */
- if (creds[i].has_passphrase)
- ret = network_set_passphrase(network,
- creds[i].passphrase);
- else
- ret = network_set_psk(network, creds[i].psk);
-
- if (!ret)
- continue;
- }
-
- station_connect_network(wsc->station, network, bss,
- wsc->pending);
- l_dbus_message_unref(wsc->pending);
- wsc->pending = NULL;
-
- return;
- }
-
- dbus_pending_reply(&wsc->pending,
- wsc_error_not_reachable(wsc->pending));
- station_set_autoconnect(wsc->station, true);
-}
-
-static void wsc_store_credentials(struct wsc_credentials_info *creds,
- unsigned int n_creds)
+static void wsc_enrollee_free(struct wsc_enrollee *wsce)
{
- unsigned int i;
-
- for (i = 0; i < n_creds; i++) {
- enum security security = creds[i].security;
- const char *ssid = creds[i].ssid;
- struct l_settings *settings = l_settings_new();
-
- l_debug("Storing credential for '%s(%s)'", ssid,
- security_to_str(security));
-
- if (security == SECURITY_PSK) {
- char *hex = l_util_hexstring(creds[i].psk,
- sizeof(creds[i].psk));
-
- l_settings_set_value(settings, "Security",
- "PreSharedKey", hex);
- explicit_bzero(hex, strlen(hex));
- l_free(hex);
- }
-
- storage_network_sync(security, ssid, settings);
- l_settings_free(settings);
-
- /*
- * TODO: Mark this network as known. We might be getting
- * multiple credentials from WSC, so there is a possibility
- * that the network is not known and / or not in scan results.
- * In both cases, the network should be considered for
- * auto-connect. Note, since we sync the settings, the next
- * reboot will put the network on the known list.
- */
- }
-}
-
-static void wsc_disconnect_cb(struct netdev *netdev, bool success,
- void *user_data)
-{
- struct wsc *wsc = user_data;
- struct l_dbus_message *reply;
-
- l_debug("%p, success: %d", wsc, success);
-
- wsc->wsc_association = false;
-
- reply = l_dbus_message_new_method_return(wsc->pending_cancel);
- l_dbus_message_set_arguments(reply, "");
- dbus_pending_reply(&wsc->pending_cancel, reply);
-
- 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;
+ l_settings_free(wsce->eap_settings);
+ explicit_bzero(wsce->creds, sizeof(wsce->creds));
+ l_free(wsce);
}
-static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
+static void wsc_enrollee_connect_cb(struct netdev *netdev,
+ enum netdev_result result,
void *event_data, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
- l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
+ l_debug("%d, result: %d", netdev_get_ifindex(wsce->netdev), result);
- wsc_connect_cleanup(wsc);
+ if (!wsce->done_cb)
+ goto done;
- if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
- struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
- uint32_t n_creds = wsc->n_creds;
-
- /*
- * Once we call done_cb, the state is assumed to be wiped,
- * so use a temporary array for creds here
- */
- 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;
+ if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsce->n_creds > 0) {
+ wsce->done_cb(0, wsce->creds, wsce->n_creds, wsce->done_data);
+ goto done;
}
switch (result) {
case NETDEV_RESULT_ABORTED:
- wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
- return;
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ break;
case NETDEV_RESULT_HANDSHAKE_FAILED:
- wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
+ wsce->done_cb(-ENOKEY, NULL, 0, wsce->done_data);
break;
default:
- wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
+ wsce->done_cb(-EIO, NULL, 0, wsce->done_data);
break;
}
- if (wsc->station)
- station_set_autoconnect(wsc->station, true);
+done:
+ wsc_enrollee_free(wsce);
}
-static void wsc_credential_obtained(struct wsc *wsc,
+static void wsc_enrollee_credential_obtained(struct wsc_enrollee *wsce,
const struct wsc_credential *cred)
{
uint16_t auth_mask;
@@ -294,7 +169,7 @@ static void wsc_credential_obtained(struct wsc *wsc,
l_debug("Key (%u): %.*s", cred->network_key_len,
cred->network_key_len, cred->network_key);
- if (wsc->n_creds == L_ARRAY_SIZE(wsc->creds)) {
+ if (wsce->n_creds == L_ARRAY_SIZE(wsce->creds)) {
l_warn("Maximum number of credentials obtained, ignoring...");
return;
}
@@ -304,8 +179,8 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- memcpy(wsc->creds[wsc->n_creds].ssid, cred->ssid, cred->ssid_len);
- wsc->creds[wsc->n_creds].ssid[cred->ssid_len] = '\0';
+ memcpy(wsce->creds[wsce->n_creds].ssid, cred->ssid, cred->ssid_len);
+ wsce->creds[wsce->n_creds].ssid[cred->ssid_len] = '\0';
/* We only support open/personal wpa/personal wpa2 */
auth_mask = WSC_AUTHENTICATION_TYPE_OPEN |
@@ -324,11 +199,11 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- wsc->creds[wsc->n_creds].security = SECURITY_NONE;
+ wsce->creds[wsce->n_creds].security = SECURITY_NONE;
} else
- wsc->creds[wsc->n_creds].security = SECURITY_PSK;
+ wsce->creds[wsce->n_creds].security = SECURITY_PSK;
- switch (wsc->creds[wsc->n_creds].security) {
+ switch (wsce->creds[wsce->n_creds].security) {
case SECURITY_NONE:
if (cred->network_key_len != 0) {
l_warn("ignoring invalid open key length");
@@ -347,14 +222,14 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- memcpy(wsc->creds[wsc->n_creds].psk, decoded, 32);
+ memcpy(wsce->creds[wsce->n_creds].psk, decoded, 32);
explicit_bzero(decoded, 32);
l_free(decoded);
} else {
- strncpy(wsc->creds[wsc->n_creds].passphrase,
+ strncpy(wsce->creds[wsce->n_creds].passphrase,
(const char *) cred->network_key,
cred->network_key_len);
- wsc->creds[wsc->n_creds].has_passphrase = true;
+ wsce->creds[wsce->n_creds].has_passphrase = true;
}
break;
@@ -362,25 +237,27 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- for (i = 0; i < wsc->n_creds; i++) {
- if (strcmp(wsc->creds[i].ssid, wsc->creds[wsc->n_creds].ssid))
+ for (i = 0; i < wsce->n_creds; i++) {
+ if (strcmp(wsce->creds[i].ssid,
+ wsce->creds[wsce->n_creds].ssid))
continue;
l_warn("Found duplicate credentials for SSID: %s",
- wsc->creds[i].ssid);
- explicit_bzero(&wsc->creds[wsc->n_creds],
- sizeof(wsc->creds[wsc->n_creds]));
+ wsce->creds[i].ssid);
+ explicit_bzero(&wsce->creds[wsce->n_creds],
+ sizeof(wsce->creds[wsce->n_creds]));
return;
}
- memcpy(wsc->creds[wsc->n_creds].addr, cred->addr, 6);
- wsc->n_creds += 1;
+ memcpy(wsce->creds[wsce->n_creds].addr, cred->addr, 6);
+ wsce->n_creds += 1;
}
-static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
+static void wsc_enrollee_netdev_event(struct netdev *netdev,
+ enum netdev_event event,
void *event_data, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
switch (event) {
case NETDEV_EVENT_AUTHENTICATING:
@@ -391,8 +268,9 @@ static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
break;
case NETDEV_EVENT_DISCONNECT_BY_AP:
l_debug("Disconnect by AP");
- wsc_connect_cb(wsc->netdev, NETDEV_RESULT_HANDSHAKE_FAILED,
- event_data, wsc);
+ wsc_enrollee_connect_cb(wsce->netdev,
+ NETDEV_RESULT_HANDSHAKE_FAILED,
+ event_data, wsce);
break;
case NETDEV_EVENT_RSSI_THRESHOLD_LOW:
case NETDEV_EVENT_RSSI_THRESHOLD_HIGH:
@@ -403,11 +281,11 @@ static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
};
}
-static void wsc_handshake_event(struct handshake_state *hs,
- enum handshake_event event, void *user_data,
- ...)
+static void wsc_enrollee_handshake_event(struct handshake_state *hs,
+ enum handshake_event event,
+ void *user_data, ...)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
va_list args;
va_start(args, user_data);
@@ -422,7 +300,7 @@ static void wsc_handshake_event(struct handshake_state *hs,
switch (eap_event) {
case EAP_WSC_EVENT_CREDENTIAL_OBTAINED:
- wsc_credential_obtained(wsc,
+ wsc_enrollee_credential_obtained(wsce,
va_arg(args, const struct wsc_credential *));
break;
default:
@@ -454,20 +332,18 @@ 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 int wsc_enrollee_connect(struct wsc_enrollee *wsce, struct scan_bss *bss,
+ const char *pin)
{
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;
-
- hs = netdev_handshake_state_new(wsc->netdev);
+ hs = netdev_handshake_state_new(wsce->netdev);
l_settings_set_string(settings, "Security", "EAP-Identity",
"WFA-SimpleConfig-Enrollee-1-0");
@@ -482,25 +358,23 @@ static void wsc_connect(struct wsc *wsc)
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)));
+ util_address_to_string(netdev_get_address(wsce->netdev)));
- if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+ if (pin) {
enum wsc_device_password_id dpid;
- if (strlen(wsc->pin) == 4 ||
- wsc_pin_is_checksum_valid(wsc->pin))
+ if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(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",
- wsc->pin);
+ l_settings_set_string(settings, "WSC", "DevicePassword", pin);
}
- handshake_state_set_event_func(hs, wsc_handshake_event, wsc);
+ handshake_state_set_event_func(hs, wsc_enrollee_handshake_event, wsce);
handshake_state_set_8021x_config(hs, settings);
- wsc->eap_settings = settings;
+ wsce->eap_settings = settings;
request.version2 = true;
request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
@@ -520,19 +394,134 @@ static void wsc_connect(struct wsc *wsc)
goto error;
}
- r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
- wsc_connect_cb, wsc);
+ r = netdev_connect(wsce->netdev, bss, hs, &ie_iov, 1,
+ wsc_enrollee_netdev_event,
+ wsc_enrollee_connect_cb, wsce);
l_free(ie_iov.iov_base);
- if (r < 0)
- goto error;
+ if (r == 0)
+ return 0;
- wsc->wsc_association = true;
- return;
error:
handshake_state_free(hs);
- wsc_connect_cleanup(wsc);
- wsc->done_cb(r, NULL, 0, wsc->done_data);
+ return r;
+}
+
+struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
+ struct scan_bss *target,
+ const char *pin,
+ wsc_done_cb_t done_cb, void *user_data)
+{
+ struct wsc_enrollee *wsce;
+
+ wsce = l_new(struct wsc_enrollee, 1);
+ wsce->netdev = netdev;
+ wsce->done_cb = done_cb;
+ wsce->done_data = user_data;
+
+ if (wsc_enrollee_connect(wsce, target, pin) == 0)
+ return wsce;
+
+ wsc_enrollee_free(wsce);
+ return NULL;
+}
+
+void wsc_enrollee_cancel(struct wsc_enrollee *wsce)
+{
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ wsce->done_cb = NULL;
+ /* Results in a call to wsc_enrollee_free */
+ netdev_disconnect(wsce->netdev, NULL, NULL);
+}
+
+static void wsc_try_credentials(struct wsc *wsc,
+ struct wsc_credentials_info *creds,
+ unsigned int n_creds)
+{
+ unsigned int i;
+ struct network *network;
+ struct scan_bss *bss;
+
+ for (i = 0; i < n_creds; i++) {
+ network = station_network_find(wsc->station, creds[i].ssid,
+ creds[i].security);
+ if (!network)
+ continue;
+
+ bss = network_bss_find_by_addr(network, creds[i].addr);
+
+ if (!bss)
+ bss = network_bss_select(network, true);
+
+ if (!bss)
+ continue;
+
+ if (creds[i].security == SECURITY_PSK) {
+ bool ret;
+
+ /*
+ * Prefer setting passphrase, this will work for both
+ * WPA2 and WPA3 since the PSK can always be generated
+ * if needed
+ */
+ if (creds[i].has_passphrase)
+ ret = network_set_passphrase(network,
+ creds[i].passphrase);
+ else
+ ret = network_set_psk(network, creds[i].psk);
+
+ if (!ret)
+ continue;
+ }
+
+ station_connect_network(wsc->station, network, bss,
+ wsc->pending);
+ l_dbus_message_unref(wsc->pending);
+ wsc->pending = NULL;
+
+ return;
+ }
+
+ dbus_pending_reply(&wsc->pending,
+ wsc_error_not_reachable(wsc->pending));
+ station_set_autoconnect(wsc->station, true);
+}
+
+static void wsc_store_credentials(struct wsc_credentials_info *creds,
+ unsigned int n_creds)
+{
+ unsigned int i;
+
+ for (i = 0; i < n_creds; i++) {
+ enum security security = creds[i].security;
+ const char *ssid = creds[i].ssid;
+ struct l_settings *settings = l_settings_new();
+
+ l_debug("Storing credential for '%s(%s)'", ssid,
+ security_to_str(security));
+
+ if (security == SECURITY_PSK) {
+ char *hex = l_util_hexstring(creds[i].psk,
+ sizeof(creds[i].psk));
+
+ l_settings_set_value(settings, "Security",
+ "PreSharedKey", hex);
+ explicit_bzero(hex, strlen(hex));
+ l_free(hex);
+ }
+
+ storage_network_sync(security, ssid, settings);
+ l_settings_free(settings);
+
+ /*
+ * TODO: Mark this network as known. We might be getting
+ * multiple credentials from WSC, so there is a possibility
+ * that the network is not known and / or not in scan results.
+ * In both cases, the network should be considered for
+ * auto-connect. Note, since we sync the settings, the next
+ * reboot will put the network on the known list.
+ */
+ }
}
/* Done callback for when WSC is triggered by DBus methods */
@@ -541,14 +530,22 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
{
struct wsc *wsc = user_data;
+ wsc->enrollee = NULL;
+ wsc->target = NULL;
+
l_debug("err=%i", err);
+ if (err && wsc->station)
+ station_set_autoconnect(wsc->station, true);
+
switch (err) {
case 0:
break;
case -ECANCELED:
- dbus_pending_reply(&wsc->pending,
+ if (wsc->pending)
+ dbus_pending_reply(&wsc->pending,
dbus_error_aborted(wsc->pending));
+
return;
case -ENOKEY:
dbus_pending_reply(&wsc->pending,
@@ -568,6 +565,21 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
wsc_try_credentials(wsc, creds, n_creds);
}
+static void wsc_connect(struct wsc *wsc)
+{
+ const char *pin = NULL;
+
+ if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
+ l_dbus_message_get_arguments(wsc->pending, "s", &pin);
+
+ wsc->enrollee = wsc_enrollee_new(wsc->netdev, wsc->target, pin,
+ wsc_dbus_done_cb, wsc);
+ if (wsc->enrollee)
+ return;
+
+ wsc_dbus_done_cb(-EIO, NULL, 0, wsc);
+}
+
static void station_state_watch(enum station_state state, void *userdata)
{
struct wsc *wsc = userdata;
@@ -594,22 +606,6 @@ 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);
@@ -633,7 +629,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
break;
}
error:
- wsc_connect_cleanup(wsc);
+ wsc->target = NULL;
dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
}
@@ -1105,21 +1101,11 @@ static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
wsc->target = NULL;
}
- if (wsc->wsc_association) {
- int r;
-
- r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
- if (r == 0) {
- wsc->pending_cancel = l_dbus_message_ref(message);
- return NULL;
- }
-
- l_warn("Unable to initiate disconnect: %s", strerror(-r));
- wsc->wsc_association = false;
- }
-
dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
+ if (wsc->enrollee)
+ wsc_enrollee_cancel(wsc->enrollee);
+
reply = l_dbus_message_new_method_return(message);
l_dbus_message_set_arguments(reply, "");
@@ -1143,7 +1129,6 @@ 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,
@@ -1155,9 +1140,8 @@ static void wsc_free(void *userdata)
dbus_pending_reply(&wsc->pending,
dbus_error_not_available(wsc->pending));
- if (wsc->pending_cancel)
- dbus_pending_reply(&wsc->pending_cancel,
- dbus_error_aborted(wsc->pending_cancel));
+ if (wsc->enrollee)
+ wsc_enrollee_cancel(wsc->enrollee);
l_free(wsc);
}
diff --git a/src/wsc.h b/src/wsc.h
index ada08c89..48a3297a 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -31,5 +31,13 @@ struct wsc_credentials_info {
bool has_passphrase;
};
+struct wsc_enrollee;
+
typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
unsigned int n_creds, void *user_data);
+
+struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
+ struct scan_bss *target,
+ const char *pin,
+ wsc_done_cb_t done_cb, void *user_data);
+void wsc_enrollee_cancel(struct wsc_enrollee *wsce);
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] wsc: Refactor WSC D-Bus interface logic
2020-01-13 13:32 [PATCH 1/3] wsc: Split out enrollee state machine to own object Andrew Zaborowski
2020-01-13 12:56 ` Denis Kenzior
@ 2020-01-13 13:32 ` Andrew Zaborowski
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-13 13:32 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 19068 bytes --]
Split the WSC D-Bus interface class (struct wsc) into a base class
common to station mode and P2P mode (struct wsc_dbus) and station-
specific logic like scanning, saving the credentials as a known network
and triggering the station-mode connection (struct wsc_station_dbus).
Make the base class and its utilities public in wsc.h for P2P use.
---
src/wsc.c | 358 +++++++++++++++++++++++++++++++++---------------------
src/wsc.h | 13 ++
2 files changed, 235 insertions(+), 136 deletions(-)
diff --git a/src/wsc.c b/src/wsc.c
index 33379b81..74fe8aa8 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -60,19 +60,6 @@ struct wsc_enrollee {
void *done_data;
};
-struct wsc {
- struct netdev *netdev;
- struct station *station;
- struct wsc_enrollee *enrollee;
- struct l_dbus_message *pending;
- uint8_t *wsc_ies;
- size_t wsc_ies_size;
- struct l_timeout *walk_timer;
- uint32_t scan_id;
- struct scan_bss *target;
- uint32_t station_state_watch;
-};
-
static struct l_dbus_message *wsc_error_session_overlap(
struct l_dbus_message *msg)
{
@@ -434,7 +421,20 @@ void wsc_enrollee_cancel(struct wsc_enrollee *wsce)
netdev_disconnect(wsce->netdev, NULL, NULL);
}
-static void wsc_try_credentials(struct wsc *wsc,
+struct wsc_station_dbus {
+ struct wsc_dbus super;
+ struct wsc_enrollee *enrollee;
+ struct scan_bss *target;
+ struct netdev *netdev;
+ struct station *station;
+ uint8_t *wsc_ies;
+ size_t wsc_ies_size;
+ struct l_timeout *walk_timer;
+ uint32_t scan_id;
+ uint32_t station_state_watch;
+};
+
+static void wsc_try_credentials(struct wsc_station_dbus *wsc,
struct wsc_credentials_info *creds,
unsigned int n_creds)
{
@@ -475,15 +475,15 @@ static void wsc_try_credentials(struct wsc *wsc,
}
station_connect_network(wsc->station, network, bss,
- wsc->pending);
- l_dbus_message_unref(wsc->pending);
- wsc->pending = NULL;
+ wsc->super.pending_connect);
+ l_dbus_message_unref(wsc->super.pending_connect);
+ wsc->super.pending_connect = NULL;
return;
}
- dbus_pending_reply(&wsc->pending,
- wsc_error_not_reachable(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ wsc_error_not_reachable(wsc->super.pending_connect));
station_set_autoconnect(wsc->station, true);
}
@@ -524,11 +524,10 @@ static void wsc_store_credentials(struct wsc_credentials_info *creds,
}
}
-/* Done callback for when WSC is triggered by DBus methods */
static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
unsigned int n_creds, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_station_dbus *wsc = user_data;
wsc->enrollee = NULL;
wsc->target = NULL;
@@ -542,22 +541,26 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
case 0:
break;
case -ECANCELED:
- if (wsc->pending)
- dbus_pending_reply(&wsc->pending,
- dbus_error_aborted(wsc->pending));
+ if (wsc->super.pending_connect)
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_aborted(
+ wsc->super.pending_connect));
return;
case -ENOKEY:
- dbus_pending_reply(&wsc->pending,
- wsc_error_no_credentials(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ wsc_error_no_credentials(
+ wsc->super.pending_connect));
return;
case -EBUSY:
- dbus_pending_reply(&wsc->pending,
- dbus_error_busy(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_busy(
+ wsc->super.pending_connect));
return;
default:
- dbus_pending_reply(&wsc->pending,
- dbus_error_failed(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_failed(
+ wsc->super.pending_connect));
return;
}
@@ -565,12 +568,14 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
wsc_try_credentials(wsc, creds, n_creds);
}
-static void wsc_connect(struct wsc *wsc)
+static void wsc_connect(struct wsc_station_dbus *wsc)
{
const char *pin = NULL;
- if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
- l_dbus_message_get_arguments(wsc->pending, "s", &pin);
+ if (!strcmp(l_dbus_message_get_member(wsc->super.pending_connect),
+ "StartPin"))
+ l_dbus_message_get_arguments(wsc->super.pending_connect, "s",
+ &pin);
wsc->enrollee = wsc_enrollee_new(wsc->netdev, wsc->target, pin,
wsc_dbus_done_cb, wsc);
@@ -582,7 +587,7 @@ static void wsc_connect(struct wsc *wsc)
static void station_state_watch(enum station_state state, void *userdata)
{
- struct wsc *wsc = userdata;
+ struct wsc_station_dbus *wsc = userdata;
if (state != STATION_STATE_DISCONNECTED)
return;
@@ -595,7 +600,8 @@ static void station_state_watch(enum station_state state, void *userdata)
wsc_connect(wsc);
}
-static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
+static void wsc_check_can_connect(struct wsc_station_dbus *wsc,
+ struct scan_bss *target)
{
l_debug("%p", wsc);
@@ -630,10 +636,11 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
}
error:
wsc->target = NULL;
- dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_failed(wsc->super.pending_connect));
}
-static void wsc_cancel_scan(struct wsc *wsc)
+static void wsc_cancel_scan(struct wsc_station_dbus *wsc)
{
l_free(wsc->wsc_ies);
wsc->wsc_ies = 0;
@@ -651,30 +658,32 @@ static void wsc_cancel_scan(struct wsc *wsc)
static void walk_timeout(struct l_timeout *timeout, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_station_dbus *wsc = user_data;
wsc_cancel_scan(wsc);
- if (wsc->pending)
- dbus_pending_reply(&wsc->pending,
- wsc_error_walk_time_expired(wsc->pending));
+ if (wsc->super.pending_connect)
+ dbus_pending_reply(&wsc->super.pending_connect,
+ wsc_error_walk_time_expired(
+ wsc->super.pending_connect));
}
static void pin_timeout(struct l_timeout *timeout, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_station_dbus *wsc = user_data;
wsc_cancel_scan(wsc);
- if (wsc->pending)
- dbus_pending_reply(&wsc->pending,
- wsc_error_time_expired(wsc->pending));
+ if (wsc->super.pending_connect)
+ dbus_pending_reply(&wsc->super.pending_connect,
+ wsc_error_time_expired(
+ wsc->super.pending_connect));
}
static bool push_button_scan_results(int err, struct l_queue *bss_list,
void *userdata)
{
- struct wsc *wsc = userdata;
+ struct wsc_station_dbus *wsc = userdata;
struct scan_bss *bss_2g;
struct scan_bss *bss_5g;
struct scan_bss *target;
@@ -685,8 +694,8 @@ static bool push_button_scan_results(int err, struct l_queue *bss_list,
if (err) {
wsc_cancel_scan(wsc);
- dbus_pending_reply(&wsc->pending,
- dbus_error_failed(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_failed(wsc->super.pending_connect));
return false;
}
@@ -787,8 +796,8 @@ static bool push_button_scan_results(int err, struct l_queue *bss_list,
session_overlap:
wsc_cancel_scan(wsc);
- dbus_pending_reply(&wsc->pending,
- wsc_error_session_overlap(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ wsc_error_session_overlap(wsc->super.pending_connect));
return false;
}
@@ -832,15 +841,15 @@ static bool pin_scan_results(int err, struct l_queue *bss_list, void *userdata)
{
static const uint8_t wildcard_address[] =
{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
- struct wsc *wsc = userdata;
+ struct wsc_station_dbus *wsc = userdata;
struct scan_bss *target = NULL;
const struct l_queue_entry *bss_entry;
struct wsc_probe_response probe_response;
if (err) {
wsc_cancel_scan(wsc);
- dbus_pending_reply(&wsc->pending,
- dbus_error_failed(wsc->pending));
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_failed(wsc->super.pending_connect));
return false;
}
@@ -932,7 +941,7 @@ static bool pin_scan_results(int err, struct l_queue *bss_list, void *userdata)
return true;
}
-static bool wsc_initiate_scan(struct wsc *wsc,
+static bool wsc_initiate_scan(struct wsc_station_dbus *wsc,
enum wsc_device_password_id dpid,
scan_notify_func_t callback)
{
@@ -996,28 +1005,117 @@ static bool wsc_initiate_scan(struct wsc *wsc,
return true;
}
+static const char *wsc_station_dbus_get_path(struct wsc_dbus *super)
+{
+ struct wsc_station_dbus *wsc =
+ l_container_of(super, struct wsc_station_dbus, super);
+
+ return netdev_get_path(wsc->netdev);
+}
+
+static void wsc_station_dbus_connect(struct wsc_dbus *super,
+ const char *pin)
+{
+ struct wsc_station_dbus *wsc =
+ l_container_of(super, struct wsc_station_dbus, super);
+ scan_notify_func_t scan_callback;
+ enum wsc_device_password_id dpid;
+
+ wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
+ if (!wsc->station) {
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_not_available(
+ wsc->super.pending_connect));
+ return;
+ }
+
+ if (pin) {
+ if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
+ dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+ else
+ dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+
+ scan_callback = pin_scan_results;
+ } else {
+ dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
+ scan_callback = push_button_scan_results;
+ }
+
+ if (!wsc_initiate_scan(wsc, dpid, scan_callback)) {
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_failed(
+ wsc->super.pending_connect));
+ return;
+ }
+
+ if (pin) {
+ wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
+ } else {
+ wsc->walk_timer = l_timeout_create(WALK_TIME, walk_timeout, wsc,
+ NULL);
+ }
+}
+
+static void wsc_station_dbus_cancel(struct wsc_dbus *super)
+{
+ struct wsc_station_dbus *wsc =
+ l_container_of(super, struct wsc_station_dbus, super);
+
+ wsc_cancel_scan(wsc);
+
+ if (wsc->station_state_watch) {
+ station_remove_state_watch(wsc->station,
+ wsc->station_state_watch);
+ wsc->station_state_watch = 0;
+ wsc->target = NULL;
+ }
+
+ dbus_pending_reply(&wsc->super.pending_connect,
+ dbus_error_aborted(wsc->super.pending_connect));
+
+ if (wsc->enrollee)
+ wsc_enrollee_cancel(wsc->enrollee);
+
+ dbus_pending_reply(&wsc->super.pending_cancel,
+ l_dbus_message_new_method_return(
+ wsc->super.pending_cancel));
+}
+
+static void wsc_station_dbus_remove(struct wsc_dbus *super)
+{
+ struct wsc_station_dbus *wsc =
+ l_container_of(super, struct wsc_station_dbus, super);
+
+ wsc_cancel_scan(wsc);
+
+ if (wsc->station_state_watch) {
+ station_remove_state_watch(wsc->station,
+ wsc->station_state_watch);
+ wsc->station_state_watch = 0;
+ }
+
+ if (wsc->enrollee)
+ wsc_enrollee_cancel(wsc->enrollee);
+
+ l_free(wsc);
+}
+
static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
struct l_dbus_message *message,
void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_dbus *wsc = user_data;
l_debug("");
- if (wsc->pending)
- return dbus_error_busy(message);
-
- wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
- if (!wsc->station)
- return dbus_error_not_available(message);
-
- if (!wsc_initiate_scan(wsc, WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON,
- push_button_scan_results))
- return dbus_error_failed(message);
+ if (!l_dbus_message_get_arguments(message, ""))
+ return dbus_error_invalid_args(message);
- wsc->walk_timer = l_timeout_create(WALK_TIME, walk_timeout, wsc, NULL);
- wsc->pending = l_dbus_message_ref(message);
+ if (wsc->pending_connect)
+ return dbus_error_busy(message);
+ wsc->pending_connect = l_dbus_message_ref(message);
+ wsc->connect(wsc, NULL);
return NULL;
}
@@ -1025,13 +1123,13 @@ static struct l_dbus_message *wsc_generate_pin(struct l_dbus *dbus,
struct l_dbus_message *message,
void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_dbus *wsc = user_data;
struct l_dbus_message *reply;
char pin[9];
l_debug("");
- if (wsc->pending)
+ if (wsc->pending_connect)
return dbus_error_busy(message);
if (!wsc_pin_generate(pin))
@@ -1047,36 +1145,22 @@ static struct l_dbus_message *wsc_start_pin(struct l_dbus *dbus,
struct l_dbus_message *message,
void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_dbus *wsc = user_data;
const char *pin;
- enum wsc_device_password_id dpid;
l_debug("");
- if (wsc->pending)
+ if (wsc->pending_connect)
return dbus_error_busy(message);
- wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
- if (!wsc->station)
- return dbus_error_not_available(message);
-
if (!l_dbus_message_get_arguments(message, "s", &pin))
return dbus_error_invalid_args(message);
if (!wsc_pin_is_valid(pin))
return dbus_error_invalid_format(message);
- 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 (!wsc_initiate_scan(wsc, dpid, pin_scan_results))
- return dbus_error_failed(message);
-
- wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
- wsc->pending = l_dbus_message_ref(message);
-
+ wsc->pending_connect = l_dbus_message_ref(message);
+ wsc->connect(wsc, pin);
return NULL;
}
@@ -1084,32 +1168,19 @@ static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
struct l_dbus_message *message,
void *user_data)
{
- struct wsc *wsc = user_data;
- struct l_dbus_message *reply;
+ struct wsc_dbus *wsc = user_data;
l_debug("");
- if (!wsc->pending)
- return dbus_error_not_available(message);
-
- wsc_cancel_scan(wsc);
-
- if (wsc->station_state_watch) {
- station_remove_state_watch(wsc->station,
- wsc->station_state_watch);
- wsc->station_state_watch = 0;
- wsc->target = NULL;
- }
-
- dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
-
- if (wsc->enrollee)
- wsc_enrollee_cancel(wsc->enrollee);
+ if (!l_dbus_message_get_arguments(message, ""))
+ return dbus_error_invalid_args(message);
- reply = l_dbus_message_new_method_return(message);
- l_dbus_message_set_arguments(reply, "");
+ if (!wsc->pending_connect || wsc->pending_cancel)
+ return dbus_error_not_available(message);
- return reply;
+ wsc->pending_cancel = l_dbus_message_ref(message);
+ wsc->cancel(wsc);
+ return NULL;
}
static void setup_wsc_interface(struct l_dbus_interface *interface)
@@ -1124,32 +1195,47 @@ static void setup_wsc_interface(struct l_dbus_interface *interface)
wsc_cancel, "", "");
}
-static void wsc_free(void *userdata)
+bool wsc_dbus_add_interface(struct wsc_dbus *wsc)
{
- struct wsc *wsc = userdata;
-
- wsc_cancel_scan(wsc);
+ struct l_dbus *dbus = dbus_get_bus();
- if (wsc->station_state_watch) {
- station_remove_state_watch(wsc->station,
- wsc->station_state_watch);
- wsc->station_state_watch = 0;
+ if (!l_dbus_object_add_interface(dbus, wsc->get_path(wsc),
+ IWD_WSC_INTERFACE, wsc)) {
+ l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
+ return false;
}
- if (wsc->pending)
- dbus_pending_reply(&wsc->pending,
- dbus_error_not_available(wsc->pending));
+ return true;
+}
- if (wsc->enrollee)
- wsc_enrollee_cancel(wsc->enrollee);
+void wsc_dbus_remove_interface(struct wsc_dbus *wsc)
+{
+ struct l_dbus *dbus = dbus_get_bus();
- l_free(wsc);
+ l_dbus_object_remove_interface(dbus, wsc->get_path(wsc),
+ IWD_WSC_INTERFACE);
}
-static void wsc_add_interface(struct netdev *netdev)
+static void wsc_dbus_free(void *user_data)
{
- struct l_dbus *dbus = dbus_get_bus();
- struct wsc *wsc;
+ struct wsc_dbus *wsc = user_data;
+
+ if (wsc->pending_connect)
+ dbus_pending_reply(&wsc->pending_connect,
+ dbus_error_not_available(
+ wsc->pending_connect));
+
+ if (wsc->pending_cancel)
+ dbus_pending_reply(&wsc->pending_cancel,
+ dbus_error_not_available(
+ wsc->pending_cancel));
+
+ wsc->remove(wsc);
+}
+
+static void wsc_add_station(struct netdev *netdev)
+{
+ struct wsc_station_dbus *wsc;
if (!wiphy_get_max_scan_ie_len(netdev_get_wiphy(netdev))) {
l_debug("Simple Configuration isn't supported by ifindex %u",
@@ -1158,18 +1244,18 @@ static void wsc_add_interface(struct netdev *netdev)
return;
}
- wsc = l_new(struct wsc, 1);
+ wsc = l_new(struct wsc_station_dbus, 1);
wsc->netdev = netdev;
+ wsc->super.get_path = wsc_station_dbus_get_path;
+ wsc->super.connect = wsc_station_dbus_connect;
+ wsc->super.cancel = wsc_station_dbus_cancel;
+ wsc->super.remove = wsc_station_dbus_remove;
- if (!l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
- IWD_WSC_INTERFACE,
- wsc)) {
- wsc_free(wsc);
- l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
- }
+ if (!wsc_dbus_add_interface(&wsc->super))
+ wsc_station_dbus_remove(&wsc->super);
}
-static void wsc_remove_interface(struct netdev *netdev)
+static void wsc_remove_station(struct netdev *netdev)
{
struct l_dbus *dbus = dbus_get_bus();
@@ -1185,11 +1271,11 @@ static void wsc_netdev_watch(struct netdev *netdev,
case NETDEV_WATCH_EVENT_NEW:
if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION &&
netdev_get_is_up(netdev))
- wsc_add_interface(netdev);
+ wsc_add_station(netdev);
break;
case NETDEV_WATCH_EVENT_DOWN:
case NETDEV_WATCH_EVENT_DEL:
- wsc_remove_interface(netdev);
+ wsc_remove_station(netdev);
break;
default:
break;
@@ -1202,7 +1288,7 @@ static int wsc_init(void)
netdev_watch = netdev_watch_add(wsc_netdev_watch, NULL, NULL);
l_dbus_register_interface(dbus_get_bus(), IWD_WSC_INTERFACE,
setup_wsc_interface,
- wsc_free, false);
+ wsc_dbus_free, false);
return 0;
}
diff --git a/src/wsc.h b/src/wsc.h
index 48a3297a..3cb7ab18 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -41,3 +41,16 @@ struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
const char *pin,
wsc_done_cb_t done_cb, void *user_data);
void wsc_enrollee_cancel(struct wsc_enrollee *wsce);
+
+struct wsc_dbus {
+ struct l_dbus_message *pending_connect;
+ struct l_dbus_message *pending_cancel;
+
+ const char *(*get_path)(struct wsc_dbus *wsc);
+ void (*connect)(struct wsc_dbus *wsc, const char *pin);
+ void (*cancel)(struct wsc_dbus *wsc);
+ void (*remove)(struct wsc_dbus *wsc);
+};
+
+bool wsc_dbus_add_interface(struct wsc_dbus *wsc);
+void wsc_dbus_remove_interface(struct wsc_dbus *wsc);
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-13 18:38 ` Andrew Zaborowski
@ 2020-01-13 13:48 ` Denis Kenzior
2020-01-15 1:23 ` Andrew Zaborowski
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2020-01-13 13:48 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]
Hi Andrew,
>>> case -ECANCELED:
>>> - dbus_pending_reply(&wsc->pending,
>>> + if (wsc->pending)
>>> + dbus_pending_reply(&wsc->pending,
>>> dbus_error_aborted(wsc->pending));
>>
>> Hmm, why the if ()?
>
> In some scenario we're sending a not_available reply so this is a
> mechanism to override the specific reply.
This happens only on wsc_free, and I think you should be bypassing
calling wsc_enrollee_cancel in that case. The netdev is likely already
gone by then anyway.
>
>>
>> Also, if you wait until netdev_disconnect_cb to signal done_cb with
>> -ECANCELED, you can reply to both the pending and cancel messages here.
>
> I think it's actually nicer to send the "pending" reply early. It's
> also a nice property that we can use wsc_enrollee_cancel to
> immediately forget the done callback, also from P2P point of view
> where we're destroying the netdev right after we call this. So I
> think it'll need a separate optional callback.
This is what we have now, so I'm fine with the above as well.
>
> I wonder if we need to re-check that wsc->station still exists every
> time we use it.
>
station and wsc would be created or destroyed at the same time as part
of the netdev notification procedure. So I don't think checking it is
needed.
>>> +void wsc_enrollee_cancel(struct wsc_enrollee *wsce);
>>>
>>
>> You may still want an explicit wsc_enrollee_free for cases where
>> netdev_disconnect shouldn't be used / attempted.
>
> We'd need to be very sure that the netdev is also being freed. We
> mainly use netdev_disconnect to make sure netdev forgets our connect
> callbacks, note that station also does this in station_free.
Not sure why station_free does this. Maybe it is to force certain
signals to go out. I suspect this is something that we should fix in
station and not a desirable behavior to emulate. Sending a
netdev_disconnect when the device is gone or down anyway seems pretty
pointless.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-13 12:56 ` Denis Kenzior
@ 2020-01-13 18:38 ` Andrew Zaborowski
2020-01-13 13:48 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-13 18:38 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4069 bytes --]
Hi Denis!
On Mon, 13 Jan 2020 at 18:56, Denis Kenzior <denkenz@gmail.com> wrote:
> On 1/13/20 7:32 AM, Andrew Zaborowski wrote:
> > +void wsc_enrollee_cancel(struct wsc_enrollee *wsce)
> > +{
> > + wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
> > + wsce->done_cb = NULL;
> > + /* Results in a call to wsc_enrollee_free */
> > + netdev_disconnect(wsce->netdev, NULL, NULL);
>
> Are you sure you don't want to wait for netdev_disconnect to finish
> prior to signaling the request as aborted & canceled? In theory this
> could lead to station bouncing against an -EBUSY error once
> station_set_autoconnect is called.
True, with dbus Cancel() this could happen, I'll add the callback.
>
> > +
> > + /*
> > + * TODO: Mark this network as known. We might be getting
> > + * multiple credentials from WSC, so there is a possibility
> > + * that the network is not known and / or not in scan results.
> > + * In both cases, the network should be considered for
> > + * auto-connect. Note, since we sync the settings, the next
> > + * reboot will put the network on the known list.
> > + */
>
> This TODO comment is now likely stale and can be removed (in a separate
> commit).
Ok.
>
> > + }
> > }
> >
> > /* Done callback for when WSC is triggered by DBus methods */
> > @@ -541,14 +530,22 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
> > {
> > struct wsc *wsc = user_data;
> >
> > + wsc->enrollee = NULL;
> > + wsc->target = NULL;
> > +
> > l_debug("err=%i", err);
> >
> > + if (err && wsc->station)
> > + station_set_autoconnect(wsc->station, true);
> > +
> > switch (err) {
> > case 0:
> > break;
> > case -ECANCELED:
> > - dbus_pending_reply(&wsc->pending,
> > + if (wsc->pending)
> > + dbus_pending_reply(&wsc->pending,
> > dbus_error_aborted(wsc->pending));
>
> Hmm, why the if ()?
In some scenario we're sending a not_available reply so this is a
mechanism to override the specific reply.
>
> Also, if you wait until netdev_disconnect_cb to signal done_cb with
> -ECANCELED, you can reply to both the pending and cancel messages here.
I think it's actually nicer to send the "pending" reply early. It's
also a nice property that we can use wsc_enrollee_cancel to
immediately forget the done callback, also from P2P point of view
where we're destroying the netdev right after we call this. So I
think it'll need a separate optional callback.
I wonder if we need to re-check that wsc->station still exists every
time we use it.
>
> > +
> > return;
> > case -ENOKEY:
> > dbus_pending_reply(&wsc->pending,
>
> <snip>
>
> > diff --git a/src/wsc.h b/src/wsc.h
> > index ada08c89..48a3297a 100644
> > --- a/src/wsc.h
> > +++ b/src/wsc.h
> > @@ -31,5 +31,13 @@ struct wsc_credentials_info {
> > bool has_passphrase;
> > };
> >
> > +struct wsc_enrollee;
> > +
> > typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
> > unsigned int n_creds, void *user_data);
> > +
> > +struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
> > + struct scan_bss *target,
> > + const char *pin,
> > + wsc_done_cb_t done_cb, void *user_data);
> > +void wsc_enrollee_cancel(struct wsc_enrollee *wsce);
> >
>
> You may still want an explicit wsc_enrollee_free for cases where
> netdev_disconnect shouldn't be used / attempted.
We'd need to be very sure that the netdev is also being freed. We
mainly use netdev_disconnect to make sure netdev forgets our connect
callbacks, note that station also does this in station_free.
Best regards
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-15 1:23 ` Andrew Zaborowski
@ 2020-01-14 20:53 ` Denis Kenzior
2020-01-15 2:41 ` Andrew Zaborowski
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2020-01-14 20:53 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]
Hi Andrew,
On 1/14/20 7:23 PM, Andrew Zaborowski wrote:
> Hi Denis,
>
> On Mon, 13 Jan 2020 at 19:49, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>> case -ECANCELED:
>>>>> - dbus_pending_reply(&wsc->pending,
>>>>> + if (wsc->pending)
>>>>> + dbus_pending_reply(&wsc->pending,
>>>>> dbus_error_aborted(wsc->pending));
>>>>
>>>> Hmm, why the if ()?
>>>
>>> In some scenario we're sending a not_available reply so this is a
>>> mechanism to override the specific reply.
>>
>> This happens only on wsc_free, and I think you should be bypassing
>> calling wsc_enrollee_cancel in that case. The netdev is likely already
>> gone by then anyway.
>
> wsc_free may happen because the netdev is going away or because iwd is
> shutting down, or we're switching modes. In those cases if we have a
There are two cases, in both cases it is too late to send CMD_DISCONNECT:
1. netdev_set_iftype path. If the interface is up, we bring it down
first. This is what causes the NETDEV_WATCH_EVENT_DOWN. Trying to send
netdev_disconnect seems sort of pointless.
2. netdev itself is removed. At which point netdev_connect_free has
been called or we cancel the netdev_disconnect command if it is pending.
netdev object is destroyed shortly thereafter. So it is absolutely
wrong to call netdev_connect in the watch callback here.
> WSC negotiation running we should try to send the disconnect, although
> I think this is automatically triggered at some lower layer too. But
It is. And on an iface type switch in case you're not running mac80211
and the driver supports 'live' iftype switching.
> we also use netdev_disconnect to sort of make sure we never get
> another connect callback from netdev and I don't think skipping it is
See above. We really shouldn't anyway. station doing it is absolutely
wrong in the NETDEV_WATCH_EVENT_DEL case for sure. And even the
EVENT_DOWN case is likely wrong as well.
> a good idea (we'd need to analyse a few code paths and add comments
> and we might end up getting it wrong anyway). This is why I think
> station_free does it and I don't think it's wrong.
>
I disagree :) I think likely what is saving station in this case is
that the Disconnect event comes first (most of the time). But
inherently we might get this event via RTNL first and then we can go *boom*.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-15 2:41 ` Andrew Zaborowski
@ 2020-01-14 22:00 ` Denis Kenzior
2020-01-15 22:17 ` Andrew Zaborowski
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2020-01-14 22:00 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]
Hi Andrew,
On 1/14/20 8:41 PM, Andrew Zaborowski wrote:
> Hi Denis,
>
> On Wed, 15 Jan 2020 at 02:54, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 1/14/20 7:23 PM, Andrew Zaborowski wrote:
>>> On Mon, 13 Jan 2020 at 19:49, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> This happens only on wsc_free, and I think you should be bypassing
>>>> calling wsc_enrollee_cancel in that case. The netdev is likely already
>>>> gone by then anyway.
>>>
>>> wsc_free may happen because the netdev is going away or because iwd is
>>> shutting down, or we're switching modes. In those cases if we have a
>>
>> There are two cases, in both cases it is too late to send CMD_DISCONNECT:
>>
>> 1. netdev_set_iftype path. If the interface is up, we bring it down
>> first. This is what causes the NETDEV_WATCH_EVENT_DOWN. Trying to send
>> netdev_disconnect seems sort of pointless.
>
> Trying to send CMD_DISCONNECT is pointless there but netdev_disconnect
> still achieves what we need which is cancel the connect callback.
I think this is the wrong way to accomplish this. It feels like a hack
/ shortcut. We simply should not be trying to utilize API that is meant
to schedule a clean tear down when we already know this is futile.
>
>>
>> 2. netdev itself is removed. At which point netdev_connect_free has
>> been called or we cancel the netdev_disconnect command if it is pending.
>> netdev object is destroyed shortly thereafter. So it is absolutely
>> wrong to call netdev_connect in the watch callback here.
I did mean netdev_disconnect here by the way :)
>
> Right, but the wsc_enrollee object will have been freed inside that
> netdev_connect_free call and we wouldn't be calling
> wsc_enrollee_cancel or netdev_disconnect anymore. Conceptually
> whenever we do have a netdev_connect running who's callback is still
Are you sure? netdev_connect_free doesn't invoke callbacks. It is
netdev_connect_failed and netdev_connect_ok that do.
> pending, I believe we should call netdev_disconnect, and whenever we
> have a wsc_enrollee object we need to call wsc_enrollee_cancel.
I think we should not try to lump together a 'clean-async-shutdown' and
'cleanup-right-away' concepts.
>> I disagree :) I think likely what is saving station in this case is
>> that the Disconnect event comes first (most of the time). But
>> inherently we might get this event via RTNL first and then we can go *boom*.
>
> Mostly what can happen is that the CMD_DISCONNECT command returns an
> error so we've sent a useless command (if we'd cancelled a wrong
> connection attempt that would be slightly worse). On the other hand
> if we free station without calling netdev_disconnect, then things can
> actually go *boom* because we could get the connect callback with an
> outdated user_data pointer.
Aha! And this is likely why this was added in the first place:
git show daf248e1b
But it doesn't make it 'right' from an API perspective. I suspect that
netdev should be invoking connect_cb with ABORTED in this case instead.
In fact I'm still a bit lost what this call is accomplishing at the
moment. The only thing station_connect_cb does when
NETDEV_RESULT_ABORTED is set is to generate a dbus reply. And then we
do it unconditionally later in station_free.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-13 13:48 ` Denis Kenzior
@ 2020-01-15 1:23 ` Andrew Zaborowski
2020-01-14 20:53 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-15 1:23 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]
Hi Denis,
On Mon, 13 Jan 2020 at 19:49, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> case -ECANCELED:
> >>> - dbus_pending_reply(&wsc->pending,
> >>> + if (wsc->pending)
> >>> + dbus_pending_reply(&wsc->pending,
> >>> dbus_error_aborted(wsc->pending));
> >>
> >> Hmm, why the if ()?
> >
> > In some scenario we're sending a not_available reply so this is a
> > mechanism to override the specific reply.
>
> This happens only on wsc_free, and I think you should be bypassing
> calling wsc_enrollee_cancel in that case. The netdev is likely already
> gone by then anyway.
wsc_free may happen because the netdev is going away or because iwd is
shutting down, or we're switching modes. In those cases if we have a
WSC negotiation running we should try to send the disconnect, although
I think this is automatically triggered at some lower layer too. But
we also use netdev_disconnect to sort of make sure we never get
another connect callback from netdev and I don't think skipping it is
a good idea (we'd need to analyse a few code paths and add comments
and we might end up getting it wrong anyway). This is why I think
station_free does it and I don't think it's wrong.
Best regards
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-14 20:53 ` Denis Kenzior
@ 2020-01-15 2:41 ` Andrew Zaborowski
2020-01-14 22:00 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-15 2:41 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]
Hi Denis,
On Wed, 15 Jan 2020 at 02:54, Denis Kenzior <denkenz@gmail.com> wrote:
> On 1/14/20 7:23 PM, Andrew Zaborowski wrote:
> > On Mon, 13 Jan 2020 at 19:49, Denis Kenzior <denkenz@gmail.com> wrote:
> >> This happens only on wsc_free, and I think you should be bypassing
> >> calling wsc_enrollee_cancel in that case. The netdev is likely already
> >> gone by then anyway.
> >
> > wsc_free may happen because the netdev is going away or because iwd is
> > shutting down, or we're switching modes. In those cases if we have a
>
> There are two cases, in both cases it is too late to send CMD_DISCONNECT:
>
> 1. netdev_set_iftype path. If the interface is up, we bring it down
> first. This is what causes the NETDEV_WATCH_EVENT_DOWN. Trying to send
> netdev_disconnect seems sort of pointless.
Trying to send CMD_DISCONNECT is pointless there but netdev_disconnect
still achieves what we need which is cancel the connect callback.
>
> 2. netdev itself is removed. At which point netdev_connect_free has
> been called or we cancel the netdev_disconnect command if it is pending.
> netdev object is destroyed shortly thereafter. So it is absolutely
> wrong to call netdev_connect in the watch callback here.
Right, but the wsc_enrollee object will have been freed inside that
netdev_connect_free call and we wouldn't be calling
wsc_enrollee_cancel or netdev_disconnect anymore. Conceptually
whenever we do have a netdev_connect running who's callback is still
pending, I believe we should call netdev_disconnect, and whenever we
have a wsc_enrollee object we need to call wsc_enrollee_cancel.
>
> > WSC negotiation running we should try to send the disconnect, although
> > I think this is automatically triggered at some lower layer too. But
>
> It is. And on an iface type switch in case you're not running mac80211
> and the driver supports 'live' iftype switching.
>
> > we also use netdev_disconnect to sort of make sure we never get
> > another connect callback from netdev and I don't think skipping it is
>
> See above. We really shouldn't anyway. station doing it is absolutely
> wrong in the NETDEV_WATCH_EVENT_DEL case for sure. And even the
> EVENT_DOWN case is likely wrong as well.
>
> > a good idea (we'd need to analyse a few code paths and add comments
> > and we might end up getting it wrong anyway). This is why I think
> > station_free does it and I don't think it's wrong.
> >
>
> I disagree :) I think likely what is saving station in this case is
> that the Disconnect event comes first (most of the time). But
> inherently we might get this event via RTNL first and then we can go *boom*.
Mostly what can happen is that the CMD_DISCONNECT command returns an
error so we've sent a useless command (if we'd cancelled a wrong
connection attempt that would be slightly worse). On the other hand
if we free station without calling netdev_disconnect, then things can
actually go *boom* because we could get the connect callback with an
outdated user_data pointer.
Best regards
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-14 22:00 ` Denis Kenzior
@ 2020-01-15 22:17 ` Andrew Zaborowski
2020-01-15 23:09 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-15 22:17 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]
Hi Denis,
On Wed, 15 Jan 2020 at 04:01, Denis Kenzior <denkenz@gmail.com> wrote:
> On 1/14/20 8:41 PM, Andrew Zaborowski wrote:
> > On Wed, 15 Jan 2020 at 02:54, Denis Kenzior <denkenz@gmail.com> wrote:
> >> There are two cases, in both cases it is too late to send CMD_DISCONNECT:
> >>
> >> 1. netdev_set_iftype path. If the interface is up, we bring it down
> >> first. This is what causes the NETDEV_WATCH_EVENT_DOWN. Trying to send
> >> netdev_disconnect seems sort of pointless.
> >
> > Trying to send CMD_DISCONNECT is pointless there but netdev_disconnect
> > still achieves what we need which is cancel the connect callback.
>
> I think this is the wrong way to accomplish this. It feels like a hack
> / shortcut. We simply should not be trying to utilize API that is meant
> to schedule a clean tear down when we already know this is futile.
>
> >
> >>
> >> 2. netdev itself is removed. At which point netdev_connect_free has
> >> been called or we cancel the netdev_disconnect command if it is pending.
> >> netdev object is destroyed shortly thereafter. So it is absolutely
> >> wrong to call netdev_connect in the watch callback here.
>
> I did mean netdev_disconnect here by the way :)
>
> >
> > Right, but the wsc_enrollee object will have been freed inside that
> > netdev_connect_free call and we wouldn't be calling
> > wsc_enrollee_cancel or netdev_disconnect anymore. Conceptually
> > whenever we do have a netdev_connect running who's callback is still
>
> Are you sure? netdev_connect_free doesn't invoke callbacks. It is
> netdev_connect_failed and netdev_connect_ok that do.
True, I misread something.
>
> > pending, I believe we should call netdev_disconnect, and whenever we
> > have a wsc_enrollee object we need to call wsc_enrollee_cancel.
>
> I think we should not try to lump together a 'clean-async-shutdown' and
> 'cleanup-right-away' concepts.
Ok, then we need two methods for wsc_enrollee. I also think that
station and wsc_enrollee shouldn't assume whether the netdev is going
away or not, based on whether station and/or wsc_enrollee are going
away. So rather than assume the connect callback is not getting
happening anymore, they also should call a cleanup-right-away function
for netdev. Or ideally they'd just call a 'cleanup' function and it
would be netdev figuring out which cleanup it needs, but if you want
two separate methods I can add this.
>
> >> I disagree :) I think likely what is saving station in this case is
> >> that the Disconnect event comes first (most of the time). But
> >> inherently we might get this event via RTNL first and then we can go *boom*.
> >
> > Mostly what can happen is that the CMD_DISCONNECT command returns an
> > error so we've sent a useless command (if we'd cancelled a wrong
> > connection attempt that would be slightly worse). On the other hand
> > if we free station without calling netdev_disconnect, then things can
> > actually go *boom* because we could get the connect callback with an
> > outdated user_data pointer.
>
> Aha! And this is likely why this was added in the first place:
> git show daf248e1b
>
> But it doesn't make it 'right' from an API perspective. I suspect that
> netdev should be invoking connect_cb with ABORTED in this case instead.
> In fact I'm still a bit lost what this call is accomplishing at the
> moment.
In the current code it is preventing a crash in station_connect_cb though.
> The only thing station_connect_cb does when
> NETDEV_RESULT_ABORTED is set is to generate a dbus reply. And then we
> do it unconditionally later in station_free.
Well it's conditional and I guess the condition is always false
because we always send the reply in station_connect_cb (but we don't
enter the CONNECTED stated until netconfig is successful? should we
also delay the dbus reply?)
So it looks like we should either not send the reply in
station_connect_cb if the result is aborted, or not send it in
station_free (that'd be my preference). I think we should also be
able to cancel a netdev_disconnect() if one is running asynchronously.
Best regards
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-15 22:17 ` Andrew Zaborowski
@ 2020-01-15 23:09 ` Denis Kenzior
2020-01-15 23:45 ` Andrew Zaborowski
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2020-01-15 23:09 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4193 bytes --]
Hi Andrew,
<snip>
>> Are you sure? netdev_connect_free doesn't invoke callbacks. It is
>> netdev_connect_failed and netdev_connect_ok that do.
>
> True, I misread something.
>
Also note that by extension, netdev_disconnect only calls the connect_cb
with ABORTED. It doesn't do anything if the connection is 'operational'.
>>
>>> pending, I believe we should call netdev_disconnect, and whenever we
>>> have a wsc_enrollee object we need to call wsc_enrollee_cancel.
>>
>> I think we should not try to lump together a 'clean-async-shutdown' and
>> 'cleanup-right-away' concepts.
>
> Ok, then we need two methods for wsc_enrollee. I also think that
Isn't this what I keep trying to tell you? :)
> station and wsc_enrollee shouldn't assume whether the netdev is going
> away or not, based on whether station and/or wsc_enrollee are going
They should not. But in effect they're being told that they're being
removed and to clean up after themselves immediately. No async cleanup
is possible.
> away. So rather than assume the connect callback is not getting
> happening anymore, they also should call a cleanup-right-away function
> for netdev. Or ideally they'd just call a 'cleanup' function and it
> would be netdev figuring out which cleanup it needs, but if you want
> two separate methods I can add this.
They should not. netdev already knows everything it needs to clean up
after itself.
>
>>
>>>> I disagree :) I think likely what is saving station in this case is
>>>> that the Disconnect event comes first (most of the time). But
>>>> inherently we might get this event via RTNL first and then we can go *boom*.
>>>
>>> Mostly what can happen is that the CMD_DISCONNECT command returns an
>>> error so we've sent a useless command (if we'd cancelled a wrong
>>> connection attempt that would be slightly worse). On the other hand
>>> if we free station without calling netdev_disconnect, then things can
>>> actually go *boom* because we could get the connect callback with an
>>> outdated user_data pointer.
>>
>> Aha! And this is likely why this was added in the first place:
>> git show daf248e1b
>>
>> But it doesn't make it 'right' from an API perspective. I suspect that
>> netdev should be invoking connect_cb with ABORTED in this case instead.
>> In fact I'm still a bit lost what this call is accomplishing at the
>> moment.
>
> In the current code it is preventing a crash in station_connect_cb though.
>
Hmm, I take it because we call connect_cb with an invalid station
object? In other words:
1. we get rtnl event that netdev is down.
2. This results in station being removed.
3. Then nl80211 sends us a Disconnect event.
4. netdev calls connect_cb with an invalid object
But again, IMO the fix should be in netdev, not station.
netdev should cleanup whatever stale state it has when it receives 1.
It *knows* what just happened, it should not be depending on every
client it has to do the right thing instead.
Maybe you want it to invoke connect_cb with ABORTED after step 1, but
looking at station code it already takes care of this in station_free.
>> The only thing station_connect_cb does when
>> NETDEV_RESULT_ABORTED is set is to generate a dbus reply. And then we
>> do it unconditionally later in station_free.
>
> Well it's conditional and I guess the condition is always false
> because we always send the reply in station_connect_cb (but we don't
> enter the CONNECTED stated until netconfig is successful? should we
> also delay the dbus reply?)
Right. Imprecise wording from me. We do it if connect_pending is
non-NULL, but we always check it.
>
> So it looks like we should either not send the reply in
> station_connect_cb if the result is aborted, or not send it in
> station_free (that'd be my preference). I think we should also be
I'd just have netdev invoke netdev_connect_free prior to
NETDEV_WATCH_EVENT_DOWN. And remove netdev_disconnect from station_free.
> able to cancel a netdev_disconnect() if one is running asynchronously.
Curious. For what purpose?
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-15 23:09 ` Denis Kenzior
@ 2020-01-15 23:45 ` Andrew Zaborowski
2020-01-16 1:54 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-15 23:45 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5595 bytes --]
On Thu, 16 Jan 2020 at 00:09, Denis Kenzior <denkenz@gmail.com> wrote:
> >> Are you sure? netdev_connect_free doesn't invoke callbacks. It is
> >> netdev_connect_failed and netdev_connect_ok that do.
> >
> > True, I misread something.
> >
>
> Also note that by extension, netdev_disconnect only calls the connect_cb
> with ABORTED. It doesn't do anything if the connection is 'operational'.
It calls netdev_connect_free, and that wipes the callback so it does
exactly what we need.
>
> >>
> >>> pending, I believe we should call netdev_disconnect, and whenever we
> >>> have a wsc_enrollee object we need to call wsc_enrollee_cancel.
> >>
> >> I think we should not try to lump together a 'clean-async-shutdown' and
> >> 'cleanup-right-away' concepts.
> >
> > Ok, then we need two methods for wsc_enrollee. I also think that
>
> Isn't this what I keep trying to tell you? :)
Oh, ok. Then we agree on this since early in this thread :)
>
> > station and wsc_enrollee shouldn't assume whether the netdev is going
> > away or not, based on whether station and/or wsc_enrollee are going
>
> They should not. But in effect they're being told that they're being
> removed and to clean up after themselves immediately. No async cleanup
> is possible.
So here you're agreeing that station_free should not be assuming that
the netdev is going away --> station_free *should* call some sort of
netdev cleanup function (netdev_disconnect or a new function that
doesn't send a CMD_DISCONNECT, but does call netdev_connect_free)...
>
> > away. So rather than assume the connect callback is not getting
> > happening anymore, they also should call a cleanup-right-away function
> > for netdev. Or ideally they'd just call a 'cleanup' function and it
> > would be netdev figuring out which cleanup it needs, but if you want
> > two separate methods I can add this.
>
> They should not. netdev already knows everything it needs to clean up
> after itself.
...But here you're saying they shouldn't call any netdev cleanup
function? Perhaps I'm misunderstanding you.
>
> >
> >>
> >>>> I disagree :) I think likely what is saving station in this case is
> >>>> that the Disconnect event comes first (most of the time). But
> >>>> inherently we might get this event via RTNL first and then we can go *boom*.
> >>>
> >>> Mostly what can happen is that the CMD_DISCONNECT command returns an
> >>> error so we've sent a useless command (if we'd cancelled a wrong
> >>> connection attempt that would be slightly worse). On the other hand
> >>> if we free station without calling netdev_disconnect, then things can
> >>> actually go *boom* because we could get the connect callback with an
> >>> outdated user_data pointer.
> >>
> >> Aha! And this is likely why this was added in the first place:
> >> git show daf248e1b
> >>
> >> But it doesn't make it 'right' from an API perspective. I suspect that
> >> netdev should be invoking connect_cb with ABORTED in this case instead.
> >> In fact I'm still a bit lost what this call is accomplishing at the
> >> moment.
> >
> > In the current code it is preventing a crash in station_connect_cb though.
> >
>
> Hmm, I take it because we call connect_cb with an invalid station
> object? In other words:
>
> 1. we get rtnl event that netdev is down.
> 2. This results in station being removed.
> 3. Then nl80211 sends us a Disconnect event.
> 4. netdev calls connect_cb with an invalid object
>
> But again, IMO the fix should be in netdev, not station.
>
> netdev should cleanup whatever stale state it has when it receives 1.
> It *knows* what just happened, it should not be depending on every
> client it has to do the right thing instead.
That will hardcode the assumption that the netdev is disappearing in
station. If they have a "client" type relationship then station
should take care of cleaning up the processes it has initiated.
Otherwise it would need to be very well documented in the comments and
I bet we'll still occasionally get it wrong, the relationship just
becomes very complex.
>
> Maybe you want it to invoke connect_cb with ABORTED after step 1, but
> looking at station code it already takes care of this in station_free.
>
> >> The only thing station_connect_cb does when
> >> NETDEV_RESULT_ABORTED is set is to generate a dbus reply. And then we
> >> do it unconditionally later in station_free.
> >
> > Well it's conditional and I guess the condition is always false
> > because we always send the reply in station_connect_cb (but we don't
> > enter the CONNECTED stated until netconfig is successful? should we
> > also delay the dbus reply?)
>
> Right. Imprecise wording from me. We do it if connect_pending is
> non-NULL, but we always check it.
>
> >
> > So it looks like we should either not send the reply in
> > station_connect_cb if the result is aborted, or not send it in
> > station_free (that'd be my preference). I think we should also be
>
> I'd just have netdev invoke netdev_connect_free prior to
> NETDEV_WATCH_EVENT_DOWN. And remove netdev_disconnect from station_free.
>
> > able to cancel a netdev_disconnect() if one is running asynchronously.
>
> Curious. For what purpose?
Same as the netdev_disconnect call makes netdev forget station's
connect_cb, as far as I'm reading netdev.c there's currently no place
where disconnect_cb is forgotten when station gets freed so we likely
have the same crash there.
Best regards
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-15 23:45 ` Andrew Zaborowski
@ 2020-01-16 1:54 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2020-01-16 1:54 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]
Hi Andrew,
On 1/15/20 5:45 PM, Andrew Zaborowski wrote:
> On Thu, 16 Jan 2020 at 00:09, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> Are you sure? netdev_connect_free doesn't invoke callbacks. It is
>>>> netdev_connect_failed and netdev_connect_ok that do.
>>>
>>> True, I misread something.
>>>
>>
>> Also note that by extension, netdev_disconnect only calls the connect_cb
>> with ABORTED. It doesn't do anything if the connection is 'operational'.
>
> It calls netdev_connect_free, and that wipes the callback so it does
> exactly what we need.
>
Yes, exactly. But it doesn't call the event filter. So the only reason
for station to call netdev is to have netdev perform cleanup.
To state it another way, netdev is relying on one of its clients to be
well-behaved and remember to clean up after itself. I think this is the
wrong approach. netdev already knows what it needs to do, let it clean
up itself.
The above makes sense even from a re-use point of view, i.e. centralize
this cleanup instead of having each module depending on netdev doing it
(or more likely forgetting to do it) manually.
>>> station and wsc_enrollee shouldn't assume whether the netdev is going
>>> away or not, based on whether station and/or wsc_enrollee are going
>>
>> They should not. But in effect they're being told that they're being
>> removed and to clean up after themselves immediately. No async cleanup
>> is possible.
>
> So here you're agreeing that station_free should not be assuming that
> the netdev is going away --> station_free *should* call some sort of
> netdev cleanup function (netdev_disconnect or a new function that
> doesn't send a CMD_DISCONNECT, but does call netdev_connect_free)...
No. See above. station shouldn't care what netdev is doing. All it
cares about is it was just informed that netdev is down and no longer in
a state where it can accept any commands. It should clean up after
itself immediately and cease to exist.
>
>>
>>> away. So rather than assume the connect callback is not getting
>>> happening anymore, they also should call a cleanup-right-away function
>>> for netdev. Or ideally they'd just call a 'cleanup' function and it
>>> would be netdev figuring out which cleanup it needs, but if you want
>>> two separate methods I can add this.
>>
>> They should not. netdev already knows everything it needs to clean up
>> after itself.
>
> ...But here you're saying they shouldn't call any netdev cleanup
> function? Perhaps I'm misunderstanding you.
>
Yes, because netdev will clean up after itself. So we don't have to
rely on station, ap, wsc, adhoc, etc to remember to do it for netdev.
>> netdev should cleanup whatever stale state it has when it receives 1.
>> It *knows* what just happened, it should not be depending on every
>> client it has to do the right thing instead.
>
> That will hardcode the assumption that the netdev is disappearing in
> station. If they have a "client" type relationship then station
> should take care of cleaning up the processes it has initiated.
> Otherwise it would need to be very well documented in the comments and
> I bet we'll still occasionally get it wrong, the relationship just
> becomes very complex.
I don't see how you came to this conclusion :) netdev is telling its
clients that it is either
1. Disappearing and ceasing to exist.
2. In the DOWN state where pretty much nothing works.
In both cases netdev should have already cleaned up whatever pending
operations there were and there's not much for the clients to do anyway.
>> Curious. For what purpose?
>
> Same as the netdev_disconnect call makes netdev forget station's
> connect_cb, as far as I'm reading netdev.c there's currently no place
> where disconnect_cb is forgotten when station gets freed so we likely
> have the same crash there.
>
I see. Makes sense. But I'd rather have the disconnect state cleaned
up by netdev itself.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] wsc: Split out enrollee state machine to own object
2020-01-16 4:29 [PATCH 1/3] wsc: Split out enrollee state machine to own object Andrew Zaborowski
@ 2020-01-17 18:51 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2020-01-17 18:51 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]
Hi Andrew,
On 1/15/20 10:29 PM, Andrew Zaborowski wrote:
> Create struct wsc_enrollee which is allocated with wsc_enrollee_new,
> taking a done callback as a parameter. The callback is always
> called so there's no need for a separate destroy callback. The object
> only lives until the done callback happens so wsc_enrollee_cancel/destroy
> can only be used before this.
>
> Looks like the rest of the file is simplified thanks to this.
> ---
> src/wsc.c | 504 +++++++++++++++++++++++++++++-------------------------
> src/wsc.h | 9 +
> 2 files changed, 276 insertions(+), 237 deletions(-)
>
<snip>
> @@ -1157,7 +1184,10 @@ static void wsc_free(void *userdata)
>
> if (wsc->pending_cancel)
> dbus_pending_reply(&wsc->pending_cancel,
> - dbus_error_aborted(wsc->pending_cancel));
> + dbus_error_not_available(wsc->pending_cancel));
I left this as error_aborted for now. I'd rather not do so without a
good reason given that the API is frozen. At the very least, lets do
this in a separate patch so it is visible / documented. If you think
it should be not_available, please send a separate patch.
> +
> + if (wsc->enrollee)
> + wsc_enrollee_destroy(wsc->enrollee);
>
> l_free(wsc);
> }
Otherwise, everything in this series has now been applied, thanks! I
did push out a few tweaks on top, please review.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] wsc: Split out enrollee state machine to own object
@ 2020-01-16 4:29 Andrew Zaborowski
2020-01-17 18:51 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Zaborowski @ 2020-01-16 4:29 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 23073 bytes --]
Create struct wsc_enrollee which is allocated with wsc_enrollee_new,
taking a done callback as a parameter. The callback is always
called so there's no need for a separate destroy callback. The object
only lives until the done callback happens so wsc_enrollee_cancel/destroy
can only be used before this.
Looks like the rest of the file is simplified thanks to this.
---
src/wsc.c | 504 +++++++++++++++++++++++++++++-------------------------
src/wsc.h | 9 +
2 files changed, 276 insertions(+), 237 deletions(-)
diff --git a/src/wsc.c b/src/wsc.c
index d50f240f..e381b84f 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -51,9 +51,20 @@
static uint32_t netdev_watch = 0;
+struct wsc_enrollee {
+ struct netdev *netdev;
+ struct wsc_credentials_info creds[3];
+ uint32_t n_creds;
+ struct l_settings *eap_settings;
+ wsc_done_cb_t done_cb;
+ void *done_data;
+ bool disconnecting : 1;
+};
+
struct wsc {
struct netdev *netdev;
struct station *station;
+ struct wsc_enrollee *enrollee;
struct l_dbus_message *pending;
struct l_dbus_message *pending_cancel;
uint8_t *wsc_ies;
@@ -62,16 +73,6 @@ struct wsc {
uint32_t scan_id;
struct scan_bss *target;
uint32_t station_state_watch;
- struct wsc_credentials_info 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;
};
static struct l_dbus_message *wsc_error_session_overlap(
@@ -113,171 +114,50 @@ static struct l_dbus_message *wsc_error_time_expired(struct l_dbus_message *msg)
"No APs in PIN mode found in "
"the alloted time");
}
-static void wsc_try_credentials(struct wsc *wsc,
- struct wsc_credentials_info *creds,
- unsigned int n_creds)
-{
- unsigned int i;
- struct network *network;
- struct scan_bss *bss;
-
- for (i = 0; i < n_creds; i++) {
- network = station_network_find(wsc->station, creds[i].ssid,
- creds[i].security);
- if (!network)
- continue;
-
- bss = network_bss_find_by_addr(network, creds[i].addr);
-
- if (!bss)
- bss = network_bss_select(network, true);
-
- if (!bss)
- continue;
-
- if (creds[i].security == SECURITY_PSK) {
- bool ret;
-
- /*
- * Prefer setting passphrase, this will work for both
- * WPA2 and WPA3 since the PSK can always be generated
- * if needed
- */
- if (creds[i].has_passphrase)
- ret = network_set_passphrase(network,
- creds[i].passphrase);
- else
- ret = network_set_psk(network, creds[i].psk);
-
- if (!ret)
- continue;
- }
-
- station_connect_network(wsc->station, network, bss,
- wsc->pending);
- l_dbus_message_unref(wsc->pending);
- wsc->pending = NULL;
-
- return;
- }
-
- dbus_pending_reply(&wsc->pending,
- wsc_error_not_reachable(wsc->pending));
- station_set_autoconnect(wsc->station, true);
-}
-static void wsc_store_credentials(struct wsc_credentials_info *creds,
- unsigned int n_creds)
+static void wsc_enrollee_free(struct wsc_enrollee *wsce)
{
- unsigned int i;
-
- for (i = 0; i < n_creds; i++) {
- enum security security = creds[i].security;
- const char *ssid = creds[i].ssid;
- struct l_settings *settings = l_settings_new();
-
- l_debug("Storing credential for '%s(%s)'", ssid,
- security_to_str(security));
-
- if (security == SECURITY_PSK) {
- char *hex = l_util_hexstring(creds[i].psk,
- sizeof(creds[i].psk));
-
- l_settings_set_value(settings, "Security",
- "PreSharedKey", hex);
- explicit_bzero(hex, strlen(hex));
- l_free(hex);
- }
-
- storage_network_sync(security, ssid, settings);
- l_settings_free(settings);
-
- /*
- * TODO: Mark this network as known. We might be getting
- * multiple credentials from WSC, so there is a possibility
- * that the network is not known and / or not in scan results.
- * In both cases, the network should be considered for
- * auto-connect. Note, since we sync the settings, the next
- * reboot will put the network on the known list.
- */
- }
+ l_settings_free(wsce->eap_settings);
+ explicit_bzero(wsce->creds, sizeof(wsce->creds));
+ l_free(wsce);
}
-static void wsc_disconnect_cb(struct netdev *netdev, bool success,
- void *user_data)
-{
- struct wsc *wsc = user_data;
- struct l_dbus_message *reply;
-
- l_debug("%p, success: %d", wsc, success);
-
- wsc->wsc_association = false;
-
- reply = l_dbus_message_new_method_return(wsc->pending_cancel);
- l_dbus_message_set_arguments(reply, "");
- dbus_pending_reply(&wsc->pending_cancel, reply);
-
- 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,
+static void wsc_enrollee_connect_cb(struct netdev *netdev,
+ enum netdev_result result,
void *event_data, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
- l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
+ l_debug("%d, result: %d", netdev_get_ifindex(wsce->netdev), result);
- wsc_connect_cleanup(wsc);
+ if (wsce->disconnecting)
+ return; /* Free the state in the disconnect callback */
- if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
- struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
- uint32_t n_creds = wsc->n_creds;
+ if (!wsce->done_cb)
+ goto done;
- /*
- * Once we call done_cb, the state is assumed to be wiped,
- * so use a temporary array for creds here
- */
- 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;
+ if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsce->n_creds > 0) {
+ wsce->done_cb(0, wsce->creds, wsce->n_creds, wsce->done_data);
+ goto done;
}
switch (result) {
case NETDEV_RESULT_ABORTED:
- wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
- return;
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ break;
case NETDEV_RESULT_HANDSHAKE_FAILED:
- wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
+ wsce->done_cb(-ENOKEY, NULL, 0, wsce->done_data);
break;
default:
- wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
+ wsce->done_cb(-EIO, NULL, 0, wsce->done_data);
break;
}
- if (wsc->station)
- station_set_autoconnect(wsc->station, true);
+done:
+ wsc_enrollee_free(wsce);
}
-static void wsc_credential_obtained(struct wsc *wsc,
+static void wsc_enrollee_credential_obtained(struct wsc_enrollee *wsce,
const struct wsc_credential *cred)
{
uint16_t auth_mask;
@@ -294,7 +174,7 @@ static void wsc_credential_obtained(struct wsc *wsc,
l_debug("Key (%u): %.*s", cred->network_key_len,
cred->network_key_len, cred->network_key);
- if (wsc->n_creds == L_ARRAY_SIZE(wsc->creds)) {
+ if (wsce->n_creds == L_ARRAY_SIZE(wsce->creds)) {
l_warn("Maximum number of credentials obtained, ignoring...");
return;
}
@@ -304,8 +184,8 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- memcpy(wsc->creds[wsc->n_creds].ssid, cred->ssid, cred->ssid_len);
- wsc->creds[wsc->n_creds].ssid[cred->ssid_len] = '\0';
+ memcpy(wsce->creds[wsce->n_creds].ssid, cred->ssid, cred->ssid_len);
+ wsce->creds[wsce->n_creds].ssid[cred->ssid_len] = '\0';
/* We only support open/personal wpa/personal wpa2 */
auth_mask = WSC_AUTHENTICATION_TYPE_OPEN |
@@ -324,11 +204,11 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- wsc->creds[wsc->n_creds].security = SECURITY_NONE;
+ wsce->creds[wsce->n_creds].security = SECURITY_NONE;
} else
- wsc->creds[wsc->n_creds].security = SECURITY_PSK;
+ wsce->creds[wsce->n_creds].security = SECURITY_PSK;
- switch (wsc->creds[wsc->n_creds].security) {
+ switch (wsce->creds[wsce->n_creds].security) {
case SECURITY_NONE:
if (cred->network_key_len != 0) {
l_warn("ignoring invalid open key length");
@@ -347,14 +227,14 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- memcpy(wsc->creds[wsc->n_creds].psk, decoded, 32);
+ memcpy(wsce->creds[wsce->n_creds].psk, decoded, 32);
explicit_bzero(decoded, 32);
l_free(decoded);
} else {
- strncpy(wsc->creds[wsc->n_creds].passphrase,
+ strncpy(wsce->creds[wsce->n_creds].passphrase,
(const char *) cred->network_key,
cred->network_key_len);
- wsc->creds[wsc->n_creds].has_passphrase = true;
+ wsce->creds[wsce->n_creds].has_passphrase = true;
}
break;
@@ -362,25 +242,27 @@ static void wsc_credential_obtained(struct wsc *wsc,
return;
}
- for (i = 0; i < wsc->n_creds; i++) {
- if (strcmp(wsc->creds[i].ssid, wsc->creds[wsc->n_creds].ssid))
+ for (i = 0; i < wsce->n_creds; i++) {
+ if (strcmp(wsce->creds[i].ssid,
+ wsce->creds[wsce->n_creds].ssid))
continue;
l_warn("Found duplicate credentials for SSID: %s",
- wsc->creds[i].ssid);
- explicit_bzero(&wsc->creds[wsc->n_creds],
- sizeof(wsc->creds[wsc->n_creds]));
+ wsce->creds[i].ssid);
+ explicit_bzero(&wsce->creds[wsce->n_creds],
+ sizeof(wsce->creds[wsce->n_creds]));
return;
}
- memcpy(wsc->creds[wsc->n_creds].addr, cred->addr, 6);
- wsc->n_creds += 1;
+ memcpy(wsce->creds[wsce->n_creds].addr, cred->addr, 6);
+ wsce->n_creds += 1;
}
-static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
+static void wsc_enrollee_netdev_event(struct netdev *netdev,
+ enum netdev_event event,
void *event_data, void *user_data)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
switch (event) {
case NETDEV_EVENT_AUTHENTICATING:
@@ -391,8 +273,9 @@ static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
break;
case NETDEV_EVENT_DISCONNECT_BY_AP:
l_debug("Disconnect by AP");
- wsc_connect_cb(wsc->netdev, NETDEV_RESULT_HANDSHAKE_FAILED,
- event_data, wsc);
+ wsc_enrollee_connect_cb(wsce->netdev,
+ NETDEV_RESULT_HANDSHAKE_FAILED,
+ event_data, wsce);
break;
case NETDEV_EVENT_RSSI_THRESHOLD_LOW:
case NETDEV_EVENT_RSSI_THRESHOLD_HIGH:
@@ -403,11 +286,11 @@ static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
};
}
-static void wsc_handshake_event(struct handshake_state *hs,
- enum handshake_event event, void *user_data,
- ...)
+static void wsc_enrollee_handshake_event(struct handshake_state *hs,
+ enum handshake_event event,
+ void *user_data, ...)
{
- struct wsc *wsc = user_data;
+ struct wsc_enrollee *wsce = user_data;
va_list args;
va_start(args, user_data);
@@ -422,7 +305,7 @@ static void wsc_handshake_event(struct handshake_state *hs,
switch (eap_event) {
case EAP_WSC_EVENT_CREDENTIAL_OBTAINED:
- wsc_credential_obtained(wsc,
+ wsc_enrollee_credential_obtained(wsce,
va_arg(args, const struct wsc_credential *));
break;
default:
@@ -454,20 +337,18 @@ 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 int wsc_enrollee_connect(struct wsc_enrollee *wsce, struct scan_bss *bss,
+ const char *pin)
{
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;
-
- hs = netdev_handshake_state_new(wsc->netdev);
+ hs = netdev_handshake_state_new(wsce->netdev);
l_settings_set_string(settings, "Security", "EAP-Identity",
"WFA-SimpleConfig-Enrollee-1-0");
@@ -482,25 +363,23 @@ static void wsc_connect(struct wsc *wsc)
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)));
+ util_address_to_string(netdev_get_address(wsce->netdev)));
- if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+ if (pin) {
enum wsc_device_password_id dpid;
- if (strlen(wsc->pin) == 4 ||
- wsc_pin_is_checksum_valid(wsc->pin))
+ if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(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",
- wsc->pin);
+ l_settings_set_string(settings, "WSC", "DevicePassword", pin);
}
- handshake_state_set_event_func(hs, wsc_handshake_event, wsc);
+ handshake_state_set_event_func(hs, wsc_enrollee_handshake_event, wsce);
handshake_state_set_8021x_config(hs, settings);
- wsc->eap_settings = settings;
+ wsce->eap_settings = settings;
request.version2 = true;
request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
@@ -520,19 +399,164 @@ static void wsc_connect(struct wsc *wsc)
goto error;
}
- r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
- wsc_connect_cb, wsc);
+ r = netdev_connect(wsce->netdev, bss, hs, &ie_iov, 1,
+ wsc_enrollee_netdev_event,
+ wsc_enrollee_connect_cb, wsce);
l_free(ie_iov.iov_base);
- if (r < 0)
- goto error;
+ if (r == 0)
+ return 0;
- wsc->wsc_association = true;
- return;
error:
handshake_state_free(hs);
- wsc_connect_cleanup(wsc);
- wsc->done_cb(r, NULL, 0, wsc->done_data);
+ return r;
+}
+
+struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
+ struct scan_bss *target,
+ const char *pin,
+ wsc_done_cb_t done_cb, void *user_data)
+{
+ struct wsc_enrollee *wsce;
+
+ wsce = l_new(struct wsc_enrollee, 1);
+ wsce->netdev = netdev;
+ wsce->done_cb = done_cb;
+ wsce->done_data = user_data;
+
+ if (wsc_enrollee_connect(wsce, target, pin) == 0)
+ return wsce;
+
+ wsc_enrollee_free(wsce);
+ return NULL;
+}
+
+static void wsc_enrollee_disconnect_cb(struct netdev *netdev, bool result,
+ void *user_data)
+{
+ struct wsc_enrollee *wsce = user_data;
+
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ wsc_enrollee_free(wsce);
+}
+
+void wsc_enrollee_cancel(struct wsc_enrollee *wsce, bool defer_cb)
+{
+ if (defer_cb) {
+ wsce->disconnecting = true;
+ netdev_disconnect(wsce->netdev, wsc_enrollee_disconnect_cb,
+ wsce);
+ } else {
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ wsce->done_cb = NULL;
+ /*
+ * Results in a call to
+ * wsc_enrollee_connect_cb -> wsc_enrollee_free
+ */
+ netdev_disconnect(wsce->netdev, NULL, NULL);
+ }
+}
+
+/* To be called instead of wsc_enrollee_cancel if we know the netdev is gone */
+void wsc_enrollee_destroy(struct wsc_enrollee *wsce)
+{
+ /*
+ * Note: wsc_enrollee_cancel(wsce, true) may have been called before
+ * but since the netdev is going away wsc_enrollee_disconnect_cb
+ * is not happening anymore.
+ */
+ wsce->done_cb(-ECANCELED, NULL, 0, wsce->done_data);
+ wsc_enrollee_free(wsce);
+}
+
+static void wsc_try_credentials(struct wsc *wsc,
+ struct wsc_credentials_info *creds,
+ unsigned int n_creds)
+{
+ unsigned int i;
+ struct network *network;
+ struct scan_bss *bss;
+
+ for (i = 0; i < n_creds; i++) {
+ network = station_network_find(wsc->station, creds[i].ssid,
+ creds[i].security);
+ if (!network)
+ continue;
+
+ bss = network_bss_find_by_addr(network, creds[i].addr);
+
+ if (!bss)
+ bss = network_bss_select(network, true);
+
+ if (!bss)
+ continue;
+
+ if (creds[i].security == SECURITY_PSK) {
+ bool ret;
+
+ /*
+ * Prefer setting passphrase, this will work for both
+ * WPA2 and WPA3 since the PSK can always be generated
+ * if needed
+ */
+ if (creds[i].has_passphrase)
+ ret = network_set_passphrase(network,
+ creds[i].passphrase);
+ else
+ ret = network_set_psk(network, creds[i].psk);
+
+ if (!ret)
+ continue;
+ }
+
+ station_connect_network(wsc->station, network, bss,
+ wsc->pending);
+ l_dbus_message_unref(wsc->pending);
+ wsc->pending = NULL;
+
+ return;
+ }
+
+ dbus_pending_reply(&wsc->pending,
+ wsc_error_not_reachable(wsc->pending));
+ station_set_autoconnect(wsc->station, true);
+}
+
+static void wsc_store_credentials(struct wsc_credentials_info *creds,
+ unsigned int n_creds)
+{
+ unsigned int i;
+
+ for (i = 0; i < n_creds; i++) {
+ enum security security = creds[i].security;
+ const char *ssid = creds[i].ssid;
+ struct l_settings *settings = l_settings_new();
+
+ l_debug("Storing credential for '%s(%s)'", ssid,
+ security_to_str(security));
+
+ if (security == SECURITY_PSK) {
+ char *hex = l_util_hexstring(creds[i].psk,
+ sizeof(creds[i].psk));
+
+ l_settings_set_value(settings, "Security",
+ "PreSharedKey", hex);
+ explicit_bzero(hex, strlen(hex));
+ l_free(hex);
+ }
+
+ storage_network_sync(security, ssid, settings);
+ l_settings_free(settings);
+
+ /*
+ * TODO: Mark this network as known. We might be getting
+ * multiple credentials from WSC, so there is a possibility
+ * that the network is not known and / or not in scan results.
+ * In both cases, the network should be considered for
+ * auto-connect. Note, since we sync the settings, the next
+ * reboot will put the network on the known list.
+ */
+ }
}
/* Done callback for when WSC is triggered by DBus methods */
@@ -541,14 +565,28 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
{
struct wsc *wsc = user_data;
+ wsc->enrollee = NULL;
+ wsc->target = NULL;
+
l_debug("err=%i", err);
+ if (err && wsc->station)
+ station_set_autoconnect(wsc->station, true);
+
switch (err) {
case 0:
break;
case -ECANCELED:
- dbus_pending_reply(&wsc->pending,
+ /* Send reply if we haven't already sent one e.g. in Cancel() */
+ if (wsc->pending)
+ dbus_pending_reply(&wsc->pending,
dbus_error_aborted(wsc->pending));
+
+ if (wsc->pending_cancel)
+ dbus_pending_reply(&wsc->pending_cancel,
+ l_dbus_message_new_method_return(
+ wsc->pending_cancel));
+
return;
case -ENOKEY:
dbus_pending_reply(&wsc->pending,
@@ -568,6 +606,21 @@ static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
wsc_try_credentials(wsc, creds, n_creds);
}
+static void wsc_connect(struct wsc *wsc)
+{
+ const char *pin = NULL;
+
+ if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
+ l_dbus_message_get_arguments(wsc->pending, "s", &pin);
+
+ wsc->enrollee = wsc_enrollee_new(wsc->netdev, wsc->target, pin,
+ wsc_dbus_done_cb, wsc);
+ if (wsc->enrollee)
+ return;
+
+ wsc_dbus_done_cb(-EIO, NULL, 0, wsc);
+}
+
static void station_state_watch(enum station_state state, void *userdata)
{
struct wsc *wsc = userdata;
@@ -594,22 +647,6 @@ 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);
@@ -633,7 +670,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
break;
}
error:
- wsc_connect_cleanup(wsc);
+ wsc->target = NULL;
dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
}
@@ -1089,13 +1126,15 @@ static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
void *user_data)
{
struct wsc *wsc = user_data;
- struct l_dbus_message *reply;
l_debug("");
if (!wsc->pending)
return dbus_error_not_available(message);
+ if (wsc->pending_cancel)
+ return dbus_error_busy(message);
+
wsc_cancel_scan(wsc);
if (wsc->station_state_watch) {
@@ -1105,25 +1144,14 @@ static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
wsc->target = NULL;
}
- if (wsc->wsc_association) {
- int r;
-
- r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
- if (r == 0) {
- wsc->pending_cancel = l_dbus_message_ref(message);
- return NULL;
- }
-
- l_warn("Unable to initiate disconnect: %s", strerror(-r));
- wsc->wsc_association = false;
- }
-
dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
- reply = l_dbus_message_new_method_return(message);
- l_dbus_message_set_arguments(reply, "");
-
- return reply;
+ if (wsc->enrollee) {
+ wsc_enrollee_cancel(wsc->enrollee, true);
+ wsc->pending_cancel = l_dbus_message_ref(message);
+ return NULL;
+ } else
+ return l_dbus_message_new_method_return(message);
}
static void setup_wsc_interface(struct l_dbus_interface *interface)
@@ -1143,7 +1171,6 @@ 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,
@@ -1157,7 +1184,10 @@ static void wsc_free(void *userdata)
if (wsc->pending_cancel)
dbus_pending_reply(&wsc->pending_cancel,
- dbus_error_aborted(wsc->pending_cancel));
+ dbus_error_not_available(wsc->pending_cancel));
+
+ if (wsc->enrollee)
+ wsc_enrollee_destroy(wsc->enrollee);
l_free(wsc);
}
diff --git a/src/wsc.h b/src/wsc.h
index ada08c89..14084aea 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -31,5 +31,14 @@ struct wsc_credentials_info {
bool has_passphrase;
};
+struct wsc_enrollee;
+
typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
unsigned int n_creds, void *user_data);
+
+struct wsc_enrollee *wsc_enrollee_new(struct netdev *netdev,
+ struct scan_bss *target,
+ const char *pin,
+ wsc_done_cb_t done_cb, void *user_data);
+void wsc_enrollee_cancel(struct wsc_enrollee *wsce, bool defer_cb);
+void wsc_enrollee_destroy(struct wsc_enrollee *wsce);
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-01-17 18:51 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 13:32 [PATCH 1/3] wsc: Split out enrollee state machine to own object Andrew Zaborowski
2020-01-13 12:56 ` Denis Kenzior
2020-01-13 18:38 ` Andrew Zaborowski
2020-01-13 13:48 ` Denis Kenzior
2020-01-15 1:23 ` Andrew Zaborowski
2020-01-14 20:53 ` Denis Kenzior
2020-01-15 2:41 ` Andrew Zaborowski
2020-01-14 22:00 ` Denis Kenzior
2020-01-15 22:17 ` Andrew Zaborowski
2020-01-15 23:09 ` Denis Kenzior
2020-01-15 23:45 ` Andrew Zaborowski
2020-01-16 1:54 ` Denis Kenzior
2020-01-13 13:32 ` [PATCH 2/3] wsc: Refactor WSC D-Bus interface logic Andrew Zaborowski
2020-01-16 4:29 [PATCH 1/3] wsc: Split out enrollee state machine to own object Andrew Zaborowski
2020-01-17 18:51 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.