core: better handle DHCP expiry/nak during initial lease acquisition (bgo #739482)

When dhclient trieds to request a previous lease and the server NAKs that
lease, dhclient emits the EXPIRE state.  dhcpcd has also been known to emit
the 'nak' state for the same reason.

(systemd's DHCP client code does not push a NAK up to NetworkManager, but
jumps to the REBOOT state instead, so it is unaffected by this issue.)

NetworkManager saw the expire during IP configuration and treated that as
full activation failure.  The connection would be restarted, the same lease
requested, and the same NAK delivered, over and over.  Before a lease is
acquired, there is (by definition) no lease to expire, so these events
should be ignored.

We do, however, still want to handle abnormal failures, which is why
this patch splits the EXPIRE case from the FAIL case and handles them
separately.

https://bugzilla.gnome.org/show_bug.cgi?id=739482

4 files changed, 25 insertions, 10 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 060a7aa6f5..cd548d260e 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -3006,9 +3006,13 @@ dhcp4_state_changed (NMDhcpClient *client,
 	case NM_DHCP_STATE_TIMEOUT:
 		dhcp4_fail (self, TRUE);
 		break;
+	case NM_DHCP_STATE_EXPIRE:
+		/* Ignore expiry before we even have a lease (NAK, old lease, etc) */
+		if (priv->ip4_state == IP_CONF)
+			break;
+		/* Fall through */
 	case NM_DHCP_STATE_DONE:
 	case NM_DHCP_STATE_FAIL:
-		/* dhclient quit and can't get/renew a lease; so kill the connection */
 		dhcp4_fail (self, FALSE);
 		break;
 	default:
@@ -3597,6 +3601,11 @@ dhcp6_state_changed (NMDhcpClient *client,
 	case NM_DHCP_STATE_TIMEOUT:
 		dhcp6_timeout (self, client);
 		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, FALSE);
+		break;
 	case NM_DHCP_STATE_DONE:
 		/* 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
@@ -3606,7 +3615,6 @@ dhcp6_state_changed (NMDhcpClient *client,
 			break;
 		/* Otherwise, fall through */
 	case NM_DHCP_STATE_FAIL:
-		/* dhclient quit and can't get/renew a lease; so kill the connection */
 		dhcp6_fail (self, FALSE);
 		break;
 	default:
diff --git a/src/dhcp-manager/nm-dhcp-client.c b/src/dhcp-manager/nm-dhcp-client.c
index 4e56550028..47fa53171f 100644
--- a/src/dhcp-manager/nm-dhcp-client.c
+++ b/src/dhcp-manager/nm-dhcp-client.c
@@ -183,6 +183,7 @@ 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",
 };
@@ -208,13 +209,14 @@ reason_to_state (const char *iface, const char *reason)
 		return NM_DHCP_STATE_BOUND;
 	else if (g_ascii_strcasecmp (reason, "timeout") == 0)
 		return NM_DHCP_STATE_TIMEOUT;
+	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)
 		return NM_DHCP_STATE_DONE;
 	else if (g_ascii_strcasecmp (reason, "fail") == 0 ||
-	         g_ascii_strcasecmp (reason, "abend") == 0 ||
-	         g_ascii_strcasecmp (reason, "nak") == 0 ||
-	         g_ascii_strcasecmp (reason, "expire") == 0 ||
-	         g_ascii_strcasecmp (reason, "expire6") == 0)
+	         g_ascii_strcasecmp (reason, "abend") == 0)
 		return NM_DHCP_STATE_FAIL;
 
 	nm_log_dbg (LOGD_DHCP, "(%s): unmapped DHCP state '%s'", iface, reason);
@@ -744,6 +746,8 @@ nm_dhcp_client_handle_event (gpointer unused,
 
 	old_state = priv->state;
 	new_state = reason_to_state (priv->iface, reason);
+	nm_log_dbg (LOGD_DHCP, "(%s): DHCP reason '%s' -> state '%s'",
+	            iface, reason, state_to_string (new_state));
 
 	if (new_state == NM_DHCP_STATE_BOUND) {
 		/* Copy options */
diff --git a/src/dhcp-manager/nm-dhcp-client.h b/src/dhcp-manager/nm-dhcp-client.h
index 0876061f8a..971c9dc04e 100644
--- a/src/dhcp-manager/nm-dhcp-client.h
+++ b/src/dhcp-manager/nm-dhcp-client.h
@@ -46,10 +46,11 @@
 
 typedef enum {
 	NM_DHCP_STATE_UNKNOWN = 0,
-	NM_DHCP_STATE_BOUND,        /* lease changed (state_is_bound) */
-	NM_DHCP_STATE_TIMEOUT,      /* TIMEOUT */
-	NM_DHCP_STATE_DONE,         /* END */
-	NM_DHCP_STATE_FAIL,          /* failed or quit unexpectedly */
+	NM_DHCP_STATE_BOUND,        /* new lease or lease changed */
+	NM_DHCP_STATE_TIMEOUT,      /* timed out contacting server */
+	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_MAX,
 	NM_DHCP_STATE_MAX = __NM_DHCP_STATE_MAX - 1,
 } NMDhcpState;
diff --git a/src/dhcp-manager/nm-dhcp-systemd.c b/src/dhcp-manager/nm-dhcp-systemd.c
index 5d36282cff..12dc03cc84 100644
--- a/src/dhcp-manager/nm-dhcp-systemd.c
+++ b/src/dhcp-manager/nm-dhcp-systemd.c
@@ -485,6 +485,8 @@ dhcp_event_cb (sd_dhcp_client *client, int event, gpointer user_data)
 
 	switch (event) {
 	case DHCP_EVENT_EXPIRED:
+		nm_dhcp_client_set_state (NM_DHCP_CLIENT (user_data), NM_DHCP_STATE_EXPIRE, NULL, NULL);
+		break;
 	case DHCP_EVENT_STOP:
 		nm_dhcp_client_set_state (NM_DHCP_CLIENT (user_data), NM_DHCP_STATE_FAIL, NULL, NULL);
 		break;