From 37274a16a16614fbce9a01448aa470844b4b44c3 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 28 Sep 2018 14:54:59 +0200 Subject: [PATCH 1/5] dhcp6: fix handling of failure events The effect of a DHCPv6 failure should depend only on current IP state. This in the analogous of commit bd63d39252ea ("dhcp: fix handling of failure events") for IPv6. --- src/devices/nm-device.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 8bc98d6155..0293e65158 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -8058,12 +8058,13 @@ dhcp6_grace_period_expired (gpointer user_data) } static void -dhcp6_fail (NMDevice *self, gboolean timeout) +dhcp6_fail (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); gboolean is_dhcp_managed; - _LOGD (LOGD_DHCP6, "DHCPv6 failed%s", timeout ? " (timeout)" : ""); + _LOGD (LOGD_DHCP6, "DHCPv6 failed (ip_state %s)", + _ip_state_to_string (priv->ip6_state)); is_dhcp_managed = (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_MANAGED); @@ -8080,7 +8081,7 @@ dhcp6_fail (NMDevice *self, gboolean timeout) * configuration. */ if ( !priv->dhcp6.was_active - && (timeout || priv->ip6_state == IP_CONF)) { + && priv->ip6_state == IP_CONF) { dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); nm_device_activate_schedule_ip6_config_timeout (self); return; @@ -8122,7 +8123,7 @@ dhcp6_timeout (NMDevice *self, NMDhcpClient *client) NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); if (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_MANAGED) - dhcp6_fail (self, TRUE); + dhcp6_fail (self); else { /* not a hard failure; just live with the RA info */ dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); @@ -8188,7 +8189,7 @@ dhcp6_state_changed (NMDhcpClient *client, nm_device_activate_schedule_ip6_config_result (self); } else if (priv->ip6_state == IP_DONE) if (!dhcp6_lease_change (self)) - dhcp6_fail (self, FALSE); + dhcp6_fail (self); break; case NM_DHCP_STATE_TIMEOUT: dhcp6_timeout (self, client); @@ -8196,7 +8197,7 @@ dhcp6_state_changed (NMDhcpClient *client, case NM_DHCP_STATE_EXPIRE: /* Ignore expiry before we even have a lease (NAK, old lease, etc) */ if (priv->ip6_state != IP_CONF) - dhcp6_fail (self, FALSE); + dhcp6_fail (self); break; case NM_DHCP_STATE_DONE: /* In IPv6 info-only mode, the client doesn't handle leases so it @@ -8207,7 +8208,7 @@ dhcp6_state_changed (NMDhcpClient *client, break; /* fall through */ case NM_DHCP_STATE_FAIL: - dhcp6_fail (self, FALSE); + dhcp6_fail (self); break; default: break; From 54064144d4956eb127b2193a0be682760fc94a1e Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 28 Sep 2018 14:58:37 +0200 Subject: [PATCH 2/5] dhcp: log whether the client was active It is useful to understand why the grace period was started. --- src/devices/nm-device.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 0293e65158..8de4db55d8 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7310,8 +7310,9 @@ dhcp4_fail (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - _LOGD (LOGD_DHCP4, "DHCPv4 failed (ip_state %s)", - _ip_state_to_string (priv->ip4_state)); + _LOGD (LOGD_DHCP4, "DHCPv4 failed (ip_state %s, was_active %d)", + _ip_state_to_string (priv->ip4_state), + priv->dhcp4.was_active); /* Keep client running if there are static addresses configured * on the interface. @@ -8063,8 +8064,9 @@ dhcp6_fail (NMDevice *self) NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); gboolean is_dhcp_managed; - _LOGD (LOGD_DHCP6, "DHCPv6 failed (ip_state %s)", - _ip_state_to_string (priv->ip6_state)); + _LOGD (LOGD_DHCP6, "DHCPv6 failed (ip_state %s, was_active %d)", + _ip_state_to_string (priv->ip6_state), + priv->dhcp6.was_active); is_dhcp_managed = (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_MANAGED); From 81aa1a3bb3e51bf6af86a656ebcab083b6225f18 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 28 Sep 2018 15:22:03 +0200 Subject: [PATCH 3/5] dhcp: reset @was_active on cleanup The @was_active flag indicates that we started DHCP on an assumed connection. The idea is that if DHCP succeeded before, any failure must be treated like a renewal failure (and so it should start a grace period) rather than a failure in getting an initial lease (which fails the IP method). When we clean up the DHCP instance, the flag must be reset to FALSE, otherwise it will be potentially considered for other connections. --- src/devices/nm-device.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 8de4db55d8..a5b34cf3c5 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7028,6 +7028,7 @@ dhcp4_cleanup (NMDevice *self, CleanupType cleanup_type, gboolean release) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + priv->dhcp4.was_active = FALSE; nm_clear_g_source (&priv->dhcp4.grace_id); g_clear_pointer (&priv->dhcp4.pac_url, g_free); g_clear_pointer (&priv->dhcp4.root_path, g_free); @@ -7986,6 +7987,7 @@ dhcp6_cleanup (NMDevice *self, CleanupType cleanup_type, gboolean release) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + priv->dhcp6.was_active = FALSE; priv->dhcp6.mode = NM_NDISC_DHCP_LEVEL_NONE; applied_config_clear (&priv->dhcp6.ip6_config); g_clear_pointer (&priv->dhcp6.event_id, g_free); From 0a25b9081376312c403270cb1052fbb6f0655075 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 8 Oct 2018 10:10:05 +0200 Subject: [PATCH 4/5] dhcp: introduce terminated dhcp-state When the client terminates, we really don't care if it exited cleanly, with an error or killed by a signal. We expect the client to never exit and so all these situations are equally bad for us. Introduce a new TERMINATED state instead of reusing existing FAIL or DONE states, which are set when receiving particular events from the client. --- src/devices/nm-device.c | 4 +++- src/dhcp/nm-dhcp-client.c | 21 ++++++++------------- src/dhcp/nm-dhcp-client.h | 1 + 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a5b34cf3c5..e09f3c054e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7446,6 +7446,7 @@ dhcp4_state_changed (NMDhcpClient *client, /* fall through */ case NM_DHCP_STATE_DONE: case NM_DHCP_STATE_FAIL: + case NM_DHCP_STATE_TERMINATED: dhcp4_fail (self); break; default: @@ -8203,7 +8204,7 @@ dhcp6_state_changed (NMDhcpClient *client, if (priv->ip6_state != IP_CONF) dhcp6_fail (self); break; - case NM_DHCP_STATE_DONE: + case NM_DHCP_STATE_TERMINATED: /* In IPv6 info-only mode, the client doesn't handle leases so it * may exit right after getting a response from the server. That's * normal. In that case we just ignore the exit. @@ -8211,6 +8212,7 @@ dhcp6_state_changed (NMDhcpClient *client, if (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_OTHERCONF) break; /* fall through */ + case NM_DHCP_STATE_DONE: case NM_DHCP_STATE_FAIL: dhcp6_fail (self); break; diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 16db830681..d60a9bdaba 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -288,12 +288,13 @@ nm_dhcp_client_get_use_fqdn (NMDhcpClient *self) /*****************************************************************************/ static const char *state_table[NM_DHCP_STATE_MAX + 1] = { - [NM_DHCP_STATE_UNKNOWN] = "unknown", - [NM_DHCP_STATE_BOUND] = "bound", - [NM_DHCP_STATE_TIMEOUT] = "timeout", - [NM_DHCP_STATE_EXPIRE] = "expire", - [NM_DHCP_STATE_DONE] = "done", - [NM_DHCP_STATE_FAIL] = "fail", + [NM_DHCP_STATE_UNKNOWN] = "unknown", + [NM_DHCP_STATE_BOUND] = "bound", + [NM_DHCP_STATE_TIMEOUT] = "timeout", + [NM_DHCP_STATE_EXPIRE] = "expire", + [NM_DHCP_STATE_DONE] = "done", + [NM_DHCP_STATE_FAIL] = "fail", + [NM_DHCP_STATE_TERMINATED] = "terminated", }; static const char * @@ -449,7 +450,6 @@ daemon_watch_cb (GPid pid, int status, gpointer user_data) { NMDhcpClient *self = NM_DHCP_CLIENT (user_data); NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE (self); - NMDhcpState new_state; g_return_if_fail (priv->watch_id); priv->watch_id = 0; @@ -465,14 +465,9 @@ daemon_watch_cb (GPid pid, int status, gpointer user_data) else _LOGW ("client died abnormally"); - if (!WIFEXITED (status)) - new_state = NM_DHCP_STATE_FAIL; - else - new_state = NM_DHCP_STATE_DONE; - priv->pid = -1; - nm_dhcp_client_set_state (self, new_state, NULL, NULL); + nm_dhcp_client_set_state (self, NM_DHCP_STATE_TERMINATED, NULL, NULL); } void diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 86d60e3874..af0a97fad5 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -56,6 +56,7 @@ typedef enum { NM_DHCP_STATE_DONE, /* client quit or stopped */ 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_MAX, NM_DHCP_STATE_MAX = __NM_DHCP_STATE_MAX - 1, } NMDhcpState; From 567e277e64a8d9d7e9c46e8e61a6e15948c49279 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 28 Sep 2018 15:58:02 +0200 Subject: [PATCH 5/5] dhcp: don't start grace period if the client is not running We shouldn't start a grace period when the client is not running. --- src/devices/nm-device.c | 64 ++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index e09f3c054e..7a9f03800a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7307,7 +7307,7 @@ dhcp4_grace_period_expired (gpointer user_data) } static void -dhcp4_fail (NMDevice *self) +dhcp4_fail (NMDevice *self, NMDhcpState dhcp_state) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); @@ -7323,11 +7323,14 @@ dhcp4_fail (NMDevice *self) && nm_ip4_config_get_num_addresses (priv->con_ip_config_4) > 0) goto clear_config; - /* Fail the method in case of timeout or failure during initial - * configuration. + /* Fail the method when one of the following is true: + * 1) the DHCP client terminated: it does not make sense to start a grace + * period without a client running; + * 2) we failed to get an initial lease AND the client was + * not active before. */ - if ( !priv->dhcp4.was_active - && priv->ip4_state == IP_CONF) { + if ( dhcp_state == NM_DHCP_STATE_TERMINATED + || (!priv->dhcp4.was_active && priv->ip4_state == IP_CONF)) { dhcp4_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); nm_device_activate_schedule_ip4_config_timeout (self); return; @@ -7390,7 +7393,7 @@ dhcp4_state_changed (NMDhcpClient *client, case NM_DHCP_STATE_BOUND: if (!ip4_config) { _LOGW (LOGD_DHCP4, "failed to get IPv4 config in response to DHCP event."); - dhcp4_fail (self); + dhcp4_fail (self, state); break; } @@ -7433,11 +7436,11 @@ dhcp4_state_changed (NMDhcpClient *client, if (dhcp4_lease_change (self, ip4_config)) nm_device_update_metered (self); else - dhcp4_fail (self); + dhcp4_fail (self, state); } break; case NM_DHCP_STATE_TIMEOUT: - dhcp4_fail (self); + dhcp4_fail (self, state); break; case NM_DHCP_STATE_EXPIRE: /* Ignore expiry before we even have a lease (NAK, old lease, etc) */ @@ -7447,7 +7450,7 @@ dhcp4_state_changed (NMDhcpClient *client, case NM_DHCP_STATE_DONE: case NM_DHCP_STATE_FAIL: case NM_DHCP_STATE_TERMINATED: - dhcp4_fail (self); + dhcp4_fail (self, state); break; default: break; @@ -8062,7 +8065,7 @@ dhcp6_grace_period_expired (gpointer user_data) } static void -dhcp6_fail (NMDevice *self) +dhcp6_fail (NMDevice *self, NMDhcpState dhcp_state) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); gboolean is_dhcp_managed; @@ -8082,11 +8085,14 @@ dhcp6_fail (NMDevice *self) && nm_ip6_config_get_num_addresses (priv->con_ip_config_6)) goto clear_config; - /* Fail the method in case of timeout or failure during initial - * configuration. + /* Fail the method when one of the following is true: + * 1) the DHCP client terminated: it does not make sense to start a grace + * period without a client running; + * 2) we failed to get an initial lease AND the client was + * not active before. */ - if ( !priv->dhcp6.was_active - && priv->ip6_state == IP_CONF) { + if ( dhcp_state == NM_DHCP_STATE_TERMINATED + || (!priv->dhcp6.was_active && priv->ip6_state == IP_CONF)) { dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); nm_device_activate_schedule_ip6_config_timeout (self); return; @@ -8122,21 +8128,6 @@ clear_config: } } -static void -dhcp6_timeout (NMDevice *self, NMDhcpClient *client) -{ - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - - if (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_MANAGED) - dhcp6_fail (self); - else { - /* not a hard failure; just live with the RA info */ - dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); - if (priv->ip6_state == IP_CONF) - nm_device_activate_schedule_ip6_config_result (self); - } -} - static void dhcp6_state_changed (NMDhcpClient *client, NMDhcpState state, @@ -8194,15 +8185,22 @@ dhcp6_state_changed (NMDhcpClient *client, nm_device_activate_schedule_ip6_config_result (self); } else if (priv->ip6_state == IP_DONE) if (!dhcp6_lease_change (self)) - dhcp6_fail (self); + dhcp6_fail (self, state); break; case NM_DHCP_STATE_TIMEOUT: - dhcp6_timeout (self, client); + if (priv->dhcp6.mode == NM_NDISC_DHCP_LEVEL_MANAGED) + dhcp6_fail (self, state); + else { + /* not a hard failure; just live with the RA info */ + dhcp6_cleanup (self, CLEANUP_TYPE_DECONFIGURE, FALSE); + if (priv->ip6_state == IP_CONF) + nm_device_activate_schedule_ip6_config_result (self); + } break; case NM_DHCP_STATE_EXPIRE: /* Ignore expiry before we even have a lease (NAK, old lease, etc) */ if (priv->ip6_state != IP_CONF) - dhcp6_fail (self); + dhcp6_fail (self, state); break; case NM_DHCP_STATE_TERMINATED: /* In IPv6 info-only mode, the client doesn't handle leases so it @@ -8214,7 +8212,7 @@ dhcp6_state_changed (NMDhcpClient *client, /* fall through */ case NM_DHCP_STATE_DONE: case NM_DHCP_STATE_FAIL: - dhcp6_fail (self); + dhcp6_fail (self, state); break; default: break;