Merge "Nat464Xlat: correct racefree teardown"

am: 4112ecb7a6

Change-Id: I8612db5e5050690db8cf41dd04944b4c22da340c

services/core/java/com/android/server/ConnectivityService.java

@@ -2023,16 +2023,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     break;
                 }
                 case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: {
-                    if (VDBG) {
+                    handleUpdateLinkProperties(nai, (LinkProperties) msg.obj);
-                        log("Update of LinkProperties for " + nai.name() +
-                                "; created=" + nai.created +
-                                "; everConnected=" + nai.everConnected);
-                    }
-                    LinkProperties oldLp = nai.linkProperties;
-                    synchronized (nai) {
-                        nai.linkProperties = (LinkProperties)msg.obj;
-                    }
-                    if (nai.everConnected) updateLinkProperties(nai, oldLp);
                     break;
                 }
                 case NetworkAgent.EVENT_NETWORK_INFO_CHANGED: {
@@ -2281,7 +2272,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
             }
             nai.networkMonitor.sendMessage(NetworkMonitor.CMD_NETWORK_DISCONNECTED);
             mNetworkAgentInfos.remove(msg.replyTo);
-            maybeStopClat(nai);
+            nai.maybeStopClat();
             synchronized (mNetworkForNetId) {
                 // Remove the NetworkAgent, but don't mark the netId as
                 // available until we've told netd to delete it below.
@@ -4375,7 +4366,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         updateDnses(newLp, oldLp, netId);
 
         // Start or stop clat accordingly to network state.
-        updateClat(networkAgent);
+        networkAgent.updateClat(mNetd);
         if (isDefaultNetwork(networkAgent)) {
             handleApplyDefaultProxy(newLp.getHttpProxy());
         } else {
@@ -4390,32 +4381,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
         mKeepaliveTracker.handleCheckKeepalivesStillValid(networkAgent);
     }
 
-    private void updateClat(NetworkAgentInfo nai) {
-        if (Nat464Xlat.requiresClat(nai)) {
-            maybeStartClat(nai);
-        } else {
-            maybeStopClat(nai);
-        }
-    }
-
-    /** Ensure clat has started for this network. */
-    private void maybeStartClat(NetworkAgentInfo nai) {
-        if (nai.clatd != null && nai.clatd.isStarted()) {
-            return;
-        }
-        nai.clatd = new Nat464Xlat(mNetd, mTrackerHandler, nai);
-        nai.clatd.start();
-    }
-
-    /** Ensure clat has stopped for this network. */
-    private void maybeStopClat(NetworkAgentInfo nai) {
-        if (nai.clatd == null) {
-            return;
-        }
-        nai.clatd.stop();
-        nai.clatd = null;
-    }
-
     private void wakeupModifyInterface(String iface, NetworkCapabilities caps, boolean add) {
         // Marks are only available on WiFi interaces. Checking for
         // marks on unsupported interfaces is harmless.
@@ -4650,6 +4615,26 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) {
+        if (mNetworkForNetId.get(nai.network.netId) != nai) {
+            // Ignore updates for disconnected networks
+            return;
+        }
+
+        if (VDBG) {
+            log("Update of LinkProperties for " + nai.name() +
+                    "; created=" + nai.created +
+                    "; everConnected=" + nai.everConnected);
+        }
+        LinkProperties oldLp = nai.linkProperties;
+        synchronized (nai) {
+            nai.linkProperties = newLp;
+        }
+        if (nai.everConnected) {
+            updateLinkProperties(nai, oldLp);
+        }
+    }
+
     private void sendUpdatedScoreToFactories(NetworkAgentInfo nai) {
         for (int i = 0; i < nai.numNetworkRequests(); i++) {
             NetworkRequest nr = nai.requestAt(i);

services/core/java/com/android/server/connectivity/Nat464Xlat.java

@@ -20,22 +20,21 @@ import android.net.InterfaceConfiguration;
 import android.net.ConnectivityManager;
 import android.net.LinkAddress;
 import android.net.LinkProperties;
-import android.net.NetworkAgent;
 import android.net.RouteInfo;
-import android.os.Handler;
-import android.os.Message;
 import android.os.INetworkManagementService;
 import android.os.RemoteException;
 import android.util.Slog;
 
-import com.android.server.net.BaseNetworkObserver;
 import com.android.internal.util.ArrayUtils;
+import com.android.server.net.BaseNetworkObserver;
 
 import java.net.Inet4Address;
 import java.util.Objects;
 
 /**
- * Class to manage a 464xlat CLAT daemon.
+ * Class to manage a 464xlat CLAT daemon. Nat464Xlat is not thread safe and should be manipulated
+ * from a consistent and unique thread context. It is the responsibility of ConnectivityService to
+ * call into this class from its own Handler thread.
  *
  * @hide
  */
@@ -55,28 +54,23 @@ public class Nat464Xlat extends BaseNetworkObserver {
 
     private final INetworkManagementService mNMService;
 
-    // ConnectivityService Handler for LinkProperties updates.
-    private final Handler mHandler;
-
     // The network we're running on, and its type.
     private final NetworkAgentInfo mNetwork;
 
     private enum State {
         IDLE,       // start() not called. Base iface and stacked iface names are null.
         STARTING,   // start() called. Base iface and stacked iface names are known.
-        RUNNING;    // start() called, and the stacked iface is known to be up.
+        RUNNING,    // start() called, and the stacked iface is known to be up.
+        STOPPING;   // stop() called, this Nat464Xlat is still registered as a network observer for
+                    // the stacked interface.
     }
 
-    // Once mIface is non-null and isStarted() is true, methods called by ConnectivityService on
-    // its handler thread must not modify any internal state variables; they are only updated by the
-    // interface observers, called on the notification threads.
     private String mBaseIface;
     private String mIface;
-    private volatile State mState = State.IDLE;
+    private State mState = State.IDLE;
 
-    public Nat464Xlat(INetworkManagementService nmService, Handler handler, NetworkAgentInfo nai) {
+    public Nat464Xlat(INetworkManagementService nmService, NetworkAgentInfo nai) {
         mNMService = nmService;
-        mHandler = handler;
         mNetwork = nai;
     }
 
@@ -89,6 +83,8 @@ public class Nat464Xlat extends BaseNetworkObserver {
         // TODO: migrate to NetworkCapabilities.TRANSPORT_*.
         final int netType = nai.networkInfo.getType();
         final boolean supported = ArrayUtils.contains(NETWORK_TYPES, nai.networkInfo.getType());
+        // TODO: this should also consider if the network is in SUSPENDED state to avoid stopping
+        // clatd in SUSPENDED state.
         final boolean connected = nai.networkInfo.isConnected();
         // We only run clat on networks that don't have a native IPv4 address.
         final boolean hasIPv4Address =
@@ -104,6 +100,13 @@ public class Nat464Xlat extends BaseNetworkObserver {
         return mState != State.IDLE;
     }
 
+    /**
+     * @return true if clatd has been started but the stacked interface is not yet up.
+     */
+    public boolean isStarting() {
+        return mState == State.STARTING;
+    }
+
     /**
      * @return true if clatd has been started and the stacked interface is up.
      */
@@ -112,25 +115,77 @@ public class Nat464Xlat extends BaseNetworkObserver {
     }
 
     /**
-     * Sets internal state.
+     * @return true if clatd has been stopped.
+     */
+    public boolean isStopping() {
+        return mState == State.STOPPING;
+    }
+
+    /**
+     * Start clatd, register this Nat464Xlat as a network observer for the stacked interface,
+     * and set internal state.
      */
     private void enterStartingState(String baseIface) {
+        try {
+            mNMService.registerObserver(this);
+        } catch(RemoteException e) {
+            Slog.e(TAG,
+                    "startClat: Can't register interface observer for clat on " + mNetwork.name());
+            return;
+        }
+        try {
+            mNMService.startClatd(baseIface);
+        } catch(RemoteException|IllegalStateException e) {
+            Slog.e(TAG, "Error starting clatd on " + baseIface, e);
+        }
         mIface = CLAT_PREFIX + baseIface;
         mBaseIface = baseIface;
         mState = State.STARTING;
     }
 
     /**
-     * Clears internal state. Must not be called by ConnectivityService.
+     * Enter running state just after getting confirmation that the stacked interface is up, and
+     * turn ND offload off if on WiFi.
+     */
+    private void enterRunningState() {
+        maybeSetIpv6NdOffload(mBaseIface, false);
+        mState = State.RUNNING;
+    }
+
+    /**
+     * Stop clatd, and turn ND offload on if it had been turned off.
+     */
+    private void enterStoppingState() {
+        if (isRunning()) {
+            maybeSetIpv6NdOffload(mBaseIface, true);
+        }
+
+        try {
+            mNMService.stopClatd(mBaseIface);
+        } catch(RemoteException|IllegalStateException e) {
+            Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
+        }
+
+        mState = State.STOPPING;
+    }
+
+    /**
+     * Unregister as a base observer for the stacked interface, and clear internal state.
      */
     private void enterIdleState() {
+        try {
+            mNMService.unregisterObserver(this);
+        } catch(RemoteException|IllegalStateException e) {
+            Slog.e(TAG, "Error unregistering clatd observer on " + mBaseIface, e);
+        }
+
         mIface = null;
         mBaseIface = null;
         mState = State.IDLE;
     }
 
     /**
-     * Starts the clat daemon. Called by ConnectivityService on the handler thread.
+     * Starts the clat daemon.
      */
     public void start() {
         if (isStarted()) {
@@ -143,53 +198,30 @@ public class Nat464Xlat extends BaseNetworkObserver {
             return;
         }
 
-        try {
-            mNMService.registerObserver(this);
-        } catch(RemoteException e) {
-            Slog.e(TAG, "startClat: Can't register interface observer for clat on " + mNetwork);
-            return;
-        }
-
         String baseIface = mNetwork.linkProperties.getInterfaceName();
         if (baseIface == null) {
             Slog.e(TAG, "startClat: Can't start clat on null interface");
             return;
         }
         // TODO: should we only do this if mNMService.startClatd() succeeds?
+        Slog.i(TAG, "Starting clatd on " + baseIface);
         enterStartingState(baseIface);
-
-        Slog.i(TAG, "Starting clatd on " + mBaseIface);
-        try {
-            mNMService.startClatd(mBaseIface);
-        } catch(RemoteException|IllegalStateException e) {
-            Slog.e(TAG, "Error starting clatd on " + mBaseIface, e);
-        }
     }
 
     /**
-     * Stops the clat daemon. Called by ConnectivityService on the handler thread.
+     * Stops the clat daemon.
      */
     public void stop() {
         if (!isStarted()) {
-            Slog.e(TAG, "stopClat: already stopped or not started");
             return;
         }
-
         Slog.i(TAG, "Stopping clatd on " + mBaseIface);
-        try {
-            mNMService.stopClatd(mBaseIface);
-        } catch(RemoteException|IllegalStateException e) {
-            Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
-        }
-        // When clatd stops and its interface is deleted, interfaceRemoved() will notify
-        // ConnectivityService and call enterIdleState().
-    }
 
-    private void updateConnectivityService(LinkProperties lp) {
+        boolean wasStarting = isStarting();
-        Message msg = mHandler.obtainMessage(NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED, lp);
+        enterStoppingState();
-        msg.replyTo = mNetwork.messenger;
+        if (wasStarting) {
-        Slog.i(TAG, "sending message to ConnectivityService: " + msg);
+            enterIdleState();
-        msg.sendToTarget();
+        }
     }
 
     /**
@@ -257,59 +289,58 @@ public class Nat464Xlat extends BaseNetworkObserver {
     }
 
     /**
-     * Adds stacked link on base link and transitions to Running state
+     * Adds stacked link on base link and transitions to RUNNING state.
-     * This is called by the InterfaceObserver on its own thread, so can race with stop().
      */
-    @Override
+    private void handleInterfaceLinkStateChanged(String iface, boolean up) {
-    public void interfaceLinkStateChanged(String iface, boolean up) {
+        if (!isStarting() || !up || !Objects.equals(mIface, iface)) {
-        if (!isStarted() || !up || !Objects.equals(mIface, iface)) {
-            return;
-        }
-        if (isRunning()) {
             return;
         }
+
         LinkAddress clatAddress = getLinkAddress(iface);
         if (clatAddress == null) {
+            Slog.e(TAG, "clatAddress was null for stacked iface " + iface);
             return;
         }
-        mState = State.RUNNING;
+
         Slog.i(TAG, String.format("interface %s is up, adding stacked link %s on top of %s",
                 mIface, mIface, mBaseIface));
-
+        enterRunningState();
-        maybeSetIpv6NdOffload(mBaseIface, false);
         LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
         lp.addStackedLink(makeLinkProperties(clatAddress));
-        updateConnectivityService(lp);
+        mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
     }
 
-    @Override
+    /**
-    public void interfaceRemoved(String iface) {
+     * Removes stacked link on base link and transitions to IDLE state.
-        if (!isStarted() || !Objects.equals(mIface, iface)) {
+     */
+    private void handleInterfaceRemoved(String iface) {
+        if (!Objects.equals(mIface, iface)) {
             return;
         }
-        if (!isRunning()) {
+        if (!isRunning() && !isStopping()) {
             return;
         }
 
         Slog.i(TAG, "interface " + iface + " removed");
-        // The interface going away likely means clatd has crashed. Ask netd to stop it,
+        if (!isStopping()) {
-        // because otherwise when we try to start it again on the same base interface netd
+            // Ensure clatd is stopped if stop() has not been called: this likely means that clatd
-        // will complain that it's already started.
+            // has crashed.
-        //
+            enterStoppingState();
-        // Note that this method can be called by the interface observer at the same time
-        // that ConnectivityService calls stop(). In this case, the second call to
-        // stopClatd() will just throw IllegalStateException, which we'll ignore.
-        try {
-            mNMService.unregisterObserver(this);
-            mNMService.stopClatd(mBaseIface);
-        } catch (RemoteException|IllegalStateException e) {
-            // Well, we tried.
         }
-        maybeSetIpv6NdOffload(mBaseIface, true);
-        LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
-        lp.removeStackedLink(mIface);
         enterIdleState();
-        updateConnectivityService(lp);
+        LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
+        lp.removeStackedLink(iface);
+        mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
+    }
+
+    @Override
+    public void interfaceLinkStateChanged(String iface, boolean up) {
+        mNetwork.handler().post(() -> { handleInterfaceLinkStateChanged(iface, up); });
+    }
+
+    @Override
+    public void interfaceRemoved(String iface) {
+        mNetwork.handler().post(() -> { handleInterfaceRemoved(iface); });
     }
 
     @Override

services/core/java/com/android/server/connectivity/NetworkAgentInfo.java

@@ -27,7 +27,9 @@ import android.net.NetworkMisc;
 import android.net.NetworkRequest;
 import android.net.NetworkState;
 import android.os.Handler;
+import android.os.INetworkManagementService;
 import android.os.Messenger;
+import android.os.RemoteException;
 import android.os.SystemClock;
 import android.util.Log;
 import android.util.SparseArray;
@@ -268,6 +270,14 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
         networkMisc = misc;
     }
 
+    public ConnectivityService connService() {
+        return mConnService;
+    }
+
+    public Handler handler() {
+        return mHandler;
+    }
+
     // Functions for manipulating the requests satisfied by this network.
     //
     // These functions must only called on ConnectivityService's main thread.
@@ -551,6 +561,32 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
         for (LingerTimer timer : mLingerTimers) { pw.println(timer); }
     }
 
+    public void updateClat(INetworkManagementService netd) {
+        if (Nat464Xlat.requiresClat(this)) {
+            maybeStartClat(netd);
+        } else {
+            maybeStopClat();
+        }
+    }
+
+    /** Ensure clat has started for this network. */
+    public void maybeStartClat(INetworkManagementService netd) {
+        if (clatd != null && clatd.isStarted()) {
+            return;
+        }
+        clatd = new Nat464Xlat(netd, this);
+        clatd.start();
+    }
+
+    /** Ensure clat has stopped for this network. */
+    public void maybeStopClat() {
+        if (clatd == null) {
+            return;
+        }
+        clatd.stop();
+        clatd = null;
+    }
+
     public String toString() {
         return "NetworkAgentInfo{ ni{" + networkInfo + "}  " +
                 "network{" + network + "}  nethandle{" + network.getNetworkHandle() + "}  " +

tests/net/java/com/android/server/connectivity/Nat464XlatTest.java

@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.connectivity;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import android.net.ConnectivityManager;
+import android.net.InterfaceConfiguration;
+import android.net.LinkAddress;
+import android.net.LinkProperties;
+import android.net.NetworkInfo;
+import android.os.Handler;
+import android.os.INetworkManagementService;
+import android.os.test.TestLooper;
+import android.support.test.filters.SmallTest;
+import android.support.test.runner.AndroidJUnit4;
+
+import com.android.server.ConnectivityService;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+@RunWith(AndroidJUnit4.class)
+@SmallTest
+public class Nat464XlatTest {
+
+    static final String BASE_IFACE = "test0";
+    static final String STACKED_IFACE = "v4-test0";
+    static final LinkAddress ADDR = new LinkAddress("192.0.2.5/29");
+
+    @Mock ConnectivityService mConnectivity;
+    @Mock INetworkManagementService mNms;
+    @Mock InterfaceConfiguration mConfig;
+    @Mock NetworkAgentInfo mNai;
+
+    TestLooper mLooper;
+    Handler mHandler;
+
+    Nat464Xlat makeNat464Xlat() {
+        return new Nat464Xlat(mNms, mNai);
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        mLooper = new TestLooper();
+        mHandler = new Handler(mLooper.getLooper());
+
+        MockitoAnnotations.initMocks(this);
+
+        mNai.linkProperties = new LinkProperties();
+        mNai.linkProperties.setInterfaceName(BASE_IFACE);
+        mNai.networkInfo = new NetworkInfo(null);
+        mNai.networkInfo.setType(ConnectivityManager.TYPE_WIFI);
+        when(mNai.connService()).thenReturn(mConnectivity);
+        when(mNai.handler()).thenReturn(mHandler);
+
+        when(mNms.getInterfaceConfig(eq(STACKED_IFACE))).thenReturn(mConfig);
+        when(mConfig.getLinkAddress()).thenReturn(ADDR);
+    }
+
+    @Test
+    public void testNormalStartAndStop() throws Exception {
+        Nat464Xlat nat = makeNat464Xlat();
+        ArgumentCaptor<LinkProperties> c = ArgumentCaptor.forClass(LinkProperties.class);
+
+        // ConnectivityService starts clat.
+        nat.start();
+
+        verify(mNms).registerObserver(eq(nat));
+        verify(mNms).startClatd(eq(BASE_IFACE));
+
+        // Stacked interface up notification arrives.
+        nat.interfaceLinkStateChanged(STACKED_IFACE, true);
+        mLooper.dispatchNext();
+
+        verify(mNms).getInterfaceConfig(eq(STACKED_IFACE));
+        verify(mNms).setInterfaceIpv6NdOffload(eq(BASE_IFACE), eq(false));
+        verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
+        assertFalse(c.getValue().getStackedLinks().isEmpty());
+        assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+        assertRunning(nat);
+
+        // ConnectivityService stops clat (Network disconnects, IPv4 addr appears, ...).
+        nat.stop();
+
+        verify(mNms).stopClatd(eq(BASE_IFACE));
+        verify(mNms).setInterfaceIpv6NdOffload(eq(BASE_IFACE), eq(true));
+
+        // Stacked interface removed notification arrives.
+        nat.interfaceRemoved(STACKED_IFACE);
+        mLooper.dispatchNext();
+
+        verify(mNms).unregisterObserver(eq(nat));
+        verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
+        assertTrue(c.getValue().getStackedLinks().isEmpty());
+        assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+        assertIdle(nat);
+
+        verifyNoMoreInteractions(mNms, mConnectivity);
+    }
+
+    @Test
+    public void testClatdCrashWhileRunning() throws Exception {
+        Nat464Xlat nat = makeNat464Xlat();
+        ArgumentCaptor<LinkProperties> c = ArgumentCaptor.forClass(LinkProperties.class);
+
+        // ConnectivityService starts clat.
+        nat.start();
+
+        verify(mNms).registerObserver(eq(nat));
+        verify(mNms).startClatd(eq(BASE_IFACE));
+
+        // Stacked interface up notification arrives.
+        nat.interfaceLinkStateChanged(STACKED_IFACE, true);
+        mLooper.dispatchNext();
+
+        verify(mNms).getInterfaceConfig(eq(STACKED_IFACE));
+        verify(mNms).setInterfaceIpv6NdOffload(eq(BASE_IFACE), eq(false));
+        verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
+        assertFalse(c.getValue().getStackedLinks().isEmpty());
+        assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+        assertRunning(nat);
+
+        // Stacked interface removed notification arrives (clatd crashed, ...).
+        nat.interfaceRemoved(STACKED_IFACE);
+        mLooper.dispatchNext();
+
+        verify(mNms).unregisterObserver(eq(nat));
+        verify(mNms).stopClatd(eq(BASE_IFACE));
+        verify(mNms).setInterfaceIpv6NdOffload(eq(BASE_IFACE), eq(true));
+        verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
+        assertTrue(c.getValue().getStackedLinks().isEmpty());
+        assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+        assertIdle(nat);
+
+        // ConnectivityService stops clat: no-op.
+        nat.stop();
+
+        verifyNoMoreInteractions(mNms, mConnectivity);
+    }
+
+    @Test
+    public void testStopBeforeClatdStarts() throws Exception {
+        Nat464Xlat nat = makeNat464Xlat();
+
+        // ConnectivityService starts clat.
+        nat.start();
+
+        verify(mNms).registerObserver(eq(nat));
+        verify(mNms).startClatd(eq(BASE_IFACE));
+
+        // ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
+        nat.stop();
+
+        verify(mNms).unregisterObserver(eq(nat));
+        verify(mNms).stopClatd(eq(BASE_IFACE));
+        assertIdle(nat);
+
+        // In-flight interface up notification arrives: no-op
+        nat.interfaceLinkStateChanged(STACKED_IFACE, true);
+        mLooper.dispatchNext();
+
+
+        // Interface removed notification arrives after stopClatd() takes effect: no-op.
+        nat.interfaceRemoved(STACKED_IFACE);
+        mLooper.dispatchNext();
+
+        assertIdle(nat);
+
+        verifyNoMoreInteractions(mNms, mConnectivity);
+    }
+
+    @Test
+    public void testStopAndClatdNeverStarts() throws Exception {
+        Nat464Xlat nat = makeNat464Xlat();
+
+        // ConnectivityService starts clat.
+        nat.start();
+
+        verify(mNms).registerObserver(eq(nat));
+        verify(mNms).startClatd(eq(BASE_IFACE));
+
+        // ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
+        nat.stop();
+
+        verify(mNms).unregisterObserver(eq(nat));
+        verify(mNms).stopClatd(eq(BASE_IFACE));
+        assertIdle(nat);
+
+        verifyNoMoreInteractions(mNms, mConnectivity);
+    }
+
+    static void assertIdle(Nat464Xlat nat) {
+        assertTrue("Nat464Xlat was not IDLE", !nat.isStarted());
+    }
+
+    static void assertRunning(Nat464Xlat nat) {
+        assertTrue("Nat464Xlat was not RUNNING", nat.isRunning());
+    }
+}