From b4f300d70d9c7885261daba91076037e51430aa1 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Thu, 29 Jun 2023 02:41:32 +0000 Subject: [PATCH 1/2] Perform VPN data stall recovery on unvalidated VPN network This commit stops relying on data stall detection event for VPN network recovery. Data stall detection will also result in an unvalidated result, so VPN is not necessary to listen to data stall event to perform the recovery. When VPN receives network validation failure event, VPN will try to perform recovery starting from MOBIKE to session reset after this change. This helps to recover the stalls that are not detected or triggered by data stall detection, e.g. A stall happens when the VPN underlying network is switched. System triggers the reevaluation. If the validation fails, data stall mechanism will not be triggered and result in no recovery in VPN. Bug: 289746779 Test: atest FrameworksNetTests Change-Id: Ibb8197a689220db5868f351bffa9ac27334355e2 --- .../com/android/server/connectivity/Vpn.java | 93 ++++++++----------- 1 file changed, 37 insertions(+), 56 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 6b69e1caa985e..0786411695b03 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -22,7 +22,6 @@ import static android.content.pm.PackageManager.PERMISSION_GRANTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; -import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.RouteInfo.RTN_THROW; import static android.net.RouteInfo.RTN_UNREACHABLE; import static android.net.VpnManager.NOTIFICATION_CHANNEL_VPN; @@ -280,15 +279,22 @@ public class Vpn { private static final int VPN_DEFAULT_SCORE = 101; /** - * The reset session timer for data stall. If a session has not successfully revalidated after - * the delay, the session will be torn down and restarted in an attempt to recover. Delay + * The recovery timer for data stall. If a session has not successfully revalidated after + * the delay, the session will perform MOBIKE or be restarted in an attempt to recover. Delay * counter is reset on successful validation only. * + *

The first {@code MOBIKE_RECOVERY_ATTEMPT} timers are used for performing MOBIKE. + * System will perform session reset for the remaining timers. *

If retries have exceeded the length of this array, the last entry in the array will be * used as a repeating interval. */ - private static final long[] DATA_STALL_RESET_DELAYS_SEC = {30L, 60L, 120L, 240L, 480L, 960L}; - + // TODO: use ms instead to speed up the test. + private static final long[] DATA_STALL_RECOVERY_DELAYS_SEC = + {1L, 5L, 30L, 60L, 120L, 240L, 480L, 960L}; + /** + * Maximum attempts to perform MOBIKE when the network is bad. + */ + private static final int MAX_MOBIKE_RECOVERY_ATTEMPT = 2; /** * The initial token value of IKE session. */ @@ -392,7 +398,6 @@ public class Vpn { private final UserManager mUserManager; private final VpnProfileStore mVpnProfileStore; - protected boolean mDataStallSuspected = false; @VisibleForTesting VpnProfileStore getVpnProfileStore() { @@ -685,14 +690,14 @@ public class Vpn { } /** - * Get the length of time to wait before resetting the ike session when a data stall is - * suspected. + * Get the length of time to wait before perform data stall recovery when the validation + * result is bad. */ - public long getDataStallResetSessionSeconds(int count) { - if (count >= DATA_STALL_RESET_DELAYS_SEC.length) { - return DATA_STALL_RESET_DELAYS_SEC[DATA_STALL_RESET_DELAYS_SEC.length - 1]; + public long getValidationFailRecoverySeconds(int count) { + if (count >= DATA_STALL_RECOVERY_DELAYS_SEC.length) { + return DATA_STALL_RECOVERY_DELAYS_SEC[DATA_STALL_RECOVERY_DELAYS_SEC.length - 1]; } else { - return DATA_STALL_RESET_DELAYS_SEC[count]; + return DATA_STALL_RECOVERY_DELAYS_SEC[count]; } } @@ -3044,7 +3049,6 @@ public class Vpn { @Nullable private IkeSessionWrapper mSession; @Nullable private IkeSessionConnectionInfo mIkeConnectionInfo; - @Nullable private VpnConnectivityDiagnosticsCallback mDiagnosticsCallback; // mMobikeEnabled can only be updated after IKE AUTH is finished. private boolean mMobikeEnabled = false; @@ -3055,7 +3059,7 @@ public class Vpn { *

This variable controls the retry delay, and is reset when the VPN pass network * validation. */ - private int mDataStallRetryCount = 0; + private int mValidationFailRetryCount = 0; /** * The number of attempts since the last successful connection. @@ -3136,15 +3140,6 @@ public class Vpn { mConnectivityManager.registerSystemDefaultNetworkCallback(mNetworkCallback, new Handler(mLooper)); } - - // DiagnosticsCallback may return more than one alive VPNs, but VPN will filter based on - // Network object. - final NetworkRequest diagRequest = new NetworkRequest.Builder() - .addTransportType(TRANSPORT_VPN) - .removeCapability(NET_CAPABILITY_NOT_VPN).build(); - mDiagnosticsCallback = new VpnConnectivityDiagnosticsCallback(); - mConnectivityDiagnosticsManager.registerConnectivityDiagnosticsCallback( - diagRequest, mExecutor, mDiagnosticsCallback); } private boolean isActiveNetwork(@Nullable Network network) { @@ -3875,39 +3870,12 @@ public class Vpn { } } - class VpnConnectivityDiagnosticsCallback - extends ConnectivityDiagnosticsManager.ConnectivityDiagnosticsCallback { - // The callback runs in the executor thread. - @Override - public void onDataStallSuspected( - ConnectivityDiagnosticsManager.DataStallReport report) { - synchronized (Vpn.this) { - // Ignore stale runner. - if (mVpnRunner != Vpn.IkeV2VpnRunner.this) return; - - // Handle the report only for current VPN network. If data stall is already - // reported, ignoring the other reports. It means that the stall is not - // recovered by MOBIKE and should be on the way to reset the ike session. - if (mNetworkAgent != null - && mNetworkAgent.getNetwork().equals(report.getNetwork()) - && !mDataStallSuspected) { - Log.d(TAG, "Data stall suspected"); - - // Trigger MOBIKE. - maybeMigrateIkeSessionAndUpdateVpnTransportInfo(mActiveNetwork); - mDataStallSuspected = true; - } - } - } - } - public void onValidationStatus(int status) { mEventChanges.log("[Validation] validation status " + status); if (status == NetworkAgent.VALIDATION_STATUS_VALID) { // No data stall now. Reset it. mExecutor.execute(() -> { - mDataStallSuspected = false; - mDataStallRetryCount = 0; + mValidationFailRetryCount = 0; if (mScheduledHandleDataStallFuture != null) { Log.d(TAG, "Recovered from stall. Cancel pending reset action."); mScheduledHandleDataStallFuture.cancel(false /* mayInterruptIfRunning */); @@ -3918,8 +3886,21 @@ public class Vpn { // Skip other invalid status if the scheduled recovery exists. if (mScheduledHandleDataStallFuture != null) return; + if (mValidationFailRetryCount < MAX_MOBIKE_RECOVERY_ATTEMPT) { + Log.d(TAG, "Validation failed"); + + // Trigger MOBIKE to recover first. + mExecutor.schedule(() -> { + maybeMigrateIkeSessionAndUpdateVpnTransportInfo(mActiveNetwork); + }, mDeps.getValidationFailRecoverySeconds(mValidationFailRetryCount++), + TimeUnit.SECONDS); + return; + } + + // Data stall is not recovered by MOBIKE. Try to reset session to recover it. mScheduledHandleDataStallFuture = mExecutor.schedule(() -> { - if (mDataStallSuspected) { + // Only perform the recovery when the network is still bad. + if (mValidationFailRetryCount > 0) { Log.d(TAG, "Reset session to recover stalled network"); // This will reset old state if it exists. startIkeSession(mActiveNetwork); @@ -3928,7 +3909,9 @@ public class Vpn { // Reset mScheduledHandleDataStallFuture since it's already run on executor // thread. mScheduledHandleDataStallFuture = null; - }, mDeps.getDataStallResetSessionSeconds(mDataStallRetryCount++), TimeUnit.SECONDS); + // TODO: compute the delay based on the last recovery timestamp + }, mDeps.getValidationFailRecoverySeconds(mValidationFailRetryCount++), + TimeUnit.SECONDS); } } @@ -4231,8 +4214,6 @@ public class Vpn { mCarrierConfigManager.unregisterCarrierConfigChangeListener( mCarrierConfigChangeListener); mConnectivityManager.unregisterNetworkCallback(mNetworkCallback); - mConnectivityDiagnosticsManager.unregisterConnectivityDiagnosticsCallback( - mDiagnosticsCallback); clearVpnNetworkPreference(mSessionKey); mExecutor.shutdown(); @@ -5216,7 +5197,7 @@ public class Vpn { pw.println("MOBIKE " + (runner.mMobikeEnabled ? "enabled" : "disabled")); pw.println("Profile: " + runner.mProfile); pw.println("Token: " + runner.mCurrentToken); - if (mDataStallSuspected) pw.println("Data stall suspected"); + pw.println("Validation failed retry count:" + runner.mValidationFailRetryCount); if (runner.mScheduledHandleDataStallFuture != null) { pw.println("Reset session scheduled"); } From 6585022b7419f6776290e328976373d637034f39 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Tue, 4 Jul 2023 02:39:27 +0000 Subject: [PATCH 2/2] Update the time unit for recovery timer Tests have to wait at least one second if tests have to execute after certain delay. Use millisecond as its unit so that tests could be executed faster. Test: atest FrameworksNetTests Change-Id: I653235c267c6b625e63f51f6c12b6fd5424f736c --- .../com/android/server/connectivity/Vpn.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 0786411695b03..79c50804ac5fc 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -288,9 +288,8 @@ public class Vpn { *

If retries have exceeded the length of this array, the last entry in the array will be * used as a repeating interval. */ - // TODO: use ms instead to speed up the test. - private static final long[] DATA_STALL_RECOVERY_DELAYS_SEC = - {1L, 5L, 30L, 60L, 120L, 240L, 480L, 960L}; + private static final long[] DATA_STALL_RECOVERY_DELAYS_MS = + {1000L, 5000L, 30000L, 60000L, 120000L, 240000L, 480000L, 960000L}; /** * Maximum attempts to perform MOBIKE when the network is bad. */ @@ -693,11 +692,11 @@ public class Vpn { * Get the length of time to wait before perform data stall recovery when the validation * result is bad. */ - public long getValidationFailRecoverySeconds(int count) { - if (count >= DATA_STALL_RECOVERY_DELAYS_SEC.length) { - return DATA_STALL_RECOVERY_DELAYS_SEC[DATA_STALL_RECOVERY_DELAYS_SEC.length - 1]; + public long getValidationFailRecoveryMs(int count) { + if (count >= DATA_STALL_RECOVERY_DELAYS_MS.length) { + return DATA_STALL_RECOVERY_DELAYS_MS[DATA_STALL_RECOVERY_DELAYS_MS.length - 1]; } else { - return DATA_STALL_RECOVERY_DELAYS_SEC[count]; + return DATA_STALL_RECOVERY_DELAYS_MS[count]; } } @@ -3892,8 +3891,8 @@ public class Vpn { // Trigger MOBIKE to recover first. mExecutor.schedule(() -> { maybeMigrateIkeSessionAndUpdateVpnTransportInfo(mActiveNetwork); - }, mDeps.getValidationFailRecoverySeconds(mValidationFailRetryCount++), - TimeUnit.SECONDS); + }, mDeps.getValidationFailRecoveryMs(mValidationFailRetryCount++), + TimeUnit.MILLISECONDS); return; } @@ -3910,8 +3909,8 @@ public class Vpn { // thread. mScheduledHandleDataStallFuture = null; // TODO: compute the delay based on the last recovery timestamp - }, mDeps.getValidationFailRecoverySeconds(mValidationFailRetryCount++), - TimeUnit.SECONDS); + }, mDeps.getValidationFailRecoveryMs(mValidationFailRetryCount++), + TimeUnit.MILLISECONDS); } }