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);