From 5c5b485013e88e20119aa17d68ae6b3dd8b306bc Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 20 May 2021 16:42:42 -0700 Subject: [PATCH 1/3] Prevent concurrent modification exceptions in VcnManagementService In order to prevent concurrent modifications from triggering an exception in the dump, proxy it to the handler thread. Adding locks was considered, but would require auditing all callbacks and proxying them to the handler threads in order to ensure lock inversion and deadlocks never occur. Bug: 188840899 Test: FrameworksVcnTests Test: adb shell dumpsys vcn_management Change-Id: Ibcc4bdc06301dd77260adaa0c086529f8f524679 Merged-In: Ibcc4bdc06301dd77260adaa0c086529f8f524679 (cherry picked from commit f8ce665d9fd85e1daadc251f905058ad5f88b44c) --- .../android/server/VcnManagementService.java | 69 ++++++++++--------- 1 file changed, 37 insertions(+), 32 deletions(-) diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index bcbd692a2f7d4..be9221c43611e 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -148,6 +148,7 @@ import java.util.concurrent.TimeUnit; // TODO(b/180451994): ensure all incoming + outgoing calls have a cleared calling identity public class VcnManagementService extends IVcnManagementService.Stub { @NonNull private static final String TAG = VcnManagementService.class.getSimpleName(); + private static final long DUMP_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(5); public static final boolean VDBG = false; // STOPSHIP: if true @@ -1001,46 +1002,50 @@ public class VcnManagementService extends IVcnManagementService.Stub { final IndentingPrintWriter pw = new IndentingPrintWriter(writer, " "); - pw.println("VcnManagementService dump:"); - pw.increaseIndent(); - - pw.println("mNetworkProvider:"); - pw.increaseIndent(); - mNetworkProvider.dump(pw); - pw.decreaseIndent(); - pw.println(); - - pw.println("mTrackingNetworkCallback:"); - pw.increaseIndent(); - mTrackingNetworkCallback.dump(pw); - pw.decreaseIndent(); - pw.println(); - - synchronized (mLock) { - pw.println("mLastSnapshot:"); + // Post to handler thread to prevent ConcurrentModificationExceptions, and avoid lock-hell. + mHandler.runWithScissors(() -> { + pw.println("VcnManagementService dump:"); pw.increaseIndent(); - mLastSnapshot.dump(pw); + + pw.println("mNetworkProvider:"); + pw.increaseIndent(); + mNetworkProvider.dump(pw); pw.decreaseIndent(); pw.println(); - pw.println("mConfigs:"); + pw.println("mTrackingNetworkCallback:"); pw.increaseIndent(); - for (Entry entry : mConfigs.entrySet()) { - pw.println(entry.getKey() + ": " + entry.getValue().getProvisioningPackageName()); + mTrackingNetworkCallback.dump(pw); + pw.decreaseIndent(); + pw.println(); + + synchronized (mLock) { + pw.println("mLastSnapshot:"); + pw.increaseIndent(); + mLastSnapshot.dump(pw); + pw.decreaseIndent(); + pw.println(); + + pw.println("mConfigs:"); + pw.increaseIndent(); + for (Entry entry : mConfigs.entrySet()) { + pw.println(entry.getKey() + ": " + + entry.getValue().getProvisioningPackageName()); + } + pw.decreaseIndent(); + pw.println(); + + pw.println("mVcns:"); + pw.increaseIndent(); + for (Vcn vcn : mVcns.values()) { + vcn.dump(pw); + } + pw.decreaseIndent(); + pw.println(); } - pw.decreaseIndent(); - pw.println(); - pw.println("mVcns:"); - pw.increaseIndent(); - for (Vcn vcn : mVcns.values()) { - vcn.dump(pw); - } pw.decreaseIndent(); - pw.println(); - } - - pw.decreaseIndent(); + }, DUMP_TIMEOUT_MILLIS); } // TODO(b/180452282): Make name more generic and implement directly with VcnManagementService From e237e35dff5bf5ed35427b09f6b33e074f47dbfc Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 21 May 2021 17:13:14 -0700 Subject: [PATCH 2/3] Copy up/downstream bandwidth and TCP buffer sizes from underlying This change copies the relevant pieces of information used to tune the system for performance Bug: 188943592 Test: atest FrameworksVcnTests Change-Id: I2d5dc5b9000fa9d0da9d34772b3704110694c526 Merged-In: I2d5dc5b9000fa9d0da9d34772b3704110694c526 (cherry picked from commit 9d92b77ad79048540640a3f206c5c60b9cc02670) --- .../server/vcn/VcnGatewayConnection.java | 21 ++++++++++++++++++- ...cnGatewayConnectionConnectedStateTest.java | 12 ++++++++--- .../server/vcn/VcnGatewayConnectionTest.java | 7 +++++++ .../vcn/VcnGatewayConnectionTestBase.java | 4 ++++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java index dff04bfc6d7cb..345e7d9143125 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -1969,6 +1969,9 @@ public class VcnGatewayConnection extends StateMachine { } builder.setAdministratorUids(adminUids); + builder.setLinkUpstreamBandwidthKbps(underlyingCaps.getLinkUpstreamBandwidthKbps()); + builder.setLinkDownstreamBandwidthKbps(underlyingCaps.getLinkDownstreamBandwidthKbps()); + // Set TransportInfo for SysUI use (never parcelled out of SystemServer). if (underlyingCaps.hasTransport(TRANSPORT_WIFI) && underlyingCaps.getTransportInfo() instanceof WifiInfo) { @@ -1985,6 +1988,11 @@ public class VcnGatewayConnection extends StateMachine { "Unknown transport type or missing TransportInfo/NetworkSpecifier for" + " non-null underlying network"); } + } else { + Slog.wtf( + TAG, + "No underlying network while building network capabilities", + new IllegalStateException()); } return builder.build(); @@ -2012,7 +2020,18 @@ public class VcnGatewayConnection extends StateMachine { lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null /*gateway*/, null /*iface*/, RouteInfo.RTN_UNICAST)); - final int underlyingMtu = (underlying == null) ? 0 : underlying.linkProperties.getMtu(); + int underlyingMtu = 0; + if (underlying != null) { + final LinkProperties underlyingLp = underlying.linkProperties; + + lp.setTcpBufferSizes(underlyingLp.getTcpBufferSizes()); + underlyingMtu = underlyingLp.getMtu(); + } else { + Slog.wtf( + TAG, + "No underlying network while building link properties", + new IllegalStateException()); + } lp.setMtu( MtuUtils.getMtu( ikeTunnelParams.getTunnelModeChildSessionParams().getSaProposals(), diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java index 39f7386cbb766..9c93f8111427f 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java @@ -200,6 +200,9 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection public void testMigration() throws Exception { triggerChildOpened(); + mGatewayConnection + .getUnderlyingNetworkTrackerCallback() + .onSelectedUnderlyingNetworkChanged(TEST_UNDERLYING_NETWORK_RECORD_2); getChildSessionCallback() .onIpSecTransformsMigrated(makeDummyIpSecTransform(), makeDummyIpSecTransform()); mTestLooper.dispatchAll(); @@ -207,7 +210,7 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection verify(mIpSecSvc, times(2)) .setNetworkForTunnelInterface( eq(TEST_IPSEC_TUNNEL_RESOURCE_ID), - eq(TEST_UNDERLYING_NETWORK_RECORD_1.network), + eq(TEST_UNDERLYING_NETWORK_RECORD_2.network), any()); for (int direction : new int[] {DIRECTION_IN, DIRECTION_OUT}) { @@ -226,8 +229,10 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection MtuUtils.getMtu( saProposals, mConfig.getMaxMtu(), - TEST_UNDERLYING_NETWORK_RECORD_1.linkProperties.getMtu()); - verify(mNetworkAgent).sendLinkProperties(argThat(lp -> expectedMtu == lp.getMtu())); + TEST_UNDERLYING_NETWORK_RECORD_2.linkProperties.getMtu()); + verify(mNetworkAgent).sendLinkProperties( + argThat(lp -> expectedMtu == lp.getMtu() + && TEST_TCP_BUFFER_SIZES_2.equals(lp.getTcpBufferSizes()))); } private void triggerChildOpened() { @@ -297,6 +302,7 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection final LinkProperties lp = lpCaptor.getValue(); assertEquals(Collections.singletonList(TEST_INTERNAL_ADDR), lp.getLinkAddresses()); assertEquals(Collections.singletonList(TEST_DNS_ADDR), lp.getDnsServers()); + assertEquals(TEST_TCP_BUFFER_SIZES_1, lp.getTcpBufferSizes()); final NetworkCapabilities nc = ncCaptor.getValue(); assertTrue(nc.hasTransport(TRANSPORT_CELLULAR)); diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java index 9705f0fc6bbc6..a4f95e03e9bd7 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java @@ -74,6 +74,9 @@ public class VcnGatewayConnectionTest extends VcnGatewayConnectionTestBase { private static final SubscriptionInfo TEST_SUBINFO_2 = mock(SubscriptionInfo.class); private static final Map TEST_SUBID_TO_GROUP_MAP; + private static final int TEST_UPSTREAM_BANDWIDTH = 1234; + private static final int TEST_DOWNSTREAM_BANDWIDTH = 2345; + static { final Map subIdToGroupMap = new HashMap<>(); subIdToGroupMap.put(TEST_SUBSCRIPTION_ID_1, TEST_PARCEL_UUID); @@ -106,6 +109,8 @@ public class VcnGatewayConnectionTest extends VcnGatewayConnectionTestBase { capBuilder.setNetworkSpecifier( new TelephonyNetworkSpecifier(TEST_SUBSCRIPTION_ID_1)); } + capBuilder.setLinkUpstreamBandwidthKbps(TEST_UPSTREAM_BANDWIDTH); + capBuilder.setLinkDownstreamBandwidthKbps(TEST_DOWNSTREAM_BANDWIDTH); capBuilder.setAdministratorUids(new int[] {TEST_UID}); UnderlyingNetworkRecord record = new UnderlyingNetworkRecord( mock(Network.class, CALLS_REAL_METHODS), @@ -130,6 +135,8 @@ public class VcnGatewayConnectionTest extends VcnGatewayConnectionTestBase { assertArrayEquals(new int[] {TEST_UID}, vcnCaps.getAdministratorUids()); assertTrue(vcnCaps.getTransportInfo() instanceof VcnTransportInfo); + assertEquals(TEST_UPSTREAM_BANDWIDTH, vcnCaps.getLinkUpstreamBandwidthKbps()); + assertEquals(TEST_DOWNSTREAM_BANDWIDTH, vcnCaps.getLinkDownstreamBandwidthKbps()); final VcnTransportInfo info = (VcnTransportInfo) vcnCaps.getTransportInfo(); if (transportType == TRANSPORT_WIFI) { diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTestBase.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTestBase.java index 860a919aa9b37..b97023a95d72c 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTestBase.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTestBase.java @@ -99,6 +99,7 @@ public class VcnGatewayConnectionTestBase { protected static final long ELAPSED_REAL_TIME = 123456789L; protected static final String TEST_IPSEC_TUNNEL_IFACE = "IPSEC_IFACE"; + protected static final String TEST_TCP_BUFFER_SIZES_1 = "1,2,3,4"; protected static final UnderlyingNetworkRecord TEST_UNDERLYING_NETWORK_RECORD_1 = new UnderlyingNetworkRecord( mock(Network.class, CALLS_REAL_METHODS), @@ -108,8 +109,10 @@ public class VcnGatewayConnectionTestBase { static { TEST_UNDERLYING_NETWORK_RECORD_1.linkProperties.setMtu(1500); + TEST_UNDERLYING_NETWORK_RECORD_1.linkProperties.setTcpBufferSizes(TEST_TCP_BUFFER_SIZES_1); } + protected static final String TEST_TCP_BUFFER_SIZES_2 = "2,3,4,5"; protected static final UnderlyingNetworkRecord TEST_UNDERLYING_NETWORK_RECORD_2 = new UnderlyingNetworkRecord( mock(Network.class, CALLS_REAL_METHODS), @@ -119,6 +122,7 @@ public class VcnGatewayConnectionTestBase { static { TEST_UNDERLYING_NETWORK_RECORD_2.linkProperties.setMtu(1460); + TEST_UNDERLYING_NETWORK_RECORD_2.linkProperties.setTcpBufferSizes(TEST_TCP_BUFFER_SIZES_2); } protected static final TelephonySubscriptionSnapshot TEST_SUBSCRIPTION_SNAPSHOT = From 61d7ab9455a6a392bfda955b088f5062d287b577 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 20 May 2021 17:12:47 -0700 Subject: [PATCH 3/3] Add additional dump information for UnderlyingNetworkTracker This change adds additional dump information to facilitate the resolution of bug reports. Additionally, this method cleans up dump formatting. Bug: 188842647 Test: atest FrameworksVcnTests Test: adb shell dumpsys vcn_management Change-Id: I5ce7fe05eb21a89c2034079223ef44f05db2a087 Merged-In: I5ce7fe05eb21a89c2034079223ef44f05db2a087 (cherry picked from commit 02cb0d7b450a0e1b9ca25e07f4e21cd37e3f345d) --- .../android/server/VcnManagementService.java | 16 +--- .../server/vcn/UnderlyingNetworkTracker.java | 73 ++++++++++++++++--- .../core/java/com/android/server/vcn/Vcn.java | 3 + .../server/vcn/VcnGatewayConnection.java | 10 +-- 4 files changed, 69 insertions(+), 33 deletions(-) diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index be9221c43611e..54388f25a8fcb 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -1000,30 +1000,18 @@ public class VcnManagementService extends IVcnManagementService.Stub { protected void dump(FileDescriptor fd, PrintWriter writer, String[] args) { mContext.enforceCallingOrSelfPermission(DUMP, TAG); - final IndentingPrintWriter pw = new IndentingPrintWriter(writer, " "); + final IndentingPrintWriter pw = new IndentingPrintWriter(writer, "| "); // Post to handler thread to prevent ConcurrentModificationExceptions, and avoid lock-hell. mHandler.runWithScissors(() -> { - pw.println("VcnManagementService dump:"); - pw.increaseIndent(); - - pw.println("mNetworkProvider:"); - pw.increaseIndent(); mNetworkProvider.dump(pw); - pw.decreaseIndent(); pw.println(); - pw.println("mTrackingNetworkCallback:"); - pw.increaseIndent(); mTrackingNetworkCallback.dump(pw); - pw.decreaseIndent(); pw.println(); synchronized (mLock) { - pw.println("mLastSnapshot:"); - pw.increaseIndent(); mLastSnapshot.dump(pw); - pw.decreaseIndent(); pw.println(); pw.println("mConfigs:"); @@ -1043,8 +1031,6 @@ public class VcnManagementService extends IVcnManagementService.Stub { pw.decreaseIndent(); pw.println(); } - - pw.decreaseIndent(); }, DUMP_TIMEOUT_MILLIS); } diff --git a/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java b/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java index b05662e1678ed..92a4eededdf35 100644 --- a/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java +++ b/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java @@ -40,6 +40,7 @@ import android.telephony.TelephonyCallback; import android.telephony.TelephonyManager; import android.util.ArrayMap; import android.util.Slog; +import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.annotations.VisibleForTesting.Visibility; @@ -106,6 +107,17 @@ public class UnderlyingNetworkTracker { @VisibleForTesting(visibility = Visibility.PRIVATE) static final int PRIORITY_ANY = Integer.MAX_VALUE; + private static final SparseArray PRIORITY_TO_STRING_MAP = new SparseArray<>(); + + static { + PRIORITY_TO_STRING_MAP.put( + PRIORITY_OPPORTUNISTIC_CELLULAR, "PRIORITY_OPPORTUNISTIC_CELLULAR"); + PRIORITY_TO_STRING_MAP.put(PRIORITY_WIFI_IN_USE, "PRIORITY_WIFI_IN_USE"); + PRIORITY_TO_STRING_MAP.put(PRIORITY_WIFI_PROSPECTIVE, "PRIORITY_WIFI_PROSPECTIVE"); + PRIORITY_TO_STRING_MAP.put(PRIORITY_MACRO_CELLULAR, "PRIORITY_MACRO_CELLULAR"); + PRIORITY_TO_STRING_MAP.put(PRIORITY_ANY, "PRIORITY_ANY"); + } + @NonNull private final VcnContext mVcnContext; @NonNull private final ParcelUuid mSubscriptionGroup; @NonNull private final UnderlyingNetworkTrackerCallback mCb; @@ -395,12 +407,12 @@ public class UnderlyingNetworkTracker { } private void reevaluateNetworks() { - TreeSet sorted = - new TreeSet<>( - UnderlyingNetworkRecord.getComparator( - mSubscriptionGroup, mLastSnapshot, mCurrentRecord, mCarrierConfig)); - sorted.addAll(mRouteSelectionCallback.getUnderlyingNetworks()); + if (mRouteSelectionCallback == null) { + return; // UnderlyingNetworkTracker has quit. + } + TreeSet sorted = + mRouteSelectionCallback.getSortedUnderlyingNetworks(); UnderlyingNetworkRecord candidate = sorted.isEmpty() ? null : sorted.first(); if (Objects.equals(mCurrentRecord, candidate)) { return; @@ -446,17 +458,23 @@ public class UnderlyingNetworkTracker { private final Map mUnderlyingNetworkRecordBuilders = new ArrayMap<>(); - private List getUnderlyingNetworks() { - final List records = new ArrayList<>(); + private TreeSet getSortedUnderlyingNetworks() { + TreeSet sorted = + new TreeSet<>( + UnderlyingNetworkRecord.getComparator( + mSubscriptionGroup, + mLastSnapshot, + mCurrentRecord, + mCarrierConfig)); for (UnderlyingNetworkRecord.Builder builder : mUnderlyingNetworkRecordBuilders.values()) { if (builder.isValid()) { - records.add(builder.build()); + sorted.add(builder.build()); } } - return records; + return sorted; } @Override @@ -660,10 +678,21 @@ public class UnderlyingNetworkTracker { } /** Dumps the state of this record for logging and debugging purposes. */ - public void dump(IndentingPrintWriter pw) { + private void dump( + IndentingPrintWriter pw, + ParcelUuid subscriptionGroup, + TelephonySubscriptionSnapshot snapshot, + UnderlyingNetworkRecord currentlySelected, + PersistableBundle carrierConfig) { pw.println("UnderlyingNetworkRecord:"); pw.increaseIndent(); + final int priorityClass = + calculatePriorityClass( + subscriptionGroup, snapshot, currentlySelected, carrierConfig); + pw.println( + "Priority class: " + PRIORITY_TO_STRING_MAP.get(priorityClass) + " (" + + priorityClass + ")"); pw.println("mNetwork: " + network); pw.println("mNetworkCapabilities: " + networkCapabilities); pw.println("mLinkProperties: " + linkProperties); @@ -733,6 +762,30 @@ public class UnderlyingNetworkTracker { } } + /** Dumps the state of this record for logging and debugging purposes. */ + public void dump(IndentingPrintWriter pw) { + pw.println("UnderlyingNetworkTracker:"); + pw.increaseIndent(); + + pw.println("Carrier WiFi Entry Threshold: " + getWifiEntryRssiThreshold(mCarrierConfig)); + pw.println("Carrier WiFi Exit Threshold: " + getWifiExitRssiThreshold(mCarrierConfig)); + pw.println( + "Currently selected: " + (mCurrentRecord == null ? null : mCurrentRecord.network)); + + pw.println("Underlying networks:"); + pw.increaseIndent(); + if (mRouteSelectionCallback != null) { + for (UnderlyingNetworkRecord record : + mRouteSelectionCallback.getSortedUnderlyingNetworks()) { + record.dump(pw, mSubscriptionGroup, mLastSnapshot, mCurrentRecord, mCarrierConfig); + } + } + pw.decreaseIndent(); + pw.println(); + + pw.decreaseIndent(); + } + private class VcnActiveDataSubscriptionIdListener extends TelephonyCallback implements ActiveDataSubscriptionIdListener { @Override diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java index f918827e9639f..6b4ee693c9361 100644 --- a/services/core/java/com/android/server/vcn/Vcn.java +++ b/services/core/java/com/android/server/vcn/Vcn.java @@ -557,11 +557,14 @@ public class Vcn extends Handler { pw.println("mCurrentStatus: " + mCurrentStatus); pw.println("mIsMobileDataEnabled: " + mIsMobileDataEnabled); + pw.println(); pw.println("mVcnGatewayConnections:"); + pw.increaseIndent(); for (VcnGatewayConnection gw : mVcnGatewayConnections.values()) { gw.dump(pw); } + pw.decreaseIndent(); pw.println(); pw.decreaseIndent(); diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java index 345e7d9143125..54a1da0752fd8 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -2187,15 +2187,9 @@ public class VcnGatewayConnection extends StateMachine { pw.println( "mNetworkAgent.getNetwork(): " + (mNetworkAgent == null ? null : mNetworkAgent.getNetwork())); + pw.println(); - pw.println("mUnderlying:"); - pw.increaseIndent(); - if (mUnderlying != null) { - mUnderlying.dump(pw); - } else { - pw.println("null"); - } - pw.decreaseIndent(); + mUnderlyingNetworkTracker.dump(pw); pw.println(); pw.decreaseIndent();