From f8ce665d9fd85e1daadc251f905058ad5f88b44c 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 --- .../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 e7e3ce9d2d61b..9bf6a5d0d1963 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 @@ -999,46 +1000,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 9d92b77ad79048540640a3f206c5c60b9cc02670 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 --- .../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 5cecff6f93c1a..840157b7e3b46 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -1970,6 +1970,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) { @@ -1986,6 +1989,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(); @@ -2013,7 +2021,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 c747bc096a6f2..0a4fcbcb28424 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 02cb0d7b450a0e1b9ca25e07f4e21cd37e3f345d 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 --- .../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 9bf6a5d0d1963..57adcb6e3966e 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -998,30 +998,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:"); @@ -1041,8 +1029,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 fb4c6236a9a00..5414a5206abbd 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 Set mRequiredUnderlyingNetworkCapabilities; @@ -403,12 +415,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; @@ -454,17 +466,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 @@ -668,10 +686,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); @@ -741,6 +770,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 840157b7e3b46..fbbae973538ae 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -2188,15 +2188,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();