From 492e239299153a725a967cc37fe8d67a1a49dad5 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Wed, 29 Jun 2022 03:08:15 +0000 Subject: [PATCH 1/3] Stop VPN profiles by exiting VpnRunner instead of prepareInternal Change to call the VpnRunner.exit to stop vpn profile instead of using prepareInternal to leave the package intact. This aligns with the way that VpnServices are disconnected so that the package and other related information will not be reset unexpectedly. In current design, Vpn will examine if the current prepared package matches to prevent an existing always-on VPN from being dethroned by other apps when the VPN always-on is enabled. However, when the VPN is disabled using stopVpnProfile, the current package name will be updated to [LEGACY VPN]. The current package will no longer be the same with the VPN package. This resulted in the rejection of the VPN app to start the VPN using VpnManager.startVpnProfile again. Test: atest FrameworksNetTests Test: manually test with VPN app to reconnect VPN when always-on is enabled. Bug: 235322391 Change-Id: I83e1e1edf6c3a6653d87216afcd397f296f59cf2 (cherry picked from commit 03f0d12480b72780e67cce33473e8c55befc2669) Merged-In: I83e1e1edf6c3a6653d87216afcd397f296f59cf2 --- .../com/android/server/connectivity/Vpn.java | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 98238ccd93c32..8470c07606654 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -1178,20 +1178,9 @@ public class Vpn { cleanupVpnStateLocked(); } else if (mVpnRunner != null) { if (!VpnConfig.LEGACY_VPN.equals(mPackage)) { - mAppOpsManager.finishOp( - AppOpsManager.OPSTR_ESTABLISH_VPN_MANAGER, mOwnerUID, mPackage, null); - // The underlying network, NetworkCapabilities and LinkProperties are not - // necessary to send to VPN app since the purpose of this event is to notify - // VPN app that VPN is deactivated by the user. - // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from - // ConnectivityServiceTest. - if (SdkLevel.isAtLeastT()) { - sendEventToVpnManagerApp(VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, - -1 /* errorClass */, -1 /* errorCode*/, mPackage, - getSessionKeyLocked(), makeVpnProfileStateLocked(), - null /* underlyingNetwork */, null /* nc */, null /* lp */); - } + notifyVpnManagerVpnStopped(mPackage, mOwnerUID); } + // cleanupVpnStateLocked() is called from mVpnRunner.exit() mVpnRunner.exit(); } @@ -4090,7 +4079,25 @@ public class Vpn { // To stop the VPN profile, the caller must be the current prepared package and must be // running an Ikev2VpnProfile. if (isCurrentIkev2VpnLocked(packageName)) { - prepareInternal(VpnConfig.LEGACY_VPN); + notifyVpnManagerVpnStopped(packageName, mOwnerUID); + + mVpnRunner.exit(); + } + } + + private synchronized void notifyVpnManagerVpnStopped(String packageName, int ownerUID) { + mAppOpsManager.finishOp( + AppOpsManager.OPSTR_ESTABLISH_VPN_MANAGER, ownerUID, packageName, null); + // The underlying network, NetworkCapabilities and LinkProperties are not + // necessary to send to VPN app since the purpose of this event is to notify + // VPN app that VPN is deactivated by the user. + // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from + // ConnectivityServiceTest. + if (SdkLevel.isAtLeastT()) { + sendEventToVpnManagerApp(VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, + -1 /* errorClass */, -1 /* errorCode*/, packageName, + getSessionKeyLocked(), makeVpnProfileStateLocked(), + null /* underlyingNetwork */, null /* nc */, null /* lp */); } } From 0e411890d4093a9cd2d4e36ad5570414d0d7d884 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Thu, 11 Aug 2022 01:19:01 +0000 Subject: [PATCH 2/3] Invert the order of event sending and VpnRunner.exit() Invert the order to prevent confusing VPN disconnected notification being sent before the runner actually exits. Bug: 235322391 Test: atest FrameworksNetTests Change-Id: Ie83c5ee35509f6ba10465fd9867c596a956e189a Merged-In: Ie83c5ee35509f6ba10465fd9867c596a956e189a --- .../com/android/server/connectivity/Vpn.java | 54 +++++++++++++++---- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 8470c07606654..8d028037aa60e 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -747,7 +747,7 @@ public class Vpn { return true; } - private boolean sendEventToVpnManagerApp(@NonNull String category, int errorClass, + private Intent buildVpnManagerEventIntent(@NonNull String category, int errorClass, int errorCode, @NonNull final String packageName, @Nullable final String sessionKey, @NonNull final VpnProfileState profileState, @Nullable final Network underlyingNetwork, @Nullable final NetworkCapabilities nc, @Nullable final LinkProperties lp) { @@ -766,6 +766,20 @@ public class Vpn { intent.putExtra(VpnManager.EXTRA_ERROR_CODE, errorCode); } + return intent; + } + + private boolean sendEventToVpnManagerApp(@NonNull String category, int errorClass, + int errorCode, @NonNull final String packageName, @Nullable final String sessionKey, + @NonNull final VpnProfileState profileState, @Nullable final Network underlyingNetwork, + @Nullable final NetworkCapabilities nc, @Nullable final LinkProperties lp) { + final Intent intent = buildVpnManagerEventIntent(category, errorClass, errorCode, + packageName, sessionKey, profileState, underlyingNetwork, nc, lp); + return sendEventToVpnManagerApp(intent, packageName); + } + + private boolean sendEventToVpnManagerApp(@NonNull final Intent intent, + @NonNull final String packageName) { // Allow VpnManager app to temporarily run background services to handle this error. // If an app requires anything beyond this grace period, they MUST either declare // themselves as a foreground service, or schedule a job/workitem. @@ -1177,12 +1191,25 @@ public class Vpn { mContext.unbindService(mConnection); cleanupVpnStateLocked(); } else if (mVpnRunner != null) { - if (!VpnConfig.LEGACY_VPN.equals(mPackage)) { - notifyVpnManagerVpnStopped(mPackage, mOwnerUID); + // Build intent first because the sessionKey will be reset after performing + // VpnRunner.exit(). Also, cache mOwnerUID even if ownerUID will not be changed in + // VpnRunner.exit() to prevent design being changed in the future. + // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from + // ConnectivityServiceTest. + final int ownerUid = mOwnerUID; + Intent intent = null; + if (SdkLevel.isAtLeastT() && isVpnApp(mPackage)) { + intent = buildVpnManagerEventIntent( + VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, + -1 /* errorClass */, -1 /* errorCode*/, mPackage, + getSessionKeyLocked(), makeVpnProfileStateLocked(), + null /* underlyingNetwork */, null /* nc */, null /* lp */); } - // cleanupVpnStateLocked() is called from mVpnRunner.exit() mVpnRunner.exit(); + if (intent != null && isVpnApp(mPackage)) { + notifyVpnManagerVpnStopped(mPackage, ownerUid, intent); + } } try { @@ -4079,13 +4106,23 @@ public class Vpn { // To stop the VPN profile, the caller must be the current prepared package and must be // running an Ikev2VpnProfile. if (isCurrentIkev2VpnLocked(packageName)) { - notifyVpnManagerVpnStopped(packageName, mOwnerUID); + // Build intent first because the sessionKey will be reset after performing + // VpnRunner.exit(). Also, cache mOwnerUID even if ownerUID will not be changed in + // VpnRunner.exit() to prevent design being changed in the future. + final int ownerUid = mOwnerUID; + final Intent intent = buildVpnManagerEventIntent( + VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, + -1 /* errorClass */, -1 /* errorCode*/, packageName, + getSessionKeyLocked(), makeVpnProfileStateLocked(), + null /* underlyingNetwork */, null /* nc */, null /* lp */); mVpnRunner.exit(); + notifyVpnManagerVpnStopped(packageName, ownerUid, intent); } } - private synchronized void notifyVpnManagerVpnStopped(String packageName, int ownerUID) { + private synchronized void notifyVpnManagerVpnStopped(String packageName, int ownerUID, + Intent intent) { mAppOpsManager.finishOp( AppOpsManager.OPSTR_ESTABLISH_VPN_MANAGER, ownerUID, packageName, null); // The underlying network, NetworkCapabilities and LinkProperties are not @@ -4094,10 +4131,7 @@ public class Vpn { // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from // ConnectivityServiceTest. if (SdkLevel.isAtLeastT()) { - sendEventToVpnManagerApp(VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, - -1 /* errorClass */, -1 /* errorCode*/, packageName, - getSessionKeyLocked(), makeVpnProfileStateLocked(), - null /* underlyingNetwork */, null /* nc */, null /* lp */); + sendEventToVpnManagerApp(intent, packageName); } } From 704321da7db7edff9bb57c3bb95732f4b37691b3 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Mon, 22 Aug 2022 02:14:57 +0000 Subject: [PATCH 3/3] Skip events on stale Ikev2VpnRunner The exit() of VpnRunner will do exitVpnRunner() and cleanupVpnStateLocked(). But the exitVpnRunner() posts a runnable to the executor. This means that disconnectVpnRunner() might run concurrently with cleanupVpnStateLocked(), and it might even complete after cleanupVpnStateLocked() finishes. After exiting the runner, the states are reset. The remaining events in the executor are irrelavent and should be skipped to prevent accessing any of the outer class's members. Also add missing synchronized block by verifying that there is no other field of the outer Vpn class that is used inside Ikev2VpnRunner implicitly. Bug: 235322391 Test: atest FrameworksNetTests Change-Id: I9eed58b2e96ebaf33e557a42e83525a74a4697d8 Merged-In: I9eed58b2e96ebaf33e557a42e83525a74a4697d8 --- .../com/android/server/connectivity/Vpn.java | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 8d028037aa60e..b988b5731f9b5 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -2903,6 +2903,9 @@ public class Vpn { final LinkProperties lp; synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + mInterface = interfaceName; mConfig.mtu = maxMtu; mConfig.interfaze = mInterface; @@ -3004,6 +3007,9 @@ public class Vpn { try { synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + mConfig.underlyingNetworks = new Network[] {network}; mNetworkCapabilities = new NetworkCapabilities.Builder(mNetworkCapabilities) @@ -3093,7 +3099,12 @@ public class Vpn { // Clear mInterface to prevent Ikev2VpnRunner being cleared when // interfaceRemoved() is called. - mInterface = null; + synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + + mInterface = null; + } // Without MOBIKE, we have no way to seamlessly migrate. Close on old // (non-default) network, and start the new one. resetIkeState(); @@ -3278,6 +3289,9 @@ public class Vpn { /** Marks the state as FAILED, and disconnects. */ private void markFailedAndDisconnect(Exception exception) { synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + updateState(DetailedState.FAILED, exception.getMessage()); } @@ -3316,6 +3330,9 @@ public class Vpn { cancelHandleNetworkLostTimeout(); synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + if (exception instanceof IkeProtocolException) { final IkeProtocolException ikeException = (IkeProtocolException) exception; @@ -3436,6 +3453,9 @@ public class Vpn { Log.d(TAG, "Resetting state for token: " + mCurrentToken); synchronized (Vpn.this) { + // Ignore stale runner. + if (mVpnRunner != this) return; + // Since this method handles non-fatal errors only, set mInterface to null to // prevent the NetworkManagementEventObserver from killing this VPN based on the // interface going down (which we expect).