From 96dba63235dc60ec76c2e4bf8a6bd68086d88864 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 2 Dec 2020 00:48:09 +0900 Subject: [PATCH] Generalize support for underlying networks. Currently, ConnectivityService assumes that only VPNs can have underlying networks. Make the code decide this based only on the return value of NetworkAgentInfo#supportsUnderlyingNetworks. This allows non-VPN network types to support underlying networks in the future. This requires storing the original agent's capabilities in NetworkAgentInfo so that applyUnderlyingCapabilities can mix in the underlying network capabilities without overwriting the capabilities of the network itself. Currently, the only information that applyUnderlyingCapabilities takes from the original agent's capabilities are the metered bit (stored in NetworkAgentInfo#declaredMetered) and the transports (assumed to be exactly {TRANSPORT_VPN}. Store the full capabilities instead. This is more state than needed but it ensures that we do not need to make any changes if in the future we want to propagate new types of information from the underlying networks. This should have no impact on current use cases (i.e., VPNs). There is a change in ordering: in disconnectAndDestroyNetwork, the new code propagates underlying network capabilities before removing the network from LegacyTypeTracker, instead of after. This is done to simplify the new code. When the new code propagates underlying network capabilities in response to a change for a particular network (e.g., connect, disconnect, capabilities change), it only considers networks that have the changed network as underlying. Because determining the underlying networks requires knowing the default network, the new code runs before the default network is changed and LegacyTypeTracker is updated. This shouldn't have app implications because the connectivity broadcasts sent by LegacyTypeTracker and the callbacks cannot be ordered, since they run on separate threads with unpredictable delays. The capability change callbacks resulting from propagation of underlying network capabilities were already sent before the rematch, so the callbacks themselves are not reordered in any way. Bug: 173331190 Test: atest FrameworksNetTests \ CtsNetTestCases:NetworkAgentTest \ CtsNetTestCases:Ikev2VpnTest \ CtsNetTestCases:VpnServiceTest \ CtsNetTestCases:android.net.cts.ConnectivityDiagnosticsManagerTest \ HostsideVpnTests com.android.server.connectivity.VpnTest Change-Id: Ic5353a928a3a3541dcf953c35f47277c5e295db8 --- .../android/server/ConnectivityService.java | 96 +++++++++++-------- .../server/connectivity/NetworkAgentInfo.java | 7 +- .../server/ConnectivityServiceTest.java | 24 ++--- 3 files changed, 71 insertions(+), 56 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 16cff7bb7f..479c885812 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2812,6 +2812,7 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case NetworkAgent.EVENT_UNDERLYING_NETWORKS_CHANGED: { + // TODO: prevent loops, e.g., if a network declares itself as underlying. if (!nai.supportsUnderlyingNetworks()) { Log.wtf(TAG, "Non-virtual networks cannot have underlying networks"); break; @@ -3410,6 +3411,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } nai.clearLingerState(); + propagateUnderlyingNetworkCapabilities(nai.network); if (nai.isSatisfyingRequest(mDefaultRequest.requestId)) { mDefaultNetworkNai = null; updateDataActivityTracking(null /* newNetwork */, nai); @@ -3417,9 +3419,6 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureNetworkTransitionWakelock(nai.toShortString()); } mLegacyTypeTracker.remove(nai, wasDefault); - if (!nai.networkCapabilities.hasTransport(TRANSPORT_VPN)) { - propagateUnderlyingNetworkCapabilities(); - } rematchAllNetworksAndRequests(); mLingerMonitor.noteDisconnect(nai); if (nai.created) { @@ -4803,17 +4802,35 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private Network[] underlyingNetworksOrDefault(Network[] underlyingNetworks) { + final Network defaultNetwork = getNetwork(getDefaultNetwork()); + if (underlyingNetworks == null && defaultNetwork != null) { + // null underlying networks means to track the default. + underlyingNetworks = new Network[] { defaultNetwork }; + } + return underlyingNetworks; + } + + // Returns true iff |network| is an underlying network of |nai|. + private boolean hasUnderlyingNetwork(NetworkAgentInfo nai, Network network) { + // TODO: support more than one level of underlying networks, either via a fixed-depth search + // (e.g., 2 levels of underlying networks), or via loop detection, or.... + if (!nai.supportsUnderlyingNetworks()) return false; + final Network[] underlying = underlyingNetworksOrDefault(nai.declaredUnderlyingNetworks); + return ArrayUtils.contains(underlying, network); + } + /** - * Ask all networks with underlying networks to recompute and update their capabilities. + * Recompute the capabilities for any networks that had a specific network as underlying. * * When underlying networks change, such networks may have to update capabilities to reflect * things like the metered bit, their transports, and so on. The capabilities are calculated * immediately. This method runs on the ConnectivityService thread. */ - private void propagateUnderlyingNetworkCapabilities() { + private void propagateUnderlyingNetworkCapabilities(Network updatedNetwork) { ensureRunningOnConnectivityServiceThread(); for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { - if (nai.supportsUnderlyingNetworks()) { + if (updatedNetwork == null || hasUnderlyingNetwork(nai, updatedNetwork)) { updateCapabilitiesForNetwork(nai); } } @@ -6351,27 +6368,28 @@ public class ConnectivityService extends IConnectivityManager.Stub * This method should never alter the agent's NetworkCapabilities, only store data in |nai|. */ private void processCapabilitiesFromAgent(NetworkAgentInfo nai, NetworkCapabilities nc) { - nai.declaredMetered = !nc.hasCapability(NET_CAPABILITY_NOT_METERED); + // Note: resetting the owner UID before storing the agent capabilities in NAI means that if + // the agent attempts to change the owner UID, then nai.declaredCapabilities will not + // actually be the same as the capabilities sent by the agent. Still, it is safer to reset + // the owner UID here and behave as if the agent had never tried to change it. if (nai.networkCapabilities.getOwnerUid() != nc.getOwnerUid()) { Log.e(TAG, nai.toShortString() + ": ignoring attempt to change owner from " + nai.networkCapabilities.getOwnerUid() + " to " + nc.getOwnerUid()); nc.setOwnerUid(nai.networkCapabilities.getOwnerUid()); } + nai.declaredCapabilities = new NetworkCapabilities(nc); } - /** Modifies |caps| based on the capabilities of the specified underlying networks. */ + /** Modifies |newNc| based on the capabilities of |underlyingNetworks| and |agentCaps|. */ @VisibleForTesting void applyUnderlyingCapabilities(@Nullable Network[] underlyingNetworks, - @NonNull NetworkCapabilities caps, boolean declaredMetered) { - final Network defaultNetwork = getNetwork(getDefaultNetwork()); - if (underlyingNetworks == null && defaultNetwork != null) { - // null underlying networks means to track the default. - underlyingNetworks = new Network[] { defaultNetwork }; - } - int[] transportTypes = new int[] { NetworkCapabilities.TRANSPORT_VPN }; + @NonNull NetworkCapabilities agentCaps, @NonNull NetworkCapabilities newNc) { + underlyingNetworks = underlyingNetworksOrDefault(underlyingNetworks); + int[] transportTypes = agentCaps.getTransportTypes(); int downKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; int upKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; - boolean metered = declaredMetered; // metered if any underlying is metered, or agentMetered + // metered if any underlying is metered, or originally declared metered by the agent. + boolean metered = !agentCaps.hasCapability(NET_CAPABILITY_NOT_METERED); boolean roaming = false; // roaming if any underlying is roaming boolean congested = false; // congested if any underlying is congested boolean suspended = true; // suspended if all underlying are suspended @@ -6417,13 +6435,13 @@ public class ConnectivityService extends IConnectivityManager.Stub suspended = false; } - caps.setTransportTypes(transportTypes); - caps.setLinkDownstreamBandwidthKbps(downKbps); - caps.setLinkUpstreamBandwidthKbps(upKbps); - caps.setCapability(NET_CAPABILITY_NOT_METERED, !metered); - caps.setCapability(NET_CAPABILITY_NOT_ROAMING, !roaming); - caps.setCapability(NET_CAPABILITY_NOT_CONGESTED, !congested); - caps.setCapability(NET_CAPABILITY_NOT_SUSPENDED, !suspended); + newNc.setTransportTypes(transportTypes); + newNc.setLinkDownstreamBandwidthKbps(downKbps); + newNc.setLinkUpstreamBandwidthKbps(upKbps); + newNc.setCapability(NET_CAPABILITY_NOT_METERED, !metered); + newNc.setCapability(NET_CAPABILITY_NOT_ROAMING, !roaming); + newNc.setCapability(NET_CAPABILITY_NOT_CONGESTED, !congested); + newNc.setCapability(NET_CAPABILITY_NOT_SUSPENDED, !suspended); } /** @@ -6480,7 +6498,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } if (nai.supportsUnderlyingNetworks()) { - applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, newNc, nai.declaredMetered); + applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, nai.declaredCapabilities, + newNc); } return newNc; @@ -6559,11 +6578,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - if (!newNc.hasTransport(TRANSPORT_VPN)) { - // Tell VPNs about updated capabilities, since they may need to - // bubble those changes through. - propagateUnderlyingNetworkCapabilities(); - } + // This network might have been underlying another network. Propagate its capabilities. + propagateUnderlyingNetworkCapabilities(nai.network); if (!newNc.equalsTransportTypes(prevNc)) { mDnsManager.updateTransportsForNetwork(nai.network.netId, newNc.getTransportTypes()); @@ -6885,8 +6901,10 @@ public class ConnectivityService extends IConnectivityManager.Stub updateTcpBufferSizes(null != newNetwork ? newNetwork.linkProperties.getTcpBufferSizes() : null); notifyIfacesChangedForNetworkStats(); - // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. - propagateUnderlyingNetworkCapabilities(); + // Fix up the NetworkCapabilities of any networks that have this network as underlying. + if (newNetwork != null) { + propagateUnderlyingNetworkCapabilities(newNetwork.network); + } } private void processListenRequests(@NonNull final NetworkAgentInfo nai) { @@ -7342,13 +7360,11 @@ public class ConnectivityService extends IConnectivityManager.Stub networkAgent.networkCapabilities.addCapability(NET_CAPABILITY_FOREGROUND); if (!createNativeNetwork(networkAgent)) return; - if (networkAgent.isVPN()) { - // Initialize the VPN capabilities to their starting values according to the - // underlying networks. This will avoid a spurious callback to - // onCapabilitiesUpdated being sent in updateAllVpnCapabilities below as - // the VPN would switch from its default, blank capabilities to those - // that reflect the capabilities of its underlying networks. - propagateUnderlyingNetworkCapabilities(); + if (networkAgent.supportsUnderlyingNetworks()) { + // Initialize the network's capabilities to their starting values according to the + // underlying networks. This ensures that the capabilities are correct before + // anything happens to the network. + updateCapabilitiesForNetwork(networkAgent); } networkAgent.created = true; } @@ -7390,10 +7406,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // doing. updateSignalStrengthThresholds(networkAgent, "CONNECT", null); - if (networkAgent.supportsUnderlyingNetworks()) { - propagateUnderlyingNetworkCapabilities(); - } - // Consider network even though it is not yet validated. rematchAllNetworksAndRequests(); diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 3270dd5521..e189815c7b 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -138,9 +138,10 @@ public class NetworkAgentInfo implements Comparable { // not guaranteed to be current or correct, or even to exist. public @Nullable Network[] declaredUnderlyingNetworks; - // Whether this network is always metered even if its underlying networks are unmetered. - // Only relevant if #supportsUnderlyingNetworks is true. - public boolean declaredMetered; + // The capabilities originally announced by the NetworkAgent, regardless of any capabilities + // that were added or removed due to this network's underlying networks. + // Only set if #supportsUnderlyingNetworks is true. + public @Nullable NetworkCapabilities declaredCapabilities; // Indicates if netd has been told to create this Network. From this point on the appropriate // routing rules are setup and routes are added so packets can begin flowing over the Network. diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index a4b6e31182..a7ba64b38e 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5458,6 +5458,7 @@ public class ConnectivityServiceTest { final Network wifi = mWiFiNetworkAgent.getNetwork(); final NetworkCapabilities initialCaps = new NetworkCapabilities(); + initialCaps.addTransportType(TRANSPORT_VPN); initialCaps.addCapability(NET_CAPABILITY_INTERNET); initialCaps.removeCapability(NET_CAPABILITY_NOT_VPN); @@ -5489,44 +5490,45 @@ public class ConnectivityServiceTest { withWifiAndMobileUnderlying.setLinkDownstreamBandwidthKbps(10); withWifiAndMobileUnderlying.setLinkUpstreamBandwidthKbps(20); + final NetworkCapabilities initialCapsNotMetered = new NetworkCapabilities(initialCaps); + initialCapsNotMetered.addCapability(NET_CAPABILITY_NOT_METERED); + NetworkCapabilities caps = new NetworkCapabilities(initialCaps); - final boolean notDeclaredMetered = false; - mService.applyUnderlyingCapabilities(new Network[]{}, caps, notDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{}, initialCapsNotMetered, caps); assertEquals(withNoUnderlying, caps); caps = new NetworkCapabilities(initialCaps); - mService.applyUnderlyingCapabilities(new Network[]{null}, caps, notDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{null}, initialCapsNotMetered, caps); assertEquals(withNoUnderlying, caps); caps = new NetworkCapabilities(initialCaps); - mService.applyUnderlyingCapabilities(new Network[]{mobile}, caps, notDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{mobile}, initialCapsNotMetered, caps); assertEquals(withMobileUnderlying, caps); - mService.applyUnderlyingCapabilities(new Network[]{wifi}, caps, notDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{wifi}, initialCapsNotMetered, caps); assertEquals(withWifiUnderlying, caps); - final boolean isDeclaredMetered = true; withWifiUnderlying.removeCapability(NET_CAPABILITY_NOT_METERED); caps = new NetworkCapabilities(initialCaps); - mService.applyUnderlyingCapabilities(new Network[]{wifi}, caps, isDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{wifi}, initialCaps, caps); assertEquals(withWifiUnderlying, caps); caps = new NetworkCapabilities(initialCaps); - mService.applyUnderlyingCapabilities(new Network[]{mobile, wifi}, caps, isDeclaredMetered); + mService.applyUnderlyingCapabilities(new Network[]{mobile, wifi}, initialCaps, caps); assertEquals(withWifiAndMobileUnderlying, caps); withWifiUnderlying.addCapability(NET_CAPABILITY_NOT_METERED); caps = new NetworkCapabilities(initialCaps); mService.applyUnderlyingCapabilities(new Network[]{null, mobile, null, wifi}, - caps, notDeclaredMetered); + initialCapsNotMetered, caps); assertEquals(withWifiAndMobileUnderlying, caps); caps = new NetworkCapabilities(initialCaps); mService.applyUnderlyingCapabilities(new Network[]{null, mobile, null, wifi}, - caps, notDeclaredMetered); + initialCapsNotMetered, caps); assertEquals(withWifiAndMobileUnderlying, caps); - mService.applyUnderlyingCapabilities(null, caps, notDeclaredMetered); + mService.applyUnderlyingCapabilities(null, initialCapsNotMetered, caps); assertEquals(withWifiUnderlying, caps); }