From 36888bdf42c7e5f0d5a0930194954dbac83af879 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 24 Mar 2023 20:55:16 +0000 Subject: [PATCH] Match underlying networks based on interface name This patch changes the VcnManagementService policy matching from using capabilities to match against underlying networks to using iface names. This is significantly more precise than the network capability matching logic, as that will not catch any new capabilities that were added (eg ENTERPRISE, LOW_LATENCY, HIGH_BANDWIDTH, etc). Bug: 274861645 Test: atest FrameworksVcnTests CtsVcnTestCases Change-Id: Iea2f5afb242649517f83f85c77355cfdbbf1e202 --- .../android/server/VcnManagementService.java | 36 ++++++++--- .../server/VcnManagementServiceTest.java | 64 ++++++++++++++++--- 2 files changed, 80 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index f652cb050cbd5..78d4708e70a29 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -1104,7 +1104,7 @@ public class VcnManagementService extends IVcnManagementService.Stub { final NetworkCapabilities result = ncBuilder.build(); final VcnUnderlyingNetworkPolicy policy = new VcnUnderlyingNetworkPolicy( mTrackingNetworkCallback - .requiresRestartForImmutableCapabilityChanges(result), + .requiresRestartForImmutableCapabilityChanges(result, linkProperties), result); logVdbg("getUnderlyingNetworkPolicy() called for caps: " + networkCapabilities @@ -1354,19 +1354,29 @@ public class VcnManagementService extends IVcnManagementService.Stub { * without requiring a Network restart. */ private class TrackingNetworkCallback extends ConnectivityManager.NetworkCallback { + private final Object mLockObject = new Object(); private final Map mCaps = new ArrayMap<>(); + private final Map mLinkProperties = new ArrayMap<>(); @Override public void onCapabilitiesChanged(Network network, NetworkCapabilities caps) { - synchronized (mCaps) { + synchronized (mLockObject) { mCaps.put(network, caps); } } + @Override + public void onLinkPropertiesChanged(Network network, LinkProperties lp) { + synchronized (mLockObject) { + mLinkProperties.put(network, lp); + } + } + @Override public void onLost(Network network) { - synchronized (mCaps) { + synchronized (mLockObject) { mCaps.remove(network); + mLinkProperties.remove(network); } } @@ -1393,22 +1403,28 @@ public class VcnManagementService extends IVcnManagementService.Stub { return true; } - private boolean requiresRestartForImmutableCapabilityChanges(NetworkCapabilities caps) { + private boolean requiresRestartForImmutableCapabilityChanges( + NetworkCapabilities caps, LinkProperties lp) { if (caps.getSubscriptionIds() == null) { return false; } - synchronized (mCaps) { - for (NetworkCapabilities existing : mCaps.values()) { - if (caps.getSubscriptionIds().equals(existing.getSubscriptionIds()) - && hasSameTransportsAndCapabilities(caps, existing)) { - // Restart if any immutable capabilities have changed - return existing.hasCapability(NET_CAPABILITY_NOT_RESTRICTED) + synchronized (mLockObject) { + // Search for an existing network (using interfce names) + // TODO: Get network from NetworkFactory (if exists) for this match. + for (Entry lpEntry : mLinkProperties.entrySet()) { + if (lp.getInterfaceName() != null + && !lp.getInterfaceName().isEmpty() + && Objects.equals( + lp.getInterfaceName(), lpEntry.getValue().getInterfaceName())) { + return mCaps.get(lpEntry.getKey()) + .hasCapability(NET_CAPABILITY_NOT_RESTRICTED) != caps.hasCapability(NET_CAPABILITY_NOT_RESTRICTED); } } } + // If no network found, by definition does not need restart. return false; } diff --git a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java index 075bc5e5214e1..4123f8070e36c 100644 --- a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java +++ b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java @@ -17,7 +17,9 @@ package com.android.server; import static android.net.ConnectivityManager.NetworkCallback; +import static android.net.NetworkCapabilities.NET_CAPABILITY_ENTERPRISE; import static android.net.NetworkCapabilities.NET_CAPABILITY_IMS; +import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; @@ -67,7 +69,6 @@ import android.net.LinkProperties; import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkRequest; -import android.net.TelephonyNetworkSpecifier; import android.net.Uri; import android.net.vcn.IVcnStatusCallback; import android.net.vcn.IVcnUnderlyingNetworkPolicyListener; @@ -128,6 +129,15 @@ public class VcnManagementServiceTest { private static final VcnConfig TEST_VCN_CONFIG; private static final VcnConfig TEST_VCN_CONFIG_PKG_2; private static final int TEST_UID = Process.FIRST_APPLICATION_UID; + private static final String TEST_IFACE_NAME = "TEST_IFACE"; + private static final String TEST_IFACE_NAME_2 = "TEST_IFACE2"; + private static final LinkProperties TEST_LP_1 = new LinkProperties(); + private static final LinkProperties TEST_LP_2 = new LinkProperties(); + + static { + TEST_LP_1.setInterfaceName(TEST_IFACE_NAME); + TEST_LP_2.setInterfaceName(TEST_IFACE_NAME_2); + } static { final Context mockConfigContext = mock(Context.class); @@ -1034,8 +1044,7 @@ public class VcnManagementServiceTest { setupSubscriptionAndStartVcn(subId, subGrp, isVcnActive); return mVcnMgmtSvc.getUnderlyingNetworkPolicy( - getNetworkCapabilitiesBuilderForTransport(subId, transport).build(), - new LinkProperties()); + getNetworkCapabilitiesBuilderForTransport(subId, transport).build(), TEST_LP_1); } private void checkGetRestrictedTransportsFromCarrierConfig( @@ -1260,7 +1269,7 @@ public class VcnManagementServiceTest { false /* expectRestricted */); } - private void setupTrackedCarrierWifiNetwork(NetworkCapabilities caps) { + private void setupTrackedNetwork(NetworkCapabilities caps, LinkProperties lp) { mVcnMgmtSvc.systemReady(); final ArgumentCaptor captor = @@ -1269,7 +1278,10 @@ public class VcnManagementServiceTest { .registerNetworkCallback( eq(new NetworkRequest.Builder().clearCapabilities().build()), captor.capture()); - captor.getValue().onCapabilitiesChanged(mock(Network.class, CALLS_REAL_METHODS), caps); + + Network mockNetwork = mock(Network.class, CALLS_REAL_METHODS); + captor.getValue().onCapabilitiesChanged(mockNetwork, caps); + captor.getValue().onLinkPropertiesChanged(mockNetwork, lp); } @Test @@ -1279,7 +1291,7 @@ public class VcnManagementServiceTest { getNetworkCapabilitiesBuilderForTransport(TEST_SUBSCRIPTION_ID, TRANSPORT_WIFI) .removeCapability(NET_CAPABILITY_NOT_RESTRICTED) .build(); - setupTrackedCarrierWifiNetwork(existingNetworkCaps); + setupTrackedNetwork(existingNetworkCaps, TEST_LP_1); // Trigger test without VCN instance alive; expect restart due to change of NOT_RESTRICTED // immutable capability @@ -1288,7 +1300,7 @@ public class VcnManagementServiceTest { getNetworkCapabilitiesBuilderForTransport( TEST_SUBSCRIPTION_ID, TRANSPORT_WIFI) .build(), - new LinkProperties()); + TEST_LP_1); assertTrue(policy.isTeardownRequested()); } @@ -1298,7 +1310,7 @@ public class VcnManagementServiceTest { final NetworkCapabilities existingNetworkCaps = getNetworkCapabilitiesBuilderForTransport(TEST_SUBSCRIPTION_ID, TRANSPORT_WIFI) .build(); - setupTrackedCarrierWifiNetwork(existingNetworkCaps); + setupTrackedNetwork(existingNetworkCaps, TEST_LP_1); final VcnUnderlyingNetworkPolicy policy = startVcnAndGetPolicyForTransport( @@ -1315,7 +1327,7 @@ public class VcnManagementServiceTest { .addCapability(NET_CAPABILITY_NOT_RESTRICTED) .removeCapability(NET_CAPABILITY_IMS) .build(); - setupTrackedCarrierWifiNetwork(existingNetworkCaps); + setupTrackedNetwork(existingNetworkCaps, TEST_LP_1); final VcnUnderlyingNetworkPolicy policy = mVcnMgmtSvc.getUnderlyingNetworkPolicy( @@ -1336,7 +1348,7 @@ public class VcnManagementServiceTest { new NetworkCapabilities.Builder() .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR) .addCapability(NET_CAPABILITY_NOT_VCN_MANAGED) - .setNetworkSpecifier(new TelephonyNetworkSpecifier(TEST_SUBSCRIPTION_ID_2)) + .setSubscriptionIds(Collections.singleton(TEST_SUBSCRIPTION_ID_2)) .build(); VcnUnderlyingNetworkPolicy policy = @@ -1346,6 +1358,38 @@ public class VcnManagementServiceTest { assertEquals(nc, policy.getMergedNetworkCapabilities()); } + /** + * Checks that networks with similar capabilities do not clobber each other. + * + *

In previous iterations, the VcnMgmtSvc used capability-matching to check if a network + * undergoing policy checks were the same as an existing networks. However, this meant that if + * there were newly added capabilities that the VCN did not check, two networks differing only + * by that capability would restart each other constantly. + */ + @Test + public void testGetUnderlyingNetworkPolicySimilarNetworks() throws Exception { + NetworkCapabilities nc1 = + new NetworkCapabilities.Builder() + .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR) + .addCapability(NET_CAPABILITY_NOT_VCN_MANAGED) + .addCapability(NET_CAPABILITY_INTERNET) + .setSubscriptionIds(Collections.singleton(TEST_SUBSCRIPTION_ID_2)) + .build(); + + NetworkCapabilities nc2 = + new NetworkCapabilities.Builder(nc1) + .addCapability(NET_CAPABILITY_ENTERPRISE) + .removeCapability(NET_CAPABILITY_NOT_RESTRICTED) + .build(); + + setupTrackedNetwork(nc1, TEST_LP_1); + + VcnUnderlyingNetworkPolicy policy = mVcnMgmtSvc.getUnderlyingNetworkPolicy(nc2, TEST_LP_2); + + assertFalse(policy.isTeardownRequested()); + assertEquals(nc2, policy.getMergedNetworkCapabilities()); + } + @Test(expected = SecurityException.class) public void testGetUnderlyingNetworkPolicyInvalidPermission() { doReturn(PackageManager.PERMISSION_DENIED)