Merge changes I2d30b75c,I57ecf4f5

* changes:
  Switch to using list of subIds for policy generation
  Switch to using list-of-subIds for VCN selection of underlying networks

services/core/java/com/android/server/VcnManagementService.java

@@ -36,7 +36,6 @@ import android.content.IntentFilter;
 import android.net.ConnectivityManager;
 import android.net.LinkProperties;
 import android.net.NetworkCapabilities;
-import android.net.TelephonyNetworkSpecifier;
 import android.net.vcn.IVcnManagementService;
 import android.net.vcn.IVcnStatusCallback;
 import android.net.vcn.IVcnUnderlyingNetworkPolicyListener;
@@ -717,19 +716,29 @@ public class VcnManagementService extends IVcnManagementService.Stub {
         });
     }
 
-    private int getSubIdForNetworkCapabilities(@NonNull NetworkCapabilities networkCapabilities) {
+    private ParcelUuid getSubGroupForNetworkCapabilities(
-        if (networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)
+            @NonNull NetworkCapabilities networkCapabilities) {
-                && networkCapabilities.getNetworkSpecifier() instanceof TelephonyNetworkSpecifier) {
+        ParcelUuid subGrp = null;
-            TelephonyNetworkSpecifier telephonyNetworkSpecifier =
+        final TelephonySubscriptionSnapshot snapshot;
-                    (TelephonyNetworkSpecifier) networkCapabilities.getNetworkSpecifier();
+
-            return telephonyNetworkSpecifier.getSubscriptionId();
+        // Always access mLastSnapshot under lock. Technically this can be treated as a volatile
-        } else if (networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)
+        // but for consistency and safety, always access under lock.
-                && networkCapabilities.getTransportInfo() instanceof WifiInfo) {
+        synchronized (mLock) {
-            WifiInfo wifiInfo = (WifiInfo) networkCapabilities.getTransportInfo();
+            snapshot = mLastSnapshot;
-            return mDeps.getSubIdForWifiInfo(wifiInfo);
         }
 
-        return SubscriptionManager.INVALID_SUBSCRIPTION_ID;
+        // If multiple subscription IDs exist, they MUST all point to the same subscription
+        // group. Otherwise undefined behavior may occur.
+        for (int subId : networkCapabilities.getSubIds()) {
+            // Verify that all subscriptions point to the same group
+            if (subGrp != null && !subGrp.equals(snapshot.getGroupForSubId(subId))) {
+                Slog.wtf(TAG, "Got multiple subscription groups for a single network");
+            }
+
+            subGrp = snapshot.getGroupForSubId(subId);
+        }
+
+        return subGrp;
     }
 
     /**
@@ -754,23 +763,19 @@ public class VcnManagementService extends IVcnManagementService.Stub {
             // mutates
             final NetworkCapabilities ncCopy = new NetworkCapabilities(networkCapabilities);
 
-            final int subId = getSubIdForNetworkCapabilities(ncCopy);
+            final ParcelUuid subGrp = getSubGroupForNetworkCapabilities(ncCopy);
             boolean isVcnManagedNetwork = false;
             boolean isRestrictedCarrierWifi = false;
-            if (subId != SubscriptionManager.INVALID_SUBSCRIPTION_ID) {
+            synchronized (mLock) {
-                synchronized (mLock) {
+                final Vcn vcn = mVcns.get(subGrp);
-                    ParcelUuid subGroup = mLastSnapshot.getGroupForSubId(subId);
+                if (vcn != null) {
+                    if (vcn.isActive()) {
+                        isVcnManagedNetwork = true;
+                    }
 
-                    final Vcn vcn = mVcns.get(subGroup);
+                    if (ncCopy.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
-                    if (vcn != null) {
+                        // Carrier WiFi always restricted if VCN exists (even in safe mode).
-                        if (vcn.isActive()) {
+                        isRestrictedCarrierWifi = true;
-                            isVcnManagedNetwork = true;
-                        }
-
-                        if (ncCopy.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
-                            // Carrier WiFi always restricted if VCN exists (even in safe mode).
-                            isRestrictedCarrierWifi = true;
-                        }
                     }
                 }
             }

services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java

@@ -27,15 +27,14 @@ import android.net.NetworkRequest;
 import android.net.TelephonyNetworkSpecifier;
 import android.os.Handler;
 import android.os.ParcelUuid;
-import android.util.ArrayMap;
-import android.util.ArraySet;
 import android.util.Slog;
 
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.annotations.VisibleForTesting.Visibility;
 import com.android.server.vcn.TelephonySubscriptionTracker.TelephonySubscriptionSnapshot;
 
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 
@@ -59,9 +58,9 @@ public class UnderlyingNetworkTracker {
     @NonNull private final Handler mHandler;
     @NonNull private final ConnectivityManager mConnectivityManager;
 
-    @NonNull private final Map<Integer, NetworkCallback> mCellBringupCallbacks = new ArrayMap<>();
+    @NonNull private final List<NetworkCallback> mCellBringupCallbacks = new ArrayList<>();
-    @NonNull private final NetworkCallback mWifiBringupCallback = new NetworkBringupCallback();
+    @Nullable private NetworkCallback mWifiBringupCallback;
-    @NonNull private final NetworkCallback mRouteSelectionCallback = new RouteSelectionCallback();
+    @Nullable private NetworkCallback mRouteSelectionCallback;
 
     @NonNull private TelephonySubscriptionSnapshot mLastSnapshot;
     private boolean mIsQuitting = false;
@@ -105,36 +104,59 @@ public class UnderlyingNetworkTracker {
 
         mConnectivityManager = mVcnContext.getContext().getSystemService(ConnectivityManager.class);
 
-        registerNetworkRequests();
+        registerOrUpdateNetworkRequests();
     }
 
-    private void registerNetworkRequests() {
+    private void registerOrUpdateNetworkRequests() {
-        // register bringup requests for underlying Networks
+        NetworkCallback oldRouteSelectionCallback = mRouteSelectionCallback;
-        mConnectivityManager.requestBackgroundNetwork(
+        NetworkCallback oldWifiCallback = mWifiBringupCallback;
-                getWifiNetworkRequest(), mHandler, mWifiBringupCallback);
+        List<NetworkCallback> oldCellCallbacks = new ArrayList<>(mCellBringupCallbacks);
-        updateSubIdsAndCellularRequests();
+        mCellBringupCallbacks.clear();
 
-        // Register Network-selection request used to decide selected underlying Network. All
+        // Register new callbacks. Make-before-break; always register new callbacks before removal
-        // underlying networks must be VCN managed in order to be used.
+        // of old callbacks
-        mConnectivityManager.requestBackgroundNetwork(
+        if (!mIsQuitting) {
-                getBaseNetworkRequest(true /* requireVcnManaged */).build(),
+            mRouteSelectionCallback = new RouteSelectionCallback();
-                mHandler,
+            mConnectivityManager.requestBackgroundNetwork(
-                mRouteSelectionCallback);
+                    getBaseNetworkRequestBuilder().build(), mHandler, mRouteSelectionCallback);
+
+            mWifiBringupCallback = new NetworkBringupCallback();
+            mConnectivityManager.requestBackgroundNetwork(
+                    getWifiNetworkRequest(), mHandler, mWifiBringupCallback);
+
+            for (final int subId : mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup)) {
+                final NetworkBringupCallback cb = new NetworkBringupCallback();
+                mCellBringupCallbacks.add(cb);
+
+                mConnectivityManager.requestBackgroundNetwork(
+                        getCellNetworkRequestForSubId(subId), mHandler, cb);
+            }
+        } else {
+            mRouteSelectionCallback = null;
+            mWifiBringupCallback = null;
+            // mCellBringupCallbacks already cleared above.
+        }
+
+        // Unregister old callbacks (as necessary)
+        if (oldRouteSelectionCallback != null) {
+            mConnectivityManager.unregisterNetworkCallback(oldRouteSelectionCallback);
+        }
+        if (oldWifiCallback != null) {
+            mConnectivityManager.unregisterNetworkCallback(oldWifiCallback);
+        }
+        for (NetworkCallback cellBringupCallback : oldCellCallbacks) {
+            mConnectivityManager.unregisterNetworkCallback(cellBringupCallback);
+        }
     }
 
     private NetworkRequest getWifiNetworkRequest() {
-        // Request exclusively VCN managed networks to ensure that we only ever keep carrier wifi
+        return getBaseNetworkRequestBuilder()
-        // alive.
-        return getBaseNetworkRequest(true /* requireVcnManaged */)
                 .addTransportType(NetworkCapabilities.TRANSPORT_WIFI)
                 .build();
     }
 
     private NetworkRequest getCellNetworkRequestForSubId(int subId) {
-        // Do not request NOT_VCN_MANAGED to ensure that the TelephonyNetworkFactory has a
+        return getBaseNetworkRequestBuilder()
-        // fulfillable request to bring up underlying cellular Networks even if the VCN is already
-        // connected.
-        return getBaseNetworkRequest(false /* requireVcnManaged */)
                 .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR)
                 .setNetworkSpecifier(new TelephonyNetworkSpecifier(subId))
                 .build();
@@ -143,67 +165,19 @@ public class UnderlyingNetworkTracker {
     /**
      * Builds and returns a NetworkRequest builder common to all Underlying Network requests
      *
-     * <p>A NetworkRequest may either (1) Require the presence of a capability by using
+     * <p>This request is guaranteed to select carrier-owned, non-VCN underlying networks by virtue
-     * addCapability(), (2) require the absence of a capability using unwanted capabilities, or (3)
+     * of a populated set of subIds as expressed in NetworkCapabilities#getSubIds(). Only carrier
-     * allow any state. Underlying networks are never desired to have the NOT_VCN_MANAGED
+     * owned networks may be selected, as the request specifies only subIds in the VCN's
-     * capability, and only cases (2) and (3) are used.
+     * subscription group, while the VCN networks are excluded by virtue of not having subIds set on
-     *
+     * the VCN-exposed networks.
-     * @param requireVcnManaged whether the underlying network is required to be VCN managed to
-     *     match this request. If {@code true}, the NOT_VCN_MANAGED capability will be set as
-     *     unwanted. Else, the NOT_VCN_MANAGED capability will be removed, and any state is
-     *     acceptable.
      */
-    private NetworkRequest.Builder getBaseNetworkRequest(boolean requireVcnManaged) {
+    private NetworkRequest.Builder getBaseNetworkRequestBuilder() {
-        NetworkRequest.Builder requestBase =
+        return new NetworkRequest.Builder()
-                new NetworkRequest.Builder()
+                .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
-                        .removeCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED)
+                .removeCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED)
-                        .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED)
+                .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED)
-                        .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED);
+                .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED)
-
+                .setSubIds(mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup));
-        for (int capability : mRequiredUnderlyingNetworkCapabilities) {
-            requestBase.addCapability(capability);
-        }
-
-        if (requireVcnManaged) {
-            requestBase.addUnwantedCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED);
-        }
-
-        return requestBase;
-    }
-
-    /**
-     * Update the current subIds and Cellular bringup requests for this UnderlyingNetworkTracker.
-     */
-    private void updateSubIdsAndCellularRequests() {
-        mVcnContext.ensureRunningOnLooperThread();
-
-        // Don't bother re-filing NetworkRequests if this Tracker has been torn down.
-        if (mIsQuitting) {
-            return;
-        }
-
-        final Set<Integer> subIdsInSubGroup = mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup);
-
-        // new subIds to track = (updated list of subIds) - (currently tracked subIds)
-        final Set<Integer> subIdsToRegister = new ArraySet<>(subIdsInSubGroup);
-        subIdsToRegister.removeAll(mCellBringupCallbacks.keySet());
-
-        // subIds to stop tracking = (currently tracked subIds) - (updated list of subIds)
-        final Set<Integer> subIdsToUnregister = new ArraySet<>(mCellBringupCallbacks.keySet());
-        subIdsToUnregister.removeAll(subIdsInSubGroup);
-
-        for (final int subId : subIdsToRegister) {
-            final NetworkBringupCallback cb = new NetworkBringupCallback();
-            mCellBringupCallbacks.put(subId, cb);
-
-            mConnectivityManager.requestBackgroundNetwork(
-                    getCellNetworkRequestForSubId(subId), mHandler, cb);
-        }
-
-        for (final int subId : subIdsToUnregister) {
-            final NetworkCallback cb = mCellBringupCallbacks.remove(subId);
-            mConnectivityManager.unregisterNetworkCallback(cb);
-        }
     }
 
     /**
@@ -217,22 +191,16 @@ public class UnderlyingNetworkTracker {
         Objects.requireNonNull(snapshot, "Missing snapshot");
 
         mLastSnapshot = snapshot;
-        updateSubIdsAndCellularRequests();
+        registerOrUpdateNetworkRequests();
     }
 
     /** Tears down this Tracker, and releases all underlying network requests. */
     public void teardown() {
         mVcnContext.ensureRunningOnLooperThread();
-
-        mConnectivityManager.unregisterNetworkCallback(mWifiBringupCallback);
-        mConnectivityManager.unregisterNetworkCallback(mRouteSelectionCallback);
-
-        for (final NetworkCallback cb : mCellBringupCallbacks.values()) {
-            mConnectivityManager.unregisterNetworkCallback(cb);
-        }
-        mCellBringupCallbacks.clear();
-
         mIsQuitting = true;
+
+        // Will unregister all existing callbacks, but not register new ones due to quitting flag.
+        registerOrUpdateNetworkRequests();
     }
 
     /** Returns whether the currently selected Network matches the given network. */

tests/vcn/java/com/android/server/VcnManagementServiceTest.java

@@ -34,8 +34,8 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
-import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.argThat;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doNothing;
@@ -45,7 +45,6 @@ import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 
 import android.annotation.NonNull;
 import android.app.AppOpsManager;
@@ -63,7 +62,6 @@ import android.net.vcn.VcnConfig;
 import android.net.vcn.VcnConfigTest;
 import android.net.vcn.VcnManager;
 import android.net.vcn.VcnUnderlyingNetworkPolicy;
-import android.net.wifi.WifiInfo;
 import android.os.IBinder;
 import android.os.ParcelUuid;
 import android.os.PersistableBundle;
@@ -708,22 +706,12 @@ public class VcnManagementServiceTest {
             int subId, ParcelUuid subGrp, boolean isVcnActive, int transport) {
         setupSubscriptionAndStartVcn(subId, subGrp, isVcnActive);
 
-        final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder();
+        final NetworkCapabilities.Builder ncBuilder =
-        ncBuilder.addCapability(NET_CAPABILITY_NOT_VCN_MANAGED);
+                new NetworkCapabilities.Builder()
-        if (transport == TRANSPORT_CELLULAR) {
+                        .addCapability(NET_CAPABILITY_NOT_VCN_MANAGED)
-            ncBuilder
+                        .addTransportType(transport);
-                    .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR)
+        if (subId != SubscriptionManager.INVALID_SUBSCRIPTION_ID) {
-                    .setNetworkSpecifier(new TelephonyNetworkSpecifier(TEST_SUBSCRIPTION_ID));
+            ncBuilder.setSubIds(Collections.singleton(subId));
-        } else if (transport == TRANSPORT_WIFI) {
-            WifiInfo wifiInfo = mock(WifiInfo.class);
-            when(wifiInfo.makeCopy(anyLong())).thenReturn(wifiInfo);
-            when(mMockDeps.getSubIdForWifiInfo(eq(wifiInfo))).thenReturn(TEST_SUBSCRIPTION_ID);
-
-            ncBuilder
-                    .addTransportType(NetworkCapabilities.TRANSPORT_WIFI)
-                    .setTransportInfo(wifiInfo);
-        } else {
-            throw new IllegalArgumentException("Unknown transport");
         }
 
         return mVcnMgmtSvc.getUnderlyingNetworkPolicy(ncBuilder.build(), new LinkProperties());

tests/vcn/java/com/android/server/vcn/UnderlyingNetworkTrackerTest.java

@@ -146,40 +146,34 @@ public class UnderlyingNetworkTrackerTest {
 
     @Test
     public void testNetworkCallbacksRegisteredOnStartup() {
-        // verify NetworkCallbacks registered when instantiated
+        verifyNetworkRequestsRegistered(INITIAL_SUB_IDS);
-        verify(mConnectivityManager)
-                .requestBackgroundNetwork(
-                        eq(getWifiRequest()),
-                        any(),
-                        any(NetworkBringupCallback.class));
-        verifyBackgroundCellRequests(mSubscriptionSnapshot, SUB_GROUP, INITIAL_SUB_IDS);
-
-        verify(mConnectivityManager)
-                .requestBackgroundNetwork(
-                        eq(getRouteSelectionRequest()),
-                        any(),
-                        any(RouteSelectionCallback.class));
     }
 
-    private void verifyBackgroundCellRequests(
+    private void verifyNetworkRequestsRegistered(Set<Integer> expectedSubIds) {
-            TelephonySubscriptionSnapshot snapshot,
+        verify(mConnectivityManager)
-            ParcelUuid subGroup,
+                .requestBackgroundNetwork(
-            Set<Integer> expectedSubIds) {
+                        eq(getWifiRequest(expectedSubIds)),
-        verify(snapshot).getAllSubIdsInGroup(eq(subGroup));
+                        any(),
-
+                        any(NetworkBringupCallback.class));
         for (final int subId : expectedSubIds) {
             verify(mConnectivityManager)
                     .requestBackgroundNetwork(
-                            eq(getCellRequestForSubId(subId)),
+                            eq(getCellRequestForSubId(subId, expectedSubIds)),
                             any(),
                             any(NetworkBringupCallback.class));
         }
+
+        verify(mConnectivityManager)
+                .requestBackgroundNetwork(
+                        eq(getRouteSelectionRequest(expectedSubIds)),
+                        any(),
+                        any(RouteSelectionCallback.class));
     }
 
     @Test
     public void testUpdateSubscriptionSnapshot() {
         // Verify initial cell background requests filed
-        verifyBackgroundCellRequests(mSubscriptionSnapshot, SUB_GROUP, INITIAL_SUB_IDS);
+        verifyNetworkRequestsRegistered(INITIAL_SUB_IDS);
 
         TelephonySubscriptionSnapshot subscriptionUpdate =
                 mock(TelephonySubscriptionSnapshot.class);
@@ -187,40 +181,38 @@ public class UnderlyingNetworkTrackerTest {
 
         mUnderlyingNetworkTracker.updateSubscriptionSnapshot(subscriptionUpdate);
 
-        // verify that initially-filed bringup requests are unregistered
+        // verify that initially-filed bringup requests are unregistered (cell + wifi)
-        verify(mConnectivityManager, times(INITIAL_SUB_IDS.size()))
+        verify(mConnectivityManager, times(INITIAL_SUB_IDS.size() + 1))
                 .unregisterNetworkCallback(any(NetworkBringupCallback.class));
-        verifyBackgroundCellRequests(subscriptionUpdate, SUB_GROUP, UPDATED_SUB_IDS);
+        verify(mConnectivityManager).unregisterNetworkCallback(any(RouteSelectionCallback.class));
+        verifyNetworkRequestsRegistered(UPDATED_SUB_IDS);
     }
 
-    private NetworkRequest getWifiRequest() {
+    private NetworkRequest getWifiRequest(Set<Integer> netCapsSubIds) {
-        return getExpectedRequestBase(true)
+        return getExpectedRequestBase(netCapsSubIds)
                 .addTransportType(NetworkCapabilities.TRANSPORT_WIFI)
                 .build();
     }
 
-    private NetworkRequest getCellRequestForSubId(int subId) {
+    private NetworkRequest getCellRequestForSubId(int subId, Set<Integer> netCapsSubIds) {
-        return getExpectedRequestBase(false)
+        return getExpectedRequestBase(netCapsSubIds)
                 .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR)
                 .setNetworkSpecifier(new TelephonyNetworkSpecifier(subId))
                 .build();
     }
 
-    private NetworkRequest getRouteSelectionRequest() {
+    private NetworkRequest getRouteSelectionRequest(Set<Integer> netCapsSubIds) {
-        return getExpectedRequestBase(true).build();
+        return getExpectedRequestBase(netCapsSubIds).build();
     }
 
-    private NetworkRequest.Builder getExpectedRequestBase(boolean requireVcnManaged) {
+    private NetworkRequest.Builder getExpectedRequestBase(Set<Integer> subIds) {
         final NetworkRequest.Builder builder =
                 new NetworkRequest.Builder()
                         .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
                         .removeCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED)
                         .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED)
-                        .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED);
+                        .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED)
-
+                        .setSubIds(subIds);
-        if (requireVcnManaged) {
-            builder.addUnwantedCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED);
-        }
 
         return builder;
     }
@@ -274,7 +266,7 @@ public class UnderlyingNetworkTrackerTest {
             NetworkCapabilities networkCapabilities) {
         verify(mConnectivityManager)
                 .requestBackgroundNetwork(
-                        eq(getRouteSelectionRequest()),
+                        eq(getRouteSelectionRequest(INITIAL_SUB_IDS)),
                         any(),
                         mRouteSelectionCallbackCaptor.capture());