[PATCH v3 1/2] wiphy: add wiphy_radio_work_is_running

[PATCH v3 1/2] wiphy: add wiphy_radio_work_is_running

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

This provides a way to know if a work item is actually running
vs only been queued and waiting to run.
---
 src/wiphy.c | 10 ++++++++++
 src/wiphy.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index 9cf0e07c..5943721e 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -1615,6 +1615,16 @@ void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id)
 		wiphy_radio_work_next(wiphy);
 }
 
+bool wiphy_radio_work_is_running(struct wiphy *wiphy, uint32_t id)
+{
+	struct wiphy_radio_work_item *item = l_queue_peek_head(wiphy->work);
+
+	if (!item)
+		return false;
+
+	return item->id == id;
+}
+
 static int wiphy_init(void)
 {
 	struct l_genl *genl = iwd_get_genl();
diff --git a/src/wiphy.h b/src/wiphy.h
index 6c91220c..50fcb182 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -116,3 +116,4 @@ uint32_t wiphy_radio_work_insert(struct wiphy *wiphy,
 				int priority,
 				const struct wiphy_radio_work_item_ops *ops);
 void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id);
+bool wiphy_radio_work_is_running(struct wiphy *wiphy, uint32_t id);
-- 
2.26.2

[PATCH v3 2/2] netdev: fix crash from carefully timed Connect()

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

There were a few bugs that this patch addresses. First was
the disconnect callback being called immediately for a
rare case where disconnect was not not required. This should
have never been done, and instead an l_idle can be used so
the callback comes in asynchronously as expected.

The second bug was how the send_disconnect flag was being
set. In netdev_disconnect the check for connect_cmd_id was
not a great way of testing if disconnect should be sent. This
is because once CMD_CONNECT completes this command ID is set
to zero, meaning we end up setting send_disconnect = false.
In this case we actually do want to send the disconnect since
we are presumably in EAPoL.

The only situation where disconnect should not be sent is if
CMD_CONNECT has not yet been issued. This can only happen if
IWD is waiting to queue the connect work item and now this
is checked as such (wiphy_radio_work_is_running).

Prior to this patch, the crashing behavior can be tested using
the following script (or some variant of it, your system timing
may not be the same as mine).

iwctl station wlan0 disconnect
iwctl station wlan0 connect <network1> &
sleep 0.02
iwctl station wlan0 connect <network2>

++++++++ backtrace ++++++++
0  0x7f4e1504e530 in /lib64/libc.so.6
1  0x432b54 in network_get_security() at src/network.c:253
2  0x416e92 in station_handshake_setup() at src/station.c:937
3  0x41a505 in __station_connect_network() at src/station.c:2551
4  0x41a683 in station_disconnect_onconnect_cb() at src/station.c:2581
5  0x40b4ae in netdev_disconnect() at src/netdev.c:3142
6  0x41a719 in station_disconnect_onconnect() at src/station.c:2603
7  0x41a89d in station_connect_network() at src/station.c:2652
8  0x433f1d in network_connect_psk() at src/network.c:886
9  0x43483a in network_connect() at src/network.c:1183
10 0x4add11 in _dbus_object_tree_dispatch() at ell/dbus-service.c:1802
11 0x49ff54 in message_read_handler() at ell/dbus.c:285
12 0x496d2f in io_callback() at ell/io.c:120
13 0x495894 in l_main_iterate() at ell/main.c:478
14 0x49599b in l_main_run() at ell/main.c:521
15 0x495cb3 in l_main_run_with_signal() at ell/main.c:647
16 0x404add in main() at src/main.c:490
17 0x7f4e15038b25 in /lib64/libc.so.6
---
 src/netdev.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index e7ffb635..2414c83e 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3182,6 +3182,13 @@ build_cmd_connect:
 						event_filter, cb, user_data);
 }
 
+static void disconnect_idle(void *user_data)
+{
+	struct netdev *netdev = user_data;
+
+	netdev->disconnect_cb(netdev, true, netdev->user_data);
+}
+
 int netdev_disconnect(struct netdev *netdev,
 				netdev_disconnect_cb_t cb, void *user_data)
 {
@@ -3214,7 +3221,8 @@ int netdev_disconnect(struct netdev *netdev,
 		if (netdev->connect_cmd_id) {
 			l_genl_family_cancel(nl80211, netdev->connect_cmd_id);
 			netdev->connect_cmd_id = 0;
-		} else
+		} else if (!wiphy_radio_work_is_running(netdev->wiphy,
+							netdev->work.id))
 			send_disconnect = false;
 
 		netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
@@ -3239,7 +3247,7 @@ int netdev_disconnect(struct netdev *netdev,
 		netdev->user_data = user_data;
 		netdev->aborting = true;
 	} else if (cb)
-		cb(netdev, true, user_data);
+		l_idle_oneshot(disconnect_idle, netdev, NULL);
 
 	return 0;
 }
-- 
2.26.2

Re: [PATCH v3 2/2] netdev: fix crash from carefully timed Connect()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3885 bytes --]

Hi James,

On 4/5/21 4:53 PM, James Prestwood wrote:
> There were a few bugs that this patch addresses. First was
> the disconnect callback being called immediately for a
> rare case where disconnect was not not required. This should
> have never been done, and instead an l_idle can be used so
> the callback comes in asynchronously as expected.

So really, this should be two commits ;)

> 
> The second bug was how the send_disconnect flag was being
> set. In netdev_disconnect the check for connect_cmd_id was
> not a great way of testing if disconnect should be sent. This
> is because once CMD_CONNECT completes this command ID is set
> to zero, meaning we end up setting send_disconnect = false.
> In this case we actually do want to send the disconnect since
> we are presumably in EAPoL.
> 
> The only situation where disconnect should not be sent is if
> CMD_CONNECT has not yet been issued. This can only happen if
> IWD is waiting to queue the connect work item and now this
> is checked as such (wiphy_radio_work_is_running).
> 
> Prior to this patch, the crashing behavior can be tested using
> the following script (or some variant of it, your system timing
> may not be the same as mine).
> 
> iwctl station wlan0 disconnect
> iwctl station wlan0 connect <network1> &
> sleep 0.02
> iwctl station wlan0 connect <network2>
> 
> ++++++++ backtrace ++++++++
> 0  0x7f4e1504e530 in /lib64/libc.so.6
> 1  0x432b54 in network_get_security() at src/network.c:253
> 2  0x416e92 in station_handshake_setup() at src/station.c:937
> 3  0x41a505 in __station_connect_network() at src/station.c:2551
> 4  0x41a683 in station_disconnect_onconnect_cb() at src/station.c:2581
> 5  0x40b4ae in netdev_disconnect() at src/netdev.c:3142
> 6  0x41a719 in station_disconnect_onconnect() at src/station.c:2603
> 7  0x41a89d in station_connect_network() at src/station.c:2652
> 8  0x433f1d in network_connect_psk() at src/network.c:886
> 9  0x43483a in network_connect() at src/network.c:1183
> 10 0x4add11 in _dbus_object_tree_dispatch() at ell/dbus-service.c:1802
> 11 0x49ff54 in message_read_handler() at ell/dbus.c:285
> 12 0x496d2f in io_callback() at ell/io.c:120
> 13 0x495894 in l_main_iterate() at ell/main.c:478
> 14 0x49599b in l_main_run() at ell/main.c:521
> 15 0x495cb3 in l_main_run_with_signal() at ell/main.c:647
> 16 0x404add in main() at src/main.c:490
> 17 0x7f4e15038b25 in /lib64/libc.so.6
> ---
>   src/netdev.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index e7ffb635..2414c83e 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -3182,6 +3182,13 @@ build_cmd_connect:
>   						event_filter, cb, user_data);
>   }
>   
> +static void disconnect_idle(void *user_data)
> +{
> +	struct netdev *netdev = user_data;
> +
> +	netdev->disconnect_cb(netdev, true, netdev->user_data);

Hmm, how is disconnect_cb being set...?

> +}
> +
>   int netdev_disconnect(struct netdev *netdev,
>   				netdev_disconnect_cb_t cb, void *user_data)
>   {
> @@ -3214,7 +3221,8 @@ int netdev_disconnect(struct netdev *netdev,
>   		if (netdev->connect_cmd_id) {
>   			l_genl_family_cancel(nl80211, netdev->connect_cmd_id);
>   			netdev->connect_cmd_id = 0;
> -		} else
> +		} else if (!wiphy_radio_work_is_running(netdev->wiphy,
> +							netdev->work.id))
>   			send_disconnect = false;
>   
>   		netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
> @@ -3239,7 +3247,7 @@ int netdev_disconnect(struct netdev *netdev,
>   		netdev->user_data = user_data;
>   		netdev->aborting = true;
>   	} else if (cb)
> -		cb(netdev, true, user_data);
> +		l_idle_oneshot(disconnect_idle, netdev, NULL);

It looks like it is only set on the if (send_disconnect) path?

>   
>   	return 0;
>   }
> 

Regards,
-Denis

Re: [PATCH v3 1/2] wiphy: add wiphy_radio_work_is_running

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

Hi James,

On 4/5/21 4:53 PM, James Prestwood wrote:
> This provides a way to know if a work item is actually running
> vs only been queued and waiting to run.
> ---
>   src/wiphy.c | 10 ++++++++++
>   src/wiphy.h |  1 +
>   2 files changed, 11 insertions(+)
> 

Applied, thanks.

Regards,
-Denis