From 5a5c1bb116caddf65065007c8f36ca7516882504 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 27 Jun 2017 15:13:20 +0900 Subject: [PATCH 1/3] Nat464Xlat: clat management cleanup This patch does some minor refactoring of clat starting/stopping code: - remove unused LinkProperties arguments in updateClat - remove unused Context argument in Nat464Xlat ctor - introduce ensureClatIsStarted and ensureClatIsStopped methods and simplify updateClat - add clatd to NetworkAgentInfo toString() method - clarify some comments This changes prepare for moving BaseNetworkObserver callbacks to ConnectivityService. Bug: 62997041 Bug: 64571917 Test: runtest frameworks-net manually connected to IPv6 only network and went to test-ipv6.com Merged-In: Idb204784614cfe700f73255a7a7b78c5e9ee6eca Merged-In: Ic3808a1afe48efac745b1b378fb12cc5678918ec Merged-In: Ia769aef6ef8b258f44f8979003d271c96264f1b5 Merged-In: I1a19e6fbb0cb13262e90b171d861062469078fb6 Merged-In: I06661bd6bd1456ba34a3bbdb52c120ac01da9d61 Merged-In: Ifccff9f3cfccdb2cdddf2f07561f0787a48bc0f8 (cherry picked from commit b577d65825e623a9868664486482ed137b98b504) Change-Id: Ibb02888633df9643030336c4dbea6c569a47554c --- .../android/server/ConnectivityService.java | 37 +++++++++++++------ .../server/connectivity/Nat464Xlat.java | 29 +++++++++------ .../server/connectivity/NetworkAgentInfo.java | 4 +- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index ec83a03f8a..adf536bbf4 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2269,7 +2269,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } nai.networkMonitor.sendMessage(NetworkMonitor.CMD_NETWORK_DISCONNECTED); mNetworkAgentInfos.remove(msg.replyTo); - updateClat(null, nai.linkProperties, nai); + maybeStopClat(nai); synchronized (mNetworkForNetId) { // Remove the NetworkAgent, but don't mark the netId as // available until we've told netd to delete it below. @@ -4382,7 +4382,8 @@ public class ConnectivityService extends IConnectivityManager.Stub updateRoutes(newLp, oldLp, netId); updateDnses(newLp, oldLp, netId); - updateClat(newLp, oldLp, networkAgent); + // Start or stop clat accordingly to network state. + updateClat(networkAgent); if (isDefaultNetwork(networkAgent)) { handleApplyDefaultProxy(newLp.getHttpProxy()); } else { @@ -4397,18 +4398,32 @@ public class ConnectivityService extends IConnectivityManager.Stub mKeepaliveTracker.handleCheckKeepalivesStillValid(networkAgent); } - private void updateClat(LinkProperties newLp, LinkProperties oldLp, NetworkAgentInfo nai) { - final boolean wasRunningClat = nai.clatd != null && nai.clatd.isStarted(); - final boolean shouldRunClat = Nat464Xlat.requiresClat(nai); - - if (!wasRunningClat && shouldRunClat) { - nai.clatd = new Nat464Xlat(mContext, mNetd, mTrackerHandler, nai); - nai.clatd.start(); - } else if (wasRunningClat && !shouldRunClat) { - nai.clatd.stop(); + 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. diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index b390884713..27426d7bde 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -18,7 +18,6 @@ package com.android.server.connectivity; import java.net.Inet4Address; -import android.content.Context; import android.net.InterfaceConfiguration; import android.net.ConnectivityManager; import android.net.LinkAddress; @@ -35,17 +34,18 @@ import com.android.server.net.BaseNetworkObserver; import com.android.internal.util.ArrayUtils; /** - * @hide - * * Class to manage a 464xlat CLAT daemon. + * + * @hide */ public class Nat464Xlat extends BaseNetworkObserver { - private static final String TAG = "Nat464Xlat"; + private static final String TAG = Nat464Xlat.class.getSimpleName(); // This must match the interface prefix in clatd.c. private static final String CLAT_PREFIX = "v4-"; - // The network types we will start clatd on. + // The network types we will start clatd on, + // allowing clat only on networks for which we can support IPv6-only. private static final int[] NETWORK_TYPES = { ConnectivityManager.TYPE_MOBILE, ConnectivityManager.TYPE_WIFI, @@ -76,9 +76,7 @@ public class Nat464Xlat extends BaseNetworkObserver { private String mIface; private boolean mIsRunning; - public Nat464Xlat( - Context context, INetworkManagementService nmService, - Handler handler, NetworkAgentInfo nai) { + public Nat464Xlat(INetworkManagementService nmService, Handler handler, NetworkAgentInfo nai) { mNMService = nmService; mHandler = handler; mNetwork = nai; @@ -90,13 +88,14 @@ public class Nat464Xlat extends BaseNetworkObserver { * @return true if the network requires clat, false otherwise. */ public static boolean requiresClat(NetworkAgentInfo nai) { + // TODO: migrate to NetworkCapabilities.TRANSPORT_*. final int netType = nai.networkInfo.getType(); + final boolean supported = ArrayUtils.contains(NETWORK_TYPES, nai.networkInfo.getType()); final boolean connected = nai.networkInfo.isConnected(); + // We only run clat on networks that don't have a native IPv4 address. final boolean hasIPv4Address = - (nai.linkProperties != null) ? nai.linkProperties.hasIPv4Address() : false; - // Only support clat on mobile and wifi for now, because these are the only IPv6-only - // networks we can connect to. - return connected && !hasIPv4Address && ArrayUtils.contains(NETWORK_TYPES, netType); + (nai.linkProperties != null) && nai.linkProperties.hasIPv4Address(); + return supported && connected && !hasIPv4Address; } /** @@ -227,6 +226,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } private void maybeSetIpv6NdOffload(String iface, boolean on) { + // TODO: migrate to NetworkCapabilities.TRANSPORT_*. if (mNetwork.networkInfo.getType() != ConnectivityManager.TYPE_WIFI) { return; } @@ -286,4 +286,9 @@ public class Nat464Xlat extends BaseNetworkObserver { } } } + + @Override + public String toString() { + return "mBaseIface: " + mBaseIface + ", mIface: " + mIface + ", mIsRunning: " + mIsRunning; + } } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 2a618bcc2e..872923a032 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -562,13 +562,13 @@ public class NetworkAgentInfo implements Comparable { "acceptUnvalidated{" + networkMisc.acceptUnvalidated + "} " + "everCaptivePortalDetected{" + everCaptivePortalDetected + "} " + "lastCaptivePortalDetected{" + lastCaptivePortalDetected + "} " + + "clat{" + clatd + "} " + "}"; } public String name() { return "NetworkAgentInfo [" + networkInfo.getTypeName() + " (" + - networkInfo.getSubtypeName() + ") - " + - (network == null ? "null" : network.toString()) + "]"; + networkInfo.getSubtypeName() + ") - " + Objects.toString(network) + "]"; } // Enables sorting in descending order of score. From 9cd15c7f2eb0315b155a69cb3fc0ebf2e3474985 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Thu, 29 Jun 2017 14:04:13 +0900 Subject: [PATCH 2/3] Nat464Xlat: internal state guards cleanup + state enum This patch does some cleanup of Nat464Xlat internal state guards against the Nat464Xlat state Idle | Started | Running, which reduces code nesting. It also replaces introspection of internal state for distinguishing between different stages in 464xlat lifecycle with an enum explicitly introducing these three Idle | Started | Running states. Bug: 62997041 Bug: 64571917 Test: runtest frameworks-net manually connected to ipv6 network and went to test-ipv6.com Merged-In: I6efc9fed2420ca488731a2b9b9c3c025b16eca10 Merged-In: I188ac4c367db11cb33b67fe92df3a120e3c6fbce Merged-In: I7e2c5db8d537fb0ab47cde37158b7f04d7786942 Merged-In: Ic2246a97618c596dbdbf48cda39c2f5b66e3bfb6 Merged-In: Ib04b9a3d56e9daf61b299a30e24a3c8839819a00 Merged-In: Icc1558a0f0e7c299270f550897347458e2bd3188 (cherry pick from commit 4f6f139869ddadf6f9ed50967c106a10a2e8ce3f) Change-Id: Iebc7d153d8cd0b90d074d8d6eed821fbc3f1370d --- .../server/connectivity/Nat464Xlat.java | 191 ++++++++++-------- 1 file changed, 108 insertions(+), 83 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index 27426d7bde..f8d23d4c8d 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -16,8 +16,6 @@ package com.android.server.connectivity; -import java.net.Inet4Address; - import android.net.InterfaceConfiguration; import android.net.ConnectivityManager; import android.net.LinkAddress; @@ -33,6 +31,9 @@ import android.util.Slog; import com.android.server.net.BaseNetworkObserver; import com.android.internal.util.ArrayUtils; +import java.net.Inet4Address; +import java.util.Objects; + /** * Class to manage a 464xlat CLAT daemon. * @@ -60,21 +61,18 @@ public class Nat464Xlat extends BaseNetworkObserver { // The network we're running on, and its type. private final NetworkAgentInfo mNetwork; - // Internal state variables. - // - // The possible states are: - // - Idle: start() not called. Everything is null. - // - Starting: start() called. Interfaces are non-null. isStarted() returns true. - // mIsRunning is false. - // - Running: start() called, and interfaceLinkStateChanged() told us that mIface is up. - // mIsRunning is true. - // + 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. + } + // 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 boolean mIsRunning; + private volatile State mState = State.IDLE; public Nat464Xlat(INetworkManagementService nmService, Handler handler, NetworkAgentInfo nai) { mNMService = nmService; @@ -99,20 +97,36 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Determines whether clatd is started. Always true, except a) if start has not yet been called, - * or b) if our interface was removed. + * @return true if clatd has been started and has not yet stopped. + * A true result corresponds to internal states STARTING and RUNNING. */ public boolean isStarted() { - return mIface != null; + return mState != State.IDLE; + } + + /** + * @return true if clatd has been started and the stacked interface is up. + */ + public boolean isRunning() { + return mState == State.RUNNING; + } + + /** + * Sets internal state. + */ + private void enterStartingState(String baseIface) { + mIface = CLAT_PREFIX + baseIface; + mBaseIface = baseIface; + mState = State.STARTING; } /** * Clears internal state. Must not be called by ConnectivityService. */ - private void clear() { + private void enterIdleState() { mIface = null; mBaseIface = null; - mIsRunning = false; + mState = State.IDLE; } /** @@ -136,19 +150,19 @@ public class Nat464Xlat extends BaseNetworkObserver { return; } - mBaseIface = mNetwork.linkProperties.getInterfaceName(); - if (mBaseIface == null) { + String baseIface = mNetwork.linkProperties.getInterfaceName(); + if (baseIface == null) { Slog.e(TAG, "startClat: Can't start clat on null interface"); return; } - mIface = CLAT_PREFIX + mBaseIface; - // From now on, isStarted() will return true. + // TODO: should we only do this if mNMService.startClatd() succeeds? + enterStartingState(baseIface); Slog.i(TAG, "Starting clatd on " + mBaseIface); try { mNMService.startClatd(mBaseIface); } catch(RemoteException|IllegalStateException e) { - Slog.e(TAG, "Error starting clatd: " + e); + Slog.e(TAG, "Error starting clatd on " + mBaseIface, e); } } @@ -156,18 +170,19 @@ public class Nat464Xlat extends BaseNetworkObserver { * Stops the clat daemon. Called by ConnectivityService on the handler thread. */ public void stop() { - if (isStarted()) { - Slog.i(TAG, "Stopping clatd"); - try { - mNMService.stopClatd(mBaseIface); - } catch(RemoteException|IllegalStateException e) { - Slog.e(TAG, "Error stopping clatd: " + e); - } - // When clatd stops and its interface is deleted, interfaceRemoved() will notify - // ConnectivityService and call clear(). - } else { - Slog.e(TAG, "clatd: already stopped"); + 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) { @@ -183,16 +198,19 @@ public class Nat464Xlat extends BaseNetworkObserver { * has no idea that 464xlat is running on top of it. */ public void fixupLinkProperties(LinkProperties oldLp) { - if (mNetwork.clatd != null && - mIsRunning && - mNetwork.linkProperties != null && - !mNetwork.linkProperties.getAllInterfaceNames().contains(mIface)) { - Slog.d(TAG, "clatd running, updating NAI for " + mIface); - for (LinkProperties stacked: oldLp.getStackedLinks()) { - if (mIface.equals(stacked.getInterfaceName())) { - mNetwork.linkProperties.addStackedLink(stacked); - break; - } + if (!isRunning()) { + return; + } + LinkProperties lp = mNetwork.linkProperties; + if (lp == null || lp.getAllInterfaceNames().contains(mIface)) { + return; + } + + Slog.d(TAG, "clatd running, updating NAI for " + mIface); + for (LinkProperties stacked: oldLp.getStackedLinks()) { + if (Objects.equals(mIface, stacked.getInterfaceName())) { + lp.addStackedLink(stacked); + return; } } } @@ -238,57 +256,64 @@ 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(). + */ @Override public void interfaceLinkStateChanged(String iface, boolean up) { - // Called by the InterfaceObserver on its own thread, so can race with stop(). - if (isStarted() && up && mIface.equals(iface)) { - Slog.i(TAG, "interface " + iface + " is up, mIsRunning " + mIsRunning + "->true"); - - if (!mIsRunning) { - LinkAddress clatAddress = getLinkAddress(iface); - if (clatAddress == null) { - return; - } - mIsRunning = true; - maybeSetIpv6NdOffload(mBaseIface, false); - LinkProperties lp = new LinkProperties(mNetwork.linkProperties); - lp.addStackedLink(makeLinkProperties(clatAddress)); - Slog.i(TAG, "Adding stacked link " + mIface + " on top of " + mBaseIface); - updateConnectivityService(lp); - } + if (!isStarted() || !up || !Objects.equals(mIface, iface)) { + return; } + if (isRunning()) { + return; + } + LinkAddress clatAddress = getLinkAddress(iface); + if (clatAddress == null) { + 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); + LinkProperties lp = new LinkProperties(mNetwork.linkProperties); + lp.addStackedLink(makeLinkProperties(clatAddress)); + updateConnectivityService(lp); } @Override public void interfaceRemoved(String iface) { - if (isStarted() && mIface.equals(iface)) { - Slog.i(TAG, "interface " + iface + " removed, mIsRunning " + mIsRunning + "->false"); - - if (mIsRunning) { - // 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. - } - maybeSetIpv6NdOffload(mBaseIface, true); - LinkProperties lp = new LinkProperties(mNetwork.linkProperties); - lp.removeStackedLink(mIface); - clear(); - updateConnectivityService(lp); - } + if (!isStarted() || !Objects.equals(mIface, iface)) { + return; } + if (!isRunning()) { + 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. + } + maybeSetIpv6NdOffload(mBaseIface, true); + LinkProperties lp = new LinkProperties(mNetwork.linkProperties); + lp.removeStackedLink(mIface); + enterIdleState(); + updateConnectivityService(lp); } @Override public String toString() { - return "mBaseIface: " + mBaseIface + ", mIface: " + mIface + ", mIsRunning: " + mIsRunning; + return "mBaseIface: " + mBaseIface + ", mIface: " + mIface + ", mState: " + mState; } } From 39e10e211184a11fcb61e49d33b850597ff943d3 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 5 Jul 2017 11:08:48 +0900 Subject: [PATCH 3/3] Nat464Xlat: interface notification handler on ConnectivityService This patch adds a layer of asynchonicity to the NetworkBaseObserver callbacks implemented by Nat464Xlat in order to allow these callbacks to run on the main ConnectivityService handler. This allows to run interfaceLinkStateChanged and interfaceRemoved callbacks in the same thread context as other Nat464Xlat methods and solves the following issues: - NPE risk due to race between fixupLinkProperties called on the ConnectivityService thread and interfaceRemoved called as a callback by NetworkManagementService. - stale LinkProperties reads in both callbacks not called on ConnectivityService handler. - removes the race between stop() and interfaceRemoved(). This patch also: - removes/simplifies comments related to the threading model which are no obsolete. - extract clatd management logic from ConnectivityService into NetworkAgentInfo Bug: 62997041 Bug: 64571917 Test: runtest frameworks-net manually connected to ipv6 network and went to test-ipv6.com Merged-In: I889d98e47423ff3d4746d6ed8015b265286e7c52 Merged-In: I2f002cd197e2eeaaadadd747a6b33d264cd34433 Merged-In: Id3ab064cf9f4417c0e8988fff4167b65b3c8c414 Merged-In: Ib224392c9a185f6bd79fd60cd5cb5549f2a7851e Merged-In: I9116a493ca1cbdf6a25664a1b0017aa6c9b38eb4 Merged-In: I12918d208364eef55067ae9d59fbc38477e1f1c6 (cherry picked from commit 771d5c2f0126ba692897c9716f4098ae6e3a870c) Change-Id: I34c4a0c32ce7c9b7bd7acf8f87b932e15c573bd8 --- .../android/server/ConnectivityService.java | 56 ++++-------- .../server/connectivity/Nat464Xlat.java | 90 +++++++++---------- .../server/connectivity/NetworkAgentInfo.java | 44 +++++++-- 3 files changed, 96 insertions(+), 94 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index adf536bbf4..17292b4490 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2011,16 +2011,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: { @@ -2269,7 +2260,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. @@ -4383,7 +4374,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 { @@ -4398,32 +4389,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. @@ -4658,6 +4623,21 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) { + 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 f8d23d4c8d..10c8b8b1e0 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 responsability of ConnectivityService to + * call into this class from its own Handler thread. * * @hide */ @@ -55,9 +54,6 @@ 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; @@ -67,16 +63,12 @@ public class Nat464Xlat extends BaseNetworkObserver { RUNNING; // start() called, and the stacked iface is known to be up. } - // 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; } @@ -104,6 +96,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. */ @@ -121,7 +120,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Clears internal state. Must not be called by ConnectivityService. + * Clears internal state. */ private void enterIdleState() { mIface = null; @@ -130,7 +129,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Starts the clat daemon. Called by ConnectivityService on the handler thread. + * Starts the clat daemon. */ public void start() { if (isStarted()) { @@ -167,7 +166,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Stops the clat daemon. Called by ConnectivityService on the handler thread. + * Stops the clat daemon. */ public void stop() { if (!isStarted()) { @@ -181,15 +180,8 @@ public class Nat464Xlat extends BaseNetworkObserver { } 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(); + // When clatd stops and its interface is deleted, handleInterfaceRemoved() will trigger + // ConnectivityService#handleUpdateLinkProperties and call enterIdleState(). } /** @@ -257,19 +249,15 @@ 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, "cladAddress was null for stacked iface " + iface); return; } mState = State.RUNNING; @@ -279,15 +267,14 @@ public class Nat464Xlat extends BaseNetworkObserver { 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) { - if (!isStarted() || !Objects.equals(mIface, iface)) { - return; - } - if (!isRunning()) { + /** + * Removes stacked link on base link and transitions to IDLE state. + */ + private void handleInterfaceRemoved(String iface) { + if (!isRunning() || !Objects.equals(mIface, iface)) { return; } @@ -295,21 +282,28 @@ public class Nat464Xlat extends BaseNetworkObserver { // 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); + // TODO: add STOPPING state to avoid calling stopClatd twice. mNMService.stopClatd(mBaseIface); - } catch (RemoteException|IllegalStateException e) { - // Well, we tried. + } catch(RemoteException|IllegalStateException e) { + Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e); } maybeSetIpv6NdOffload(mBaseIface, true); LinkProperties lp = new LinkProperties(mNetwork.linkProperties); lp.removeStackedLink(mIface); enterIdleState(); - updateConnectivityService(lp); + 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 872923a032..7c4ef0f0f3 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; @@ -247,9 +249,9 @@ public class NetworkAgentInfo implements Comparable { private static final String TAG = ConnectivityService.class.getSimpleName(); private static final boolean VDBG = false; - private final ConnectivityService mConnService; + public final ConnectivityService connService; private final Context mContext; - private final Handler mHandler; + final Handler handler; public NetworkAgentInfo(Messenger messenger, AsyncChannel ac, Network net, NetworkInfo info, LinkProperties lp, NetworkCapabilities nc, int score, Context context, Handler handler, @@ -261,10 +263,10 @@ public class NetworkAgentInfo implements Comparable { linkProperties = lp; networkCapabilities = nc; currentScore = score; - mConnService = connService; + this.connService = connService; mContext = context; - mHandler = handler; - networkMonitor = mConnService.createNetworkMonitor(context, handler, this, defaultRequest); + this.handler = handler; + networkMonitor = connService.createNetworkMonitor(context, handler, this, defaultRequest); networkMisc = misc; } @@ -430,7 +432,7 @@ public class NetworkAgentInfo implements Comparable { private boolean ignoreWifiUnvalidationPenalty() { boolean isWifi = networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) && networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET); - boolean avoidBadWifi = mConnService.avoidBadWifi() || avoidUnvalidated; + boolean avoidBadWifi = connService.avoidBadWifi() || avoidUnvalidated; return isWifi && !avoidBadWifi && everValidated; } @@ -514,8 +516,8 @@ public class NetworkAgentInfo implements Comparable { } if (newExpiry > 0) { - mLingerMessage = mConnService.makeWakeupMessage( - mContext, mHandler, + mLingerMessage = connService.makeWakeupMessage( + mContext, handler, "NETWORK_LINGER_COMPLETE." + network.netId, EVENT_NETWORK_LINGER_COMPLETE, this); mLingerMessage.schedule(newExpiry); @@ -551,6 +553,32 @@ public class NetworkAgentInfo implements Comparable { 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() + "} " +