Merge "Revert "Nat464Xlat: interface notification handler on ConnectivityService"" into oc-mr1-dev

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);

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
+ * Class to manage a 464xlat CLAT daemon.
- * from a consistent and unique thread context. It is the responsability of ConnectivityService to
- * call into this class from its own Handler thread.
  *
  * @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
+        // When clatd stops and its interface is deleted, interfaceRemoved() will notify
-        // ConnectivityService#handleUpdateLinkProperties and call enterIdleState().
+        // 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) {
+    @Override
-        if (!isStarting() || !up || !Objects.equals(mIface, iface)) {
+    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);
     }
 
-    /**
+    @Override
-     * Removes stacked link on base link and transitions to IDLE state.
+    public void interfaceRemoved(String iface) {
-     */
+        if (!isStarted() || !Objects.equals(mIface, iface)) {
-    private void handleInterfaceRemoved(String iface) {
+            return;
-        if (!isRunning() || !Objects.equals(mIface, iface)) {
+        }
+        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);
+            // Well, we tried.
         }
         maybeSetIpv6NdOffload(mBaseIface, true);
         LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
         lp.removeStackedLink(mIface);
         enterIdleState();
-        mNetwork.connService.handleUpdateLinkProperties(mNetwork, lp);
+        updateConnectivityService(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

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<NetworkAgentInfo> {
 
     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<NetworkAgentInfo> {
         linkProperties = lp;
         networkCapabilities = nc;
         currentScore = score;
-        this.connService = connService;
+        mConnService = connService;
         mContext = context;
-        this.handler = handler;
+        mHandler = handler;
-        networkMonitor = connService.createNetworkMonitor(context, handler, this, defaultRequest);
+        networkMonitor = mConnService.createNetworkMonitor(context, handler, this, defaultRequest);
         networkMisc = misc;
     }
 
@@ -432,7 +430,7 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
     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<NetworkAgentInfo> {
         }
 
         if (newExpiry > 0) {
-            mLingerMessage = connService.makeWakeupMessage(
+            mLingerMessage = mConnService.makeWakeupMessage(
-                    mContext, handler,
+                    mContext, mHandler,
                     "NETWORK_LINGER_COMPLETE." + network.netId,
                     EVENT_NETWORK_LINGER_COMPLETE, this);
             mLingerMessage.schedule(newExpiry);
@@ -553,32 +551,6 @@ 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() + "}  " +