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
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index adf536b..17292b4 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -2011,16 +2011,7 @@
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 @@
}
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 @@
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 @@
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 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 f8d23d4..10c8b8b 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.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 @@
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 @@
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;
}
@@ -105,6 +97,13 @@
}
/**
+ * @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() {
@@ -121,7 +120,7 @@
}
/**
- * Clears internal state. Must not be called by ConnectivityService.
+ * Clears internal state.
*/
private void enterIdleState() {
mIface = null;
@@ -130,7 +129,7 @@
}
/**
- * Starts the clat daemon. Called by ConnectivityService on the handler thread.
+ * Starts the clat daemon.
*/
public void start() {
if (isStarted()) {
@@ -167,7 +166,7 @@
}
/**
- * Stops the clat daemon. Called by ConnectivityService on the handler thread.
+ * Stops the clat daemon.
*/
public void stop() {
if (!isStarted()) {
@@ -181,15 +180,8 @@
} 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 @@
}
/**
- * 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 @@
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 @@
// 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 872923a..7c4ef0f 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.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 @@
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 @@
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 @@
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 @@
}
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 @@
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() + "} " +