From a58a89213bf4d0cefb155fef1ec9425f7a6ca5c8 Mon Sep 17 00:00:00 2001 From: Roy Marples Date: Tue, 19 Jan 2021 05:04:31 +0000 Subject: [PATCH] DHCP: Support dhcpcd-9.x This locks NM into dhcpcd-9.3.3 as that is the first version to support the --noconfigure option. Older versions are no longer supported by NM because they do modify the host which is undesirable. Due to the way dhcpcd-9 uses privilege separation and that it re-parents itself to PID 1, the main process cannot be reaped or waited for. So we rely on dhcpcd correctly cleaning up after itself. A new function nm_dhcp_client_stop_watch_child() has been added so that dhcpcd can perform similar cleanup to the equivalent stop call. As part of this change, the STOP and STOPPED reasons are mapped to NM_DHCP_STATE_DONE and PREINIT is mapped to a new state NM_DHCP_STATE_NOOP which means NM should just ignore this state. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/668 --- NEWS | 3 ++ src/dhcp/nm-dhcp-client.c | 20 +++++++++- src/dhcp/nm-dhcp-client.h | 3 ++ src/dhcp/nm-dhcp-dhcpcd.c | 82 ++++++++++++++++++++------------------- 4 files changed, 67 insertions(+), 41 deletions(-) diff --git a/NEWS b/NEWS index 8a48587e5..958bbe91c 100644 --- a/NEWS +++ b/NEWS @@ -45,6 +45,9 @@ USE AT YOUR OWN RISK. NOT RECOMMENDED FOR PRODUCTION USE! cmdline argument actually generates a connection which disables both ipv4 and ipv6. Previously the generated connection would disable ipv4 but ipv6 would be set to the 'auto' method. +* The dhcpcd plugin now requires a minimum version of dhcpcd-9.3.3 with + the --noconfigure option. Using an older version will cause dhcpcd to + exit with a status code of 1. ============================================= NetworkManager-1.26 diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 46ab48959..56f599abf 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -367,10 +367,13 @@ reason_to_state(NMDhcpClient *self, const char *iface, const char *reason) else if (g_ascii_strcasecmp(reason, "nak") == 0 || g_ascii_strcasecmp(reason, "expire") == 0 || g_ascii_strcasecmp(reason, "expire6") == 0) return NM_DHCP_STATE_EXPIRE; - else if (g_ascii_strcasecmp(reason, "end") == 0) + else if (g_ascii_strcasecmp(reason, "end") == 0 || g_ascii_strcasecmp(reason, "stop") == 0 + || g_ascii_strcasecmp(reason, "stopped") == 0) return NM_DHCP_STATE_DONE; else if (g_ascii_strcasecmp(reason, "fail") == 0 || g_ascii_strcasecmp(reason, "abend") == 0) return NM_DHCP_STATE_FAIL; + else if (g_ascii_strcasecmp(reason, "preinit") == 0) + return NM_DHCP_STATE_NOOP; _LOGD("unmapped DHCP state '%s'", reason); return NM_DHCP_STATE_UNKNOWN; @@ -547,6 +550,18 @@ nm_dhcp_client_watch_child(NMDhcpClient *self, pid_t pid) priv->watch_id = g_child_watch_add(pid, daemon_watch_cb, self); } +void +nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + g_return_if_fail(priv->pid == pid); + priv->pid = -1; + + watch_cleanup(self); + timeout_cleanup(self); +} + gboolean nm_dhcp_client_start_ip4(NMDhcpClient *self, GBytes * client_id, @@ -874,6 +889,9 @@ nm_dhcp_client_handle_event(gpointer unused, state_to_string(new_state), reason); + if (new_state == NM_DHCP_STATE_NOOP) + return TRUE; + if (NM_IN_SET(new_state, NM_DHCP_STATE_BOUND, NM_DHCP_STATE_EXTENDED)) { GVariantIter iter; const char * name; diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 05faa9ea5..46446849a 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -55,6 +55,7 @@ typedef enum { NM_DHCP_STATE_EXPIRE, /* lease expired or NAKed */ NM_DHCP_STATE_FAIL, /* failed for some reason */ NM_DHCP_STATE_TERMINATED, /* client is no longer running */ + NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */ __NM_DHCP_STATE_MAX, NM_DHCP_STATE_MAX = __NM_DHCP_STATE_MAX - 1, } NMDhcpState; @@ -183,6 +184,8 @@ void nm_dhcp_client_start_timeout(NMDhcpClient *self); void nm_dhcp_client_watch_child(NMDhcpClient *self, pid_t pid); +void nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid); + void nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, NMIPConfig * ip_config, diff --git a/src/dhcp/nm-dhcp-dhcpcd.c b/src/dhcp/nm-dhcp-dhcpcd.c index b2b5d28bd..7cb003859 100644 --- a/src/dhcp/nm-dhcp-dhcpcd.c +++ b/src/dhcp/nm-dhcp-dhcpcd.c @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0+ */ /* - * Copyright (C) 2008 Roy Marples + * Copyright (C) 2008,2020 Roy Marples * Copyright (C) 2010 Dan Williams */ @@ -40,7 +40,6 @@ static GType nm_dhcp_dhcpcd_get_type(void); /*****************************************************************************/ typedef struct { - char * pid_file; NMDhcpListener *dhcp_listener; } NMDhcpDhcpcdPrivate; @@ -71,39 +70,37 @@ ip4_start(NMDhcpClient *client, const char * last_ip4_address, GError ** error) { - NMDhcpDhcpcd * self = NM_DHCP_DHCPCD(client); - NMDhcpDhcpcdPrivate *priv = NM_DHCP_DHCPCD_GET_PRIVATE(self); - gs_unref_ptrarray GPtrArray *argv = NULL; - pid_t pid = -1; - GError * local = NULL; - gs_free char * cmd_str = NULL; - gs_free char * binary_name = NULL; + NMDhcpDhcpcd * self = NM_DHCP_DHCPCD(client); + gs_unref_ptrarray GPtrArray *argv = NULL; + pid_t pid; + GError * local; + gs_free char * cmd_str = NULL; const char * iface; const char * dhcpcd_path; const char * hostname; - g_return_val_if_fail(priv->pid_file == NULL, FALSE); + pid = nm_dhcp_client_get_pid(client); + g_return_val_if_fail(pid == -1, FALSE); iface = nm_dhcp_client_get_iface(client); - /* dhcpcd does not allow custom pidfiles; the pidfile is always - * RUNSTATEDIR "dhcpcd-.pid". - */ - priv->pid_file = g_strdup_printf(RUNSTATEDIR "/dhcpcd-%s.pid", iface); - dhcpcd_path = nm_dhcp_dhcpcd_get_path(); if (!dhcpcd_path) { nm_utils_error_set_literal(error, NM_UTILS_ERROR_UNKNOWN, "dhcpcd binary not found"); return FALSE; } - /* Kill any existing dhcpcd from the pidfile */ - binary_name = g_path_get_basename(dhcpcd_path); - nm_dhcp_client_stop_existing(priv->pid_file, binary_name); - argv = g_ptr_array_new(); g_ptr_array_add(argv, (gpointer) dhcpcd_path); + /* Don't configure anything, we will do that instead. + * This requires dhcpcd-9.3.3 or newer. + * Older versions only had an option not to install a default route, + * dhcpcd still added addresses and other routes so we no longer support that + * as it doesn't fit how NetworkManager wants to work. + */ + g_ptr_array_add(argv, (gpointer) "--noconfigure"); + g_ptr_array_add(argv, (gpointer) "-B"); /* Don't background on lease (disable fork()) */ g_ptr_array_add(argv, (gpointer) "-K"); /* Disable built-in carrier detection */ @@ -113,8 +110,6 @@ ip4_start(NMDhcpClient *client, /* --noarp. Don't request or claim the address by ARP; this also disables IPv4LL. */ g_ptr_array_add(argv, (gpointer) "-A"); - g_ptr_array_add(argv, (gpointer) "-G"); /* Let NM handle routing */ - g_ptr_array_add(argv, (gpointer) "-c"); /* Set script file */ g_ptr_array_add(argv, (gpointer) nm_dhcp_helper_path); @@ -146,8 +141,8 @@ ip4_start(NMDhcpClient *client, if (!g_spawn_async(NULL, (char **) argv->pdata, NULL, - G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_STDOUT_TO_DEV_NULL - | G_SPAWN_STDERR_TO_DEV_NULL, + G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL + | G_SPAWN_DO_NOT_REAP_CHILD, nm_utils_setpgid, NULL, &pid, @@ -169,23 +164,32 @@ ip4_start(NMDhcpClient *client, static void stop(NMDhcpClient *client, gboolean release) { - NMDhcpDhcpcd * self = NM_DHCP_DHCPCD(client); - NMDhcpDhcpcdPrivate *priv = NM_DHCP_DHCPCD_GET_PRIVATE(self); - int errsv; - - NM_DHCP_CLIENT_CLASS(nm_dhcp_dhcpcd_parent_class)->stop(client, release); - - if (priv->pid_file) { - if (remove(priv->pid_file) == -1) { - errsv = errno; - _LOGD("could not remove dhcp pid file \"%s\": %d (%s)", - priv->pid_file, - errsv, - nm_strerror_native(errsv)); - } + NMDhcpDhcpcd *self = NM_DHCP_DHCPCD(client); + pid_t pid; + int sig, errsv; + + pid = nm_dhcp_client_get_pid(client); + sig = release ? SIGALRM : SIGTERM; + _LOGD("sending %s to dhcpcd pid %d", sig == SIGALRM ? "SIGALRM" : "SIGTERM", pid); + + /* dhcpcd-9.x features privilege separation. + * It's not our job to track all these processes so we rely on dhcpcd + * to always cleanup after itself. + * Because it also re-parents itself to PID 1, the process cannot be + * reaped or waited for. + * As such, just send the correct signal. + */ + if (kill(pid, sig) == -1) { + errsv = errno; + _LOGE("failed to kill dhcpcd %d:%s", errsv, strerror(errsv)); } - /* FIXME: implement release... */ + /* When this function exits NM expects the PID to be -1. + * This means we also need to stop watching the pid. + * If we need to know the exit status then we need to refactor NM + * to allow a non -1 to mean we're waiting to exit still. + */ + nm_dhcp_client_stop_watch_child(client, pid); } /*****************************************************************************/ @@ -214,8 +218,6 @@ dispose(GObject *object) g_clear_object(&priv->dhcp_listener); } - nm_clear_g_free(&priv->pid_file); - G_OBJECT_CLASS(nm_dhcp_dhcpcd_parent_class)->dispose(object); } -- 2.30.0