From a2284ca391717fc41376a657e12e1ff60ed1a31d Mon Sep 17 00:00:00 2001 From: Yan Yan Date: Mon, 28 Oct 2024 13:43:34 -0700 Subject: [PATCH] Fix VcnConfig garbage collection The garbageCollectAndWriteVcnConfigsLocked is for removing configs whose subgroups do not have any subscriptions. This patch fixes the issue of it in handling race condition when subscription changes. Previously the garbage collection checks a subGroup's subscriptions based on SubscriptionManager. When SubscriptionManager and TelephonySubscriptionSnapshot are out-of-synced, the config of an active VCN instance will be removed. It can cause NPE in getUnderlyingNetworkPolicy. This patch fixes it by checking subscriptions against the snapshot. This patch also fixes the safety check in getUnderlyingNetworkPolicy so that if VCN instance exists but its config does not, it will not attempt querying data from the VcnConfig Bug: 370862489 Test: FrameworksVcnTests(new tests); CtsVcnTestCases Flag: android.net.vcn.fix_config_garbage_collection Change-Id: Id54adabcc1910102a2ab20c432b693a8f52c5c95 --- Vcn/flags/flags.aconfig | 10 +++++ .../android/server/VcnManagementService.java | 31 ++++++++++------ .../vcn/TelephonySubscriptionTracker.java | 11 ++++++ .../server/VcnManagementServiceTest.java | 37 +++++++++++++++++++ 4 files changed, 77 insertions(+), 12 deletions(-) diff --git a/Vcn/flags/flags.aconfig b/Vcn/flags/flags.aconfig index 5b306243fc..3d4401b1c7 100644 --- a/Vcn/flags/flags.aconfig +++ b/Vcn/flags/flags.aconfig @@ -14,4 +14,14 @@ flag { namespace: "vcn" description: "Feature flag for adjustable safe mode timeout" bug: "317406085" +} + +flag { + name: "fix_config_garbage_collection" + namespace: "vcn" + description: "Handle race condition in subscription change" + bug: "370862489" + metadata { + purpose: PURPOSE_BUGFIX + } } \ No newline at end of file diff --git a/Vcn/service-b/src/com/android/server/VcnManagementService.java b/Vcn/service-b/src/com/android/server/VcnManagementService.java index 51c768b80e..dc7749cb02 100644 --- a/Vcn/service-b/src/com/android/server/VcnManagementService.java +++ b/Vcn/service-b/src/com/android/server/VcnManagementService.java @@ -48,6 +48,7 @@ import android.net.LinkProperties; import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkRequest; +import android.net.vcn.Flags; import android.net.vcn.IVcnManagementService; import android.net.vcn.IVcnStatusCallback; import android.net.vcn.IVcnUnderlyingNetworkPolicyListener; @@ -878,6 +879,7 @@ public class VcnManagementService extends IVcnManagementService.Stub { private void garbageCollectAndWriteVcnConfigsLocked() { final SubscriptionManager subMgr = mContext.getSystemService(SubscriptionManager.class); + final Set subGroups = mLastSnapshot.getAllSubscriptionGroups(); boolean shouldWrite = false; @@ -885,11 +887,20 @@ public class VcnManagementService extends IVcnManagementService.Stub { while (configsIterator.hasNext()) { final ParcelUuid subGrp = configsIterator.next(); - final List subscriptions = subMgr.getSubscriptionsInGroup(subGrp); - if (subscriptions == null || subscriptions.isEmpty()) { - // Trim subGrps with no more subscriptions; must have moved to another subGrp - configsIterator.remove(); - shouldWrite = true; + if (Flags.fixConfigGarbageCollection()) { + if (!subGroups.contains(subGrp)) { + // Trim subGrps with no more subscriptions; must have moved to another subGrp + logDbg("Garbage collect VcnConfig for group=" + subGrp); + configsIterator.remove(); + shouldWrite = true; + } + } else { + final List subscriptions = subMgr.getSubscriptionsInGroup(subGrp); + if (subscriptions == null || subscriptions.isEmpty()) { + // Trim subGrps with no more subscriptions; must have moved to another subGrp + configsIterator.remove(); + shouldWrite = true; + } } } @@ -1094,13 +1105,7 @@ public class VcnManagementService extends IVcnManagementService.Stub { synchronized (mLock) { final Vcn vcn = mVcns.get(subGrp); final VcnConfig vcnConfig = mConfigs.get(subGrp); - if (vcn != null) { - if (vcnConfig == null) { - // TODO: b/284381334 Investigate for the root cause of this issue - // and handle it properly - logWtf("Vcn instance exists but VcnConfig does not for " + subGrp); - } - + if (vcn != null && vcnConfig != null) { if (vcn.getStatus() == VCN_STATUS_CODE_ACTIVE) { isVcnManagedNetwork = true; } @@ -1120,6 +1125,8 @@ public class VcnManagementService extends IVcnManagementService.Stub { } } } + } else if (vcn != null && vcnConfig == null) { + logWtf("Vcn instance exists but VcnConfig does not for " + subGrp); } } diff --git a/Vcn/service-b/src/com/android/server/vcn/TelephonySubscriptionTracker.java b/Vcn/service-b/src/com/android/server/vcn/TelephonySubscriptionTracker.java index baf84cf4aa..3392d03910 100644 --- a/Vcn/service-b/src/com/android/server/vcn/TelephonySubscriptionTracker.java +++ b/Vcn/service-b/src/com/android/server/vcn/TelephonySubscriptionTracker.java @@ -436,6 +436,17 @@ public class TelephonySubscriptionTracker extends BroadcastReceiver { return mPrivilegedPackages.keySet(); } + /** Returns all subscription groups */ + @NonNull + public Set getAllSubscriptionGroups() { + final Set subGroups = new ArraySet<>(); + for (SubscriptionInfo subInfo : mSubIdToInfoMap.values()) { + subGroups.add(subInfo.getGroupUuid()); + } + + return subGroups; + } + /** Checks if the provided package is carrier privileged for the specified sub group. */ public boolean packageHasPermissionsForSubscriptionGroup( @NonNull ParcelUuid subGrp, @NonNull String packageName) { diff --git a/Vcn/tests/unit/java/com/android/server/VcnManagementServiceTest.java b/Vcn/tests/unit/java/com/android/server/VcnManagementServiceTest.java index 7e0bbc4b3e..3828a71d7b 100644 --- a/Vcn/tests/unit/java/com/android/server/VcnManagementServiceTest.java +++ b/Vcn/tests/unit/java/com/android/server/VcnManagementServiceTest.java @@ -70,6 +70,7 @@ import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkRequest; import android.net.Uri; +import android.net.vcn.Flags; import android.net.vcn.IVcnStatusCallback; import android.net.vcn.IVcnUnderlyingNetworkPolicyListener; import android.net.vcn.VcnConfig; @@ -84,6 +85,7 @@ import android.os.Process; import android.os.UserHandle; import android.os.UserManager; import android.os.test.TestLooper; +import android.platform.test.flag.junit.SetFlagsRule; import android.telephony.SubscriptionInfo; import android.telephony.SubscriptionManager; import android.telephony.TelephonyManager; @@ -102,6 +104,7 @@ import com.android.server.vcn.util.PersistableBundleUtils; import com.android.server.vcn.util.PersistableBundleUtils.PersistableBundleWrapper; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; @@ -119,6 +122,8 @@ import java.util.UUID; @RunWith(AndroidJUnit4.class) @SmallTest public class VcnManagementServiceTest { + @Rule public final SetFlagsRule mSetFlagsRule = new SetFlagsRule(); + private static final String CONTEXT_ATTRIBUTION_TAG = "VCN"; private static final String TEST_PACKAGE_NAME = VcnManagementServiceTest.class.getPackage().getName(); @@ -288,6 +293,8 @@ public class VcnManagementServiceTest { doReturn(Collections.singleton(TRANSPORT_WIFI)) .when(mMockDeps) .getRestrictedTransports(any(), any(), any()); + + mSetFlagsRule.enableFlags(Flags.FLAG_FIX_CONFIG_GARBAGE_COLLECTION); } @@ -438,6 +445,14 @@ public class VcnManagementServiceTest { return subIds; }).when(snapshot).getAllSubIdsInGroup(any()); + doAnswer(invocation -> { + final Set subGroups = new ArraySet<>(); + for (Entry entry : subIdToGroupMap.entrySet()) { + subGroups.add(entry.getValue()); + } + return subGroups; + }).when(snapshot).getAllSubscriptionGroups(); + return snapshot; } @@ -1482,6 +1497,28 @@ public class VcnManagementServiceTest { verify(mMockPolicyListener).onPolicyChanged(); } + @Test + public void testGarbageCollectionKeepConfigUntilNewSnapshot() throws Exception { + setupActiveSubscription(TEST_UUID_2); + startAndGetVcnInstance(TEST_UUID_2); + + // Report loss of subscription from mSubMgr + doReturn(Collections.emptyList()).when(mSubMgr).getSubscriptionsInGroup(any()); + triggerSubscriptionTrackerCbAndGetSnapshot( + TEST_UUID_2, + Collections.singleton(TEST_UUID_2), + Collections.singletonMap(TEST_SUBSCRIPTION_ID, TEST_UUID_2)); + + assertTrue(mVcnMgmtSvc.getConfigs().containsKey(TEST_UUID_2)); + + // Report loss of subscription from snapshot + triggerSubscriptionTrackerCbAndGetSnapshot(null, Collections.emptySet()); + + mTestLooper.moveTimeForward(VcnManagementService.CARRIER_PRIVILEGES_LOST_TEARDOWN_DELAY_MS); + mTestLooper.dispatchAll(); + assertFalse(mVcnMgmtSvc.getConfigs().containsKey(TEST_UUID_2)); + } + @Test public void testVcnCarrierConfigChangeUpdatesPolicyListener() throws Exception { setupActiveSubscription(TEST_UUID_2);