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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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