From 7b0732fff7832e438b8609c9bc6fe1ee0f6ca2a6 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 8 Jan 2019 14:43:37 +0900 Subject: [PATCH] Change Nat464Xlat lifecycle. Currently, NetworkAgentInfo keeps a Nat464Xlat object only on networks where we're starting clatd (i.e., IPv6-only networks). Simplify this code by making the Nat464Xlat object final and always non-null. This allows us to use Nat464Xlat to store information, such as the NAT64 prefix, even if the clat daemon has not been started yet. Also, remove the STOPPING state which waits for the interface to be removed. Instead, when stop() is called, immediately enter the IDLE state. - This is necessary in order to be able to call start() again before the interface removal notification has arrived. - It's also arguably more correct than the current code, because when stop() returns clatd has already terminated (ClatdController::stopClatd calls waitpid), and thus the tun fd is already closed and the tun interface is gone. Also, now that Nat464Xlat objects are reused after stop(), add test coverage for calling start() after stop, in both cases: - The notification that the previous interface was removed arrives before the second start(). - The notification that the previous interface was removed arrives after the second start() but before the notification that the second interface was added. Also fix a couple of lint warnings. Test: builds, boots Test: atest FrameworksNetTests Test: clatd stops when IPv4 address added Test: clatd restarts after "adb shell killall clatd" Change-Id: I3dc66d155aa27606681f3473daf2170434d8c6d0 --- .../android/server/ConnectivityService.java | 13 +-- .../server/connectivity/Nat464Xlat.java | 76 ++++++------- .../server/connectivity/NetworkAgentInfo.java | 36 ++---- .../server/connectivity/Nat464XlatTest.java | 106 ++++++++++++++++-- 4 files changed, 143 insertions(+), 88 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b5fcde4bf2..113d92ca65 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2877,7 +2877,7 @@ public class ConnectivityService extends IConnectivityManager.Stub e.rethrowFromSystemServer(); } mNetworkAgentInfos.remove(nai.messenger); - nai.maybeStopClat(); + nai.updateClat(); synchronized (mNetworkForNetId) { // Remove the NetworkAgent, but don't mark the netId as // available until we've told netd to delete it below. @@ -5185,11 +5185,10 @@ public class ConnectivityService extends IConnectivityManager.Stub LinkProperties oldLp) { int netId = networkAgent.network.netId; - // The NetworkAgentInfo does not know whether clatd is running on its network or not. Before - // we do anything else, make sure its LinkProperties are accurate. - if (networkAgent.clatd != null) { - networkAgent.clatd.fixupLinkProperties(oldLp, newLp); - } + // The NetworkAgentInfo does not know whether clatd is running on its network or not, or + // whether there is a NAT64 prefix. Before we do anything else, make sure its LinkProperties + // are accurate. + networkAgent.clatd.fixupLinkProperties(oldLp, newLp); updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities); updateMtu(newLp, oldLp); @@ -5220,7 +5219,7 @@ public class ConnectivityService extends IConnectivityManager.Stub networkAgent.linkProperties = newLp; } // Start or stop clat accordingly to network state. - networkAgent.updateClat(mNMS); + networkAgent.updateClat(); notifyIfacesChangedForNetworkStats(); if (networkAgent.everConnected) { try { diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index 9d9b1cfdf6..c834eaa951 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -70,8 +70,6 @@ public class Nat464Xlat extends BaseNetworkObserver { 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. - STOPPING; // stop() called, this Nat464Xlat is still registered as a network observer for - // the stacked interface. } private String mBaseIface; @@ -123,13 +121,6 @@ public class Nat464Xlat extends BaseNetworkObserver { return mState == State.RUNNING; } - /** - * @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. @@ -137,14 +128,13 @@ public class Nat464Xlat extends BaseNetworkObserver { 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()); + } catch (RemoteException e) { + Slog.e(TAG, "Can't register interface observer for clat on " + mNetwork.name()); return; } try { mNetd.clatdStart(baseIface); - } catch(RemoteException|IllegalStateException e) { + } catch (RemoteException | IllegalStateException e) { Slog.e(TAG, "Error starting clatd on " + baseIface, e); } mIface = CLAT_PREFIX + baseIface; @@ -160,29 +150,15 @@ public class Nat464Xlat extends BaseNetworkObserver { mState = State.RUNNING; } - /** - * Stop clatd, and turn ND offload on if it had been turned off. - */ - private void enterStoppingState() { - try { - mNetd.clatdStop(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) { + } catch (RemoteException | IllegalStateException e) { Slog.e(TAG, "Error unregistering clatd observer on " + mBaseIface, e); } - mIface = null; mBaseIface = null; mState = State.IDLE; @@ -217,14 +193,30 @@ public class Nat464Xlat extends BaseNetworkObserver { */ public void stop() { if (!isStarted()) { + Slog.e(TAG, "stopClat: already stopped"); return; } - Slog.i(TAG, "Stopping clatd on " + mBaseIface); - boolean wasStarting = isStarting(); - enterStoppingState(); - if (wasStarting) { - enterIdleState(); + Slog.i(TAG, "Stopping clatd on " + mBaseIface); + try { + mNetd.clatdStop(mBaseIface); + } catch (RemoteException | IllegalStateException e) { + Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e); + } + + String iface = mIface; + boolean wasRunning = isRunning(); + + // Enter IDLE state before updating LinkProperties. handleUpdateLinkProperties ends up + // calling fixupLinkProperties, and if at that time the state is still RUNNING, + // fixupLinkProperties would wrongly inform ConnectivityService that there is still a + // stacked interface. + enterIdleState(); + + if (wasRunning) { + LinkProperties lp = new LinkProperties(mNetwork.linkProperties); + lp.removeStackedLink(iface); + mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp); } } @@ -272,7 +264,7 @@ public class Nat464Xlat extends BaseNetworkObserver { try { InterfaceConfiguration config = mNMService.getInterfaceConfig(iface); return config.getLinkAddress(); - } catch(RemoteException|IllegalStateException e) { + } catch (RemoteException | IllegalStateException e) { Slog.e(TAG, "Error getting link properties: " + e); return null; } @@ -307,20 +299,16 @@ public class Nat464Xlat extends BaseNetworkObserver { if (!Objects.equals(mIface, iface)) { return; } - if (!isRunning() && !isStopping()) { + if (!isRunning()) { return; } Slog.i(TAG, "interface " + iface + " removed"); - if (!isStopping()) { - // Ensure clatd is stopped if stop() has not been called: this likely means that clatd - // has crashed. - enterStoppingState(); - } - enterIdleState(); - LinkProperties lp = new LinkProperties(mNetwork.linkProperties); - lp.removeStackedLink(iface); - mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp); + // If we're running, and the interface was removed, then we didn't call stop(), and it's + // likely that clatd crashed. Ensure we call stop() so we can start clatd again. Calling + // stop() will also update LinkProperties, and if clatd crashed, the LinkProperties update + // will cause ConnectivityService to call start() again. + stop(); } @Override diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index d0cff25dbf..96f7283f6a 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -236,7 +236,7 @@ public class NetworkAgentInfo implements Comparable { public final AsyncChannel asyncChannel; // Used by ConnectivityService to keep track of 464xlat. - public Nat464Xlat clatd; + public final Nat464Xlat clatd; // Set after asynchronous creation of the NetworkMonitor. private volatile INetworkMonitor mNetworkMonitor; @@ -244,8 +244,6 @@ public class NetworkAgentInfo implements Comparable { private static final String TAG = ConnectivityService.class.getSimpleName(); private static final boolean VDBG = false; private final ConnectivityService mConnService; - private final INetd mNetd; - private final INetworkManagementService mNMS; private final Context mContext; private final Handler mHandler; @@ -260,9 +258,8 @@ public class NetworkAgentInfo implements Comparable { linkProperties = lp; networkCapabilities = nc; currentScore = score; + clatd = new Nat464Xlat(this, netd, nms); mConnService = connService; - mNetd = netd; - mNMS = nms; mContext = context; mHandler = handler; networkMisc = misc; @@ -595,32 +592,15 @@ public class NetworkAgentInfo implements Comparable { for (LingerTimer timer : mLingerTimers) { pw.println(timer); } } - public void updateClat(INetworkManagementService netd) { - if (Nat464Xlat.requiresClat(this)) { - maybeStartClat(); - } else { - maybeStopClat(); + /** Starts or stops clatd as required. */ + public void updateClat() { + if (!clatd.isStarted() && Nat464Xlat.requiresClat(this)) { + clatd.start(); + } else if (clatd.isStarted() && !Nat464Xlat.requiresClat(this)) { + clatd.stop(); } } - /** Ensure clat has started for this network. */ - public void maybeStartClat() { - if (clatd != null && clatd.isStarted()) { - return; - } - clatd = new Nat464Xlat(this, mNetd, mNMS); - 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() + "} " diff --git a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java index 07b1d057c8..b9450b31f1 100644 --- a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java +++ b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java @@ -19,6 +19,7 @@ package com.android.server.connectivity; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -43,6 +44,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -145,20 +147,106 @@ public class Nat464XlatTest { nat.stop(); verify(mNetd).clatdStop(eq(BASE_IFACE)); - - // 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()); + verify(mNms).unregisterObserver(eq(nat)); assertTrue(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); assertIdle(nat); + // Stacked interface removed notification arrives and is ignored. + nat.interfaceRemoved(STACKED_IFACE); + mLooper.dispatchNext(); + verifyNoMoreInteractions(mNetd, mNms, mConnectivity); } + private void checkStartStopStart(boolean interfaceRemovedFirst) throws Exception { + Nat464Xlat nat = makeNat464Xlat(); + ArgumentCaptor c = ArgumentCaptor.forClass(LinkProperties.class); + InOrder inOrder = inOrder(mNetd, mConnectivity); + + // ConnectivityService starts clat. + nat.start(); + + inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE)); + + // Stacked interface up notification arrives. + nat.interfaceLinkStateChanged(STACKED_IFACE, true); + mLooper.dispatchNext(); + + inOrder.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(); + + inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE)); + + inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture()); + assertTrue(c.getValue().getStackedLinks().isEmpty()); + assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); + assertIdle(nat); + + if (interfaceRemovedFirst) { + // Stacked interface removed notification arrives and is ignored. + nat.interfaceRemoved(STACKED_IFACE); + mLooper.dispatchNext(); + nat.interfaceLinkStateChanged(STACKED_IFACE, false); + mLooper.dispatchNext(); + } + + assertTrue(c.getValue().getStackedLinks().isEmpty()); + assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); + assertIdle(nat); + inOrder.verifyNoMoreInteractions(); + + // ConnectivityService starts clatd again. + nat.start(); + + inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE)); + + if (!interfaceRemovedFirst) { + // Stacked interface removed notification arrives and is ignored. + nat.interfaceRemoved(STACKED_IFACE); + mLooper.dispatchNext(); + nat.interfaceLinkStateChanged(STACKED_IFACE, false); + mLooper.dispatchNext(); + } + + // Stacked interface up notification arrives. + nat.interfaceLinkStateChanged(STACKED_IFACE, true); + mLooper.dispatchNext(); + + inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); + assertFalse(c.getValue().getStackedLinks().isEmpty()); + assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); + assertRunning(nat); + + // ConnectivityService stops clat again. + nat.stop(); + + inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE)); + + inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture()); + assertTrue(c.getValue().getStackedLinks().isEmpty()); + assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); + assertIdle(nat); + + inOrder.verifyNoMoreInteractions(); + } + + @Test + public void testStartStopStart() throws Exception { + checkStartStopStart(true); + } + + @Test + public void testStartStopStartBeforeInterfaceRemoved() throws Exception { + checkStartStopStart(false); + } + @Test public void testClatdCrashWhileRunning() throws Exception { Nat464Xlat nat = makeNat464Xlat(); @@ -184,9 +272,9 @@ public class Nat464XlatTest { nat.interfaceRemoved(STACKED_IFACE); mLooper.dispatchNext(); - verify(mNms).unregisterObserver(eq(nat)); verify(mNetd).clatdStop(eq(BASE_IFACE)); verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture()); + verify(mNms).unregisterObserver(eq(nat)); assertTrue(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); assertIdle(nat); @@ -210,8 +298,8 @@ public class Nat464XlatTest { // ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...) nat.stop(); - verify(mNms).unregisterObserver(eq(nat)); verify(mNetd).clatdStop(eq(BASE_IFACE)); + verify(mNms).unregisterObserver(eq(nat)); assertIdle(nat); // In-flight interface up notification arrives: no-op @@ -241,8 +329,8 @@ public class Nat464XlatTest { // ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...) nat.stop(); - verify(mNms).unregisterObserver(eq(nat)); verify(mNetd).clatdStop(eq(BASE_IFACE)); + verify(mNms).unregisterObserver(eq(nat)); assertIdle(nat); verifyNoMoreInteractions(mNetd, mNms, mConnectivity);