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

4 files changed, 67 insertions, 41 deletions

diff --git a/NEWS b/NEWS
index 0363799e9e..fd62e4d755 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,9 @@ USE AT YOUR OWN RISK.  NOT RECOMMENDED FOR PRODUCTION USE!
   the profile.
 * Increase timeout of NetworkManager-wait-online.service to 60 seconds.
 * Add "ipv4.dhcp-client-id=ipv6-duid" option for RFC4361.
+* 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.28
diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c
index 8da3e8767c..3ee8510b58 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 9fa0ef939a..72ab477dfb 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 772c696d4b..4f45c3a7e5 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-or-later */
 /*
- * Copyright (C) 2008 Roy Marples
+ * Copyright (C) 2008,2020 Roy Marples <roy@marples.name>
  * Copyright (C) 2010 Dan Williams <dcbw@redhat.com>
  */
 
@@ -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-<ifname>.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);
 }