From 7c9eefa7677dce5a36b7a9ee303b2098a11f0efc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 30 Jun 2015 14:51:42 +0200 Subject: [PATCH 1/3] core: return parent pid from nm_utils_get_start_time_for_pid() --- src/NetworkManagerUtils.c | 13 ++++++++++--- src/NetworkManagerUtils.h | 2 +- src/dhcp-manager/nm-dhcp-client.c | 2 +- src/nm-auth-subject.c | 2 +- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 79c173825..5a129215f 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -358,6 +358,7 @@ nm_utils_modprobe (GError **error, gboolean suppress_error_logging, const char * * nm_utils_get_start_time_for_pid: * @pid: the process identifier * @out_state: return the state character, like R, S, Z. See `man 5 proc`. + * @out_ppid: parent process id * * Originally copied from polkit source (src/polkit/polkitunixprocess.c) * and adjusted. @@ -368,7 +369,7 @@ nm_utils_modprobe (GError **error, gboolean suppress_error_logging, const char * * The returned start time counts since boot, in the unit HZ (with HZ usually being (1/100) seconds) **/ guint64 -nm_utils_get_start_time_for_pid (pid_t pid, char *out_state) +nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid) { guint64 start_time; gs_free gchar *filename = NULL; @@ -379,6 +380,7 @@ nm_utils_get_start_time_for_pid (pid_t pid, char *out_state) gchar *p; gchar *endp; char state = '\0'; + gint64 ppid = 0; start_time = 0; contents = NULL; @@ -410,6 +412,9 @@ nm_utils_get_start_time_for_pid (pid_t pid, char *out_state) if (num_tokens < 20) goto out; + if (out_ppid) + ppid = _nm_utils_ascii_str_to_int64 (tokens[1], 10, 1, G_MAXINT, 0); + errno = 0; start_time = strtoull (tokens[19], &endp, 10); if (*endp != '\0' || errno != 0) @@ -418,6 +423,8 @@ nm_utils_get_start_time_for_pid (pid_t pid, char *out_state) out: if (out_state) *out_state = state; + if (out_ppid) + *out_ppid = ppid; return start_time; } @@ -895,7 +902,7 @@ nm_utils_kill_process_sync (pid_t pid, guint64 start_time, int sig, NMLogDomain g_return_if_fail (log_name != NULL); g_return_if_fail (wait_before_kill_msec > 0); - start_time0 = nm_utils_get_start_time_for_pid (pid, &p_state); + start_time0 = nm_utils_get_start_time_for_pid (pid, &p_state, NULL); if (start_time0 == 0) { nm_log_dbg (log_domain, LOG_NAME_PROCESS_FMT ": cannot kill process %ld because it seems already gone", LOG_NAME_ARGS, (long int) pid); @@ -948,7 +955,7 @@ nm_utils_kill_process_sync (pid_t pid, guint64 start_time, int sig, NMLogDomain max_wait_until = 0; while (TRUE) { - start_time = nm_utils_get_start_time_for_pid (pid, &p_state); + start_time = nm_utils_get_start_time_for_pid (pid, &p_state, NULL); if (start_time != start_time0) { nm_log_dbg (log_domain, LOG_NAME_PROCESS_FMT ": process is gone after sending signal %s%s", diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index fef5d64cf..4c4f55b96 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -69,7 +69,7 @@ str_if_set (const char *str, const char *fallback) return str ? str : fallback; } -guint64 nm_utils_get_start_time_for_pid (pid_t pid, char *out_state); +guint64 nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid); void nm_utils_kill_process_sync (pid_t pid, guint64 start_time, int sig, guint64 log_domain, const char *log_name, guint32 wait_before_kill_msec, diff --git a/src/dhcp-manager/nm-dhcp-client.c b/src/dhcp-manager/nm-dhcp-client.c index aa5df9908..f94cdb249 100644 --- a/src/dhcp-manager/nm-dhcp-client.c +++ b/src/dhcp-manager/nm-dhcp-client.c @@ -592,7 +592,7 @@ nm_dhcp_client_stop_existing (const char *pid_file, const char *binary_name) const char *exe; /* Ensure the process is a DHCP client */ - start_time = nm_utils_get_start_time_for_pid (tmp, NULL); + start_time = nm_utils_get_start_time_for_pid (tmp, NULL, NULL); proc_path = g_strdup_printf ("/proc/%ld/cmdline", tmp); if ( start_time && g_file_get_contents (proc_path, &proc_contents, NULL, NULL)) { diff --git a/src/nm-auth-subject.c b/src/nm-auth-subject.c index 3fc0e64a1..c67424e78 100644 --- a/src/nm-auth-subject.c +++ b/src/nm-auth-subject.c @@ -360,7 +360,7 @@ constructed (GObject *object) if (!priv->unix_process.dbus_sender || !*priv->unix_process.dbus_sender) break; - priv->unix_process.start_time = nm_utils_get_start_time_for_pid (priv->unix_process.pid, NULL); + priv->unix_process.start_time = nm_utils_get_start_time_for_pid (priv->unix_process.pid, NULL, NULL); if (!priv->unix_process.start_time) { /* could not detect the process start time. The subject is invalid, but don't From c61c71a168d1c994424b2933169b48cbe1519614 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 30 Jun 2015 14:53:10 +0200 Subject: [PATCH 2/3] dhcp: properly reap child process in nm_dhcp_client_stop_existing() We kill the process based on the PID from the pidfile. This can be our own child process so we must use nm_utils_kill_child_sync() instead of nm_utils_kill_process_sync(). --- src/dhcp-manager/nm-dhcp-client.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/dhcp-manager/nm-dhcp-client.c b/src/dhcp-manager/nm-dhcp-client.c index f94cdb249..67be01cf4 100644 --- a/src/dhcp-manager/nm-dhcp-client.c +++ b/src/dhcp-manager/nm-dhcp-client.c @@ -590,9 +590,10 @@ nm_dhcp_client_stop_existing (const char *pid_file, const char *binary_name) if ((errno == 0) && (tmp > 1)) { guint64 start_time; const char *exe; + pid_t ppid; /* Ensure the process is a DHCP client */ - start_time = nm_utils_get_start_time_for_pid (tmp, NULL, NULL); + start_time = nm_utils_get_start_time_for_pid (tmp, NULL, &ppid); proc_path = g_strdup_printf ("/proc/%ld/cmdline", tmp); if ( start_time && g_file_get_contents (proc_path, &proc_contents, NULL, NULL)) { @@ -602,9 +603,15 @@ nm_dhcp_client_stop_existing (const char *pid_file, const char *binary_name) else exe = proc_contents; - if (!strcmp (exe, binary_name)) - nm_utils_kill_process_sync (tmp, start_time, SIGTERM, LOGD_DHCP, - "dhcp-client", 1000 / 2, 1000 / 20, 2000); + if (!strcmp (exe, binary_name)) { + if (ppid == getpid ()) { + /* the process is our own child. */ + nm_utils_kill_child_sync (tmp, SIGTERM, LOGD_DHCP, "dhcp-client", NULL, 1000 / 2, 1000 / 20); + } else { + nm_utils_kill_process_sync (tmp, start_time, SIGTERM, LOGD_DHCP, + "dhcp-client", 1000 / 2, 1000 / 20, 2000); + } + } } } From 3b21738d9c266b10400769e815a778763b48979d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 30 Jun 2015 16:13:03 +0200 Subject: [PATCH 3/3] device: fix cleanup DHCP instance when unmanaging device on removed platform link When the platform link gets removed outside of NetworkManager, we would unmanage the device first. By checking the device state reason NM_DEVICE_STATE_REASON_REMOVED, we would then not deconfigure the interface, as it is already gone. This was not correct because we must at least stop the dhcp client. Otherwise the dhclient process keeps running. That meant, if the device reappeared later, we would start dhclient again. Then we would find the PID of the still running instance in the pidfile and kill it only than. Fix it by replacing the 'deconfigure' boolean by a tri-state 'cleanup_type'. --- src/devices/nm-device.c | 66 +++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index af6d340ca..afe2fcd0a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -140,6 +140,12 @@ enum { #define PENDING_ACTION_DHCP6 "dhcp6" #define PENDING_ACTION_AUTOCONF6 "autoconf6" +typedef enum { + CLEANUP_TYPE_DECONFIGURE, + CLEANUP_TYPE_KEEP, + CLEANUP_TYPE_REMOVED, +} CleanupType; + typedef enum { IP_NONE = 0, IP_WAIT, @@ -3198,7 +3204,7 @@ ensure_con_ip6_config (NMDevice *self) /* DHCPv4 stuff */ static void -dhcp4_cleanup (NMDevice *self, gboolean stop, gboolean release) +dhcp4_cleanup (NMDevice *self, CleanupType cleanup_type, gboolean release) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); @@ -3211,7 +3217,8 @@ dhcp4_cleanup (NMDevice *self, gboolean stop, gboolean release) nm_device_remove_pending_action (self, PENDING_ACTION_DHCP4, FALSE); - if (stop) + if ( cleanup_type == CLEANUP_TYPE_DECONFIGURE + || cleanup_type == CLEANUP_TYPE_REMOVED) nm_dhcp_client_stop (priv->dhcp4_client, release); g_clear_object (&priv->dhcp4_client); @@ -3410,7 +3417,7 @@ dhcp4_fail (NMDevice *self, gboolean timeout) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - dhcp4_cleanup (self, TRUE, FALSE); + dhcp4_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); if (timeout || (priv->ip4_state == IP_CONF)) nm_device_activate_schedule_ip4_config_timeout (self); else if (priv->ip4_state == IP_DONE) @@ -3557,7 +3564,7 @@ nm_device_dhcp4_renew (NMDevice *self, gboolean release) _LOGI (LOGD_DHCP4, "DHCPv4 lease renewal requested"); /* Terminate old DHCP instance and release the old lease */ - dhcp4_cleanup (self, TRUE, release); + dhcp4_cleanup (self, CLEANUP_TYPE_DECONFIGURE, release); connection = nm_device_get_connection (self); g_assert (connection); @@ -3812,7 +3819,7 @@ act_stage3_ip4_config_start (NMDevice *self, /* DHCPv6 stuff */ static void -dhcp6_cleanup (NMDevice *self, gboolean stop, gboolean release) +dhcp6_cleanup (NMDevice *self, CleanupType cleanup_type, gboolean release) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); @@ -3825,7 +3832,8 @@ dhcp6_cleanup (NMDevice *self, gboolean stop, gboolean release) priv->dhcp6_state_sigid = 0; } - if (stop) + if ( cleanup_type == CLEANUP_TYPE_DECONFIGURE + || cleanup_type == CLEANUP_TYPE_REMOVED) nm_dhcp_client_stop (priv->dhcp6_client, release); g_clear_object (&priv->dhcp6_client); @@ -4032,7 +4040,7 @@ dhcp6_fail (NMDevice *self, gboolean timeout) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - dhcp6_cleanup (self, TRUE, FALSE); + dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); if (priv->dhcp6_mode == NM_RDISC_DHCP_LEVEL_MANAGED) { if (timeout || (priv->ip6_state == IP_CONF)) @@ -4057,7 +4065,7 @@ dhcp6_timeout (NMDevice *self, NMDhcpClient *client) dhcp6_fail (self, TRUE); else { /* not a hard failure; just live with the RA info */ - dhcp6_cleanup (self, TRUE, FALSE); + dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); if (priv->ip6_state == IP_CONF) nm_device_activate_schedule_ip6_config_result (self); } @@ -4232,7 +4240,7 @@ nm_device_dhcp6_renew (NMDevice *self, gboolean release) _LOGI (LOGD_DHCP6, "DHCPv6 lease renewal requested"); /* Terminate old DHCP instance and release the old lease */ - dhcp6_cleanup (self, TRUE, release); + dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, release); /* Start DHCP again on the interface */ return dhcp6_start (self, FALSE, NULL); @@ -4613,7 +4621,7 @@ rdisc_config_changed (NMRDisc *rdisc, NMRDiscConfigMap changed, NMDevice *self) } if (changed & NM_RDISC_CONFIG_DHCP_LEVEL) { - dhcp6_cleanup (self, TRUE, TRUE); + dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, TRUE); priv->dhcp6_mode = rdisc->dhcp_level; if (priv->dhcp6_mode != NM_RDISC_DHCP_LEVEL_NONE) { @@ -7802,16 +7810,16 @@ nm_device_has_pending_action (NMDevice *self) /***********************************************************/ static void -_cleanup_ip_pre (NMDevice *self, gboolean deconfigure) +_cleanup_ip_pre (NMDevice *self, CleanupType cleanup_type) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); priv->ip4_state = priv->ip6_state = IP_NONE; nm_device_queued_ip_config_change_clear (self); - dhcp4_cleanup (self, deconfigure, FALSE); + dhcp4_cleanup (self, cleanup_type, FALSE); arp_cleanup (self); - dhcp6_cleanup (self, deconfigure, FALSE); + dhcp6_cleanup (self, cleanup_type, FALSE); linklocal6_cleanup (self); addrconf6_cleanup (self); dnsmasq_cleanup (self); @@ -7837,14 +7845,14 @@ _cancel_activation (NMDevice *self) } static void -_cleanup_generic_pre (NMDevice *self, gboolean deconfigure) +_cleanup_generic_pre (NMDevice *self, CleanupType cleanup_type) { NMConnection *connection; _cancel_activation (self); connection = nm_device_get_connection (self); - if ( deconfigure + if ( cleanup_type == CLEANUP_TYPE_DECONFIGURE && connection && !nm_device_uses_assumed_connection (self)) { nm_firewall_manager_remove_from_zone (nm_firewall_manager_get (), @@ -7855,11 +7863,11 @@ _cleanup_generic_pre (NMDevice *self, gboolean deconfigure) /* Clear any queued transitions */ nm_device_queued_state_clear (self); - _cleanup_ip_pre (self, deconfigure); + _cleanup_ip_pre (self, cleanup_type); } static void -_cleanup_generic_post (NMDevice *self, gboolean deconfigure) +_cleanup_generic_post (NMDevice *self, CleanupType cleanup_type) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); NMDeviceStateReason ignored = NM_DEVICE_STATE_REASON_NONE; @@ -7900,7 +7908,7 @@ _cleanup_generic_post (NMDevice *self, gboolean deconfigure) g_object_notify (G_OBJECT (self), NM_DEVICE_IP4_ADDRESS); } - if (deconfigure) { + if (cleanup_type == CLEANUP_TYPE_DECONFIGURE) { /* Check if the device was deactivated, and if so, delete_link. * Don't call delete_link synchronously because we are currently * handling a state change -- which is not reentrant. */ @@ -7921,7 +7929,7 @@ _cleanup_generic_post (NMDevice *self, gboolean deconfigure) * */ static void -nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, gboolean deconfigure) +nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, CleanupType cleanup_type) { NMDevicePrivate *priv; int ifindex; @@ -7936,10 +7944,10 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, gboolean deconfig /* Save whether or not we tried IPv6 for later */ priv = NM_DEVICE_GET_PRIVATE (self); - _cleanup_generic_pre (self, deconfigure); + _cleanup_generic_pre (self, cleanup_type); /* Turn off kernel IPv6 */ - if (deconfigure) { + if (cleanup_type == CLEANUP_TYPE_DECONFIGURE) { set_disable_ipv6 (self, "1"); nm_device_ipv6_sysctl_set (self, "accept_ra", "0"); nm_device_ipv6_sysctl_set (self, "use_tempaddr", "0"); @@ -7965,7 +7973,7 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, gboolean deconfig } nm_device_update_metered (self); - _cleanup_generic_post (self, deconfigure); + _cleanup_generic_post (self, cleanup_type); } static char * @@ -8294,11 +8302,11 @@ _set_state_full (NMDevice *self, nm_device_set_firmware_missing (self, FALSE); if (old_state > NM_DEVICE_STATE_UNMANAGED) { if (reason == NM_DEVICE_STATE_REASON_REMOVED) { - nm_device_cleanup (self, reason, FALSE); + nm_device_cleanup (self, reason, CLEANUP_TYPE_REMOVED); } else { /* Clean up if the device is now unmanaged but was activated */ if (nm_device_get_act_request (self)) - nm_device_cleanup (self, reason, TRUE); + nm_device_cleanup (self, reason, CLEANUP_TYPE_DECONFIGURE); nm_device_take_down (self, TRUE); set_nm_ipv6ll (self, FALSE); restore_ip6_properties (self); @@ -8327,7 +8335,7 @@ _set_state_full (NMDevice *self, * Note that we "deactivate" the device even when coming from * UNMANAGED, to ensure that it's in a clean state. */ - nm_device_cleanup (self, reason, TRUE); + nm_device_cleanup (self, reason, CLEANUP_TYPE_DECONFIGURE); } break; case NM_DEVICE_STATE_DISCONNECTED: @@ -8337,7 +8345,7 @@ _set_state_full (NMDevice *self, */ set_nm_ipv6ll (self, TRUE); - nm_device_cleanup (self, reason, TRUE); + nm_device_cleanup (self, reason, CLEANUP_TYPE_DECONFIGURE); } else if (old_state < NM_DEVICE_STATE_DISCONNECTED) { if (reason != NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED) { /* Ensure IPv6 is set up as it may not have been done when @@ -8352,7 +8360,7 @@ _set_state_full (NMDevice *self, /* Clean up any half-done IP operations if the device's layer2 * finds out it needs authentication during IP config. */ - _cleanup_ip_pre (self, TRUE); + _cleanup_ip_pre (self, CLEANUP_TYPE_DECONFIGURE); } break; default: @@ -8991,7 +8999,7 @@ dispose (GObject *object) dispatcher_cleanup (self); - _cleanup_generic_pre (self, FALSE); + _cleanup_generic_pre (self, CLEANUP_TYPE_KEEP); g_warn_if_fail (priv->slaves == NULL); g_assert (priv->master_ready_id == 0); @@ -8999,7 +9007,7 @@ dispose (GObject *object) /* Let the kernel manage IPv6LL again */ set_nm_ipv6ll (self, FALSE); - _cleanup_generic_post (self, FALSE); + _cleanup_generic_post (self, CLEANUP_TYPE_KEEP); g_hash_table_remove_all (priv->ip6_saved_properties);