From a37eaff1ea31b21e5264a3be9baef13c74d25077 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 25 Mar 2021 23:17:36 +0900 Subject: [PATCH 1/3] Address comments on onBlockedStatusChanged(Network, int) CL. Test: m Bug: 165835257 Change-Id: I6d3007a1eac54ee6650b350aee56ed398a2c950d --- framework/src/android/net/ConnectivityManager.java | 4 +++- .../java/com/android/server/ConnectivityService.java | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index d196c1a2d1..cb3f41c66a 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -921,6 +921,7 @@ public class ConnectivityManager { BLOCKED_REASON_DOZE, BLOCKED_REASON_APP_STANDBY, BLOCKED_REASON_RESTRICTED_MODE, + BLOCKED_REASON_LOCKDOWN_VPN, BLOCKED_METERED_REASON_DATA_SAVER, BLOCKED_METERED_REASON_USER_RESTRICTED, BLOCKED_METERED_REASON_ADMIN_DISABLED, @@ -3659,7 +3660,8 @@ public class ConnectivityManager { public void onBlockedStatusChanged(@NonNull Network network, boolean blocked) {} /** - * Called when access to the specified network is blocked or unblocked. + * Called when access to the specified network is blocked or unblocked, or the reason for + * access being blocked changes. * * If a NetworkCallback object implements this method, * {@link #onBlockedStatusChanged(Network, boolean)} will not be called. diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 0c4258561f..1a31509c33 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1551,16 +1551,16 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetworkInfoBlockingLogs.log(action + " " + uid); } - private void maybeLogBlockedStatusChanged(NetworkRequestInfo nri, Network net, - boolean blocked) { + private void maybeLogBlockedStatusChanged(NetworkRequestInfo nri, Network net, int blocked) { if (nri == null || net == null || !LOGD_BLOCKED_NETWORKINFO) { return; } - final String action = blocked ? "BLOCKED" : "UNBLOCKED"; + final String action = (blocked != 0) ? "BLOCKED" : "UNBLOCKED"; final int requestId = nri.getActiveRequest() != null ? nri.getActiveRequest().requestId : nri.mRequests.get(0).requestId; mNetworkInfoBlockingLogs.log(String.format( - "%s %d(%d) on netId %d", action, nri.mAsUid, requestId, net.getNetId())); + "%s %d(%d) on netId %d: %s", action, nri.mAsUid, requestId, net.getNetId(), + blockedReasonsToString(blocked))); } /** @@ -7348,7 +7348,7 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case ConnectivityManager.CALLBACK_BLK_CHANGED: { - maybeLogBlockedStatusChanged(nri, networkAgent.network, arg1 != 0); + maybeLogBlockedStatusChanged(nri, networkAgent.network, arg1); msg.arg1 = arg1; break; } From 6d88078151e43749b506982ec67ca56bf87571f2 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 11 Mar 2021 01:32:09 +0900 Subject: [PATCH 2/3] Add a setTeardownDelayMs API to NetworkAgent. This allows transports to request that when the network is disconnected, the system should delay destroying the native network until the specified time has passed after the network disconnected. Bug: 181941583 Test: next CL in the stack Change-Id: I9765f1c9d1e55c23c6d583d6709dbe06505975b1 --- framework/api/system-current.txt | 3 +- framework/src/android/net/INetworkAgent.aidl | 2 +- .../android/net/INetworkAgentRegistry.aidl | 1 + framework/src/android/net/NetworkAgent.java | 50 ++++++++++++++++--- .../android/server/ConnectivityService.java | 26 +++++++++- .../server/connectivity/NetworkAgentInfo.java | 17 +++++-- 6 files changed, 85 insertions(+), 14 deletions(-) diff --git a/framework/api/system-current.txt b/framework/api/system-current.txt index 1ee79a425e..95ad694293 100644 --- a/framework/api/system-current.txt +++ b/framework/api/system-current.txt @@ -219,7 +219,7 @@ package android.net { method public void onAutomaticReconnectDisabled(); method public void onBandwidthUpdateRequested(); method public void onNetworkCreated(); - method public void onNetworkDisconnected(); + method public void onNetworkDestroyed(); method public void onNetworkUnwanted(); method public void onQosCallbackRegistered(int, @NonNull android.net.QosFilter); method public void onQosCallbackUnregistered(int); @@ -238,6 +238,7 @@ package android.net { method public final void sendQosSessionLost(int, int, int); method public final void sendSocketKeepaliveEvent(int, int); method @Deprecated public void setLegacySubtype(int, @NonNull String); + method public void setTeardownDelayMs(@IntRange(from=0, to=0x1388) int); method public final void setUnderlyingNetworks(@Nullable java.util.List); method public void unregister(); field public static final int VALIDATION_STATUS_NOT_VALID = 2; // 0x2 diff --git a/framework/src/android/net/INetworkAgent.aidl b/framework/src/android/net/INetworkAgent.aidl index f9d399459e..d941d4b95b 100644 --- a/framework/src/android/net/INetworkAgent.aidl +++ b/framework/src/android/net/INetworkAgent.aidl @@ -47,5 +47,5 @@ oneway interface INetworkAgent { void onQosFilterCallbackRegistered(int qosCallbackId, in QosFilterParcelable filterParcel); void onQosCallbackUnregistered(int qosCallbackId); void onNetworkCreated(); - void onNetworkDisconnected(); + void onNetworkDestroyed(); } diff --git a/framework/src/android/net/INetworkAgentRegistry.aidl b/framework/src/android/net/INetworkAgentRegistry.aidl index cbd6193744..26cb1ed6b4 100644 --- a/framework/src/android/net/INetworkAgentRegistry.aidl +++ b/framework/src/android/net/INetworkAgentRegistry.aidl @@ -41,4 +41,5 @@ oneway interface INetworkAgentRegistry { void sendNrQosSessionAvailable(int callbackId, in QosSession session, in NrQosSessionAttributes attributes); void sendQosSessionLost(int qosCallbackId, in QosSession session); void sendQosCallbackError(int qosCallbackId, int exceptionType); + void sendTeardownDelayMs(int teardownDelayMs); } diff --git a/framework/src/android/net/NetworkAgent.java b/framework/src/android/net/NetworkAgent.java index 6b55bb771c..c57da53f28 100644 --- a/framework/src/android/net/NetworkAgent.java +++ b/framework/src/android/net/NetworkAgent.java @@ -184,6 +184,20 @@ public abstract class NetworkAgent { */ public static final int EVENT_UNDERLYING_NETWORKS_CHANGED = BASE + 5; + /** + * Sent by the NetworkAgent to ConnectivityService to pass the current value of the teardown + * delay. + * arg1 = teardown delay in milliseconds + * @hide + */ + public static final int EVENT_TEARDOWN_DELAY_CHANGED = BASE + 6; + + /** + * The maximum value for the teardown delay, in milliseconds. + * @hide + */ + public static final int MAX_TEARDOWN_DELAY_MS = 5000; + /** * Sent by ConnectivityService to the NetworkAgent to inform the agent of the * networks status - whether we could use the network or could not, due to @@ -197,7 +211,6 @@ public abstract class NetworkAgent { */ public static final int CMD_REPORT_NETWORK_STATUS = BASE + 7; - /** * Network validation suceeded. * Corresponds to {@link NetworkCapabilities.NET_CAPABILITY_VALIDATED}. @@ -376,7 +389,7 @@ public abstract class NetworkAgent { * * @hide */ - public static final int CMD_NETWORK_DISCONNECTED = BASE + 23; + public static final int CMD_NETWORK_DESTROYED = BASE + 23; private static NetworkInfo getLegacyNetworkInfo(final NetworkAgentConfig config) { final NetworkInfo ni = new NetworkInfo(config.legacyType, config.legacySubType, @@ -581,8 +594,8 @@ public abstract class NetworkAgent { onNetworkCreated(); break; } - case CMD_NETWORK_DISCONNECTED: { - onNetworkDisconnected(); + case CMD_NETWORK_DESTROYED: { + onNetworkDestroyed(); break; } } @@ -732,8 +745,8 @@ public abstract class NetworkAgent { } @Override - public void onNetworkDisconnected() { - mHandler.sendMessage(mHandler.obtainMessage(CMD_NETWORK_DISCONNECTED)); + public void onNetworkDestroyed() { + mHandler.sendMessage(mHandler.obtainMessage(CMD_NETWORK_DESTROYED)); } } @@ -850,6 +863,29 @@ public abstract class NetworkAgent { queueOrSendNetworkInfo(mNetworkInfo); } + /** + * Sets the value of the teardown delay. + * + * The teardown delay is the time between when the network disconnects and when the native + * network corresponding to this {@code NetworkAgent} is destroyed. By default, the native + * network is destroyed immediately. If {@code teardownDelayMs} is non-zero, then when this + * network disconnects, the system will instead immediately mark the network as restricted + * and unavailable to unprivileged apps, but will defer destroying the native network until the + * teardown delay timer expires. + * + * The interfaces in use by this network will remain in use until the native network is + * destroyed and cannot be reused until {@link #onNetworkDestroyed()} is called. + * + * This method may be called at any time while the network is connected. It has no effect if + * the network is already disconnected and the teardown delay timer is running. + * + * @param teardownDelayMs the teardown delay to set, or 0 to disable teardown delay. + */ + public void setTeardownDelayMs( + @IntRange(from = 0, to = MAX_TEARDOWN_DELAY_MS) int teardownDelayMs) { + queueOrSendMessage(reg -> reg.sendTeardownDelayMs(teardownDelayMs)); + } + /** * Change the legacy subtype of this network agent. * @@ -1053,7 +1089,7 @@ public abstract class NetworkAgent { /** * Called when ConnectivityService has successfully destroy this NetworkAgent's native network. */ - public void onNetworkDisconnected() {} + public void onNetworkDestroyed() {} /** * Requests that the network hardware send the specified packet at the specified interval. diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1a31509c33..b437aac88f 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3122,6 +3122,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } break; } + case NetworkAgent.EVENT_TEARDOWN_DELAY_CHANGED: { + if (msg.arg1 >= 0 && msg.arg1 <= NetworkAgent.MAX_TEARDOWN_DELAY_MS) { + nai.teardownDelayMs = msg.arg1; + } else { + logwtf(nai.toShortString() + " set invalid teardown delay " + msg.arg1); + } + } } } @@ -3692,6 +3699,23 @@ public class ConnectivityService extends IConnectivityManager.Stub mLegacyTypeTracker.remove(nai, wasDefault); rematchAllNetworksAndRequests(); mLingerMonitor.noteDisconnect(nai); + + // Immediate teardown. + if (nai.teardownDelayMs == 0) { + destroyNetwork(nai); + return; + } + + // Delayed teardown. + try { + mNetd.networkSetPermissionForNetwork(nai.network.netId, INetd.PERMISSION_SYSTEM); + } catch (RemoteException e) { + Log.d(TAG, "Error marking network restricted during teardown: " + e); + } + mHandler.postDelayed(() -> destroyNetwork(nai), nai.teardownDelayMs); + } + + private void destroyNetwork(NetworkAgentInfo nai) { if (nai.created) { // Tell netd to clean up the configuration for this network // (routing rules, DNS, etc). @@ -3704,7 +3728,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mDnsManager.removeNetwork(nai.network); } mNetIdManager.releaseNetId(nai.network.getNetId()); - nai.onNetworkDisconnected(); + nai.onNetworkDestroyed(); } private boolean createNativeNetwork(@NonNull NetworkAgentInfo networkAgent) { diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 97df5bff49..ee32fbf00d 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -201,6 +201,9 @@ public class NetworkAgentInfo implements Comparable { // Set to true when partial connectivity was detected. public boolean partialConnectivity; + // Delay between when the network is disconnected and when the native network is destroyed. + public int teardownDelayMs; + // Captive portal info of the network from RFC8908, if any. // Obtained by ConnectivityService and merged into NetworkAgent-provided information. public CaptivePortalData capportApiData; @@ -589,13 +592,13 @@ public class NetworkAgentInfo implements Comparable { } /** - * Notify the NetworkAgent that the network is disconnected and destroyed. + * Notify the NetworkAgent that the native network has been destroyed. */ - public void onNetworkDisconnected() { + public void onNetworkDestroyed() { try { - networkAgent.onNetworkDisconnected(); + networkAgent.onNetworkDestroyed(); } catch (RemoteException e) { - Log.e(TAG, "Error sending network disconnected event", e); + Log.e(TAG, "Error sending network destroyed event", e); } } @@ -675,6 +678,12 @@ public class NetworkAgentInfo implements Comparable { @QosCallbackException.ExceptionType final int exceptionType) { mQosCallbackTracker.sendEventQosCallbackError(qosCallbackId, exceptionType); } + + @Override + public void sendTeardownDelayMs(int teardownDelayMs) { + mHandler.obtainMessage(NetworkAgent.EVENT_TEARDOWN_DELAY_CHANGED, + teardownDelayMs, 0, new Pair<>(NetworkAgentInfo.this, null)).sendToTarget(); + } } /** From 1bc9ad03755c126913e9cbd645a338f256dc9d95 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 26 Mar 2021 14:57:02 +0900 Subject: [PATCH 3/3] Add test coverage for NetworkAgent callbacks. Tests the onNetworkCreated, onNetworkUnwanted and onNetworkDisconnected callbacks, and the teardown delay timer. Bug: 181941583 Test: atest --rerun-until-failure 500 ConnectivityServiceTest#testNetworkAgentCallbacks Change-Id: If539cf5d01ba23193afab2433ed0ac4e7f0550ec --- .../server/ConnectivityServiceTest.java | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 10b2f1e9bc..5bcf295b1e 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -715,6 +715,9 @@ public class ConnectivityServiceTest { private int mProbesSucceeded; private String mNmValidationRedirectUrl = null; private boolean mNmProvNotificationRequested = false; + private Runnable mCreatedCallback; + private Runnable mUnwantedCallback; + private Runnable mDisconnectedCallback; private final ConditionVariable mNetworkStatusReceived = new ConditionVariable(); // Contains the redirectUrl from networkStatus(). Before reading, wait for @@ -769,6 +772,24 @@ public class ConnectivityServiceTest { mRedirectUrl = redirectUrl; mNetworkStatusReceived.open(); } + + @Override + public void onNetworkCreated() { + super.onNetworkCreated(); + if (mCreatedCallback != null) mCreatedCallback.run(); + } + + @Override + public void onNetworkUnwanted() { + super.onNetworkUnwanted(); + if (mUnwantedCallback != null) mUnwantedCallback.run(); + } + + @Override + public void onNetworkDestroyed() { + super.onNetworkDestroyed(); + if (mDisconnectedCallback != null) mDisconnectedCallback.run(); + } }; assertEquals(na.getNetwork().netId, nmNetworkCaptor.getValue().netId); @@ -970,6 +991,18 @@ public class ConnectivityServiceTest { p.timestampMillis = DATA_STALL_TIMESTAMP; mNmCallbacks.notifyDataStallSuspected(p); } + + public void setCreatedCallback(Runnable r) { + mCreatedCallback = r; + } + + public void setUnwantedCallback(Runnable r) { + mUnwantedCallback = r; + } + + public void setDisconnectedCallback(Runnable r) { + mDisconnectedCallback = r; + } } /** @@ -2798,6 +2831,94 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(callback); } + @Test + public void testNetworkAgentCallbacks() throws Exception { + // Keeps track of the order of events that happen in this test. + final LinkedBlockingQueue eventOrder = new LinkedBlockingQueue<>(); + + final NetworkRequest request = new NetworkRequest.Builder() + .addTransportType(TRANSPORT_WIFI).build(); + final TestNetworkCallback callback = new TestNetworkCallback(); + final AtomicReference wifiNetwork = new AtomicReference<>(); + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + + // Expectations for state when various callbacks fire. These expectations run on the handler + // thread and not on the test thread because they need to prevent the handler thread from + // advancing while they examine state. + + // 1. When onCreated fires, netd has been told to create the network. + mWiFiNetworkAgent.setCreatedCallback(() -> { + eventOrder.offer("onNetworkCreated"); + wifiNetwork.set(mWiFiNetworkAgent.getNetwork()); + assertNotNull(wifiNetwork.get()); + try { + verify(mMockNetd).networkCreatePhysical(wifiNetwork.get().getNetId(), + INetd.PERMISSION_NONE); + } catch (RemoteException impossible) { + fail(); + } + }); + + // 2. onNetworkUnwanted isn't precisely ordered with respect to any particular events. Just + // check that it is fired at some point after disconnect. + mWiFiNetworkAgent.setUnwantedCallback(() -> eventOrder.offer("onNetworkUnwanted")); + + // 3. While the teardown timer is running, connectivity APIs report the network is gone, but + // netd has not yet been told to destroy it. + final Runnable duringTeardown = () -> { + eventOrder.offer("timePasses"); + assertNull(mCm.getLinkProperties(wifiNetwork.get())); + try { + verify(mMockNetd, never()).networkDestroy(wifiNetwork.get().getNetId()); + } catch (RemoteException impossible) { + fail(); + } + }; + + // 4. After onNetworkDisconnected is called, connectivity APIs report the network is gone, + // and netd has been told to destroy it. + mWiFiNetworkAgent.setDisconnectedCallback(() -> { + eventOrder.offer("onNetworkDisconnected"); + assertNull(mCm.getLinkProperties(wifiNetwork.get())); + try { + verify(mMockNetd).networkDestroy(wifiNetwork.get().getNetId()); + } catch (RemoteException impossible) { + fail(); + } + }); + + // Connect a network, and file a request for it after it has come up, to ensure the nascent + // timer is cleared and the test does not have to wait for it. Filing the request after the + // network has come up is necessary because ConnectivityService does not appear to clear the + // nascent timer if the first request satisfied by the network was filed before the network + // connected. + // TODO: fix this bug, file the request before connecting, and remove the waitForIdle. + mWiFiNetworkAgent.connectWithoutInternet(); + waitForIdle(); + mCm.requestNetwork(request, callback); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + + // Set teardown delay and make sure CS has processed it. + mWiFiNetworkAgent.getNetworkAgent().setTeardownDelayMs(300); + waitForIdle(); + + // Post the duringTeardown lambda to the handler so it fires while teardown is in progress. + // The delay must be long enough it will run after the unregisterNetworkCallback has torn + // down the network and started the teardown timer, and short enough that the lambda is + // scheduled to run before the teardown timer. + final Handler h = new Handler(mCsHandlerThread.getLooper()); + h.postDelayed(duringTeardown, 150); + + // Disconnect the network and check that events happened in the right order. + mCm.unregisterNetworkCallback(callback); + assertEquals("onNetworkCreated", eventOrder.poll(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertEquals("onNetworkUnwanted", eventOrder.poll(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertEquals("timePasses", eventOrder.poll(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + assertEquals("onNetworkDisconnected", eventOrder.poll(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + + mCm.unregisterNetworkCallback(callback); + } + @Test public void testExplicitlySelected() throws Exception { NetworkRequest request = new NetworkRequest.Builder()