From d182c40d8c06a664823ef3b6416da1cd7f7b8694 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 9 Nov 2020 10:32:56 +0900 Subject: Move applying underlying caps from Vpn to ConnectivityService. Add support to ConnectivityService to track underlying networks directly instead of through the Vpn class. 1. Communicate all information necessary to propagate underlying network capabilities to ConnectivityService via NetworkAgent. This includes: a. Underlying networks: - Add SystemApi for NetworkAgent to declare its underlying networks to ConnectivityService, and use it in Vpn. - Add a new declaredUnderlyingNetworks member to NetworkAgentInfo and store the underlying networks in it. Move propagation of underlying network capabilities to mixInCapabilities, which is a natural place for it. b. "Always metered" bit: - Communicate this to ConnectivityService via the existing NOT_METERED capability. Store it in a new declaredMetered boolean in NetworkAgentInfo to separate it cleanly from the NOT_METERED bit in the capabilities, which depends on whether the underlying networks are metered or not. In order to ensure that this is only ever changed when a NC update is received from a NetworkAgent, define a new processCapabilitiesFromAgent similar to the existing processLinkPropertiesFromAgent. 2. Ensure that propagating underlying network capabilities does not read the VPN's NetworkCapabilities. In order to do this, ensure that all relevant information on underlying networks and metering is sent to ConnectivityService at NetworkAgent registration time. CS still calls Vpn#updateCapabilities when a user is added/removed, but that is deleted in a future CL. 3. Slightly generalize propagating underlying network capabilities because there may be other network types that also have underlying networks that aren't VPNs (e.g., VCN). - Introduce a new supportsUnderlyingNetworks() boolean method in NetworkAgentInfo. - Rename updateAllVpnsCapabilities to propagateUnderlyingNetworkCapabilities. This commit does not move the actual logic of calculating the underlying capabilities out of Vpn.java. That can be done in a subsequent change once CS stops calling getUnderlyingNetworks(). This commit also does not modify any of the other code in CS that directly accesses VPNs' underlying networks. Bug: 173331190 Test: passes existing tests in ConnectivityServiceTest Test: CTS test in r.android.com/1511114 Test: atest CtsNetTestCases:Ikev2VpnTest HostsideVpnTests Change-Id: I5f76cb1aa4866efed3d5c4590e931fdb0e994f8d --- core/api/system-current.txt | 1 + core/java/android/net/NetworkAgent.java | 44 ++++++++- .../com/android/server/ConnectivityService.java | 103 +++++++++++++++------ .../server/connectivity/NetworkAgentInfo.java | 16 ++++ .../java/com/android/server/connectivity/Vpn.java | 14 ++- .../android/server/ConnectivityServiceTest.java | 9 +- 6 files changed, 157 insertions(+), 30 deletions(-) diff --git a/core/api/system-current.txt b/core/api/system-current.txt index f6b1d0627312..d047a99a834f 100644 --- a/core/api/system-current.txt +++ b/core/api/system-current.txt @@ -6175,6 +6175,7 @@ package android.net { method public final void sendNetworkCapabilities(@NonNull android.net.NetworkCapabilities); method public final void sendNetworkScore(@IntRange(from=0, to=99) int); method public final void sendSocketKeepaliveEvent(int, 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 field public static final int VALIDATION_STATUS_VALID = 1; // 0x1 diff --git a/core/java/android/net/NetworkAgent.java b/core/java/android/net/NetworkAgent.java index 44ebff99f3e9..0676ad4e2322 100644 --- a/core/java/android/net/NetworkAgent.java +++ b/core/java/android/net/NetworkAgent.java @@ -40,6 +40,7 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.time.Duration; import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; @@ -173,6 +174,14 @@ public abstract class NetworkAgent { */ public static final int EVENT_NETWORK_SCORE_CHANGED = BASE + 4; + /** + * Sent by the NetworkAgent to ConnectivityService to pass the current + * list of underlying networks. + * obj = array of Network objects + * @hide + */ + public static final int EVENT_UNDERLYING_NETWORKS_CHANGED = BASE + 5; + /** * 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 @@ -217,7 +226,13 @@ public abstract class NetworkAgent { * The key for the redirect URL in the Bundle argument of {@code CMD_REPORT_NETWORK_STATUS}. * @hide */ - public static String REDIRECT_URL_KEY = "redirect URL"; + public static final String REDIRECT_URL_KEY = "redirect URL"; + + /** + * Bundle key for the underlying networks in {@code EVENT_UNDERLYING_NETWORKS_CHANGED}. + * @hide + */ + public static final String UNDERLYING_NETWORKS_KEY = "underlyingNetworks"; /** * Sent by the NetworkAgent to ConnectivityService to indicate this network was @@ -649,6 +664,33 @@ public abstract class NetworkAgent { queueOrSendMessage(EVENT_NETWORK_PROPERTIES_CHANGED, new LinkProperties(linkProperties)); } + /** + * Must be called by the agent when the network's underlying networks change. + * + *

{@code networks} is one of the following: + *

+ * + * @param underlyingNetworks the new list of underlying networks. + * @see {@link VpnService.Builder#setUnderlyingNetworks(Network[])} + */ + public final void setUnderlyingNetworks(@Nullable List underlyingNetworks) { + final ArrayList underlyingArray = (underlyingNetworks != null) + ? new ArrayList<>(underlyingNetworks) : null; + final Bundle bundle = new Bundle(); + bundle.putParcelableArrayList(UNDERLYING_NETWORKS_KEY, underlyingArray); + queueOrSendMessage(EVENT_UNDERLYING_NETWORKS_CHANGED, bundle); + } + /** * Inform ConnectivityService that this agent has now connected. * Call {@link #unregister} to disconnect. diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index f7de5c023b4f..e96958eb3243 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2771,6 +2771,7 @@ public class ConnectivityService extends IConnectivityManager.Stub networkCapabilities = new NetworkCapabilities(networkCapabilities); networkCapabilities.restrictCapabilitesForTestNetwork(nai.creatorUid); } + processCapabilitiesFromAgent(nai, networkCapabilities); updateCapabilities(nai.getCurrentScore(), nai, networkCapabilities); break; } @@ -2809,6 +2810,31 @@ public class ConnectivityService extends IConnectivityManager.Stub mKeepaliveTracker.handleEventSocketKeepalive(nai, msg); break; } + case NetworkAgent.EVENT_UNDERLYING_NETWORKS_CHANGED: { + if (!nai.supportsUnderlyingNetworks()) { + Log.wtf(TAG, "Non-virtual networks cannot have underlying networks"); + break; + } + final ArrayList underlying; + try { + underlying = ((Bundle) msg.obj).getParcelableArrayList( + NetworkAgent.UNDERLYING_NETWORKS_KEY); + } catch (NullPointerException | ClassCastException e) { + break; + } + final Network[] oldUnderlying = nai.declaredUnderlyingNetworks; + nai.declaredUnderlyingNetworks = (underlying != null) + ? underlying.toArray(new Network[0]) : null; + + if (!Arrays.equals(oldUnderlying, nai.declaredUnderlyingNetworks)) { + if (DBG) { + log(nai.toShortString() + " changed underlying networks to " + + Arrays.toString(nai.declaredUnderlyingNetworks)); + } + updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + notifyIfacesChangedForNetworkStats(); + } + } } } @@ -3394,7 +3420,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } mLegacyTypeTracker.remove(nai, wasDefault); if (!nai.networkCapabilities.hasTransport(TRANSPORT_VPN)) { - updateAllVpnsCapabilities(); + propagateUnderlyingNetworkCapabilities(); } rematchAllNetworksAndRequests(); mLingerMonitor.noteDisconnect(nai); @@ -4774,22 +4800,17 @@ public class ConnectivityService extends IConnectivityManager.Stub } /** - * Ask all VPN objects to recompute and update their capabilities. + * Ask all networks with underlying networks to recompute and update their capabilities. * - * When underlying networks change, VPNs may have to update capabilities to reflect things - * like the metered bit, their transports, and so on. This asks the VPN objects to update - * their capabilities, and as this will cause them to send messages to the ConnectivityService - * handler thread through their agent, this is asynchronous. When the capabilities objects - * are computed they will be up-to-date as they are computed synchronously from here and - * this is running on the ConnectivityService thread. + * 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 updateAllVpnsCapabilities() { - Network defaultNetwork = getNetwork(getDefaultNetwork()); - synchronized (mVpns) { - for (int i = 0; i < mVpns.size(); i++) { - final Vpn vpn = mVpns.valueAt(i); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); + private void propagateUnderlyingNetworkCapabilities() { + ensureRunningOnConnectivityServiceThread(); + for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { + if (nai.supportsUnderlyingNetworks()) { + updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); } } } @@ -5979,6 +6000,7 @@ public class ConnectivityService extends IConnectivityManager.Stub this, mNetd, mDnsResolver, mNMS, providerId, Binder.getCallingUid()); // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says. + processCapabilitiesFromAgent(nai, nc); nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc)); processLinkPropertiesFromAgent(nai, nai.linkProperties); @@ -6019,6 +6041,12 @@ public class ConnectivityService extends IConnectivityManager.Stub updateUids(nai, null, nai.networkCapabilities); } + /** + * Called when receiving LinkProperties directly from a NetworkAgent. + * Stores into |nai| any data coming from the agent that might also be written to the network's + * LinkProperties by ConnectivityService itself. This ensures that the data provided by the + * agent is not lost when updateLinkProperties is called. + */ private void processLinkPropertiesFromAgent(NetworkAgentInfo nai, LinkProperties lp) { lp.ensureDirectlyConnectedRoutes(); nai.clatd.setNat64PrefixFromRa(lp.getNat64Prefix()); @@ -6314,6 +6342,30 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** + * Called when receiving NetworkCapabilities directly from a NetworkAgent. + * Stores into |nai| any data coming from the agent that might also be written to the network's + * NetworkCapabilities by ConnectivityService itself. This ensures that the data provided by the + * agent is not lost when updateCapabilities is called. + */ + private void processCapabilitiesFromAgent(NetworkAgentInfo nai, NetworkCapabilities nc) { + nai.declaredMetered = !nc.hasCapability(NET_CAPABILITY_NOT_METERED); + } + + /** Propagates to |nc| the capabilities declared by the underlying networks of |nai|. */ + private void mixInUnderlyingCapabilities(NetworkAgentInfo nai, NetworkCapabilities nc) { + Network[] underlyingNetworks = nai.declaredUnderlyingNetworks; + Network defaultNetwork = getNetwork(getDefaultNetwork()); + if (underlyingNetworks == null && defaultNetwork != null) { + // null underlying networks means to track the default. + underlyingNetworks = new Network[] { defaultNetwork }; + } + + // TODO(b/124469351): Get capabilities directly from ConnectivityService instead. + final ConnectivityManager cm = mContext.getSystemService(ConnectivityManager.class); + Vpn.applyUnderlyingCapabilities(cm, underlyingNetworks, nc, nai.declaredMetered); + } + /** * Augments the NetworkCapabilities passed in by a NetworkAgent with capabilities that are * maintained here that the NetworkAgent is not aware of (e.g., validated, captive portal, @@ -6367,6 +6419,10 @@ public class ConnectivityService extends IConnectivityManager.Stub newNc.addCapability(NET_CAPABILITY_NOT_ROAMING); } + if (nai.supportsUnderlyingNetworks()) { + mixInUnderlyingCapabilities(nai, newNc); + } + return newNc; } @@ -6446,7 +6502,7 @@ 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. - updateAllVpnsCapabilities(); + propagateUnderlyingNetworkCapabilities(); } if (!newNc.equalsTransportTypes(prevNc)) { @@ -6766,7 +6822,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ? newNetwork.linkProperties.getTcpBufferSizes() : null); notifyIfacesChangedForNetworkStats(); // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. - updateAllVpnsCapabilities(); + propagateUnderlyingNetworkCapabilities(); } private void processListenRequests(@NonNull final NetworkAgentInfo nai) { @@ -7228,7 +7284,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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. - updateAllVpnsCapabilities(); + propagateUnderlyingNetworkCapabilities(); } networkAgent.created = true; } @@ -7270,8 +7326,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // doing. updateSignalStrengthThresholds(networkAgent, "CONNECT", null); - if (networkAgent.isVPN()) { - updateAllVpnsCapabilities(); + if (networkAgent.supportsUnderlyingNetworks()) { + propagateUnderlyingNetworkCapabilities(); } // Consider network even though it is not yet validated. @@ -7528,13 +7584,6 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); success = mVpns.get(user).setUnderlyingNetworks(networks); } - if (success) { - mHandler.post(() -> { - // Update VPN's capabilities based on updated underlying network set. - updateAllVpnsCapabilities(); - notifyIfacesChangedForNetworkStats(); - }); - } return success; } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index a9f62d91592d..3270dd55218c 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -132,6 +132,16 @@ public class NetworkAgentInfo implements Comparable { // TODO: make this private with a getter. public NetworkCapabilities networkCapabilities; public final NetworkAgentConfig networkAgentConfig; + + // Underlying networks declared by the agent. Only set if supportsUnderlyingNetworks is true. + // The networks in this list might be declared by a VPN app using setUnderlyingNetworks and are + // 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; + // 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. // This is a sticky bit; once set it is never cleared. @@ -474,10 +484,16 @@ public class NetworkAgentInfo implements Comparable { networkCapabilities); } + /** Whether this network is a VPN. */ public boolean isVPN() { return networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN); } + /** Whether this network might have underlying networks. Currently only true for VPNs. */ + public boolean supportsUnderlyingNetworks() { + return isVPN(); + } + private int getCurrentScore(boolean pretendValidated) { // TODO: We may want to refactor this into a NetworkScore class that takes a base score from // the NetworkAgent and signals from the NetworkAgent and uses those signals to modify the diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index ff1a9c0ab35f..4f5c13db1231 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -112,7 +112,6 @@ import com.android.internal.net.VpnConfig; import com.android.internal.net.VpnInfo; import com.android.internal.net.VpnProfile; import com.android.internal.util.ArrayUtils; -import com.android.server.ConnectivityService; import com.android.server.DeviceIdleInternal; import com.android.server.LocalServices; import com.android.server.net.BaseNetworkObserver; @@ -1262,6 +1261,15 @@ public class Vpn { mNetworkCapabilities.setAdministratorUids(new int[] {mOwnerUID}); mNetworkCapabilities.setUids(createUserAndRestrictedProfilesRanges(mUserId, mConfig.allowedApplications, mConfig.disallowedApplications)); + + // Only apps targeting Q and above can explicitly declare themselves as metered. + // These VPNs are assumed metered unless they state otherwise. + if (mIsPackageTargetingAtLeastQ && mConfig.isMetered) { + mNetworkCapabilities.removeCapability(NET_CAPABILITY_NOT_METERED); + } else { + mNetworkCapabilities.addCapability(NET_CAPABILITY_NOT_METERED); + } + final long token = Binder.clearCallingIdentity(); try { mNetworkAgent = new NetworkAgent(mLooper, mContext, NETWORKTYPE /* logtag */, @@ -1276,6 +1284,8 @@ public class Vpn { } finally { Binder.restoreCallingIdentity(token); } + mNetworkAgent.setUnderlyingNetworks((mConfig.underlyingNetworks != null) + ? Arrays.asList(mConfig.underlyingNetworks) : null); mNetworkInfo.setIsAvailable(true); updateState(DetailedState.CONNECTED, "agentConnect"); } @@ -1857,6 +1867,8 @@ public class Vpn { } } } + mNetworkAgent.setUnderlyingNetworks((mConfig.underlyingNetworks != null) + ? Arrays.asList(mConfig.underlyingNetworks) : null); return true; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 99f1985ff691..561c6bab9c6d 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -1084,7 +1084,7 @@ public class ConnectivityServiceTest { throws Exception { if (mAgentRegistered) throw new IllegalStateException("already registered"); setUids(uids); - mConfig.isMetered = isAlwaysMetered; + if (!isAlwaysMetered) mNetworkCapabilities.addCapability(NET_CAPABILITY_NOT_METERED); mInterface = VPN_IFNAME; mMockNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN, lp, mNetworkCapabilities); @@ -5053,6 +5053,13 @@ public class ConnectivityServiceTest { waitForIdle(); expectForceUpdateIfaces(wifiAndVpn, null); reset(mStatsService); + + // Passing in null again means follow the default network again. + mService.setUnderlyingNetworksForVpn(null); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, WIFI_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{WIFI_IFNAME}); + reset(mStatsService); } @Test -- cgit v1.2.3-59-g8ed1b From 45feed9b00393b0ed2a43dc569f4addc11903627 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 26 Nov 2020 18:05:13 +0900 Subject: Allow tests to create TRANSPORT_TEST|TRANSPORT_VPN networks. This CL allows an app that has the MANAGE_TEST_NETWORKS permission to create test VPN networks. The code enforces that such networks can never apply to any UIDs and thus will never carry any traffic. Bug: 173331190 Test: passes existing tests, moved tests pass Change-Id: I5befea0e3b4b6dce4ca0c6a04471a055186b644c --- core/java/android/net/NetworkCapabilities.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 40bb8bf11d0b..8dad11ffa731 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -712,6 +712,7 @@ public final class NetworkCapabilities implements Parcelable { if (ArrayUtils.contains(originalAdministratorUids, creatorUid)) { setAdministratorUids(new int[] {creatorUid}); } + // There is no need to clear the UIDs, they have already been cleared by clearAll() above. } /** @@ -805,7 +806,9 @@ public final class NetworkCapabilities implements Parcelable { */ private static final int TEST_NETWORKS_ALLOWED_TRANSPORTS = 1 << TRANSPORT_TEST // Test ethernet networks can be created with EthernetManager#setIncludeTestInterfaces - | 1 << TRANSPORT_ETHERNET; + | 1 << TRANSPORT_ETHERNET + // Test VPN networks can be created but their UID ranges must be empty. + | 1 << TRANSPORT_VPN; /** * Adds the given transport type to this {@code NetworkCapability} instance. -- cgit v1.2.3-59-g8ed1b From feda5907f2e343724efc2ad3cfbebc443dbb0470 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 26 Nov 2020 23:42:25 +0900 Subject: Clear calling identity in registerNetworkAgent. Much of registerNetworkAgent calls internal ConnectivityService methods which generally assume that they are not processing an IPC and are running under the system's calling identity. However, only the call to makeNetworkMonitor is run with caller identity cleared. Expand the scope of clearing the caller identity over the creation of the nai. Bug: 173331190 Test: passes existing tests in ConnectivityServiceTest Change-Id: Icad28601a612fb5e1ed0451ec9e2066f4e766d0e --- .../com/android/server/ConnectivityService.java | 33 ++++++++++++++-------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index e96958eb3243..b909b26096f7 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5980,13 +5980,29 @@ public class ConnectivityService extends IConnectivityManager.Stub int currentScore, NetworkAgentConfig networkAgentConfig, int providerId) { if (networkCapabilities.hasTransport(TRANSPORT_TEST)) { enforceAnyPermissionOf(Manifest.permission.MANAGE_TEST_NETWORKS); + } else { + enforceNetworkFactoryPermission(); + } + + final int uid = Binder.getCallingUid(); + final long token = Binder.clearCallingIdentity(); + try { + return registerNetworkAgentInternal(messenger, networkInfo, linkProperties, + networkCapabilities, currentScore, networkAgentConfig, providerId, uid); + } finally { + Binder.restoreCallingIdentity(token); + } + } + + private Network registerNetworkAgentInternal(Messenger messenger, NetworkInfo networkInfo, + LinkProperties linkProperties, NetworkCapabilities networkCapabilities, + int currentScore, NetworkAgentConfig networkAgentConfig, int providerId, int uid) { + if (networkCapabilities.hasTransport(TRANSPORT_TEST)) { // Strictly, sanitizing here is unnecessary as the capabilities will be sanitized in // the call to mixInCapabilities below anyway, but sanitizing here means the NAI never // sees capabilities that may be malicious, which might prevent mistakes in the future. networkCapabilities = new NetworkCapabilities(networkCapabilities); - networkCapabilities.restrictCapabilitesForTestNetwork(Binder.getCallingUid()); - } else { - enforceNetworkFactoryPermission(); + networkCapabilities.restrictCapabilitesForTestNetwork(uid); } LinkProperties lp = new LinkProperties(linkProperties); @@ -5997,7 +6013,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo nai = new NetworkAgentInfo(messenger, new AsyncChannel(), new Network(mNetIdManager.reserveNetId()), new NetworkInfo(networkInfo), lp, nc, currentScore, mContext, mTrackerHandler, new NetworkAgentConfig(networkAgentConfig), - this, mNetd, mDnsResolver, mNMS, providerId, Binder.getCallingUid()); + this, mNetd, mDnsResolver, mNMS, providerId, uid); // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says. processCapabilitiesFromAgent(nai, nc); @@ -6008,13 +6024,8 @@ public class ConnectivityService extends IConnectivityManager.Stub final String name = TextUtils.isEmpty(extraInfo) ? nai.networkCapabilities.getSsid() : extraInfo; if (DBG) log("registerNetworkAgent " + nai); - final long token = Binder.clearCallingIdentity(); - try { - mDeps.getNetworkStack().makeNetworkMonitor( - nai.network, name, new NetworkMonitorCallbacks(nai)); - } finally { - Binder.restoreCallingIdentity(token); - } + mDeps.getNetworkStack().makeNetworkMonitor( + nai.network, name, new NetworkMonitorCallbacks(nai)); // NetworkAgentInfo registration will finish when the NetworkMonitor is created. // If the network disconnects or sends any other event before that, messages are deferred by // NetworkAgent until nai.asyncChannel.connect(), which will be called when finalizing the -- cgit v1.2.3-59-g8ed1b From d00d440f5ca8972693f7e623cfbd810ead1b338f Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 17 Nov 2020 15:58:21 +0900 Subject: Stop accessing VPNs in getAllVpnInfo. This is only used for NetworkStatsService and only called on the handler thread, so it can be replaced by a simple scan over mNetworkAgentInfos without having to take any locks. Bug: 173331190 Test: passes existing tests in ConnectivityServiceTest Change-Id: I194e0cc55603a0f59f7138f38329f505b55da132 --- .../com/android/server/ConnectivityService.java | 54 ++++++++++++---------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b909b26096f7..5420ee2f11af 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -4730,10 +4730,9 @@ public class ConnectivityService extends IConnectivityManager.Stub if (mLockdownEnabled) { return new VpnInfo[0]; } - List infoList = new ArrayList<>(); - for (int i = 0; i < mVpns.size(); i++) { - VpnInfo info = createVpnInfo(mVpns.valueAt(i)); + for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { + VpnInfo info = createVpnInfo(nai); if (info != null) { infoList.add(info); } @@ -4746,13 +4745,10 @@ public class ConnectivityService extends IConnectivityManager.Stub * @return VPN information for accounting, or null if we can't retrieve all required * information, e.g underlying ifaces. */ - @Nullable - private VpnInfo createVpnInfo(Vpn vpn) { - VpnInfo info = vpn.getVpnInfo(); - if (info == null) { - return null; - } - Network[] underlyingNetworks = vpn.getUnderlyingNetworks(); + private VpnInfo createVpnInfo(NetworkAgentInfo nai) { + if (!nai.isVPN()) return null; + + Network[] underlyingNetworks = nai.declaredUnderlyingNetworks; // see VpnService.setUnderlyingNetworks()'s javadoc about how to interpret // the underlyingNetworks list. if (underlyingNetworks == null) { @@ -4761,23 +4757,33 @@ public class ConnectivityService extends IConnectivityManager.Stub underlyingNetworks = new Network[] { defaultNai.network }; } } - if (underlyingNetworks != null && underlyingNetworks.length > 0) { - List interfaces = new ArrayList<>(); - for (Network network : underlyingNetworks) { - LinkProperties lp = getLinkProperties(network); - if (lp != null) { - for (String iface : lp.getAllInterfaceNames()) { - if (!TextUtils.isEmpty(iface)) { - interfaces.add(iface); - } - } + + if (ArrayUtils.isEmpty(underlyingNetworks)) return null; + + List interfaces = new ArrayList<>(); + for (Network network : underlyingNetworks) { + NetworkAgentInfo underlyingNai = getNetworkAgentInfoForNetwork(network); + if (underlyingNai == null) continue; + LinkProperties lp = underlyingNai.linkProperties; + for (String iface : lp.getAllInterfaceNames()) { + if (!TextUtils.isEmpty(iface)) { + interfaces.add(iface); } } - if (!interfaces.isEmpty()) { - info.underlyingIfaces = interfaces.toArray(new String[interfaces.size()]); - } } - return info.underlyingIfaces == null ? null : info; + + if (interfaces.isEmpty()) return null; + + VpnInfo info = new VpnInfo(); + info.ownerUid = nai.networkCapabilities.getOwnerUid(); + info.vpnIface = nai.linkProperties.getInterfaceName(); + // Must be non-null or NetworkStatsService will crash. + // Cannot happen in production code because Vpn only registers the NetworkAgent after the + // tun or ipsec interface is created. + if (info.vpnIface == null) return null; + info.underlyingIfaces = interfaces.toArray(new String[0]); + + return info; } /** -- cgit v1.2.3-59-g8ed1b