3:26 a.m.
These were part of a larger set, but refactored to be stand alone.
Since these are fixing a recently discovered race condition we may
want them in the next release.
James Prestwood (4):
station: add ANQP state watch API
network: delay connect if ANQP has not completed
auto-t: add 'Delay' property to hwsim python module
auto-t: add test for delayed ANQP response
autotests/testHotspot/anqp_delay_test.py | 85 ++++++++++++++++++++++++
autotests/util/hwsim.py | 8 +++
src/network.c | 58 ++++++++++++++++
src/station.c | 21 ++++++
src/station.h | 13 ++++
5 files changed, 185 insertions(+)
create mode 100644 autotests/testHotspot/anqp_delay_test.py
--
2.21.1
3:26 a.m.
New subject: [PATCH 1/4] station: add ANQP state watch API
This is to allow network to watch for ANQP activity in order to
fix the race condition between scanning finishing and ANQP finishing.
Without this it is possible for a DBus Connect() to come in before
ANQP has completed and causing the network to return NotConfigured,
when its actually in the process of obtaining all the network info.
The watch was made globally in station due to network not having
a station object until each individual network is created. Adding a
watch during network creation would result in many watchers as well
as a lot of removal/addition as networks are found and lost.
---
src/station.c | 21 +++++++++++++++++++++
src/station.h | 13 +++++++++++++
2 files changed, 34 insertions(+)
diff --git a/src/station.c b/src/station.c
index 87fadc3f..16da4f0f 100644
--- a/src/station.c
+++ b/src/station.c
@@ -59,6 +59,7 @@ static uint32_t netdev_watch;
static uint32_t mfp_setting;
static bool anqp_disabled;
static bool netconfig_enabled;
+static struct watchlist anqp_watches;
struct station {
enum station_state state;
@@ -489,6 +490,9 @@ static void station_anqp_response_cb(enum anqp_result result,
request_done:
l_queue_remove(station->anqp_pending, entry);
+ WATCHLIST_NOTIFY(&anqp_watches, station_anqp_watch_func_t,
+ STATION_ANQP_FINISHED, network);
+
/* If no more requests, resume scanning */
if (!l_queue_isempty(station->anqp_pending))
return;
@@ -564,6 +568,8 @@ static bool station_start_anqp(struct station *station, struct network
*network,
l_queue_push_head(station->anqp_pending, entry);
+ WATCHLIST_NOTIFY(&anqp_watches, station_anqp_watch_func_t,
+ STATION_ANQP_STARTED, network);
return true;
}
@@ -1217,6 +1223,18 @@ bool station_remove_state_watch(struct station *station, uint32_t
id)
return watchlist_remove(&station->state_watches, id);
}
+uint32_t station_add_anqp_watch(station_anqp_watch_func_t func,
+ void *user_data,
+ station_destroy_func_t destroy)
+{
+ return watchlist_add(&anqp_watches, func, user_data, destroy);
+}
+
+void station_remove_anqp_watch(uint32_t id)
+{
+ watchlist_remove(&anqp_watches, id);
+}
+
bool station_set_autoconnect(struct station *station, bool autoconnect)
{
if (station->autoconnect == autoconnect)
@@ -3270,6 +3288,8 @@ static int station_init(void)
if (!netconfig_enabled)
l_info("station: Network configuration is disabled.");
+ watchlist_init(&anqp_watches, NULL);
+
return 0;
}
@@ -3279,6 +3299,7 @@ static void station_exit(void)
netdev_watch_remove(netdev_watch);
l_queue_destroy(station_list, NULL);
station_list = NULL;
+ watchlist_destroy(&anqp_watches);
}
IWD_MODULE(station, station_init, station_exit)
diff --git a/src/station.h b/src/station.h
index 4155081b..17a0f8df 100644
--- a/src/station.h
+++ b/src/station.h
@@ -43,8 +43,16 @@ enum station_state {
STATION_STATE_ROAMING
};
+enum station_anqp_state {
+ STATION_ANQP_STARTED,
+ STATION_ANQP_FINISHED,
+};
+
typedef void (*station_foreach_func_t)(struct station *, void *data);
typedef void (*station_state_watch_func_t)(enum station_state, void *userdata);
+typedef void (*station_anqp_watch_func_t)(enum station_anqp_state,
+ struct network *network,
+ void *user_data);
typedef void (*station_destroy_func_t)(void *userdata);
typedef void (*station_network_foreach_func_t)(struct network *, void *data);
@@ -66,6 +74,11 @@ uint32_t station_add_state_watch(struct station *station,
station_destroy_func_t destroy);
bool station_remove_state_watch(struct station *station, uint32_t id);
+uint32_t station_add_anqp_watch(station_anqp_watch_func_t func,
+ void *user_data,
+ station_destroy_func_t destroy);
+void station_remove_anqp_watch(uint32_t id);
+
bool station_set_autoconnect(struct station *station, bool autoconnect);
void station_ap_directed_roam(struct station *station,
--
2.21.1
7:30 a.m.
New subject: [PATCH 1/4] station: add ANQP state watch API
Hi James,
On 6/11/20 12:26 PM, James Prestwood wrote:
This is to allow network to watch for ANQP activity in order to
fix the race condition between scanning finishing and ANQP finishing.
Without this it is possible for a DBus Connect() to come in before
ANQP has completed and causing the network to return NotConfigured,
when its actually in the process of obtaining all the network info.
The watch was made globally in station due to network not having
a station object until each individual network is created. Adding a
watch during network creation would result in many watchers as well
as a lot of removal/addition as networks are found and lost.
---
src/station.c | 21 +++++++++++++++++++++
src/station.h | 13 +++++++++++++
2 files changed, 34 insertions(+)
<snip>
struct station {
enum station_state state;
@@ -489,6 +490,9 @@ static void station_anqp_response_cb(enum anqp_result result,
request_done:
l_queue_remove(station->anqp_pending, entry);
+ WATCHLIST_NOTIFY(&anqp_watches, station_anqp_watch_func_t,
+ STATION_ANQP_FINISHED, network);
+
So you don't want to delay until all anqp requests finish? Will the kernel
cancel all the anqp work items nicely in this case?
/* If no more requests, resume scanning */
if (!l_queue_isempty(station->anqp_pending))
return;
Regards,
-Denis
7:38 a.m.
New subject: [PATCH 1/4] station: add ANQP state watch API
On Thu, 2020-06-11 at 16:30 -0500, Denis Kenzior wrote:
Hi James,
On 6/11/20 12:26 PM, James Prestwood wrote:
> This is to allow network to watch for ANQP activity in order to
> fix the race condition between scanning finishing and ANQP
> finishing.
> Without this it is possible for a DBus Connect() to come in before
> ANQP has completed and causing the network to return NotConfigured,
> when its actually in the process of obtaining all the network info.
>
> The watch was made globally in station due to network not having
> a station object until each individual network is created. Adding a
> watch during network creation would result in many watchers as well
> as a lot of removal/addition as networks are found and lost.
> ---
> src/station.c | 21 +++++++++++++++++++++
> src/station.h | 13 +++++++++++++
> 2 files changed, 34 insertions(+)
>
<snip>
> struct station {
> enum station_state state;
> @@ -489,6 +490,9 @@ static void station_anqp_response_cb(enum
> anqp_result result,
> request_done:
> l_queue_remove(station->anqp_pending, entry);
>
> + WATCHLIST_NOTIFY(&anqp_watches, station_anqp_watch_func_t,
> + STATION_ANQP_FINISHED, network);
> +
So you don't want to delay until all anqp requests finish? Will the
kernel
cancel all the anqp work items nicely in this case?
Yeah good point. We could do one of two things:
- Wait until all ANQP requets come back, then notify each network of
changes, or
- Notify as they come in and cancel any pending requests if a connect
comes in.
And no the kernel doesn't do any of this for us, I see ANQP responses
coming in after we start doing a Connect() in my auto tests.
> /* If no more requests, resume scanning */
> if (!l_queue_isempty(station->anqp_pending))
> return;
Regards,
-Denis
3:26 a.m.
New subject: [PATCH 2/4] network: delay connect if ANQP has not completed
Using the new station ANQP watch network can delay the connection
request until after ANQP has finished. Since station may be
autoconnecting we must also add a check in network_autoconnect
which prevents it from autoconnecting if we have a pending Connect
request.
---
src/network.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/src/network.c b/src/network.c
index 170db3e5..2b65637f 100644
--- a/src/network.c
+++ b/src/network.c
@@ -52,6 +52,7 @@
#include "src/util.h"
static uint32_t known_networks_watch;
+static uint32_t anqp_watch;
struct network {
char ssid[33];
@@ -72,7 +73,10 @@ struct network {
bool update_psk:1; /* Whether PSK should be written to storage */
bool ask_passphrase:1; /* Whether we should force-ask agent */
bool is_hs20:1;
+ bool anqp_pending:1; /* Set if there is a pending ANQP request */
int rank;
+ /* Holds DBus Connect() message if it comes in before ANQP finishes */
+ struct l_dbus_message *connect_after_anqp;
};
static bool network_settings_load(struct network *network)
@@ -521,6 +525,10 @@ int network_autoconnect(struct network *network, struct scan_bss
*bss)
bool is_rsn;
int ret;
+ /* already waiting for an ANQP response to connect */
+ if (network->connect_after_anqp)
+ return -EALREADY;
+
switch (security) {
case SECURITY_NONE:
is_rsn = false;
@@ -1102,6 +1110,11 @@ static struct l_dbus_message *network_connect_8021x(struct network
*network,
goto error;
}
+ if (network->connect_after_anqp) {
+ l_dbus_message_unref(network->connect_after_anqp);
+ network->connect_after_anqp = NULL;
+ }
+
station_connect_network(station, network, bss, message);
return NULL;
@@ -1156,6 +1169,19 @@ static struct l_dbus_message *network_connect(struct l_dbus *dbus,
station_connect_network(station, network, bss, message);
return NULL;
case SECURITY_8021X:
+ /*
+ * If there is an ongoing ANQP request we must wait for that to
+ * finish. Save the message and wait for the ANQP watch to
+ * fire
+ */
+ if (network->anqp_pending) {
+ network->connect_after_anqp =
+ l_dbus_message_ref(message);
+ l_debug("Pending ANQP request, delaying connect to %s",
+ network->ssid);
+ return NULL;
+ }
+
if (!network_settings_load(network))
return dbus_error_not_configured(message);
@@ -1484,6 +1510,33 @@ static void known_networks_changed(enum known_networks_event
event,
}
}
+static void anqp_watch_changed(enum station_anqp_state state,
+ struct network *network, void *user_data)
+{
+ network->anqp_pending = state == STATION_ANQP_STARTED;
+
+ if (state == STATION_ANQP_FINISHED && network->connect_after_anqp) {
+ struct l_dbus_message *reply;
+
+ l_debug("ANQP complete, resuming connect to %s", network->ssid);
+
+ if (!network_settings_load(network))
+ reply = dbus_error_not_configured(
+ network->connect_after_anqp);
+ else
+ reply = network_connect_8021x(network,
+ network_bss_select(network, true),
+ network->connect_after_anqp);
+
+ if (!reply)
+ return;
+
+ dbus_pending_reply(&network->connect_after_anqp, reply);
+
+ return;
+ }
+}
+
static void setup_network_interface(struct l_dbus_interface *interface)
{
l_dbus_interface_method(interface, "Connect", 0,
@@ -1517,6 +1570,8 @@ static int network_init(void)
known_networks_watch =
known_networks_watch_add(known_networks_changed, NULL, NULL);
+ anqp_watch = station_add_anqp_watch(anqp_watch_changed, NULL, NULL);
+
return 0;
}
@@ -1525,6 +1580,9 @@ static void network_exit(void)
known_networks_watch_remove(known_networks_watch);
known_networks_watch = 0;
+ station_remove_anqp_watch(anqp_watch);
+ anqp_watch = 0;
+
l_dbus_unregister_interface(dbus_get_bus(), IWD_NETWORK_INTERFACE);
}
--
2.21.1
7:29 a.m.
New subject: [PATCH 2/4] network: delay connect if ANQP has not completed
Hi James,
On 6/11/20 12:26 PM, James Prestwood wrote:
Using the new station ANQP watch network can delay the connection
request until after ANQP has finished. Since station may be
autoconnecting we must also add a check in network_autoconnect
which prevents it from autoconnecting if we have a pending Connect
request.
---
src/network.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
<snip>
@@ -1102,6 +1110,11 @@ static struct l_dbus_message
*network_connect_8021x(struct network *network,
goto error;
}
+ if (network->connect_after_anqp) {
+ l_dbus_message_unref(network->connect_after_anqp);
+ network->connect_after_anqp = NULL;
+ }
+
Hmm, what does this do?
station_connect_network(station, network, bss, message);
return NULL;
@@ -1156,6 +1169,19 @@ static struct l_dbus_message *network_connect(struct l_dbus
*dbus,
station_connect_network(station, network, bss, message);
return NULL;
case SECURITY_8021X:
+ /*
+ * If there is an ongoing ANQP request we must wait for that to
+ * finish. Save the message and wait for the ANQP watch to
+ * fire
+ */
+ if (network->anqp_pending) {
+ network->connect_after_anqp =
+ l_dbus_message_ref(message);
+ l_debug("Pending ANQP request, delaying connect to %s",
+ network->ssid);
+ return NULL;
+ }
+
What if another request comes in during this time? Shouldn't you block the new
request with an -EBUSY or something? Otherwise you leak connect_after_anqp.
if (!network_settings_load(network))
return dbus_error_not_configured(message);
@@ -1484,6 +1510,33 @@ static void known_networks_changed(enum known_networks_event
event,
}
}
+static void anqp_watch_changed(enum station_anqp_state state,
+ struct network *network, void *user_data)
+{
+ network->anqp_pending = state == STATION_ANQP_STARTED;
+
+ if (state == STATION_ANQP_FINISHED && network->connect_after_anqp) {
+ struct l_dbus_message *reply;
+
+ l_debug("ANQP complete, resuming connect to %s", network->ssid);
+
+ if (!network_settings_load(network))
+ reply = dbus_error_not_configured(
+ network->connect_after_anqp);
+ else
+ reply = network_connect_8021x(network,
+ network_bss_select(network, true),
+ network->connect_after_anqp);
+
Hmm, perhaps isolate network->connect_after_anqp in here instead of bleeding the
details into network_connect_8021x?
Maybe something like:
if (!network_load_settings()) {
dbus_pending_reply(&network->connect_after_anqp,
dbus_error_not_configured());
return;
}
reply = network_connect_8021x();
if (reply) {
struct l_dbus *dbus = dbus_get_bus();
l_dbus_send(dbus, reply);
}
l_dbus_message_unref(network->connect_after_anqp);
network->connect_after_anqp = NULL;
+ if (!reply)
+ return;
+
+ dbus_pending_reply(&network->connect_after_anqp, reply);
+
+ return;
+ }
+}
+
static void setup_network_interface(struct l_dbus_interface *interface)
{
l_dbus_interface_method(interface, "Connect", 0,
@@ -1517,6 +1570,8 @@ static int network_init(void)
known_networks_watch =
known_networks_watch_add(known_networks_changed, NULL, NULL);
+ anqp_watch = station_add_anqp_watch(anqp_watch_changed, NULL, NULL);
+
return 0;
}
@@ -1525,6 +1580,9 @@ static void network_exit(void)
known_networks_watch_remove(known_networks_watch);
known_networks_watch = 0;
+ station_remove_anqp_watch(anqp_watch);
+ anqp_watch = 0;
+
l_dbus_unregister_interface(dbus_get_bus(), IWD_NETWORK_INTERFACE);
}
Regards,
-Denis
7:54 a.m.
New subject: [PATCH 2/4] network: delay connect if ANQP has not completed
On Thu, 2020-06-11 at 16:29 -0500, Denis Kenzior wrote:
Hi James,
On 6/11/20 12:26 PM, James Prestwood wrote:
> Using the new station ANQP watch network can delay the connection
> request until after ANQP has finished. Since station may be
> autoconnecting we must also add a check in network_autoconnect
> which prevents it from autoconnecting if we have a pending Connect
> request.
> ---
> src/network.c | 58
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
<snip>
> @@ -1102,6 +1110,11 @@ static struct l_dbus_message
> *network_connect_8021x(struct network *network,
> goto error;
> }
>
> + if (network->connect_after_anqp) {
> + l_dbus_message_unref(network-
> >connect_after_anqp);
> + network->connect_after_anqp = NULL;
> + }
> +
Hmm, what does this do?
Just cleaning up, but I refactored it like you suggested below which
makes this not needed.
> station_connect_network(station, network, bss,
> message);
>
> return NULL;
> @@ -1156,6 +1169,19 @@ static struct l_dbus_message
> *network_connect(struct l_dbus *dbus,
> station_connect_network(station, network, bss,
> message);
> return NULL;
> case SECURITY_8021X:
> + /*
> + * If there is an ongoing ANQP request we must wait for
> that to
> + * finish. Save the message and wait for the ANQP watch
> to
> + * fire
> + */
> + if (network->anqp_pending) {
> + network->connect_after_anqp =
> + l_dbus_message_ref(mess
> age);
> + l_debug("Pending ANQP request, delaying connect
> to %s",
> + network->ssid);
> + return NULL;
> + }
> +
What if another request comes in during this time? Shouldn't you
block the new
request with an -EBUSY or something? Otherwise you leak
connect_after_anqp.
Yep, fixed.
> if (!network_settings_load(network))
> return dbus_error_not_configured(message);
>
> @@ -1484,6 +1510,33 @@ static void known_networks_changed(enum
> known_networks_event event,
> }
> }
>
> +static void anqp_watch_changed(enum station_anqp_state state,
> + struct network *network, void
> *user_data)
> +{
> + network->anqp_pending = state == STATION_ANQP_STARTED;
> +
> + if (state == STATION_ANQP_FINISHED && network-
> >connect_after_anqp) {
> + struct l_dbus_message *reply;
> +
> + l_debug("ANQP complete, resuming connect to %s",
> network->ssid);
> +
> + if (!network_settings_load(network))
> + reply = dbus_error_not_configured(
> + network-
> >connect_after_anqp);
> + else
> + reply = network_connect_8021x(network,
> + network_bss_select(network,
> true),
> + network->connect_after_anqp);
> +
Hmm, perhaps isolate network->connect_after_anqp in here instead of
bleeding the
details into network_connect_8021x?
Maybe something like:
if (!network_load_settings()) {
dbus_pending_reply(&network->connect_after_anqp,
dbus_error_not_configured());
return;
}
reply = network_connect_8021x();
if (reply) {
struct l_dbus *dbus = dbus_get_bus();
l_dbus_send(dbus, reply);
}
Yeah I like this better.
l_dbus_message_unref(network->connect_after_anqp);
network->connect_after_anqp = NULL;
> + if (!reply)
> + return;
> +
> + dbus_pending_reply(&network->connect_after_anqp,
> reply);
> +
> + return;
> + }
> +}
> +
> static void setup_network_interface(struct l_dbus_interface
> *interface)
> {
> l_dbus_interface_method(interface, "Connect", 0,
> @@ -1517,6 +1570,8 @@ static int network_init(void)
> known_networks_watch =
> known_networks_watch_add(known_networks_changed, NULL,
> NULL);
>
> + anqp_watch = station_add_anqp_watch(anqp_watch_changed, NULL,
> NULL);
> +
> return 0;
> }
>
> @@ -1525,6 +1580,9 @@ static void network_exit(void)
> known_networks_watch_remove(known_networks_watch);
> known_networks_watch = 0;
>
> + station_remove_anqp_watch(anqp_watch);
> + anqp_watch = 0;
> +
> l_dbus_unregister_interface(dbus_get_bus(),
> IWD_NETWORK_INTERFACE);
> }
>
>
Regards,
-Denis
3:26 a.m.
New subject: [PATCH 3/4] auto-t: add 'Delay' property to hwsim python module
---
autotests/util/hwsim.py | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/autotests/util/hwsim.py b/autotests/util/hwsim.py
index 4d3748b1..6d2b3e8f 100755
--- a/autotests/util/hwsim.py
+++ b/autotests/util/hwsim.py
@@ -112,6 +112,14 @@ class Rule(HwsimDBusAbstract):
def drop(self, value):
self._prop_proxy.Set(self._iface_name, 'Drop', dbus.Boolean(value))
+ @property
+ def delay(self):
+ return int(self._properties['Delay'])
+
+ @delay.setter
+ def delay(self, value):
+ self._prop_proxy.Set(self._iface_name, 'Delay', dbus.UInt32(value))
+
def remove(self):
self._iface.Remove(reply_handler=self._success,
error_handler=self._failure)
--
2.21.1
3:26 a.m.
New subject: [PATCH 4/4] auto-t: add test for delayed ANQP response
---
autotests/testHotspot/anqp_delay_test.py | 85 ++++++++++++++++++++++++
1 file changed, 85 insertions(+)
create mode 100644 autotests/testHotspot/anqp_delay_test.py
diff --git a/autotests/testHotspot/anqp_delay_test.py
b/autotests/testHotspot/anqp_delay_test.py
new file mode 100644
index 00000000..431f16c1
--- /dev/null
+++ b/autotests/testHotspot/anqp_delay_test.py
@@ -0,0 +1,85 @@
+#!/usr/bin/python3
+
+import unittest
+import sys
+import os
+
+sys.path.append('../util')
+import iwd
+from iwd import IWD
+from iwd import PSKAgent
+from iwd import NetworkType
+from hostapd import HostapdCLI
+from hwsim import Hwsim
+import testutil
+from time import sleep
+
+class Test(unittest.TestCase):
+
+ def test_connection_success(self):
+ hwsim = Hwsim()
+
+ bss_radio = hwsim.get_radio('rad0')
+ rule0 = hwsim.rules.create()
+ rule0.source = bss_radio.addresses[0]
+ rule0.bidirectional = True
+ # Force the case where ANQP does not finish before Connect() comes in
+ rule0.delay = 100
+
+ wd = IWD(True, '/tmp')
+
+ hapd = HostapdCLI(config='ssidHotspot.conf')
+
+ psk_agent = PSKAgent('abc', ('domain\\user',
'testpasswd'))
+ wd.register_psk_agent(psk_agent)
+
+ devices = wd.list_devices(1)
+ device = devices[0]
+
+ # We are dependent on a periodic scan here. We want to wait for this
+ # because this is the first opportunity IWD has to do ANQP. Once ANQP
+ # has been done once the network is set up and we cannot simulate the
+ # 'Connect() before ANQP' race condition anymore.
+ condition = 'obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ condition = 'not obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ ordered_network = device.get_ordered_network('Hotspot')
+
+ self.assertEqual(ordered_network.type, NetworkType.eap)
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+ ordered_network.network_object.connect()
+
+ rule0.delay = 1
+
+ condition = 'obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+ testutil.test_iface_operstate()
+ testutil.test_ifaces_connected(device.name, hapd.ifname)
+
+ device.disconnect()
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+ wd.unregister_psk_agent(psk_agent)
+
+ @classmethod
+ def setUpClass(cls):
+ IWD.copy_to_hotspot('example.conf')
+ conf = '[General]\nDisableANQP=0\n'
+ os.system('echo "%s" > /tmp/main.conf' % conf)
+
+ @classmethod
+ def tearDownClass(cls):
+ IWD.clear_storage()
+ os.remove('/tmp/main.conf')
+
+if __name__ == '__main__':
+ unittest.main(exit=True)
--
2.21.1
829
days inactive
829
days old
8 comments
2 participants
participants (2)
-
Denis Kenzior -
James Prestwood