From 29b3fdd6b657eefb600190dfe128ae9c73be982d Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 16 Jul 2021 22:16:36 +0000 Subject: [PATCH] Ensure VcnGatewayConnection#isQuitting never gets unset after being set This change ensures that the VcnGatewayConnection can never abort a quitting command; specifically, if a non-quitting disconnect request is processed after a quitting disconnect request, the isQuitting value MUST continue to stay set (as true). Failure to OR the values results in orphaned VcnGatewayConnection(s), and the VCN network thrashing. Additionally reduce verbosity of local logs, to ensure it better captures failures and logWtf(s) Bug: 192776413 Test: atest FrameworksVcnTests Original-Change: https://android-review.googlesource.com/1768623 Merged-In: Iec8dae701838794261957609bae4e270eaa9cdc7 Change-Id: Iec8dae701838794261957609bae4e270eaa9cdc7 --- .../android/server/VcnManagementService.java | 7 ++-- .../core/java/com/android/server/vcn/Vcn.java | 3 -- .../server/vcn/VcnGatewayConnection.java | 38 +++++++++++------- .../server/vcn/util/OneWayBoolean.java | 39 +++++++++++++++++++ ...cnGatewayConnectionConnectedStateTest.java | 4 ++ ...nGatewayConnectionConnectingStateTest.java | 4 ++ ...atewayConnectionDisconnectedStateTest.java | 6 ++- ...tewayConnectionDisconnectingStateTest.java | 4 ++ ...atewayConnectionRetryTimeoutStateTest.java | 4 ++ 9 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 services/core/java/com/android/server/vcn/util/OneWayBoolean.java diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index f9fd108176275..867c0935ae2ed 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -519,12 +519,14 @@ public class VcnManagementService extends IVcnManagementService.Stub { @GuardedBy("mLock") private void stopVcnLocked(@NonNull ParcelUuid uuidToTeardown) { - final Vcn vcnToTeardown = mVcns.remove(uuidToTeardown); + // Remove in 2 steps. Make sure teardownAsync is triggered before removing from the map. + final Vcn vcnToTeardown = mVcns.get(uuidToTeardown); if (vcnToTeardown == null) { return; } vcnToTeardown.teardownAsynchronously(); + mVcns.remove(uuidToTeardown); // Now that the VCN is removed, notify all registered listeners to refresh their // UnderlyingNetworkPolicy. @@ -1010,18 +1012,15 @@ public class VcnManagementService extends IVcnManagementService.Stub { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, msg); - LOCAL_LOG.log(TAG + " VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, msg); - LOCAL_LOG.log(TAG + " DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, msg, tr); - LOCAL_LOG.log(TAG + " DBG: " + msg + tr); } private void logErr(String msg) { diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java index 44a6d13153fda..9c3721b15f32f 100644 --- a/services/core/java/com/android/server/vcn/Vcn.java +++ b/services/core/java/com/android/server/vcn/Vcn.java @@ -528,18 +528,15 @@ public class Vcn extends Handler { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, getLogPrefix() + msg, tr); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr); } private void logErr(String msg) { diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java index 38427697a9cd5..3c0a05b6edf7d 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -92,6 +92,7 @@ import com.android.server.vcn.UnderlyingNetworkTracker.UnderlyingNetworkTrackerC import com.android.server.vcn.Vcn.VcnGatewayStatusCallback; import com.android.server.vcn.util.LogUtils; import com.android.server.vcn.util.MtuUtils; +import com.android.server.vcn.util.OneWayBoolean; import java.io.IOException; import java.net.Inet4Address; @@ -551,8 +552,13 @@ public class VcnGatewayConnection extends StateMachine { *

This variable is false for the lifecycle of the VcnGatewayConnection, until a command to * teardown has been received. This may be flipped due to events such as the Network becoming * unwanted, the owning VCN entering safe mode, or an irrecoverable internal failure. + * + *

WARNING: Assignments to this MUST ALWAYS (except for testing) use the or operator ("|="), + * otherwise the flag may be flipped back to false after having been set to true. This could + * lead to a case where the Vcn parent instance has commanded a teardown, but a spurious + * non-quitting disconnect request could flip this back to true. */ - private boolean mIsQuitting = false; + private OneWayBoolean mIsQuitting = new OneWayBoolean(); /** * Whether the VcnGatewayConnection is in safe mode. @@ -794,7 +800,7 @@ public class VcnGatewayConnection extends StateMachine { private void acquireWakeLock() { mVcnContext.ensureRunningOnLooperThread(); - if (!mIsQuitting) { + if (!mIsQuitting.getValue()) { mWakeLock.acquire(); logVdbg("Wakelock acquired: " + mWakeLock); @@ -1297,7 +1303,9 @@ public class VcnGatewayConnection extends StateMachine { // TODO(b/180526152): notify VcnStatusCallback for Network loss logDbg("Tearing down. Cause: " + info.reason); - mIsQuitting = info.shouldQuit; + if (info.shouldQuit) { + mIsQuitting.setTrue(); + } teardownNetwork(); @@ -1341,7 +1349,7 @@ public class VcnGatewayConnection extends StateMachine { private class DisconnectedState extends BaseState { @Override protected void enterState() { - if (mIsQuitting) { + if (mIsQuitting.getValue()) { quitNow(); // Ignore all queued events; cleanup is complete. } @@ -1365,7 +1373,7 @@ public class VcnGatewayConnection extends StateMachine { break; case EVENT_DISCONNECT_REQUESTED: if (((EventDisconnectRequestedInfo) msg.obj).shouldQuit) { - mIsQuitting = true; + mIsQuitting.setTrue(); quitNow(); } @@ -1451,7 +1459,10 @@ public class VcnGatewayConnection extends StateMachine { break; case EVENT_DISCONNECT_REQUESTED: EventDisconnectRequestedInfo info = ((EventDisconnectRequestedInfo) msg.obj); - mIsQuitting = info.shouldQuit; + if (info.shouldQuit) { + mIsQuitting.setTrue(); + } + teardownNetwork(); if (info.reason.equals(DISCONNECT_REASON_UNDERLYING_NETWORK_LOST)) { @@ -1467,7 +1478,7 @@ public class VcnGatewayConnection extends StateMachine { case EVENT_SESSION_CLOSED: mIkeSession = null; - if (!mIsQuitting && mUnderlying != null) { + if (!mIsQuitting.getValue() && mUnderlying != null) { transitionTo(mSkipRetryTimeout ? mConnectingState : mRetryTimeoutState); } else { teardownNetwork(); @@ -1626,7 +1637,7 @@ public class VcnGatewayConnection extends StateMachine { teardownAsynchronously(); } /* networkUnwantedCallback */, (status) -> { - if (mIsQuitting) { + if (mIsQuitting.getValue()) { return; // Ignore; VcnGatewayConnection quitting or already quit } @@ -2180,18 +2191,15 @@ public class VcnGatewayConnection extends StateMachine { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, getLogPrefix() + msg, tr); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr); } private void logWarn(String msg) { @@ -2238,7 +2246,7 @@ public class VcnGatewayConnection extends StateMachine { + (getCurrentState() == null ? null : getCurrentState().getClass().getSimpleName())); - pw.println("mIsQuitting: " + mIsQuitting); + pw.println("mIsQuitting: " + mIsQuitting.getValue()); pw.println("mIsInSafeMode: " + mIsInSafeMode); pw.println("mCurrentToken: " + mCurrentToken); pw.println("mFailedAttempts: " + mFailedAttempts); @@ -2275,12 +2283,12 @@ public class VcnGatewayConnection extends StateMachine { @VisibleForTesting(visibility = Visibility.PRIVATE) boolean isQuitting() { - return mIsQuitting; + return mIsQuitting.getValue(); } @VisibleForTesting(visibility = Visibility.PRIVATE) - void setIsQuitting(boolean isQuitting) { - mIsQuitting = isQuitting; + void setQuitting() { + mIsQuitting.setTrue(); } @VisibleForTesting(visibility = Visibility.PRIVATE) diff --git a/services/core/java/com/android/server/vcn/util/OneWayBoolean.java b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java new file mode 100644 index 0000000000000..e79bb2d2547db --- /dev/null +++ b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.vcn.util; + +/** + * OneWayBoolean is an abstraction for a boolean that MUST only ever be flipped from false to true + * + *

This class allows the providing of a guarantee that a flag will never be flipped back after + * being set. + * + * @hide + */ +public class OneWayBoolean { + private boolean mValue = false; + + /** Get boolean value. */ + public boolean getValue() { + return mValue; + } + + /** Sets the value to true. */ + public void setTrue() { + mValue = true; + } +} diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java index 6bfbfb1c8496f..0f84f6ebe5221 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java @@ -578,6 +578,10 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); assertTrue(mGatewayConnection.isQuitting()); } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java index acc8bf98e95b0..d1f3a210d8707 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java @@ -127,6 +127,10 @@ public class VcnGatewayConnectionConnectingStateTest extends VcnGatewayConnectio mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); assertTrue(mGatewayConnection.isQuitting()); } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java index ac0edaa3b5796..2056eea42ce6d 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java @@ -68,7 +68,7 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect true /* isMobileDataEnabled */, mDeps); - vgc.setIsQuitting(true); + vgc.setQuitting(); vgc.transitionTo(vgc.mDisconnectedState); mTestLooper.dispatchAll(); @@ -102,6 +102,10 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertNull(mGatewayConnection.getCurrentState()); verify(mIpSecSvc).deleteTunnelInterface(eq(TEST_IPSEC_TUNNEL_RESOURCE_ID), any()); verifySafeModeTimeoutAlarmAndGetCallback(true /* expectCanceled */); diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java index 9da8b451c9fc4..78aefad9f8fff 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java @@ -79,6 +79,10 @@ public class VcnGatewayConnectionDisconnectingStateTest extends VcnGatewayConnec mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + // Should do nothing; already tearing down. assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); verifyTeardownTimeoutAlarmAndGetCallback(false /* expectCanceled */); diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java index 69407657b3c40..1c859790a2feb 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java @@ -90,6 +90,10 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect .onSelectedUnderlyingNetworkChanged(null); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);