From 10aff12526a2fc4b2d099df2710fdb040ccd9e4c Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 14 Jan 2015 16:30:39 -0600 Subject: [PATCH 1/4] dns: kill plugin child synchronously to avoid restart race (rh #1161232) (bgo #728342) NM was killing the dnsmasq local caching nameserver process and immediately starting a new one, and new process couldn't bind to 127.0.0.1 because the old one hadn't quit yet. Thus the new process quit, and the user was left with no split DNS at all. While this does introduce more synchronous waiting into the connection process, it's not that much time and NM will kill dnsmasq if it hasn't quit after 1 second. The longer-term fix is to use dnsmasq's D-Bus interface to update DNS without respawning it. https://bugzilla.gnome.org/show_bug.cgi?id=728342 https://bugzilla.redhat.com/show_bug.cgi?id=1161232 --- src/dns-manager/nm-dns-plugin.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dns-manager/nm-dns-plugin.c b/src/dns-manager/nm-dns-plugin.c index b083b1058e..c684464e59 100644 --- a/src/dns-manager/nm-dns-plugin.c +++ b/src/dns-manager/nm-dns-plugin.c @@ -198,7 +198,8 @@ nm_dns_plugin_child_spawn (NMDnsPlugin *self, return priv->pid; } -gboolean nm_dns_plugin_child_kill (NMDnsPlugin *self) +gboolean +nm_dns_plugin_child_kill (NMDnsPlugin *self) { NMDnsPluginPrivate *priv = NM_DNS_PLUGIN_GET_PRIVATE (self); @@ -208,7 +209,7 @@ gboolean nm_dns_plugin_child_kill (NMDnsPlugin *self) } if (priv->pid) { - nm_utils_kill_child_async (priv->pid, SIGTERM, LOGD_DNS, priv->progname, 2000, NULL, NULL); + nm_utils_kill_child_sync (priv->pid, SIGTERM, LOGD_DNS, priv->progname, NULL, 1000, 0); priv->pid = 0; g_free (priv->progname); priv->progname = NULL; From 74b712ca5da81e221badc93905382a3e214ce298 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 14 Jan 2015 17:03:22 -0600 Subject: [PATCH 2/4] dns: refactor building IP config lists for plugins (bgo #728342) Don't bother building the lists if no DNS plugins are enabled. https://bugzilla.gnome.org/show_bug.cgi?id=728342 --- src/dns-manager/nm-dns-manager.c | 70 ++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c index 0d93d8bb41..4094d33665 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -559,6 +559,42 @@ compute_hash (NMDnsManager *self, guint8 buffer[HASH_LEN]) g_checksum_free (sum); } +static void +build_plugin_config_lists (NMDnsManager *self, + GSList **out_vpn_configs, + GSList **out_dev_configs, + GSList **out_other_configs) +{ + NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE (self); + GSList *iter; + + g_return_if_fail (out_vpn_configs && !*out_vpn_configs); + g_return_if_fail (out_dev_configs && !*out_dev_configs); + g_return_if_fail (out_other_configs && !*out_other_configs); + + /* Build up config lists for plugins; we use the raw configs here, not the + * merged information that we write to resolv.conf so that the plugins can + * still use the domain information in each config to provide split DNS if + * they want to. + */ + if (priv->ip4_vpn_config) + *out_vpn_configs = g_slist_append (*out_vpn_configs, priv->ip4_vpn_config); + if (priv->ip6_vpn_config) + *out_vpn_configs = g_slist_append (*out_vpn_configs, priv->ip6_vpn_config); + if (priv->ip4_device_config) + *out_dev_configs = g_slist_append (*out_dev_configs, priv->ip4_device_config); + if (priv->ip6_device_config) + *out_dev_configs = g_slist_append (*out_dev_configs, priv->ip6_device_config); + + for (iter = priv->configs; iter; iter = g_slist_next (iter)) { + if ( (iter->data != priv->ip4_vpn_config) + && (iter->data != priv->ip4_device_config) + && (iter->data != priv->ip6_vpn_config) + && (iter->data != priv->ip6_device_config)) + *out_other_configs = g_slist_append (*out_other_configs, iter->data); + } +} + static gboolean update_dns (NMDnsManager *self, gboolean no_caching, @@ -566,7 +602,7 @@ update_dns (NMDnsManager *self, { NMDnsManagerPrivate *priv; NMResolvConfData rc; - GSList *iter, *vpn_configs = NULL, *dev_configs = NULL, *other_configs = NULL; + GSList *iter; const char *nis_domain = NULL; char **searches = NULL; char **nameservers = NULL; @@ -672,32 +708,11 @@ update_dns (NMDnsManager *self, nis_domain = rc.nis_domain; - /* Build up config lists for plugins; we use the raw configs here, not the - * merged information that we write to resolv.conf so that the plugins can - * still use the domain information in each config to provide split DNS if - * they want to. - */ - if (priv->ip4_vpn_config) - vpn_configs = g_slist_append (vpn_configs, priv->ip4_vpn_config); - if (priv->ip6_vpn_config) - vpn_configs = g_slist_append (vpn_configs, priv->ip6_vpn_config); - if (priv->ip4_device_config) - dev_configs = g_slist_append (dev_configs, priv->ip4_device_config); - if (priv->ip6_device_config) - dev_configs = g_slist_append (dev_configs, priv->ip6_device_config); - - for (iter = priv->configs; iter; iter = g_slist_next (iter)) { - if ( (iter->data != priv->ip4_vpn_config) - && (iter->data != priv->ip4_device_config) - && (iter->data != priv->ip6_vpn_config) - && (iter->data != priv->ip6_device_config)) - other_configs = g_slist_append (other_configs, iter->data); - } - /* Let any plugins do their thing first */ if (priv->plugin) { NMDnsPlugin *plugin = priv->plugin; const char *plugin_name = nm_dns_plugin_get_name (plugin); + GSList *vpn_configs = NULL, *dev_configs = NULL, *other_configs = NULL; if (nm_dns_plugin_is_caching (plugin)) { if (no_caching) { @@ -708,6 +723,8 @@ update_dns (NMDnsManager *self, caching = TRUE; } + build_plugin_config_lists (self, &vpn_configs, &dev_configs, &other_configs); + nm_log_dbg (LOGD_DNS, "DNS: updating plugin %s", plugin_name); if (!nm_dns_plugin_update (plugin, vpn_configs, @@ -721,15 +738,14 @@ update_dns (NMDnsManager *self, */ caching = FALSE; } + g_slist_free (vpn_configs); + g_slist_free (dev_configs); + g_slist_free (other_configs); skip: ; } - g_slist_free (vpn_configs); - g_slist_free (dev_configs); - g_slist_free (other_configs); - /* If caching was successful, we only send 127.0.0.1 to /etc/resolv.conf * to ensure that the glibc resolver doesn't try to round-robin nameservers, * but only uses the local caching nameserver. From ba593c9d9d2492e221e759fa5d54f2a04adf81d2 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 26 Feb 2015 15:04:36 -0600 Subject: [PATCH 3/4] dns: ensure that update_dns() always returns a GError on failure Callers may expect this, so make sure we do it. --- src/NetworkManagerUtils.c | 19 +++--- src/NetworkManagerUtils.h | 2 +- src/dns-manager/nm-dns-manager.c | 100 ++++++++++++++++--------------- src/dns-manager/nm-dns-unbound.c | 2 +- 4 files changed, 62 insertions(+), 61 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 505a77d166..bbfe776fb5 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -152,27 +152,26 @@ nm_utils_ip6_address_clear_host_address (struct in6_addr *dst, const struct in6_ int -nm_spawn_process (const char *args) +nm_spawn_process (const char *args, GError **error) { + GError *local = NULL; gint num_args; char **argv = NULL; int status = -1; - GError *error = NULL; g_return_val_if_fail (args != NULL, -1); + g_return_val_if_fail (!error || !*error, -1); - if (!g_shell_parse_argv (args, &num_args, &argv, &error)) { - nm_log_warn (LOGD_CORE, "could not parse arguments for '%s': %s", args, error->message); - g_error_free (error); - return -1; + if (g_shell_parse_argv (args, &num_args, &argv, &local)) { + g_spawn_sync ("/", argv, NULL, 0, nm_unblock_posix_signals, NULL, NULL, NULL, &status, &local); + g_strfreev (argv); } - if (!g_spawn_sync ("/", argv, NULL, 0, nm_unblock_posix_signals, NULL, NULL, NULL, &status, &error)) { - nm_log_warn (LOGD_CORE, "could not spawn process '%s': %s", args, error->message); - g_error_free (error); + if (local) { + nm_log_warn (LOGD_CORE, "could not spawn process '%s': %s", args, local->message); + g_propagate_error (error, local); } - g_strfreev (argv); return status; } diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index b309b5067a..ae1edd820b 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -49,7 +49,7 @@ nm_utils_ip6_route_metric_normalize (guint32 metric) return metric ? metric : 1024 /*NM_PLATFORM_ROUTE_METRIC_DEFAULT_IP6*/; } -int nm_spawn_process (const char *args); +int nm_spawn_process (const char *args, GError **error); /* check if @flags has exactly one flag (@check) set. You should call this * only with @check being a compile time constant and a power of two. */ diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c index 4094d33665..ed7b5def32 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -321,12 +321,19 @@ dispatch_netconfig (char **searches, again: - ret = waitpid (pid, NULL, 0); - if (ret < 0 && errno == EINTR) - goto again; - else if (ret < 0 && errno == ECHILD) { - /* When the netconfig exist, the errno is ECHILD, it should return TRUE */ - return TRUE; + if (waitpid (pid, NULL, 0) < 0) { + if (errno == EINTR) + goto again; + else if (errno == ECHILD) { + /* child already exited */ + ret = pid; + } else { + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_FAILED, + "Error waiting for netconfig to exit: %s", + strerror (errno)); + } } return ret > 0; @@ -342,22 +349,12 @@ write_resolv_conf (FILE *f, { char *searches_str = NULL; char *nameservers_str = NULL; - int i; gboolean retval = FALSE; + char *tmp_str; GString *str; - - if (fprintf (f, "%s","# Generated by NetworkManager\n") < 0) { - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_FAILED, - "Could not write " _PATH_RESCONF ": %s\n", - g_strerror (errno)); - return FALSE; - } + int i; if (searches) { - char *tmp_str; - tmp_str = g_strjoinv (" ", searches); searches_str = g_strconcat ("search ", tmp_str, "\n", NULL); g_free (tmp_str); @@ -385,10 +382,17 @@ write_resolv_conf (FILE *f, nameservers_str = g_string_free (str, FALSE); - if (fprintf (f, "%s%s", + if (fprintf (f, "# Generated by NetworkManager\n%s%s", searches_str ? searches_str : "", - strlen (nameservers_str) ? nameservers_str : "") != -1) + nameservers_str) > 0) retval = TRUE; + else { + g_set_error (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_FAILED, + "Could not write " _PATH_RESCONF ": %s\n", + g_strerror (errno)); + } g_free (searches_str); g_free (nameservers_str); @@ -405,9 +409,15 @@ dispatch_resolvconf (char **searches, char *cmd; FILE *f; gboolean retval = FALSE; + int errnosv, err; - if (! g_file_test (RESOLVCONF_PATH, G_FILE_TEST_IS_EXECUTABLE)) + if (!g_file_test (RESOLVCONF_PATH, G_FILE_TEST_IS_EXECUTABLE)) { + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_FAILED, + RESOLVCONF_PATH " is not executable"); return FALSE; + } if (searches || nameservers) { cmd = g_strconcat (RESOLVCONF_PATH, " -a ", "NetworkManager", NULL); @@ -421,12 +431,21 @@ dispatch_resolvconf (char **searches, g_strerror (errno)); else { retval = write_resolv_conf (f, searches, nameservers, error); - retval &= (pclose (f) == 0); + err = pclose (f); + if (err < 0) { + errnosv = errno; + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errnosv), + "Failed to close pipe to resolvconf: %d", errnosv); + retval = FALSE; + } else if (err > 0) { + nm_log_warn (LOGD_DNS, "resolvconf failed with status %d", err); + retval = FALSE; + } } } else { cmd = g_strconcat (RESOLVCONF_PATH, " -d ", "NetworkManager", NULL); nm_log_info (LOGD_DNS, "Removing DNS information from %s", RESOLVCONF_PATH); - if (nm_spawn_process (cmd) == 0) + if (nm_spawn_process (cmd, error) == 0) retval = TRUE; } @@ -610,8 +629,7 @@ update_dns (NMDnsManager *self, int num, i, len; gboolean success = FALSE, caching = FALSE; - g_return_val_if_fail (error != NULL, FALSE); - g_return_val_if_fail (*error == NULL, FALSE); + g_return_val_if_fail (!error || !*error, FALSE); priv = NM_DNS_MANAGER_GET_PRIVATE (self); @@ -797,9 +815,7 @@ plugin_failed (NMDnsPlugin *plugin, gpointer user_data) /* Disable caching until the next DNS update */ if (!update_dns (self, TRUE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } } @@ -836,9 +852,7 @@ nm_dns_manager_add_ip4_config (NMDnsManager *mgr, priv->configs = g_slist_append (priv->configs, g_object_ref (config)); if (!priv->updates_queue && !update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } @@ -870,9 +884,7 @@ nm_dns_manager_remove_ip4_config (NMDnsManager *mgr, NMIP4Config *config) g_object_unref (config); if (!priv->updates_queue && !update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } @@ -913,9 +925,7 @@ nm_dns_manager_add_ip6_config (NMDnsManager *mgr, priv->configs = g_slist_append (priv->configs, g_object_ref (config)); if (!priv->updates_queue && !update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } @@ -947,9 +957,7 @@ nm_dns_manager_remove_ip6_config (NMDnsManager *mgr, NMIP6Config *config) g_object_unref (config); if (!priv->updates_queue && !update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } @@ -991,9 +999,7 @@ nm_dns_manager_set_hostname (NMDnsManager *mgr, priv->hostname = g_strdup (filtered); if (!priv->updates_queue && !update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } } @@ -1047,9 +1053,7 @@ nm_dns_manager_end_updates (NMDnsManager *mgr, const char *func) /* Commit all the outstanding changes */ nm_log_dbg (LOGD_DNS, "(%s): committing DNS changes (%d)", func, priv->updates_queue); if (!update_dns (mgr, FALSE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } @@ -1139,9 +1143,7 @@ dispose (GObject *object) * DNS updates yet, there's no reason to touch resolv.conf on shutdown. */ if (priv->dns_touched && !update_dns (self, TRUE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes on shutdown: (%d) %s", - error ? error->code : -1, - error && error->message ? error->message : "(unknown)"); + nm_log_warn (LOGD_DNS, "could not commit DNS changes on shutdown: %s", error->message); g_clear_error (&error); priv->dns_touched = FALSE; } diff --git a/src/dns-manager/nm-dns-unbound.c b/src/dns-manager/nm-dns-unbound.c index 533290c11d..5723bf8229 100644 --- a/src/dns-manager/nm-dns-unbound.c +++ b/src/dns-manager/nm-dns-unbound.c @@ -42,7 +42,7 @@ update (NMDnsPlugin *plugin, * without calling custom scripts. The dnssec-trigger functionality * may be eventually merged into NetworkManager. */ - return nm_spawn_process ("/usr/libexec/dnssec-trigger-script --async --update") == 0; + return nm_spawn_process ("/usr/libexec/dnssec-trigger-script --async --update", NULL) == 0; } static gboolean From fc7913b308058250e40c60578afc70afd0bf13e9 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 15 Jan 2015 11:38:33 -0600 Subject: [PATCH 4/4] dns: refresh DNS if plugin child quits unexpectedly (bgo #728342) If the child dies, or something kills the child externally, refresh DNS which should respawn the child, similar to what we do with wpa_supplicant, teamd, etc. https://bugzilla.gnome.org/show_bug.cgi?id=728342 --- src/dns-manager/nm-dns-manager.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c index ed7b5def32..3240063f06 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -820,6 +820,22 @@ plugin_failed (NMDnsPlugin *plugin, gpointer user_data) } } +static void +plugin_child_quit (NMDnsPlugin *plugin, int exit_status, gpointer user_data) +{ + NMDnsManager *self = NM_DNS_MANAGER (user_data); + GError *error = NULL; + + nm_log_warn (LOGD_DNS, "DNS: plugin %s child quit unexpectedly; refreshing DNS", + nm_dns_plugin_get_name (plugin)); + + /* Let the plugin try to spawn the child again */ + if (!update_dns (self, FALSE, &error)) { + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); + g_clear_error (&error); + } +} + gboolean nm_dns_manager_add_ip4_config (NMDnsManager *mgr, const char *iface, @@ -1125,6 +1141,7 @@ nm_dns_manager_init (NMDnsManager *self) if (priv->plugin) { nm_log_info (LOGD_DNS, "DNS: loaded plugin %s", nm_dns_plugin_get_name (priv->plugin)); g_signal_connect (priv->plugin, NM_DNS_PLUGIN_FAILED, G_CALLBACK (plugin_failed), self); + g_signal_connect (priv->plugin, NM_DNS_PLUGIN_CHILD_QUIT, G_CALLBACK (plugin_child_quit), self); } } @@ -1135,7 +1152,11 @@ dispose (GObject *object) NMDnsManagerPrivate *priv = NM_DNS_MANAGER_GET_PRIVATE (self); GError *error = NULL; - g_clear_object (&priv->plugin); + if (priv->plugin) { + g_signal_handlers_disconnect_by_func (priv->plugin, plugin_failed, self); + g_signal_handlers_disconnect_by_func (priv->plugin, plugin_child_quit, self); + g_clear_object (&priv->plugin); + } /* If we're quitting, leave a valid resolv.conf in place, not one * pointing to 127.0.0.1 if any plugins were active. Thus update