diff options
4 files changed, 396 insertions, 118 deletions
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4546bfbfbce2..04b2112fae03 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2024,16 +2024,7 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: { - if (VDBG) { - 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); + handleUpdateLinkProperties(nai, (LinkProperties) msg.obj); break; } case NetworkAgent.EVENT_NETWORK_INFO_CHANGED: { @@ -2282,7 +2273,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. @@ -4402,7 +4393,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 { @@ -4417,32 +4408,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. @@ -4677,6 +4642,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); diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index f8d23d4c8d51..e6585ad194ec 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/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 = @@ -105,6 +101,13 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** + * @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. */ public boolean isRunning() { @@ -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) { - Message msg = mHandler.obtainMessage(NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED, lp); - msg.replyTo = mNetwork.messenger; - Slog.i(TAG, "sending message to ConnectivityService: " + msg); - msg.sendToTarget(); + boolean wasStarting = isStarting(); + enterStoppingState(); + if (wasStarting) { + enterIdleState(); + } } /** @@ -257,59 +289,58 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * 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(). + * Adds stacked link on base link and transitions to RUNNING state. */ - @Override - public void interfaceLinkStateChanged(String iface, boolean up) { - if (!isStarted() || !up || !Objects.equals(mIface, iface)) { - return; - } - if (isRunning()) { + private void handleInterfaceLinkStateChanged(String iface, boolean up) { + if (!isStarting() || !up || !Objects.equals(mIface, iface)) { 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)); - - maybeSetIpv6NdOffload(mBaseIface, false); + enterRunningState(); LinkProperties lp = new LinkProperties(mNetwork.linkProperties); lp.addStackedLink(makeLinkProperties(clatAddress)); - updateConnectivityService(lp); + mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp); } - @Override - public void interfaceRemoved(String iface) { - if (!isStarted() || !Objects.equals(mIface, iface)) { + /** + * Removes stacked link on base link and transitions to IDLE state. + */ + 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, - // because otherwise when we try to start it again on the same base interface netd - // will complain that it's already started. - // - // 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. + if (!isStopping()) { + // Ensure clatd is stopped if stop() has not been called: this likely means that clatd + // has crashed. + enterStoppingState(); } - 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 diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 872923a03256..e96f4b0383cb 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/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() + "} " + diff --git a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java new file mode 100644 index 000000000000..e3f46a40e2b1 --- /dev/null +++ b/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()); + } +} |