From f4c7d3aa8282fd2e4e2b7ff22e7ae930a6dbfdcb 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. (cherry picked from commit 961824d43b5bd649ff98bb8fbf05d276faff82b6) (cherry picked from commit 8e312f61689ce274f8f6d31a6c3464548632186c) --- 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 4aad9414f5..4a23e60784 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -5126,7 +5126,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'); @@ -5140,7 +5140,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 dfaee8d967513b6678d4ca48a6168de7ca2dc932 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 (cherry picked from commit 7037aa66c6fce01d3e33e1fae32c35fb1f25b363) (cherry picked from commit 7e3dccb781346863ec373f1ea4baf14aa9052878) --- 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 718e6f7d3f..7c421a3c8f 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -309,7 +309,7 @@ typedef struct { typedef enum { RESOLVER_WAIT_ADDRESS = 0, - RESOLVER_IN_PROGRESS, + RESOLVER_STARTED, RESOLVER_DONE, } ResolverState; @@ -17050,6 +17050,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) { @@ -17069,7 +17084,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 { @@ -17079,7 +17096,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)"); @@ -17105,8 +17124,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; @@ -17115,30 +17135,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); + } } static GInetAddress * @@ -17260,29 +17266,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); @@ -17300,7 +17312,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 de850e68f2..68387a2149 100644 --- a/src/core/devices/nm-device.h +++ b/src/core/devices/nm-device.h @@ -819,6 +819,6 @@ 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); #endif /* __NETWORKMANAGER_DEVICE_H__ */ diff --git a/src/core/nm-policy.c b/src/core/nm-policy.c index 0c07c29e98..0b7c9eddca 100644 --- a/src/core/nm-policy.c +++ b/src/core/nm-policy.c @@ -2564,7 +2564,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 51e3dd447d57e88ca32c734a76767fbffa7db751 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. (cherry picked from commit d65702803cb02ddd656f13917a8a9af7bc08b90a) (cherry picked from commit f1f1aee711515244322a37150de908e26cfa240f) --- 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 4a23e60784..6dc46664b5 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -4862,11 +4862,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; @@ -4902,13 +4905,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); @@ -4922,6 +4929,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); @@ -5017,6 +5028,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) { @@ -5092,10 +5121,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, @@ -5124,6 +5154,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); @@ -5149,6 +5181,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 0a00b2a95ae351c3666040d8309574b250fb8285 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. (cherry picked from commit ac5325e96bb206d7e96d2349126cfdc952345222) (cherry picked from commit 41cd94f46af5a965ed3c84f09748194b5b7f3517) --- 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 26d5ad46806a2dc69238116796be0fbb2b7c273f 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. (cherry picked from commit 4138be6a5a508af66b3b79c0eaeb43e3437ee345) (cherry picked from commit 0ebd75381963abc2fb75d39ab44367139db0e34b) --- 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 7c421a3c8f..31acc1c1fe 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -17177,6 +17177,9 @@ get_address_for_hostname_dns_lookup(NMDevice *self, int addr_family) return g_inet_address_new_from_bytes(addr->address_ptr, G_SOCKET_FAMILY_IPV4); } + if (addr->n_ifa_flags & IFA_F_TENTATIVE) + continue; + /* For IPv6 prefer, in order: * - !link-local, !deprecated * - !link-local, deprecated