From 961824d43b5bd649ff98bb8fbf05d276faff82b6 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 20 Feb 2023 18:14:14 +0100 Subject: [PATCH 1/5] core: change buffer allocation size for the daemon helper Use slightly more efficient sizes. --- src/core/nm-core-utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c index 53afaa547a..2c251855bf 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -5130,7 +5130,7 @@ nm_utils_spawn_helper(const char *const *args, fcntl(info->child_stdout, F_SETFL, fd_flags | O_NONBLOCK); /* Watch process stdin */ - info->out_buffer = NM_STR_BUF_INIT(32, TRUE); + info->out_buffer = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_40, TRUE); for (arg = args; *arg; arg++) { nm_str_buf_append(&info->out_buffer, *arg); nm_str_buf_append_c(&info->out_buffer, '\0'); @@ -5144,7 +5144,7 @@ nm_utils_spawn_helper(const char *const *args, g_source_attach(info->output_source, g_main_context_get_thread_default()); /* Watch process stdout */ - info->in_buffer = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_1000, FALSE); + info->in_buffer = NM_STR_BUF_INIT(0, FALSE); info->input_source = nm_g_unix_fd_source_new(info->child_stdout, G_IO_IN | G_IO_ERR | G_IO_HUP, G_PRIORITY_DEFAULT, From 7037aa66c6fce01d3e33e1fae32c35fb1f25b363 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 13 Feb 2023 17:08:44 +0100 Subject: [PATCH 2/5] device: improve logging for hostname-from-dns events Improve logging: - log only when something changes - print the new resolver state, instead of the old one - rename state "in-progress" to "started" - log when the resolver state is reset due to DNS changes --- src/core/devices/nm-device.c | 86 ++++++++++++++++++++---------------- src/core/devices/nm-device.h | 2 +- src/core/nm-policy.c | 2 +- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 8cd12bf1d8..0bbd5f02b3 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -312,7 +312,7 @@ typedef struct { typedef enum { RESOLVER_WAIT_ADDRESS = 0, - RESOLVER_IN_PROGRESS, + RESOLVER_STARTED, RESOLVER_DONE, } ResolverState; @@ -17110,6 +17110,21 @@ nm_device_auth_retries_try_next(NMDevice *self) return TRUE; } +static const char * +_resolver_state_to_string(ResolverState state) +{ + switch (state) { + case RESOLVER_WAIT_ADDRESS: + return "WAIT-ADDRESS"; + case RESOLVER_STARTED: + return "STARTED"; + case RESOLVER_DONE: + return "DONE"; + } + nm_assert_not_reached(); + return "UNKNOWN"; +} + static void hostname_dns_lookup_callback(GObject *source, GAsyncResult *result, gpointer user_data) { @@ -17129,7 +17144,9 @@ hostname_dns_lookup_callback(GObject *source, GAsyncResult *result, gpointer use if (error) { _LOGD(LOGD_DNS, - "hostname-from-dns: lookup error for %s: %s", + "hostname-from-dns: ipv%c resolver %s: lookup error for %s: %s", + nm_utils_addr_family_to_char(resolver->addr_family), + _resolver_state_to_string(RESOLVER_DONE), (addr_str = g_inet_address_to_string(resolver->address)), error->message); } else { @@ -17139,7 +17156,9 @@ hostname_dns_lookup_callback(GObject *source, GAsyncResult *result, gpointer use valid = nm_utils_validate_hostname(resolver->hostname); _LOGD(LOGD_DNS, - "hostname-from-dns: lookup done for %s, result %s%s%s%s", + "hostname-from-dns: ipv%c resolver %s: lookup successful for %s, result %s%s%s%s", + nm_utils_addr_family_to_char(resolver->addr_family), + _resolver_state_to_string(RESOLVER_DONE), (addr_str = g_inet_address_to_string(resolver->address)), NM_PRINT_FMT_QUOTE_STRING(resolver->hostname), valid ? "" : " (invalid)"); @@ -17165,8 +17184,9 @@ hostname_dns_address_timeout(gpointer user_data) nm_assert(!resolver->cancellable); _LOGT(LOGD_DNS, - "hostname-from-dns: timed out while waiting IPv%c address", - nm_utils_addr_family_to_char(resolver->addr_family)); + "hostname-from-dns: ipv%c state %s: timed out while waiting for address", + nm_utils_addr_family_to_char(resolver->addr_family), + _resolver_state_to_string(RESOLVER_DONE)); resolver->timeout_id = 0; resolver->state = RESOLVER_DONE; @@ -17175,30 +17195,16 @@ hostname_dns_address_timeout(gpointer user_data) return G_SOURCE_REMOVE; } -static const char * -_resolver_state_to_string(ResolverState state) -{ - switch (state) { - case RESOLVER_WAIT_ADDRESS: - return "wait-address"; - case RESOLVER_IN_PROGRESS: - return "in-progress"; - case RESOLVER_DONE: - return "done"; - default: - nm_assert_not_reached(); - return "unknown"; - } -} - void -nm_device_clear_dns_lookup_data(NMDevice *self) +nm_device_clear_dns_lookup_data(NMDevice *self, const char *reason) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); - guint i; - for (i = 0; i < 2; i++) - nm_clear_pointer(&priv->hostname_resolver_x[i], _hostname_resolver_free); + if (priv->hostname_resolver_4 || priv->hostname_resolver_6) { + _LOGT(LOGD_DNS, "hostname-from-dns: resetting (%s)", reason); + nm_clear_pointer(&priv->hostname_resolver_4, _hostname_resolver_free); + nm_clear_pointer(&priv->hostname_resolver_6, _hostname_resolver_free); + } } gboolean @@ -17332,29 +17338,35 @@ nm_device_get_hostname_from_dns_lookup(NMDevice *self, int addr_family, gboolean } else if (new_address != resolver->address) address_changed = TRUE; + if (address_changed) { + /* set new state before logging */ + if (new_address) + resolver->state = RESOLVER_STARTED; + else + resolver->state = RESOLVER_WAIT_ADDRESS; + } + { gs_free char *old_str = NULL; gs_free char *new_str = NULL; - _LOGT(LOGD_DNS, - "hostname-from-dns: ipv%c resolver state %s, old address %s, new address %s", - nm_utils_addr_family_to_char(resolver->addr_family), - _resolver_state_to_string(resolver->state), - resolver->address ? (old_str = g_inet_address_to_string(resolver->address)) - : "(null)", - new_address ? (new_str = g_inet_address_to_string(new_address)) : "(null)"); + if (address_changed) { + _LOGT(LOGD_DNS, + "hostname-from-dns: ipv%c resolver %s, address changed from %s to %s", + nm_utils_addr_family_to_char(resolver->addr_family), + _resolver_state_to_string(resolver->state), + resolver->address ? (old_str = g_inet_address_to_string(resolver->address)) + : "(null)", + new_address ? (new_str = g_inet_address_to_string(new_address)) : "(null)"); + } } - /* In every state, if the address changed, we restart - * the resolution with the new address */ if (address_changed) { nm_clear_g_cancellable(&resolver->cancellable); g_clear_object(&resolver->address); - resolver->state = RESOLVER_WAIT_ADDRESS; } if (address_changed && new_address) { - resolver->state = RESOLVER_IN_PROGRESS; resolver->cancellable = g_cancellable_new(); resolver->address = g_steal_pointer(&new_address); @@ -17372,7 +17384,7 @@ nm_device_get_hostname_from_dns_lookup(NMDevice *self, int addr_family, gboolean resolver->timeout_id = g_timeout_add(30000, hostname_dns_address_timeout, resolver); NM_SET_OUT(out_wait, TRUE); return NULL; - case RESOLVER_IN_PROGRESS: + case RESOLVER_STARTED: NM_SET_OUT(out_wait, TRUE); return NULL; case RESOLVER_DONE: diff --git a/src/core/devices/nm-device.h b/src/core/devices/nm-device.h index fd6b9d93f8..65e3433404 100644 --- a/src/core/devices/nm-device.h +++ b/src/core/devices/nm-device.h @@ -817,7 +817,7 @@ gboolean nm_device_is_vpn(NMDevice *self); const char * nm_device_get_hostname_from_dns_lookup(NMDevice *self, int addr_family, gboolean *out_pending); -void nm_device_clear_dns_lookup_data(NMDevice *self); +void nm_device_clear_dns_lookup_data(NMDevice *self, const char *reason); gboolean nm_device_get_allow_autoconnect_on_external(NMDevice *self); diff --git a/src/core/nm-policy.c b/src/core/nm-policy.c index 4192808a7b..d7e05b7b5b 100644 --- a/src/core/nm-policy.c +++ b/src/core/nm-policy.c @@ -2581,7 +2581,7 @@ dns_config_changed(NMDnsManager *dns_manager, gpointer user_data) return; nm_manager_for_each_device (priv->manager, device, tmp_lst) { - nm_device_clear_dns_lookup_data(device); + nm_device_clear_dns_lookup_data(device, "DNS configuration changed"); } update_system_hostname(self, "DNS configuration changed"); From d65702803cb02ddd656f13917a8a9af7bc08b90a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 Feb 2023 09:23:51 +0100 Subject: [PATCH 3/5] core: print stderr from nm-daemon-helper Currently the only way to return an error code from the daemon helper is via the process exit code, but that is not enough to fully describe an error from getaddrinfo(); in fact, the function returns a EAI_* error code and when the value is EAI_SYSTEM, the error code is returned in errno. At the moment, any messages printed to stderr by the helper goes to NM stderr; instead, we want to capture it and pass it through the logging mechanism of NM, so that it can be filtered according to level and domain. --- src/core/nm-core-utils.c | 44 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c index 2c251855bf..15b3c6a80a 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -4866,11 +4866,14 @@ typedef struct { int child_stdin; int child_stdout; + int child_stderr; GSource *input_source; GSource *output_source; + GSource *error_source; NMStrBuf in_buffer; NMStrBuf out_buffer; + NMStrBuf err_buffer; gsize out_buffer_offset; } HelperInfo; @@ -4906,13 +4909,17 @@ helper_info_free(gpointer data) nm_str_buf_destroy(&info->in_buffer); nm_str_buf_destroy(&info->out_buffer); + nm_str_buf_destroy(&info->err_buffer); nm_clear_g_source_inst(&info->input_source); nm_clear_g_source_inst(&info->output_source); + nm_clear_g_source_inst(&info->error_source); if (info->child_stdout != -1) nm_close(info->child_stdout); if (info->child_stdin != -1) nm_close(info->child_stdin); + if (info->child_stderr != -1) + nm_close(info->child_stderr); if (info->pid != -1) { nm_assert(info->pid > 1); @@ -4926,6 +4933,10 @@ static void helper_complete(HelperInfo *info, GError *error) { if (error) { + if (info->err_buffer.len > 0) { + _LOG2T(info, "stderr: %s", nm_str_buf_get_str(&info->err_buffer)); + } + nm_clear_g_cancellable_disconnect(g_task_get_cancellable(info->task), &info->cancellable_id); g_task_return_error(info->task, error); @@ -5021,6 +5032,24 @@ helper_have_data(int fd, GIOCondition condition, gpointer user_data) return G_SOURCE_CONTINUE; } +static gboolean +helper_have_err_data(int fd, GIOCondition condition, gpointer user_data) +{ + HelperInfo *info = user_data; + gssize n_read; + + n_read = nm_utils_fd_read(fd, &info->err_buffer); + + if (n_read > 0) + return G_SOURCE_CONTINUE; + + nm_clear_g_source_inst(&info->error_source); + nm_close(info->child_stderr); + info->child_stderr = -1; + + return G_SOURCE_CONTINUE; +} + static void helper_child_terminated(GPid pid, int status, gpointer user_data) { @@ -5096,10 +5125,11 @@ nm_utils_spawn_helper(const char *const *args, &info->pid, &info->child_stdin, &info->child_stdout, - NULL, + &info->child_stderr, &error)) { info->child_stdin = -1; info->child_stdout = -1; + info->child_stderr = -1; info->pid = -1; g_task_return_error(info->task, g_error_new(NM_UTILS_ERROR, @@ -5128,6 +5158,8 @@ nm_utils_spawn_helper(const char *const *args, fcntl(info->child_stdin, F_SETFL, fd_flags | O_NONBLOCK); fd_flags = fcntl(info->child_stdout, F_GETFD, 0); fcntl(info->child_stdout, F_SETFL, fd_flags | O_NONBLOCK); + fd_flags = fcntl(info->child_stderr, F_GETFD, 0); + fcntl(info->child_stderr, F_SETFL, fd_flags | O_NONBLOCK); /* Watch process stdin */ info->out_buffer = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_40, TRUE); @@ -5153,6 +5185,16 @@ nm_utils_spawn_helper(const char *const *args, NULL); g_source_attach(info->input_source, g_main_context_get_thread_default()); + /* Watch process stderr */ + info->err_buffer = NM_STR_BUF_INIT(0, FALSE); + info->error_source = nm_g_unix_fd_source_new(info->child_stderr, + G_IO_IN | G_IO_ERR | G_IO_HUP, + G_PRIORITY_DEFAULT, + helper_have_err_data, + info, + NULL); + g_source_attach(info->error_source, g_main_context_get_thread_default()); + if (cancellable) { gulong signal_id; From ac5325e96bb206d7e96d2349126cfdc952345222 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 13 Feb 2023 17:42:52 +0100 Subject: [PATCH 4/5] nm-daemon-helper: log to stderr any error from getaddrinfo() Print errors from getaddrinfo() to stderr so that they will be logged by NM. --- src/nm-daemon-helper/nm-daemon-helper.c | 29 ++++++++++++++++++------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/nm-daemon-helper/nm-daemon-helper.c b/src/nm-daemon-helper/nm-daemon-helper.c index a101bf9b4f..a447d63cfe 100644 --- a/src/nm-daemon-helper/nm-daemon-helper.c +++ b/src/nm-daemon-helper/nm-daemon-helper.c @@ -11,6 +11,7 @@ #if defined(__GLIBC__) #include #endif +#include enum { RETURN_SUCCESS = 0, @@ -61,6 +62,7 @@ cmd_resolve_address(void) } sockaddr; socklen_t sockaddr_size; char name[NI_MAXHOST]; + int ret; address = read_arg(); if (!address) @@ -83,15 +85,26 @@ cmd_resolve_address(void) } else return RETURN_INVALID_ARGS; - if (getnameinfo((struct sockaddr *) &sockaddr, - sockaddr_size, - name, - sizeof(name), - NULL, - 0, - NI_NAMEREQD) - != 0) + ret = getnameinfo((struct sockaddr *) &sockaddr, + sockaddr_size, + name, + sizeof(name), + NULL, + 0, + NI_NAMEREQD); + if (ret != 0) { + if (ret == EAI_SYSTEM) { + fprintf(stderr, + "getnameinfo() failed: %d (%s), system error: %d (%s)\n", + ret, + gai_strerror(ret), + errno, + strerror(errno)); + } else { + fprintf(stderr, "getnameinfo() failed: %d (%s)\n", ret, gai_strerror(ret)); + } return RETURN_ERROR; + } printf("%s", name); From 4138be6a5a508af66b3b79c0eaeb43e3437ee345 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 Feb 2023 11:39:07 +0100 Subject: [PATCH 5/5] device: skip DNS resolution for tentative IPv6 addresses A tentative IPv6 address can still fail DAD, so don't use it to resolve the hostname via DNS. Furthermore, tentative addresses can't be used to contact the nameserver and so the resolution will fail if there is no other valid IPv6 address. Wait that the address becomes non-tentative. --- src/core/devices/nm-device.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 0bbd5f02b3..9d662bbf80 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -17245,6 +17245,9 @@ get_address_for_hostname_dns_lookup(NMDevice *self, int addr_family) return g_inet_address_new_from_bytes(addr->ax.address_ptr, G_SOCKET_FAMILY_IPV4); } + if (addr->ax.n_ifa_flags & IFA_F_TENTATIVE) + continue; + /* For IPv6 prefer, in order: * - !link-local, !deprecated * - !link-local, deprecated