From 452c224656ca6129d4ae34eee86e6ecea02fa8c2 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 3d593cce8d..5b95d1c50a 100644 --- a/src/dns-manager/nm-dns-plugin.c +++ b/src/dns-manager/nm-dns-plugin.c @@ -183,7 +183,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); @@ -193,7 +194,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 cc8d9f778c2237b3e9e6815a2e0cc5635328edab 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 f9a4fd44ae..05f2457824 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -585,6 +585,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, @@ -592,7 +628,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; @@ -698,32 +734,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) { @@ -734,6 +749,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, @@ -747,15 +764,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 06f25a3ec7c07eac5785daeb99f648200abe3feb 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 | 103 ++++++++++++++++--------------- src/dns-manager/nm-dns-unbound.c | 2 +- 4 files changed, 63 insertions(+), 63 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index f28b783e58..66108afe2b 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, NULL, NULL, NULL, NULL, &status, &local); + g_strfreev (argv); } - if (!g_spawn_sync ("/", argv, NULL, 0, NULL, 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 11dc496b4f..b74fd1cbf3 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); int nm_utils_modprobe (GError **error, const char *arg1, ...) G_GNUC_NULL_TERMINATED; diff --git a/src/dns-manager/nm-dns-manager.c b/src/dns-manager/nm-dns-manager.c index 05f2457824..7ebedb8f43 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -310,12 +310,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; @@ -331,22 +338,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); @@ -374,10 +371,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); @@ -394,9 +398,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); @@ -410,12 +420,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; } @@ -636,8 +655,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); @@ -823,9 +841,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); } } @@ -862,9 +878,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); } @@ -896,9 +910,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); } @@ -939,9 +951,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); } @@ -973,9 +983,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); } @@ -1017,9 +1025,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); } } @@ -1073,9 +1079,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); } @@ -1144,8 +1148,7 @@ config_changed_cb (NMConfig *config, init_resolv_conf_mode (self); if (!update_dns (self, TRUE, &error)) { - nm_log_warn (LOGD_DNS, "could not commit DNS changes: (%d) %s", - error->code, error->message); + nm_log_warn (LOGD_DNS, "could not commit DNS changes: %s", error->message); g_clear_error (&error); } } @@ -1181,9 +1184,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 09a05f6c3e0b4502252d70cb121654e7312520c5 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 7ebedb8f43..2b8fb7e6eb 100644 --- a/src/dns-manager/nm-dns-manager.c +++ b/src/dns-manager/nm-dns-manager.c @@ -846,6 +846,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, @@ -1131,6 +1147,7 @@ init_resolv_conf_mode (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); } } @@ -1176,7 +1193,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