From b2d70b403fdab28878cfd172b367b26c09c41933 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 5 Jul 2017 11:08:48 +0900 Subject: [PATCH] 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 Change-Id: I889d98e47423ff3d4746d6ed8015b265286e7c52 --- .../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 aa878efe61..fb20533837 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/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) { - 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: { @@ -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,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() + "} " +