Patches for the AutoConnect bug I hit in the iwd plugin

Patches for the AutoConnect bug I hit in the iwd plugin

[lists]

I did further investigation of the issues I found related to AutoConnect
being set wrong on the iwd backend and found that Connman was wrongly
setting AutoConnect to false after the KnownNetwork is created, causing
a desynchronization with its intended state. I've added some
documentation to make it easier to understand the logic, since it took
some use of rr to figure out what was going on.

Please let me know if I've made any mistakes in operating git
send-email, it's my first time sending patches to Linux Foundation
lists.

Original bug report:
https://lore.kernel.org/connman/CAFA9we9hh9gORA9EB4Sm7opJi_Rfb6c9L8KJz2rC_1zXnUShfA@mail.gmail.com/T/#t

[PATCH 1/3] plugins/iwd: Rename autoconnect fields to clarify meaning

[lists]

From: Jade Lovelace <lists@jade.fyi>

Signed-off-by: Jade Lovelace <lists@jade.fyi>
---
 plugins/iwd.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index 485e0661..5593fa74 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -96,7 +96,7 @@ struct iwd_network {
 	struct iwd_device *iwdd;
 	struct connman_network *network;
 	/* service's autoconnect */
-	bool autoconnect;
+	bool cm_autoconnect;
 };
 
 struct iwd_known_network {
@@ -106,11 +106,11 @@ struct iwd_known_network {
 	char *type;
 	bool hidden;
 	char *last_connected_time;
-	bool auto_connect;
+	bool iwd_auto_connect;
 	int auto_connect_id;
 
 	/* service's autoconnect */
-	bool autoconnect;
+	bool cm_autoconnect;
 };
 
 struct iwd_station {
@@ -246,7 +246,7 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 					"net.connman.iwd.InvalidFormat"))
 			connman_network_set_error(iwdn->network,
 					CONNMAN_NETWORK_ERROR_INVALID_KEY);
-		else if (!iwdn->autoconnect)
+		else if (!iwdn->cm_autoconnect)
 			connman_network_set_error(iwdn->network,
 					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
 		return;
@@ -474,25 +474,25 @@ static int enable_auto_connect(struct iwd_known_network *iwdkn)
 
 static int update_auto_connect(struct iwd_known_network *iwdkn)
 {
-	DBG("auto_connect %d autoconnect %d", iwdkn->auto_connect, iwdkn->autoconnect);
+	DBG("iwd_auto_connect %d cm_autoconnect %d", iwdkn->iwd_auto_connect, iwdkn->cm_autoconnect);
 
-	if (iwdkn->auto_connect == iwdkn->autoconnect)
+	if (iwdkn->iwd_auto_connect == iwdkn->cm_autoconnect)
 		return -EALREADY;
 
-	if (iwdkn->autoconnect)
+	if (iwdkn->cm_autoconnect)
 		return enable_auto_connect(iwdkn);
 	return disable_auto_connect(iwdkn);
 }
 
 static int cm_network_set_autoconnect(struct connman_network *network,
-				bool autoconnect)
+				bool cm_autoconnect)
 {
 	struct iwd_network *iwdn = connman_network_get_data(network);
 	struct iwd_known_network *iwdkn;
 
-	DBG("autoconnect %d", autoconnect);
+	DBG("cm_autoconnect %d", cm_autoconnect);
 
-	iwdn->autoconnect = autoconnect;
+	iwdn->cm_autoconnect = cm_autoconnect;
 
 	if (!iwdn->known_network)
 		return -ENOENT;
@@ -501,7 +501,7 @@ static int cm_network_set_autoconnect(struct connman_network *network,
 	if (!iwdkn)
 		return -ENOENT;
 
-	iwdkn->autoconnect = autoconnect;
+	iwdkn->cm_autoconnect = cm_autoconnect;
 
 	return update_auto_connect(iwdkn);
 }
@@ -1679,12 +1679,12 @@ static void known_network_property_change(GDBusProxy *proxy, const char *name,
 		return;
 
 	if (!strcmp(name, "AutoConnect")) {
-		dbus_bool_t auto_connect;
+		dbus_bool_t iwd_auto_connect;
 
-		dbus_message_iter_get_basic(iter, &auto_connect);
-		iwdkn->auto_connect = auto_connect;
+		dbus_message_iter_get_basic(iter, &iwd_auto_connect);
+		iwdkn->iwd_auto_connect = iwd_auto_connect;
 
-		DBG("%p auto_connect %d", path, iwdkn->auto_connect);
+		DBG("%s iwd_auto_connect %d", path, iwdkn->iwd_auto_connect);
 
 		update_auto_connect(iwdkn);
 	}
@@ -1708,7 +1708,7 @@ static void init_auto_connect(struct iwd_known_network *iwdkn)
 		if (iwdkn != kn)
 			continue;
 
-		iwdkn->autoconnect = iwdn->autoconnect;
+		iwdkn->cm_autoconnect = iwdn->cm_autoconnect;
 		update_auto_connect(iwdkn);
 		return;
 	}
@@ -1736,11 +1736,11 @@ static void create_know_network(GDBusProxy *proxy)
 	iwdkn->hidden = proxy_get_bool(proxy, "Hidden");
 	iwdkn->last_connected_time =
 		g_strdup(proxy_get_string(proxy, "LastConnectedTime"));
-	iwdkn->auto_connect = proxy_get_bool(proxy, "AutoConnect");
+	iwdkn->iwd_auto_connect = proxy_get_bool(proxy, "AutoConnect");
 
-	DBG("name '%s' type %s hidden %d, last_connection_time %s auto_connect %d",
+	DBG("name '%s' type %s hidden %d, last_connection_time %s iwd_auto_connect %d",
 		iwdkn->name, iwdkn->type, iwdkn->hidden,
-		iwdkn->last_connected_time, iwdkn->auto_connect);
+		iwdkn->last_connected_time, iwdkn->iwd_auto_connect);
 
 	init_auto_connect(iwdkn);
 
-- 
2.38.1

[PATCH 2/3] plugins/iwd: Fix typo in create_known_network name

[lists]

From: Jade Lovelace <lists@jade.fyi>

Signed-off-by: Jade Lovelace <lists@jade.fyi>
---
 plugins/iwd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index 5593fa74..9d437b3d 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -1714,7 +1714,7 @@ static void init_auto_connect(struct iwd_known_network *iwdkn)
 	}
 }
 
-static void create_know_network(GDBusProxy *proxy)
+static void create_known_network(GDBusProxy *proxy)
 {
 	const char *path = g_dbus_proxy_get_path(proxy);
 	struct iwd_known_network *iwdkn;
@@ -1825,7 +1825,7 @@ static void object_added(GDBusProxy *proxy, void *user_data)
 	else if (!strcmp(interface, IWD_NETWORK_INTERFACE))
 		create_network(proxy);
 	else if (!strcmp(interface, IWD_KNOWN_NETWORK_INTERFACE))
-		create_know_network(proxy);
+		create_known_network(proxy);
 	else if (!strcmp(interface, IWD_STATION_INTERFACE))
 		create_station(proxy);
 	else if (!strcmp(interface, IWD_AP_INTERFACE))
-- 
2.38.1

[PATCH 3/3] plugins/iwd: fix iwd autoconnect being set wrongly on new connections

[lists]

From: Jade Lovelace <lists@jade.fyi>

Due to an extremely subtle bug in tracking the autoconnect state from
connman on through to iwd, iwd was incorrectly being sent zero-initialized
default data as the autoconnect value.

In particular, what happened is as follows:
- A new iwd_network is created for the iwd.Network that appears, which
  also creates an associated connman_network. In the process of creating
  the connman_network, the iwd plugin receives a callback that correctly
  sets the cm_autoconnect state of the iwd_network.
- Connman's Service.Connect() function is called via D-Bus, which calls
  into the iwd plugin, which in turn calls iwd.Network.Connect() over
  D-Bus.
- The connection completes and the following fire:
    - iwd.KnownNetwork created event, which is supposed to initialize the
      cm_autoconnect state to that of the iwd_network, but this does not
      occur since the iwd_network does not yet have a KnownNetwork
      associated, so it remains uninitialized
    - PropertyChanged event on the corresponding iwd.Network object,
      with the new KnownNetwork property value, springing the trap set
      earlier by synchronizing the zero-initialized
      iwd_known_network.cm_autoconnect state to the iwd KnownNetwork

In practice, this looks like:
-> net.connman.iwd.Network.Connect() on /net/connman/iwd/0/3/0000000000000000000000_psk
<- RequestPassphrase()
-> (passphrase)
-> Set('AutoConnect', False) on /net/connman/iwd/0000000000000000000000_psk

This was found by investigating why my computer was not automatically
connecting to some networks after coming out of sleep, and finding that
the iwd AutoConnect setting was false on those networks while connman
thought it was true (in fact, this was the case! The connman iwd plugin
thought otherwise).

Reproduction:

connmanctl> agent on
Agent registered
connmanctl> config wifi_9cb6d0f7daaf_00000000_managed_psk --remove
connmanctl> connect wifi_9cb6d0f7daaf_00000000_managed_psk
Agent RequestInput wifi_9cb6d0f7daaf_00000000_managed_psk
  Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? 00000000
Connected wifi_9cb6d0f7daaf_00000000_managed_psk

$ busctl get-property net.connman.iwd /net/connman/iwd/00000000_psk net.connman.iwd.KnownNetwork AutoConnect
b false

Then sleep the machine and observe that the network is not automatically
reconnected.

Signed-off-by: Jade Lovelace <lists@jade.fyi>
---
 plugins/iwd.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 85 insertions(+), 4 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index 9d437b3d..1f6743e4 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -41,7 +41,15 @@ static GDBusClient *client;
 static GDBusProxy *agent_proxy;
 static GHashTable *adapters;
 static GHashTable *devices;
+/**
+ * Mapping from dbus path -> struct iwd_network, tracking the set of Network
+ * objects seen by iwd.
+ */
 static GHashTable *networks;
+/**
+ * Mapping from dbus path -> struct iwd_network, tracking the set of iwd
+ * KnownNetwork objects.
+ */
 static GHashTable *known_networks;
 static GHashTable *stations;
 static GHashTable *access_points;
@@ -84,6 +92,11 @@ struct iwd_device {
 	struct connman_device *device;
 };
 
+/**
+ * Structure tracking an net.connman.iwd.Network D-Bus object.
+ *
+ * This is mapped one-to-one to a connman_network object.
+ */
 struct iwd_network {
 	GDBusProxy *proxy;
 	char *path;
@@ -95,10 +108,17 @@ struct iwd_network {
 
 	struct iwd_device *iwdd;
 	struct connman_network *network;
-	/* service's autoconnect */
+	/*
+	 * connman_service's autoconnect.
+	 *
+	 * See Note [Managing autoconnect state] for more details.
+	 */
 	bool cm_autoconnect;
 };
 
+/**
+ * Structure tracking a net.connman.iwd.KnownNetwork D-Bus object.
+ */
 struct iwd_known_network {
 	GDBusProxy *proxy;
 	char *path;
@@ -109,7 +129,11 @@ struct iwd_known_network {
 	bool iwd_auto_connect;
 	int auto_connect_id;
 
-	/* service's autoconnect */
+	/*
+	 * connman_service's autoconnect.
+	 *
+	 * See Note [Managing autoconnect state] for more details.
+	 */
 	bool cm_autoconnect;
 };
 
@@ -1153,11 +1177,58 @@ static void network_property_change(GDBusProxy *proxy, const char *name,
 
 		iwdkn = g_hash_table_lookup(known_networks,
 					iwdn->known_network);
-		if (iwdkn)
+		if (iwdkn) {
+			/* See Note [Managing autoconnect state] */
+			iwdkn->cm_autoconnect = iwdn->cm_autoconnect;
 			update_auto_connect(iwdkn);
+		}
 	}
 }
 
+/*
+ * Note [Managing autoconnect state]:
+ *
+ * We need to set the iwd_known_network's cm_autoconnect status from the
+ * iwd_network, which has in turn been set to the corresponding
+ * connman_service's state when it first appeared (due to
+ * __connman_service_create_from_network).
+ *
+ * The management of the autoconnect state between connman and its plugins and
+ * iwd is rather subtle and prone to bugs:
+ * - Connman itself determines the autoconnect state in struct connman_service,
+ *   which we cannot directly see; we only see cm_network_set_autoconnect
+ *   callbacks.
+ *
+ * - The iwd plugin maintains an independent state machine tracking iwd's view of
+ *   the world, which processes events in a non atomic fashion; for
+ *   instance, a iwd.KnownNetwork created event will appear before the
+ *   corresponding PropertyChanged setting the KnownNetwork property of the
+ *   iwd.Network corresponding to the created iwd.KnownNetwork.
+ *
+ * A typical flow of a network being newly connected to looks like so:
+ * - An iwd.Network appears, and add_network registers a connman_network
+ *   structure with Connman. Connman then in turn creates a service via
+ *   __connman_service_create_from_network.
+ *
+ * - The iwd plugin receives a callback from connman to set the autoconnect
+ *   state, setting the cm_autoconnect state of the iwd_network. At this point,
+ *   there is no iwd_known_network yet.
+ *
+ * - Connman receives a Connect() request on the connman.Service, which is
+ *   forwarded to the iwd plugin via cm_network_connect. The iwd plugin calls
+ *   Connect() on the corresponding iwd.Network (possibly using the iwd
+ *   plugin's agent to get credentials if necessary).
+ *
+ * - Around the time that the connection completes, a iwd.KnownNetwork created
+ *   event appears, followed by a PropertyChanged event noting the change in
+ *   the iwd.Network's KnownNetwork property.
+ *
+ *   This is the first time that we can associate the iwd.KnownNetwork with the
+ *   corresponding iwd.Network and iwd_network. We have the Connman-side
+ *   autoconnect status in the iwd_network structure at this point, so we
+ *   synchronize the autoconnect state with iwd here.
+ */
+
 static unsigned char calculate_strength(int strength)
 {
 	unsigned char res;
@@ -1303,7 +1374,7 @@ static void ap_property_change(GDBusProxy *proxy, const char *name,
 	if (!iwdap)
 		return;
 
-        if (!strcmp(name, "Started")) {
+	if (!strcmp(name, "Started")) {
 		dbus_bool_t started;
 
 		dbus_message_iter_get_basic(iter, &started);
@@ -1742,6 +1813,16 @@ static void create_known_network(GDBusProxy *proxy)
 		iwdkn->name, iwdkn->type, iwdkn->hidden,
 		iwdkn->last_connected_time, iwdkn->iwd_auto_connect);
 
+	/*
+	 * Although we initialize the autoconnect state of this
+	 * iwd_known_network here, it is only initialized in the case of
+	 * networks that already existed prior to startup: in the
+	 * case of a new iwd.KnownNetwork appearing, we are called before the
+	 * iwd_network.known_network field is initialized by a subsequent
+	 * PropertyChanged event.
+	 *
+	 * See Note [Managing autoconnect state].
+	 */
 	init_auto_connect(iwdkn);
 
 	g_dbus_proxy_set_property_watch(iwdkn->proxy,
-- 
2.38.1

Re: [PATCH 3/3] plugins/iwd: fix iwd autoconnect being set wrongly on new connections

[Jade Lovelace]

I think I found that this still doesn't fix the issue, surprisingly. I
am going to investigate further.

On Sat, Jan 7, 2023 at 4:50 PM <lists@jade.fyi> wrote:
>
> From: Jade Lovelace <lists@jade.fyi>
>
> Due to an extremely subtle bug in tracking the autoconnect state from
> connman on through to iwd, iwd was incorrectly being sent zero-initialized
> default data as the autoconnect value.
>
> In particular, what happened is as follows:
> - A new iwd_network is created for the iwd.Network that appears, which
>   also creates an associated connman_network. In the process of creating
>   the connman_network, the iwd plugin receives a callback that correctly
>   sets the cm_autoconnect state of the iwd_network.
> - Connman's Service.Connect() function is called via D-Bus, which calls
>   into the iwd plugin, which in turn calls iwd.Network.Connect() over
>   D-Bus.
> - The connection completes and the following fire:
>     - iwd.KnownNetwork created event, which is supposed to initialize the
>       cm_autoconnect state to that of the iwd_network, but this does not
>       occur since the iwd_network does not yet have a KnownNetwork
>       associated, so it remains uninitialized
>     - PropertyChanged event on the corresponding iwd.Network object,
>       with the new KnownNetwork property value, springing the trap set
>       earlier by synchronizing the zero-initialized
>       iwd_known_network.cm_autoconnect state to the iwd KnownNetwork
>
> In practice, this looks like:
> -> net.connman.iwd.Network.Connect() on /net/connman/iwd/0/3/0000000000000000000000_psk
> <- RequestPassphrase()
> -> (passphrase)
> -> Set('AutoConnect', False) on /net/connman/iwd/0000000000000000000000_psk
>
> This was found by investigating why my computer was not automatically
> connecting to some networks after coming out of sleep, and finding that
> the iwd AutoConnect setting was false on those networks while connman
> thought it was true (in fact, this was the case! The connman iwd plugin
> thought otherwise).
>
> Reproduction:
>
> connmanctl> agent on
> Agent registered
> connmanctl> config wifi_9cb6d0f7daaf_00000000_managed_psk --remove
> connmanctl> connect wifi_9cb6d0f7daaf_00000000_managed_psk
> Agent RequestInput wifi_9cb6d0f7daaf_00000000_managed_psk
>   Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? 00000000
> Connected wifi_9cb6d0f7daaf_00000000_managed_psk
>
> $ busctl get-property net.connman.iwd /net/connman/iwd/00000000_psk net.connman.iwd.KnownNetwork AutoConnect
> b false
>
> Then sleep the machine and observe that the network is not automatically
> reconnected.
>
> Signed-off-by: Jade Lovelace <lists@jade.fyi>
> ---
>  plugins/iwd.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/iwd.c b/plugins/iwd.c
> index 9d437b3d..1f6743e4 100644
> --- a/plugins/iwd.c
> +++ b/plugins/iwd.c
> @@ -41,7 +41,15 @@ static GDBusClient *client;
>  static GDBusProxy *agent_proxy;
>  static GHashTable *adapters;
>  static GHashTable *devices;
> +/**
> + * Mapping from dbus path -> struct iwd_network, tracking the set of Network
> + * objects seen by iwd.
> + */
>  static GHashTable *networks;
> +/**
> + * Mapping from dbus path -> struct iwd_network, tracking the set of iwd
> + * KnownNetwork objects.
> + */
>  static GHashTable *known_networks;
>  static GHashTable *stations;
>  static GHashTable *access_points;
> @@ -84,6 +92,11 @@ struct iwd_device {
>         struct connman_device *device;
>  };
>
> +/**
> + * Structure tracking an net.connman.iwd.Network D-Bus object.
> + *
> + * This is mapped one-to-one to a connman_network object.
> + */
>  struct iwd_network {
>         GDBusProxy *proxy;
>         char *path;
> @@ -95,10 +108,17 @@ struct iwd_network {
>
>         struct iwd_device *iwdd;
>         struct connman_network *network;
> -       /* service's autoconnect */
> +       /*
> +        * connman_service's autoconnect.
> +        *
> +        * See Note [Managing autoconnect state] for more details.
> +        */
>         bool cm_autoconnect;
>  };
>
> +/**
> + * Structure tracking a net.connman.iwd.KnownNetwork D-Bus object.
> + */
>  struct iwd_known_network {
>         GDBusProxy *proxy;
>         char *path;
> @@ -109,7 +129,11 @@ struct iwd_known_network {
>         bool iwd_auto_connect;
>         int auto_connect_id;
>
> -       /* service's autoconnect */
> +       /*
> +        * connman_service's autoconnect.
> +        *
> +        * See Note [Managing autoconnect state] for more details.
> +        */
>         bool cm_autoconnect;
>  };
>
> @@ -1153,11 +1177,58 @@ static void network_property_change(GDBusProxy *proxy, const char *name,
>
>                 iwdkn = g_hash_table_lookup(known_networks,
>                                         iwdn->known_network);
> -               if (iwdkn)
> +               if (iwdkn) {
> +                       /* See Note [Managing autoconnect state] */
> +                       iwdkn->cm_autoconnect = iwdn->cm_autoconnect;
>                         update_auto_connect(iwdkn);
> +               }
>         }
>  }
>
> +/*
> + * Note [Managing autoconnect state]:
> + *
> + * We need to set the iwd_known_network's cm_autoconnect status from the
> + * iwd_network, which has in turn been set to the corresponding
> + * connman_service's state when it first appeared (due to
> + * __connman_service_create_from_network).
> + *
> + * The management of the autoconnect state between connman and its plugins and
> + * iwd is rather subtle and prone to bugs:
> + * - Connman itself determines the autoconnect state in struct connman_service,
> + *   which we cannot directly see; we only see cm_network_set_autoconnect
> + *   callbacks.
> + *
> + * - The iwd plugin maintains an independent state machine tracking iwd's view of
> + *   the world, which processes events in a non atomic fashion; for
> + *   instance, a iwd.KnownNetwork created event will appear before the
> + *   corresponding PropertyChanged setting the KnownNetwork property of the
> + *   iwd.Network corresponding to the created iwd.KnownNetwork.
> + *
> + * A typical flow of a network being newly connected to looks like so:
> + * - An iwd.Network appears, and add_network registers a connman_network
> + *   structure with Connman. Connman then in turn creates a service via
> + *   __connman_service_create_from_network.
> + *
> + * - The iwd plugin receives a callback from connman to set the autoconnect
> + *   state, setting the cm_autoconnect state of the iwd_network. At this point,
> + *   there is no iwd_known_network yet.
> + *
> + * - Connman receives a Connect() request on the connman.Service, which is
> + *   forwarded to the iwd plugin via cm_network_connect. The iwd plugin calls
> + *   Connect() on the corresponding iwd.Network (possibly using the iwd
> + *   plugin's agent to get credentials if necessary).
> + *
> + * - Around the time that the connection completes, a iwd.KnownNetwork created
> + *   event appears, followed by a PropertyChanged event noting the change in
> + *   the iwd.Network's KnownNetwork property.
> + *
> + *   This is the first time that we can associate the iwd.KnownNetwork with the
> + *   corresponding iwd.Network and iwd_network. We have the Connman-side
> + *   autoconnect status in the iwd_network structure at this point, so we
> + *   synchronize the autoconnect state with iwd here.
> + */
> +
>  static unsigned char calculate_strength(int strength)
>  {
>         unsigned char res;
> @@ -1303,7 +1374,7 @@ static void ap_property_change(GDBusProxy *proxy, const char *name,
>         if (!iwdap)
>                 return;
>
> -        if (!strcmp(name, "Started")) {
> +       if (!strcmp(name, "Started")) {
>                 dbus_bool_t started;
>
>                 dbus_message_iter_get_basic(iter, &started);
> @@ -1742,6 +1813,16 @@ static void create_known_network(GDBusProxy *proxy)
>                 iwdkn->name, iwdkn->type, iwdkn->hidden,
>                 iwdkn->last_connected_time, iwdkn->iwd_auto_connect);
>
> +       /*
> +        * Although we initialize the autoconnect state of this
> +        * iwd_known_network here, it is only initialized in the case of
> +        * networks that already existed prior to startup: in the
> +        * case of a new iwd.KnownNetwork appearing, we are called before the
> +        * iwd_network.known_network field is initialized by a subsequent
> +        * PropertyChanged event.
> +        *
> +        * See Note [Managing autoconnect state].
> +        */
>         init_auto_connect(iwdkn);
>
>         g_dbus_proxy_set_property_watch(iwdkn->proxy,
> --
> 2.38.1
>

[PATCH] service: Fix an additional case of autoconnect breakage under iwd

[lists]

From: Jade Lovelace <lists@jade.fyi>

Error flow:
- __connman_service_create_from_network() creates network, but it is not
  favorite since it is not yet saved
- trigger_autoconnect is skipped since !favorite

*** User calls Connect() ***
- iwd plugin gets connect() call
- iwd plugin finds out about new known network and copies the
  autoconnect state of the iwd_network to the iwd_known_network, which
  is false since it was zero initialized and nobody called
  connman_network_set_autoconnect on it.
- 💥 iwd plugin tells iwd to set AutoConnect to false
- service_indicate_state() calls __connman_service_set_favorite(service,
  true)
- nobody tells the iwd plugin about the change in favorite state

This patch calls trigger_autoconnect right at the end there, which will
then propagate the outcome of the favorite state to the iwd plugin. I
think this patch is the right design for the current architecture of the
autoconnect state management, but I think the autoconnect design
probably needs some changes outside the scope of this patch since it is
too easy to make these mistakes.
---
 src/service.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/service.c b/src/service.c
index 079c7a6c..6f31adb3 100644
--- a/src/service.c
+++ b/src/service.c
@@ -156,6 +156,7 @@ static struct connman_ipconfig *create_ip6config(struct connman_service *service
 		int index);
 static void dns_changed(struct connman_service *service);
 static void vpn_auto_connect(void);
+static void trigger_autoconnect(struct connman_service *service);
 
 struct find_data {
 	const char *path;
@@ -5624,6 +5625,9 @@ int __connman_service_set_favorite_delayed(struct connman_service *service,
 	service->favorite = favorite;
 
 	favorite_changed(service);
+	/* If native autoconnect is in use, the favorite state may affect the
+	 * autoconnect state, so it needs to be rerun. */
+	trigger_autoconnect(service);
 
 	if (!delay_ordering) {
 
-- 
2.38.1

Re: [PATCH] service: Fix an additional case of autoconnect breakage under iwd

[Daniel Wagner]

Hi Jade,

On Wed, Jan 11, 2023 at 02:08:37PM -0500, lists@jade.fyi wrote:
> From: Jade Lovelace <lists@jade.fyi>
> 
> Error flow:
> - __connman_service_create_from_network() creates network, but it is not
>   favorite since it is not yet saved
> - trigger_autoconnect is skipped since !favorite
> 
> *** User calls Connect() ***
> - iwd plugin gets connect() call
> - iwd plugin finds out about new known network and copies the
>   autoconnect state of the iwd_network to the iwd_known_network, which
>   is false since it was zero initialized and nobody called
>   connman_network_set_autoconnect on it.
> - 💥 iwd plugin tells iwd to set AutoConnect to false
> - service_indicate_state() calls __connman_service_set_favorite(service,
>   true)
> - nobody tells the iwd plugin about the change in favorite state
> 
> This patch calls trigger_autoconnect right at the end there, which will
> then propagate the outcome of the favorite state to the iwd plugin. I
> think this patch is the right design for the current architecture of the
> autoconnect state management, but I think the autoconnect design
> probably needs some changes outside the scope of this patch since it is
> too easy to make these mistakes.


All four patches applied after minor documentation tweaking (e.g.
s/connman/ConnMan/ etc). I removed the Signed-off-by because we don't have
defined how this has to be interpreted in this project (read we are lacking the
DCO document which explains sets the rules).

Thanks,
Daniel