[PATCH 0/2] timeserver not resolving ntp server

[PATCH 0/2] timeserver not resolving ntp server

[Nicky Geerts]

This is a series of fixes for an issue where the timeserver cannot
resolve the ntp servers.

1. when the UDP g_io_channel in the nameserver is closed or in an
invalid state (for example by a misbehaving external DNS server, during
a start up sequence), DNS resolve will fail for the ntp server, and
won't ever be able to recover

2. when a service switches from the autoconf link local address to an
address configured by DHCP, the timeserver will drop the notification,
and the nameservers will not be reloaded in the timeserver's resolv instance.

Nicky Geerts (2):
  timeserver: refresh the nameservers before each lookup
  timeserver: include the reason why a timeserver sync is requested

 include/timeserver.h |   7 +++
 src/connman.h        |   2 +-
 src/service.c        |   4 +-
 src/timeserver.c     | 118 ++++++++++++++++++++++++++-----------------
 4 files changed, 81 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH 1/2] timeserver: refresh the nameservers before each lookup

[Nicky Geerts]

There is a possibility where the UDP channel connections in the resolv
instance are being closed because of how the external DNS service might
respond, and are never opened again.  The DNS resolve will keep failing,
and there is no automatic recovery from this.

Similar to the behavior of the WISPR module, refresh the nameservers in
the resolv instance before each DNS request.

---
 src/timeserver.c | 93 ++++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/src/timeserver.c b/src/timeserver.c
index feef8e83..968739c3 100644
--- a/src/timeserver.c
+++ b/src/timeserver.c
@@ -47,6 +47,7 @@ static GResolv *resolv = NULL;
 static int resolv_id = 0;
 
 static void sync_next(void);
+static void ts_set_nameservers(struct connman_service *service);
 
 static void resolv_debug(const char *str, void *data)
 {
@@ -183,6 +184,7 @@ static void sync_next(void)
 	}
 
 	__connman_ntp_stop();
+    ts_set_nameservers(ts_service);
 
 	while (ts_list) {
 		ts_current = ts_list->data;
@@ -347,24 +349,35 @@ static void ts_recheck_enable(void)
 			NULL);
 }
 
-static void ts_reset(struct connman_service *service)
+static int ts_setup_resolv(struct connman_service *service)
 {
-	char **nameservers;
-	int i;
+    int i;
 
-	if (!resolv)
-		return;
+   	i = __connman_service_get_index(service);
+	if (i < 0)
+		return -EINVAL;
+ 
+   	if (resolv) {
+		g_resolv_unref(resolv);
+		resolv = NULL;
+	}
 
-	__connman_timeserver_set_synced(false);
+	resolv = g_resolv_new(i);
+	if (!resolv) {
+		return -ENOMEM;
+	}
 
-	/*
-	 * Before we start creating the new timeserver list we must stop
-	 * any ongoing ntp query and server resolution.
-	 */
+   	if (getenv("CONNMAN_RESOLV_DEBUG"))
+		g_resolv_set_debug(resolv, resolv_debug, "RESOLV");
 
-	__connman_ntp_stop();
+    return 0;
+}
 
-	ts_recheck_disable();
+
+static void ts_set_nameservers(struct connman_service *service)
+{
+	char **nameservers;
+    int i;
 
 	if (resolv_id > 0)
 		g_resolv_cancel_lookup(resolv, resolv_id);
@@ -378,6 +391,27 @@ static void ts_reset(struct connman_service *service)
 
 		g_strfreev(nameservers);
 	}
+}
+
+static void ts_reset(struct connman_service *service)
+{
+	int i;
+
+	if (!resolv)
+		return;
+
+	__connman_timeserver_set_synced(false);
+
+	/*
+	 * Before we start creating the new timeserver list we must stop
+	 * any ongoing ntp query and server resolution.
+	 */
+
+	__connman_ntp_stop();
+
+	ts_recheck_disable();
+
+    ts_set_nameservers(service);
 
 	g_slist_free_full(timeservers_list, g_free);
 
@@ -434,42 +468,17 @@ void __connman_timeserver_set_synced(bool status)
 
 static int timeserver_start(struct connman_service *service)
 {
-	char **nameservers;
 	int i;
+    int rv;
 
 	DBG("service %p", service);
 
-	i = __connman_service_get_index(service);
-	if (i < 0)
-		return -EINVAL;
-
-	nameservers = connman_service_get_nameservers(service);
-
-	/* Stop an already ongoing resolution, if there is one */
-	if (resolv && resolv_id > 0)
-		g_resolv_cancel_lookup(resolv, resolv_id);
-
 	/* get rid of the old resolver */
-	if (resolv) {
-		g_resolv_unref(resolv);
-		resolv = NULL;
-	}
-
-	resolv = g_resolv_new(i);
-	if (!resolv) {
-		g_strfreev(nameservers);
-		return -ENOMEM;
-	}
-
-	if (getenv("CONNMAN_RESOLV_DEBUG"))
-		g_resolv_set_debug(resolv, resolv_debug, "RESOLV");
+    rv = ts_setup_resolv(service);
+    if (rv)
+        return rv;
 
-	if (nameservers) {
-		for (i = 0; nameservers[i]; i++)
-			g_resolv_add_nameserver(resolv, nameservers[i], 53, 0);
-
-		g_strfreev(nameservers);
-	}
+    ts_set_nameservers(service);
 
 	__connman_timeserver_sync(service);
 
-- 
2.25.1

[PATCH 2/2] timeserver: include the reason why a timeserver sync is requested

[Nicky Geerts]

Except for the initial connman_timeserver_start call, and potential
updated of the default service, all subsequent calls to resynchronise
the timeserver are blocked because of the check whether service equals
ts_service in __connman_timeserver_sync.

DHCP updates, which could replace the timeserver and nameservers, and state
change updates are ignored.

As previously suggested by Daniel Wagner on Nov 19th 2019 in a mail to
Vivien Henriet, it would be best to pass the reason of the sync call,
and add the logic in __connman_timeserver_sync.

---
 include/timeserver.h |  7 +++++++
 src/connman.h        |  2 +-
 src/service.c        |  4 ++--
 src/timeserver.c     | 25 ++++++++++++++++++++-----
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/include/timeserver.h b/include/timeserver.h
index 48ea1945..f1d6f1e8 100644
--- a/include/timeserver.h
+++ b/include/timeserver.h
@@ -26,6 +26,13 @@
 extern "C" {
 #endif
 
+enum connman_timeserver_sync_reason {
+    CONNMAN_TIMESERVER_SYNC_REASON_START = 0,
+    CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE = 1,
+    CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE = 2,
+    CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE = 3,
+};
+
 int __connman_timeserver_system_set(char **server);
 
 #ifdef __cplusplus
diff --git a/src/connman.h b/src/connman.h
index 33dbec69..0985cb11 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -451,7 +451,7 @@ char **__connman_timeserver_system_get();
 GSList *__connman_timeserver_add_list(GSList *server_list,
 		const char *timeserver);
 GSList *__connman_timeserver_get_all(struct connman_service *service);
-void __connman_timeserver_sync(struct connman_service *service);
+void __connman_timeserver_sync(struct connman_service *service, enum connman_timeserver_sync_reason reason);
 void __connman_timeserver_conf_update(struct connman_service *service);
 
 bool __connman_timeserver_is_synced(void);
diff --git a/src/service.c b/src/service.c
index f1abb963..52c4ee4c 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1476,7 +1476,7 @@ static void address_updated(struct connman_service *service,
 		nameserver_add_all(service, type);
 		start_online_check(service, type);
 
-		__connman_timeserver_sync(service);
+		__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE);
 	}
 }
 
@@ -6504,7 +6504,7 @@ int __connman_service_ipconfig_indicate_state(struct connman_service *service,
 	if (!is_connected(old_state) && is_connected(new_state))
 		nameserver_add_all(service, type);
 
-	__connman_timeserver_sync(service);
+	__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE);
 
 	return service_indicate_state(service);
 }
diff --git a/src/timeserver.c b/src/timeserver.c
index 968739c3..3bae9074 100644
--- a/src/timeserver.c
+++ b/src/timeserver.c
@@ -309,7 +309,7 @@ static gboolean ts_recheck(gpointer user_data)
 		g_slist_free_full(ts, g_free);
 
 		service = connman_service_get_default();
-		__connman_timeserver_sync(service);
+		__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE);
 
 		return FALSE;
 	}
@@ -430,12 +430,27 @@ static void ts_reset(struct connman_service *service)
 	timeserver_sync_start();
 }
 
-void __connman_timeserver_sync(struct connman_service *service)
+void __connman_timeserver_sync(struct connman_service *service, enum connman_timeserver_sync_reason reason)
 {
-	if (!service || ts_service == service)
+	if (!service)
 		return;
 
-	ts_reset(service);
+    switch (reason) {
+    case CONNMAN_TIMESERVER_SYNC_REASON_START:
+    case CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE:
+        if (ts_service == service)
+            return;
+        break;
+    case CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE:
+    case CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE:
+        if (ts_service != service)
+            return;
+        break;
+    default:
+        return;
+    }
+
+    ts_reset(service);
 }
 
 void __connman_timeserver_conf_update(struct connman_service *service)
@@ -480,7 +495,7 @@ static int timeserver_start(struct connman_service *service)
 
     ts_set_nameservers(service);
 
-	__connman_timeserver_sync(service);
+	__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_START);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 0/2] timeserver not resolving ntp server

[Daniel Wagner]

Hi Nicky,

On Mon, Feb 28, 2022 at 01:49:28PM +0100, Nicky Geerts wrote:
> This is a series of fixes for an issue where the timeserver cannot
> resolve the ntp servers.
> 
> 1. when the UDP g_io_channel in the nameserver is closed or in an
> invalid state (for example by a misbehaving external DNS server, during
> a start up sequence), DNS resolve will fail for the ntp server, and
> won't ever be able to recover
> 
> 2. when a service switches from the autoconf link local address to an
> address configured by DHCP, the timeserver will drop the notification,
> and the nameservers will not be reloaded in the timeserver's resolv instance.

Thanks for the excellent problem description. It really helps. The
changes look good though they seem to have the wrong ident level. Could
you check if you have used tabs?

Thanks,
Daniel

[PATCH 1/2] timeserver: refresh the nameservers before each lookup

[Nicky Geerts]

There is a possibility where the UDP channel connections in the resolv
instance are being closed because of how the external DNS service might
respond, and are never opened again.  The DNS resolve will keep failing,
and there is no automatic recovery from this.

Similar to the behavior of the WISPR module, refresh the nameservers in
the resolv instance before each DNS request.

---
 src/timeserver.c | 91 ++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 41 deletions(-)

diff --git a/src/timeserver.c b/src/timeserver.c
index feef8e83..0ea73d63 100644
--- a/src/timeserver.c
+++ b/src/timeserver.c
@@ -47,6 +47,7 @@ static GResolv *resolv = NULL;
 static int resolv_id = 0;
 
 static void sync_next(void);
+static void ts_set_nameservers(struct connman_service *service);
 
 static void resolv_debug(const char *str, void *data)
 {
@@ -183,6 +184,7 @@ static void sync_next(void)
 	}
 
 	__connman_ntp_stop();
+	ts_set_nameservers(ts_service);
 
 	while (ts_list) {
 		ts_current = ts_list->data;
@@ -347,24 +349,35 @@ static void ts_recheck_enable(void)
 			NULL);
 }
 
-static void ts_reset(struct connman_service *service)
+static int ts_setup_resolv(struct connman_service *service)
 {
-	char **nameservers;
 	int i;
 
-	if (!resolv)
-		return;
+	i = __connman_service_get_index(service);
+	if (i < 0)
+		return -EINVAL;
+ 
+	if (resolv) {
+		g_resolv_unref(resolv);
+		resolv = NULL;
+	}
 
-	__connman_timeserver_set_synced(false);
+	resolv = g_resolv_new(i);
+	if (!resolv) {
+		return -ENOMEM;
+	}
 
-	/*
-	 * Before we start creating the new timeserver list we must stop
-	 * any ongoing ntp query and server resolution.
-	 */
+	if (getenv("CONNMAN_RESOLV_DEBUG"))
+		g_resolv_set_debug(resolv, resolv_debug, "RESOLV");
 
-	__connman_ntp_stop();
+	return 0;
+}
 
-	ts_recheck_disable();
+
+static void ts_set_nameservers(struct connman_service *service)
+{
+	char **nameservers;
+	int i;
 
 	if (resolv_id > 0)
 		g_resolv_cancel_lookup(resolv, resolv_id);
@@ -378,6 +391,27 @@ static void ts_reset(struct connman_service *service)
 
 		g_strfreev(nameservers);
 	}
+}
+
+static void ts_reset(struct connman_service *service)
+{
+	int i;
+
+	if (!resolv)
+		return;
+
+	__connman_timeserver_set_synced(false);
+
+	/*
+	 * Before we start creating the new timeserver list we must stop
+	 * any ongoing ntp query and server resolution.
+	 */
+
+	__connman_ntp_stop();
+
+	ts_recheck_disable();
+
+	ts_set_nameservers(service);
 
 	g_slist_free_full(timeservers_list, g_free);
 
@@ -434,42 +468,17 @@ void __connman_timeserver_set_synced(bool status)
 
 static int timeserver_start(struct connman_service *service)
 {
-	char **nameservers;
 	int i;
+	int rv;
 
 	DBG("service %p", service);
 
-	i = __connman_service_get_index(service);
-	if (i < 0)
-		return -EINVAL;
-
-	nameservers = connman_service_get_nameservers(service);
-
-	/* Stop an already ongoing resolution, if there is one */
-	if (resolv && resolv_id > 0)
-		g_resolv_cancel_lookup(resolv, resolv_id);
-
 	/* get rid of the old resolver */
-	if (resolv) {
-		g_resolv_unref(resolv);
-		resolv = NULL;
-	}
-
-	resolv = g_resolv_new(i);
-	if (!resolv) {
-		g_strfreev(nameservers);
-		return -ENOMEM;
-	}
+	rv = ts_setup_resolv(service);
+	if (rv)
+		return rv;
 
-	if (getenv("CONNMAN_RESOLV_DEBUG"))
-		g_resolv_set_debug(resolv, resolv_debug, "RESOLV");
-
-	if (nameservers) {
-		for (i = 0; nameservers[i]; i++)
-			g_resolv_add_nameserver(resolv, nameservers[i], 53, 0);
-
-		g_strfreev(nameservers);
-	}
+	ts_set_nameservers(service);
 
 	__connman_timeserver_sync(service);
 
-- 
2.25.1

[PATCH 2/2] timeserver: include the reason why a timeserver sync is requested

[Nicky Geerts]

Except for the initial connman_timeserver_start call, and potential
updated of the default service, all subsequent calls to resynchronise
the timeserver are blocked because of the check whether service equals
ts_service in __connman_timeserver_sync.

DHCP updates, which could replace the timeserver and nameservers, and state
change updates are ignored.

As previously suggested by Daniel Wagner on Nov 19th 2019 in a mail to
Vivien Henriet, it would be best to pass the reason of the sync call,
and add the logic in __connman_timeserver_sync.

---
 include/timeserver.h |  7 +++++++
 src/connman.h        |  2 +-
 src/service.c        |  4 ++--
 src/timeserver.c     | 23 +++++++++++++++++++----
 4 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/include/timeserver.h b/include/timeserver.h
index 48ea1945..2e209f1c 100644
--- a/include/timeserver.h
+++ b/include/timeserver.h
@@ -26,6 +26,13 @@
 extern "C" {
 #endif
 
+enum connman_timeserver_sync_reason {
+	CONNMAN_TIMESERVER_SYNC_REASON_START = 0,
+	CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE = 1,
+	CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE = 2,
+	CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE = 3,
+};
+
 int __connman_timeserver_system_set(char **server);
 
 #ifdef __cplusplus
diff --git a/src/connman.h b/src/connman.h
index 33dbec69..0985cb11 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -451,7 +451,7 @@ char **__connman_timeserver_system_get();
 GSList *__connman_timeserver_add_list(GSList *server_list,
 		const char *timeserver);
 GSList *__connman_timeserver_get_all(struct connman_service *service);
-void __connman_timeserver_sync(struct connman_service *service);
+void __connman_timeserver_sync(struct connman_service *service, enum connman_timeserver_sync_reason reason);
 void __connman_timeserver_conf_update(struct connman_service *service);
 
 bool __connman_timeserver_is_synced(void);
diff --git a/src/service.c b/src/service.c
index f1abb963..52c4ee4c 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1476,7 +1476,7 @@ static void address_updated(struct connman_service *service,
 		nameserver_add_all(service, type);
 		start_online_check(service, type);
 
-		__connman_timeserver_sync(service);
+		__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE);
 	}
 }
 
@@ -6504,7 +6504,7 @@ int __connman_service_ipconfig_indicate_state(struct connman_service *service,
 	if (!is_connected(old_state) && is_connected(new_state))
 		nameserver_add_all(service, type);
 
-	__connman_timeserver_sync(service);
+	__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE);
 
 	return service_indicate_state(service);
 }
diff --git a/src/timeserver.c b/src/timeserver.c
index 0ea73d63..4efd84d0 100644
--- a/src/timeserver.c
+++ b/src/timeserver.c
@@ -309,7 +309,7 @@ static gboolean ts_recheck(gpointer user_data)
 		g_slist_free_full(ts, g_free);
 
 		service = connman_service_get_default();
-		__connman_timeserver_sync(service);
+		__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE);
 
 		return FALSE;
 	}
@@ -430,11 +430,26 @@ static void ts_reset(struct connman_service *service)
 	timeserver_sync_start();
 }
 
-void __connman_timeserver_sync(struct connman_service *service)
+void __connman_timeserver_sync(struct connman_service *service, enum connman_timeserver_sync_reason reason)
 {
-	if (!service || ts_service == service)
+	if (!service)
 		return;
 
+	switch (reason) {
+	case CONNMAN_TIMESERVER_SYNC_REASON_START:
+	case CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE:
+		if (ts_service == service)
+			return;
+		break;
+	case CONNMAN_TIMESERVER_SYNC_REASON_ADDRESS_UPDATE:
+	case CONNMAN_TIMESERVER_SYNC_REASON_TS_CHANGE:
+		if (ts_service != service)
+			return;
+		break;
+	default:
+		return;
+	}
+
 	ts_reset(service);
 }
 
@@ -480,7 +495,7 @@ static int timeserver_start(struct connman_service *service)
 
 	ts_set_nameservers(service);
 
-	__connman_timeserver_sync(service);
+	__connman_timeserver_sync(service, CONNMAN_TIMESERVER_SYNC_REASON_START);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 1/2] timeserver: refresh the nameservers before each lookup

[Daniel Wagner]

Hi Nicky,

On Mon, Mar 07, 2022 at 09:44:31AM +0100, Nicky Geerts wrote:
> There is a possibility where the UDP channel connections in the resolv
> instance are being closed because of how the external DNS service might
> respond, and are never opened again.  The DNS resolve will keep failing,
> and there is no automatic recovery from this.
> 
> Similar to the behavior of the WISPR module, refresh the nameservers in
> the resolv instance before each DNS request.

Patch applied.

I had to fix a few small fix though:

src/timeserver.c: In function ‘ts_reset’:
src/timeserver.c:398:13: error: unused variable ‘i’ [-Werror=unused-variable]
  398 |         int i;
      |             ^
src/timeserver.c: In function ‘timeserver_start’:
src/timeserver.c:471:13: error: unused variable ‘i’ [-Werror=unused-variable]
  471 |         int i;
      |             ^

and couple of white space damages.

Thanks,
Daniel

Re: [PATCH 2/2] timeserver: include the reason why a timeserver sync is requested

[Daniel Wagner]

On Mon, Mar 07, 2022 at 09:44:32AM +0100, Nicky Geerts wrote:
> Except for the initial connman_timeserver_start call, and potential
> updated of the default service, all subsequent calls to resynchronise
> the timeserver are blocked because of the check whether service equals
> ts_service in __connman_timeserver_sync.
> 
> DHCP updates, which could replace the timeserver and nameservers, and state
> change updates are ignored.
> 
> As previously suggested by Daniel Wagner on Nov 19th 2019 in a mail to
> Vivien Henriet, it would be best to pass the reason of the sync call,
> and add the logic in __connman_timeserver_sync.

Patch applied.

FWIW, I added a few newlines so the lines are not over 80 chars.

Thanks,
Daniel