core: fix re-activation of connections on EXTERNAL_DOWN interfacesjk/nm-1-0-int2

When userspace IPv6LL capability is compiled into NetworkManager,
during deactivation NM will toggle userspace IPv6LL in some cases.
This causes link change events in the platform, which show up
in nm-device.c::device_link_changed().

When an EXTERNAL_DOWN interface was activated, the EXTERNAL_DOWN
flag was never cleared even if the device was set IFF_UP or if
a connection was activated via D-Bus (which explicitly sets the
device up).

Second, the device_link_changed() code changed device state
whether or not IFF_UP had actually changed, it simply looked at
the current value.

Together, this caused the first activation of an EXTERNAL_DOWN
device to succeed, but the EXTERNAL_DOWN flag was never cleared
even though the activation set the device IFF_UP.  When a second
activation request came in, the device was moved to DISCONNECTED
state and IPv6LL genmode was reset, causing device_link_changed()
to run.  Since the device had EXTERNAL_DOWN and IFF_UP were still
set, nm_device_set_unmanaged_flag() code was triggered to clear
EXTERNAL_DOWN, which resulted in a state transition to UNAVAILABLE
with a reason of CONNECTION_ASSUMED.  This caused the second
activation request to fail because UNAVAILABLE devices cannot
activate connections by definition.

The fix has three parts:

1) Only change EXTERNAL_DOWN if IFF_UP actually changes, to prevent
spurious changes when something other than IFF_UP changes

2) Only clear EXTERNAL_DOWN when IFF_UP changes while the device
is UNMANAGED, since any state higher than UNMANAGED implies that
either an activation request was received (and thus the device
should be managed) or IFF_UP was set

3) Clear EXTERNAL_DOWN (without triggering state changes) when
any state higher than UNAVAILABLE is entered, since this implies
that a connection is activating or the device is no longer
IFF_UP

fixes:NetworkManager_Test108_testcase_303655

1 files changed, 44 insertions, 29 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index f2690b2e8c..a9899535df 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -229,6 +229,7 @@ typedef struct {
 	guint           carrier_wait_id;
 	gboolean        ignore_carrier;
 	guint32         mtu;
+	gboolean        up;   /* IFF_UP */
 
 	/* Generic DHCP stuff */
 	guint32         dhcp_timeout;
@@ -1250,35 +1251,46 @@ device_link_changed (NMDevice *self, NMPlatformLink *info)
 	if (ip_ifname_changed)
 		update_for_ip_ifname_change (self);
 
-	/* Manage externally-created software interfaces only when they are IFF_UP */
-	if (   is_software_external (self)
-	    && (nm_device_get_state (self) <= NM_DEVICE_STATE_DISCONNECTED)
-	    && priv->ifindex > 0) {
-		gboolean external_down = nm_device_get_unmanaged_flag (self, NM_UNMANAGED_EXTERNAL_DOWN);
-
-		if (external_down && info->up) {
-			/* Ensure the assume check is queued before any queued state changes
-			 * from the transition to UNAVAILABLE.
-			 */
-			nm_device_queue_recheck_assume (self);
-
-			/* Resetting the EXTERNAL_DOWN flag may change the device's state
-			 * to UNAVAILABLE.  To ensure that the state change doesn't touch
-			 * the device before assumption occurs, pass
-			 * NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED as the reason.
-			 */
-			nm_device_set_unmanaged (self,
-			                         NM_UNMANAGED_EXTERNAL_DOWN,
-			                         FALSE,
-			                         NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED);
-		} else if (!external_down && !info->up) {
-			/* If the device is already disconnected and is set !IFF_UP,
-			 * unmanage it.
-			 */
-			nm_device_set_unmanaged (self,
-			                         NM_UNMANAGED_EXTERNAL_DOWN,
-			                         TRUE,
-			                         NM_DEVICE_STATE_REASON_USER_REQUESTED);
+	if (priv->up != info->up) {
+		priv->up = info->up;
+
+		/* Manage externally-created software interfaces only when they are IFF_UP */
+		g_assert (priv->ifindex > 0);
+		if (is_software_external (self)) {
+			gboolean external_down = nm_device_get_unmanaged_flag (self, NM_UNMANAGED_EXTERNAL_DOWN);
+
+			if (external_down && info->up) {
+				if (nm_device_get_state (self) < NM_DEVICE_STATE_DISCONNECTED) {
+					/* Ensure the assume check is queued before any queued state changes
+					 * from the transition to UNAVAILABLE.
+					 */
+					nm_device_queue_recheck_assume (self);
+
+					/* Resetting the EXTERNAL_DOWN flag may change the device's state
+					 * to UNAVAILABLE.  To ensure that the state change doesn't touch
+					 * the device before assumption occurs, pass
+					 * NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED as the reason.
+					 */
+					nm_device_set_unmanaged (self,
+					                         NM_UNMANAGED_EXTERNAL_DOWN,
+					                         FALSE,
+					                         NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED);
+				} else {
+					/* Don't trigger a state change; if the device is in a
+					 * state higher than UNAVAILABLE, it is already IFF_UP
+					 * or an explicit activation request was received.
+					 */
+					priv->unmanaged_flags &= ~NM_UNMANAGED_EXTERNAL_DOWN;
+				}
+			} else if (!external_down && !info->up && nm_device_get_state (self) <= NM_DEVICE_STATE_DISCONNECTED) {
+				/* If the device is already disconnected and is set !IFF_UP,
+				 * unmanage it.
+				 */
+				nm_device_set_unmanaged (self,
+				                         NM_UNMANAGED_EXTERNAL_DOWN,
+				                         TRUE,
+				                         NM_DEVICE_STATE_REASON_USER_REQUESTED);
+			}
 		}
 	}
 }
@@ -8228,6 +8240,7 @@ set_property (GObject *object, guint prop_id,
 			g_free (priv->iface);
 			priv->iface = g_strdup (platform_device->name);
 			priv->ifindex = platform_device->ifindex;
+			priv->up = platform_device->up;
 			g_free (priv->driver);
 			priv->driver = g_strdup (platform_device->driver);
 		}
@@ -8243,6 +8256,8 @@ set_property (GObject *object, guint prop_id,
 			g_free (priv->iface);
 			priv->iface = g_value_dup_string (value);
 			priv->ifindex = nm_platform_link_get_ifindex (priv->iface);
+			if (priv->ifindex > 0)
+				priv->up = nm_platform_link_is_up (priv->ifindex);
 		}
 		break;
 	case PROP_DRIVER: