net: connectivity manager: `iface` taken down before disconnect actioned

[JordanYates]

Describe the bug

The Connection Manager library incorrectly takes interfaces down when conn_mgr_if_disconnect is called, before the disconnect is actioned.

The disconnect implementation is required to be non-blocking, which implies that for at least some backends (e.g. HostAP) the work must be offloaded to a different thread for processing.

zephyr/include/zephyr/net/conn_mgr_connectivity_impl.h

Lines 56 to 64 in e1f258d

	* @brief When called, the connectivity implementation should disconnect (disassociate), or
	* stop any in-progress attempts to associate to a network, the bound iface pointed to by
	* if_conn->iface.
	*
	* Must be non-blocking.
	*
	* Called by @ref conn_mgr_if_disconnect.
	*/
	int (*disconnect)(struct conn_mgr_conn_binding *const binding);

Inside conn_mgr_if_disconnect however, the disconnect function is called, immediately followed by conn_mgr_conn_if_auto_admin_down.

zephyr/subsys/net/conn_mgr/conn_mgr_connectivity.c

Lines 78 to 91 in e1f258d

	status = api->disconnect(binding);
	out:
	conn_mgr_binding_unlock(binding);
	/* Since the connectivity implementation will not automatically attempt to reconnect after
	* a call to conn_mgr_if_disconnect, conn_mgr_conn_if_auto_admin_down should be called.
	*
	* conn_mgr_conn_handle_iface_down will only call conn_mgr_conn_if_auto_admin_down if
	* persistence is disabled. To ensure conn_mgr_conn_if_auto_admin_down is called in all
	* cases, we must call it directly from here. If persistence is disabled, this will result
	* in conn_mgr_conn_if_auto_admin_down being called twice, but that is not an issue.
	*/
	conn_mgr_conn_if_auto_admin_down(iface);

zephyr/subsys/net/conn_mgr/conn_mgr_connectivity.c

Lines 288 to 292 in e1f258d

	/* Take the iface admin-down if AUTO_DOWN is enabled */
	if (IS_ENABLED(CONFIG_NET_CONNECTION_MANAGER_AUTO_IF_DOWN) &&
	!conn_mgr_if_get_flag(iface, CONN_MGR_IF_NO_AUTO_DOWN)) {
	net_if_down(iface);
	}

As a result, attempting to disconnect an interface that doesn't have CONN_MGR_IF_NO_AUTO_DOWN set can result in many errors. For example with HostAP:

Expected behavior

Calling conn_mgr_if_disconnect should not result in the interface being turned off before the disconnect takes place.

Logs and console output

Example log errors while trying to disconnect from a Wi-Fi network on a nRF7002, where the NET_REQUEST_WIFI_DISCONNECT command is run from the system workqueue after being scheduled by the disconnect implementation:

[00:02:31.338,858] <dbg> conn_mgr_conn: conn_mgr_if_disconnect: iface 0x20001c10 disconnect
[00:02:31.357,778] <inf> wifi_supplicant: Network interface 1 (0x20001c10) down
[00:02:31.357,904] <err> wpa_supp: wpa_drv_zep_deauthenticate: deauthenticate op failed
[00:02:31.368,493] <dbg> conn_mgr: conn_mgr_iface_events_handler: (net_mgmt): Iface event 0xd0010003 received on iface 1 (0x20001c10)
[00:02:31.368,514] <dbg> conn_mgr: conn_mgr_iface_events_handler: (net_mgmt): Iface index 0
[00:02:31.368,521] <inf> epacket_udp: Downlink watchdog cancelled
[00:02:31.368,553] <dbg> conn_mgr: conn_mgr_iface_events_handler: (net_mgmt): Iface event 0xd0010001 received on iface 1 (0x20001c10)
[00:02:31.368,566] <dbg> conn_mgr: conn_mgr_iface_events_handler: (net_mgmt): Iface index 0
[00:02:31.368,605] <inf> wifi_mgmt: Connection released
[00:02:31.368,661] <err> app: INTERFACE STATE 0
[00:02:31.368,664] <inf> epacket_udp: Network disconnected
[00:02:31.368,688] <err> epacket_udp: Socket closed
[00:02:31.368,786] <err> wpa_supp: zephyr_get_handle_by_ifname: Unable to get wpa_s handle for wlan0
[00:02:31.368,827] <err> wpa_supp: Interface wlan0 not found

Environment (please complete the following information):

• Zephyr v4.1

[rlubos]

Calling net_if_down() right after non-blocking disconnect() clearly seems wrong. My first thought was to simply get rid of it, since we monitor the operational interface state anyway to do exactly the same:

zephyr/subsys/net/conn_mgr/conn_mgr_connectivity.c

Lines 302 to 318 in 252f8fe

	static void conn_mgr_conn_handle_iface_down(struct net_if *iface)
	{
	/* Ignore ifaces that don't have connectivity implementations */
	if (!conn_mgr_if_is_bound(iface)) {
	return;
	}
	/* If the iface is persistent, we expect it to try to reconnect, so nothing else to do */
	if (conn_mgr_if_get_flag(iface, CONN_MGR_IF_PERSISTENT)) {
	return;
	}
	/* Otherwise, we do not expect the iface to reconnect, and we should call
	* conn_mgr_conn_if_auto_admin_down
	*/
	conn_mgr_conn_if_auto_admin_down(iface);
	}

However, this seemed to clash with the behavior reflected by CONN_MGR_IF_PERSISTENT flag. One idea is to introduce a new internal flag, indicating that we're in an active disconnect, and use it to decide whether we should consider putting the interface down or not, something like:

diff

diff --git a/include/zephyr/net/conn_mgr_connectivity.h b/include/zephyr/net/conn_mgr_connectivity.h
index 8979e8cdfea..a5bec42afe1 100644
--- a/include/zephyr/net/conn_mgr_connectivity.h
+++ b/include/zephyr/net/conn_mgr_connectivity.h
@@ -89,6 +89,8 @@ enum conn_mgr_if_flag {
 	 */
 	CONN_MGR_IF_NO_AUTO_DOWN,
 
+	CONN_MGR_IF_DISCONNECTING,
+
 /** @cond INTERNAL_HIDDEN */
 	/* Total number of flags - must be at the end of the enum */
 	CONN_MGR_NUM_IF_FLAGS,
diff --git a/subsys/net/conn_mgr/conn_mgr_connectivity.c b/subsys/net/conn_mgr/conn_mgr_connectivity.c
index 7ffb5367586..f796f2113d7 100644
--- a/subsys/net/conn_mgr/conn_mgr_connectivity.c
+++ b/subsys/net/conn_mgr/conn_mgr_connectivity.c
@@ -43,6 +43,8 @@ int conn_mgr_if_connect(struct net_if *iface)
 
 	status = api->connect(binding);
 
+	conn_mgr_if_set_flag(iface, CONN_MGR_IF_DISCONNECTING, false);
+
 out:
 	conn_mgr_binding_unlock(binding);
 
@@ -77,19 +79,11 @@ int conn_mgr_if_disconnect(struct net_if *iface)
 
 	status = api->disconnect(binding);
 
+	conn_mgr_if_set_flag(iface, CONN_MGR_IF_DISCONNECTING, true);
+
 out:
 	conn_mgr_binding_unlock(binding);
 
-	/* Since the connectivity implementation will not automatically attempt to reconnect after
-	 * a call to conn_mgr_if_disconnect, conn_mgr_conn_if_auto_admin_down should be called.
-	 *
-	 * conn_mgr_conn_handle_iface_down will only call conn_mgr_conn_if_auto_admin_down if
-	 * persistence is disabled. To ensure conn_mgr_conn_if_auto_admin_down is called in all
-	 * cases, we must call it directly from here. If persistence is disabled, this will result
-	 * in conn_mgr_conn_if_auto_admin_down being called twice, but that is not an issue.
-	 */
-	conn_mgr_conn_if_auto_admin_down(iface);
-
 	return status;
 }
 
@@ -307,7 +301,8 @@ static void conn_mgr_conn_handle_iface_down(struct net_if *iface)
 	}
 
 	/* If the iface is persistent, we expect it to try to reconnect, so nothing else to do */
-	if (conn_mgr_if_get_flag(iface, CONN_MGR_IF_PERSISTENT)) {
+	if (conn_mgr_if_get_flag(iface, CONN_MGR_IF_PERSISTENT) &&
+	    !conn_mgr_if_get_flag(iface, CONN_MGR_IF_DISCONNECTING)) {
 		return;
 	}
 

At least this seems to work well with the implemented tests. Alternatively, we could perhaps also consider making conn_mgr_if_disconnect() blocking, and simply wait until the dormant flag is set to ON on the interface before calling admin down.

Any thoughts/preferences?

[jukkar]

What about making disconnect blocking, it should not take long time to do so we would not block for a long time and it would simplify the internal logic.

[JordanYates]

What about making disconnect blocking, it should not take long time to do so we would not block for a long time and it would simplify the internal logic.

That seems reasonable to me, unless there is some deeper reason the note about must be non-blocking exists.
@glarsennordic wrote the initial implementation and the note was present then.

[rlubos]

I think it'd make most sense to remove that non-blocking requirement, and make the connection manager work regardless of whether disconnect() is blocking or not. I think that would be achieved with my initial proposal.
I'll just send a PR so we can discuss the actual solution.

[rlubos]

#90015