diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 17292b4490..adf536bbf4 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2011,7 +2011,16 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: { - handleUpdateLinkProperties(nai, (LinkProperties) msg.obj); + 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); break; } case NetworkAgent.EVENT_NETWORK_INFO_CHANGED: { @@ -2260,7 +2269,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } nai.networkMonitor.sendMessage(NetworkMonitor.CMD_NETWORK_DISCONNECTED); mNetworkAgentInfos.remove(msg.replyTo); - nai.maybeStopClat(); + maybeStopClat(nai); synchronized (mNetworkForNetId) { // Remove the NetworkAgent, but don't mark the netId as // available until we've told netd to delete it below. @@ -4374,7 +4383,7 @@ public class ConnectivityService extends IConnectivityManager.Stub updateDnses(newLp, oldLp, netId); // Start or stop clat accordingly to network state. - networkAgent.updateClat(mNetd); + updateClat(networkAgent); if (isDefaultNetwork(networkAgent)) { handleApplyDefaultProxy(newLp.getHttpProxy()); } else { @@ -4389,6 +4398,32 @@ 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. @@ -4623,21 +4658,6 @@ 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 10c8b8b1e0..f8d23d4c8d 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -20,21 +20,22 @@ 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.internal.util.ArrayUtils; 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. 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. + * Class to manage a 464xlat CLAT daemon. * * @hide */ @@ -54,6 +55,9 @@ 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; @@ -63,12 +67,16 @@ 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 State mState = State.IDLE; + private volatile State mState = State.IDLE; - public Nat464Xlat(INetworkManagementService nmService, NetworkAgentInfo nai) { + public Nat464Xlat(INetworkManagementService nmService, Handler handler, NetworkAgentInfo nai) { mNMService = nmService; + mHandler = handler; mNetwork = nai; } @@ -96,13 +104,6 @@ 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. */ @@ -120,7 +121,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Clears internal state. + * Clears internal state. Must not be called by ConnectivityService. */ private void enterIdleState() { mIface = null; @@ -129,7 +130,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Starts the clat daemon. + * Starts the clat daemon. Called by ConnectivityService on the handler thread. */ public void start() { if (isStarted()) { @@ -166,7 +167,7 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Stops the clat daemon. + * Stops the clat daemon. Called by ConnectivityService on the handler thread. */ public void stop() { if (!isStarted()) { @@ -180,8 +181,15 @@ 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, handleInterfaceRemoved() will trigger - // ConnectivityService#handleUpdateLinkProperties and call enterIdleState(). + // 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(); } /** @@ -249,15 +257,19 @@ public class Nat464Xlat extends BaseNetworkObserver { } /** - * Adds stacked link on base link and transitions to RUNNING state. + * 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(). */ - private void handleInterfaceLinkStateChanged(String iface, boolean up) { - if (!isStarting() || !up || !Objects.equals(mIface, iface)) { + @Override + public void interfaceLinkStateChanged(String iface, boolean up) { + if (!isStarted() || !up || !Objects.equals(mIface, iface)) { + return; + } + if (isRunning()) { return; } LinkAddress clatAddress = getLinkAddress(iface); if (clatAddress == null) { - Slog.e(TAG, "cladAddress was null for stacked iface " + iface); return; } mState = State.RUNNING; @@ -267,14 +279,15 @@ public class Nat464Xlat extends BaseNetworkObserver { maybeSetIpv6NdOffload(mBaseIface, false); LinkProperties lp = new LinkProperties(mNetwork.linkProperties); lp.addStackedLink(makeLinkProperties(clatAddress)); - mNetwork.connService.handleUpdateLinkProperties(mNetwork, lp); + updateConnectivityService(lp); } - /** - * Removes stacked link on base link and transitions to IDLE state. - */ - private void handleInterfaceRemoved(String iface) { - if (!isRunning() || !Objects.equals(mIface, iface)) { + @Override + public void interfaceRemoved(String iface) { + if (!isStarted() || !Objects.equals(mIface, iface)) { + return; + } + if (!isRunning()) { return; } @@ -282,28 +295,21 @@ 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) { - Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e); + } catch (RemoteException|IllegalStateException e) { + // Well, we tried. } maybeSetIpv6NdOffload(mBaseIface, true); LinkProperties lp = new LinkProperties(mNetwork.linkProperties); lp.removeStackedLink(mIface); enterIdleState(); - 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); }); + updateConnectivityService(lp); } @Override diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 7c4ef0f0f3..872923a032 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -27,9 +27,7 @@ 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; @@ -249,9 +247,9 @@ public class NetworkAgentInfo implements Comparable { private static final String TAG = ConnectivityService.class.getSimpleName(); private static final boolean VDBG = false; - public final ConnectivityService connService; + private final ConnectivityService mConnService; private final Context mContext; - final Handler handler; + private final Handler mHandler; public NetworkAgentInfo(Messenger messenger, AsyncChannel ac, Network net, NetworkInfo info, LinkProperties lp, NetworkCapabilities nc, int score, Context context, Handler handler, @@ -263,10 +261,10 @@ public class NetworkAgentInfo implements Comparable { linkProperties = lp; networkCapabilities = nc; currentScore = score; - this.connService = connService; + mConnService = connService; mContext = context; - this.handler = handler; - networkMonitor = connService.createNetworkMonitor(context, handler, this, defaultRequest); + mHandler = handler; + networkMonitor = mConnService.createNetworkMonitor(context, handler, this, defaultRequest); networkMisc = misc; } @@ -432,7 +430,7 @@ public class NetworkAgentInfo implements Comparable { private boolean ignoreWifiUnvalidationPenalty() { boolean isWifi = networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) && networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET); - boolean avoidBadWifi = connService.avoidBadWifi() || avoidUnvalidated; + boolean avoidBadWifi = mConnService.avoidBadWifi() || avoidUnvalidated; return isWifi && !avoidBadWifi && everValidated; } @@ -516,8 +514,8 @@ public class NetworkAgentInfo implements Comparable { } if (newExpiry > 0) { - mLingerMessage = connService.makeWakeupMessage( - mContext, handler, + mLingerMessage = mConnService.makeWakeupMessage( + mContext, mHandler, "NETWORK_LINGER_COMPLETE." + network.netId, EVENT_NETWORK_LINGER_COMPLETE, this); mLingerMessage.schedule(newExpiry); @@ -553,32 +551,6 @@ 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() + "} " +