diff options
author | Varun Anand <vaanand@google.com> | 2019-03-05 19:23:50 +0000 |
---|---|---|
committer | android-build-team Robot <android-build-team-robot@google.com> | 2019-03-13 18:01:56 +0000 |
commit | 0aa677c75909c0d307fc4a2d3642ae8e85679280 (patch) | |
tree | 107ee1f46277a025b42592d45b1bf15e80d70de8 | |
parent | d374204d68d4eecf1acecc8ee47945eb3a7ab092 (diff) | |
download | base-0aa677c75909c0d307fc4a2d3642ae8e85679280.tar.gz |
Revert "Update VPN capabilities when its underlying network set is null."
This reverts commit ad8805a6c217338b12511f220cce22211a19080a.
Bug: 126245192
Reason for revert: This change can lead to a deadlock that was fixed in http://ag/6580635. However, platform PMs think that fixing this is risky enough as this is not a recent problem and has been in the field for 3/4 of the year.
Note: The merged-in tag is used to avoid this change from getting merged into pi-dev-plus-aosp. This is to avoid merge conflicts since we mostly work in aosp/master which merges into pi-dev-plus-aosp.
Change-Id: I3814bcec87efb059f50f00617406501aaeac3b4d
Merged-In: Id0abc4d304bb096e92479a118168690ccce634ed
(cherry picked from commit 6a050c7c50fa0838d7e29b8c6e244018044246db)
4 files changed, 40 insertions, 208 deletions
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1c66c5a77151..c9f9ab675a75 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -867,8 +867,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mPermissionMonitor = new PermissionMonitor(mContext, mNetd); - // Set up the listener for user state for creating user VPNs. - // Should run on mHandler to avoid any races. + //set up the listener for user state for creating user VPNs IntentFilter intentFilter = new IntentFilter(); intentFilter.addAction(Intent.ACTION_USER_STARTED); intentFilter.addAction(Intent.ACTION_USER_STOPPED); @@ -876,11 +875,7 @@ public class ConnectivityService extends IConnectivityManager.Stub intentFilter.addAction(Intent.ACTION_USER_REMOVED); intentFilter.addAction(Intent.ACTION_USER_UNLOCKED); mContext.registerReceiverAsUser( - mUserIntentReceiver, - UserHandle.ALL, - intentFilter, - null /* broadcastPermission */, - mHandler); + mUserIntentReceiver, UserHandle.ALL, intentFilter, null, null); mContext.registerReceiverAsUser(mUserPresentReceiver, UserHandle.SYSTEM, new IntentFilter(Intent.ACTION_USER_PRESENT), null, null); @@ -3820,27 +3815,17 @@ public class ConnectivityService extends IConnectivityManager.Stub * 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. + * TODO : Fix this and call updateCapabilities inline to remove out-of-order events. */ 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); + vpn.updateCapabilities(); } } } - private void updateVpnCapabilities(Vpn vpn, @Nullable NetworkCapabilities nc) { - ensureRunningOnConnectivityServiceThread(); - NetworkAgentInfo vpnNai = getNetworkAgentInfoForNetId(vpn.getNetId()); - if (vpnNai == null || nc == null) { - return; - } - updateCapabilities(vpnNai.getCurrentScore(), vpnNai, nc); - } - @Override public boolean updateLockdownVpn() { if (Binder.getCallingUid() != Process.SYSTEM_UID) { @@ -4147,27 +4132,21 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void onUserAdded(int userId) { - Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserAdded(userId); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); } } } private void onUserRemoved(int userId) { - Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserRemoved(userId); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); } } } @@ -4186,7 +4165,6 @@ public class ConnectivityService extends IConnectivityManager.Stub private BroadcastReceiver mUserIntentReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - ensureRunningOnConnectivityServiceThread(); final String action = intent.getAction(); final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL); if (userId == UserHandle.USER_NULL) return; @@ -4672,19 +4650,6 @@ public class ConnectivityService extends IConnectivityManager.Stub return getNetworkForRequest(mDefaultRequest.requestId); } - @Nullable - private Network getNetwork(@Nullable NetworkAgentInfo nai) { - return nai != null ? nai.network : null; - } - - private void ensureRunningOnConnectivityServiceThread() { - if (mHandler.getLooper().getThread() != Thread.currentThread()) { - throw new IllegalStateException( - "Not running on ConnectivityService thread: " - + Thread.currentThread().getName()); - } - } - private boolean isDefaultNetwork(NetworkAgentInfo nai) { return nai == getDefaultNetwork(); } @@ -5232,8 +5197,6 @@ public class ConnectivityService extends IConnectivityManager.Stub updateTcpBufferSizes(newNetwork); mDnsManager.setDefaultDnsSystemProperties(newNetwork.linkProperties.getDnsServers()); notifyIfacesChangedForNetworkStats(); - // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. - updateAllVpnsCapabilities(); } private void processListenRequests(NetworkAgentInfo nai, boolean capabilitiesChanged) { @@ -5667,10 +5630,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // doing. updateSignalStrengthThresholds(networkAgent, "CONNECT", null); - if (networkAgent.isVPN()) { - updateAllVpnsCapabilities(); - } - // Consider network even though it is not yet validated. final long now = SystemClock.elapsedRealtime(); rematchNetworkAndRequests(networkAgent, ReapUnvalidatedNetworks.REAP, now); @@ -5864,11 +5823,7 @@ public class ConnectivityService extends IConnectivityManager.Stub success = mVpns.get(user).setUnderlyingNetworks(networks); } if (success) { - mHandler.post(() -> { - // Update VPN's capabilities based on updated underlying network set. - updateAllVpnsCapabilities(); - notifyIfacesChangedForNetworkStats(); - }); + mHandler.post(() -> notifyIfacesChangedForNetworkStats()); } return success; } diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 56510b7e18b3..2a80f0e7c291 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -273,7 +273,7 @@ public class Vpn { mNetworkCapabilities = new NetworkCapabilities(); mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN); mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN); - updateCapabilities(null /* defaultNetwork */); + updateCapabilities(); loadAlwaysOnPackage(); } @@ -300,39 +300,18 @@ public class Vpn { updateAlwaysOnNotification(detailedState); } - /** - * Updates {@link #mNetworkCapabilities} based on current underlying networks and returns a - * defensive copy. - * - * <p>Does not propagate updated capabilities to apps. - * - * @param defaultNetwork underlying network for VPNs following platform's default - */ - public synchronized NetworkCapabilities updateCapabilities( - @Nullable Network defaultNetwork) { - if (mConfig == null) { - // VPN is not running. - return null; - } - - Network[] underlyingNetworks = mConfig.underlyingNetworks; - if (underlyingNetworks == null && defaultNetwork != null) { - // null underlying networks means to track the default. - underlyingNetworks = new Network[] { defaultNetwork }; - } - - applyUnderlyingCapabilities( - mContext.getSystemService(ConnectivityManager.class), - underlyingNetworks, + public void updateCapabilities() { + final Network[] underlyingNetworks = (mConfig != null) ? mConfig.underlyingNetworks : null; + updateCapabilities(mContext.getSystemService(ConnectivityManager.class), underlyingNetworks, mNetworkCapabilities); - return new NetworkCapabilities(mNetworkCapabilities); + if (mNetworkAgent != null) { + mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + } } @VisibleForTesting - public static void applyUnderlyingCapabilities( - ConnectivityManager cm, - Network[] underlyingNetworks, + public static void updateCapabilities(ConnectivityManager cm, Network[] underlyingNetworks, NetworkCapabilities caps) { int[] transportTypes = new int[] { NetworkCapabilities.TRANSPORT_VPN }; int downKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; @@ -344,7 +323,6 @@ public class Vpn { boolean hadUnderlyingNetworks = false; if (null != underlyingNetworks) { for (Network underlying : underlyingNetworks) { - // TODO(b/124469351): Get capabilities directly from ConnectivityService instead. final NetworkCapabilities underlyingCaps = cm.getNetworkCapabilities(underlying); if (underlyingCaps == null) continue; hadUnderlyingNetworks = true; @@ -1015,8 +993,9 @@ public class Vpn { } /** - * Establish a VPN network and return the file descriptor of the VPN interface. This methods - * returns {@code null} if the application is revoked or not prepared. + * Establish a VPN network and return the file descriptor of the VPN + * interface. This methods returns {@code null} if the application is + * revoked or not prepared. * * @param config The parameters to configure the network. * @return The file descriptor of the VPN interface. @@ -1263,11 +1242,6 @@ public class Vpn { return ranges; } - /** - * Updates UID ranges for this VPN and also updates its internal capabilities. - * - * <p>Should be called on primary ConnectivityService thread. - */ public void onUserAdded(int userHandle) { // If the user is restricted tie them to the parent user's VPN UserInfo user = UserManager.get(mContext).getUserInfo(userHandle); @@ -1278,9 +1252,8 @@ public class Vpn { try { addUserToRanges(existingRanges, userHandle, mConfig.allowedApplications, mConfig.disallowedApplications); - // ConnectivityService will call {@link #updateCapabilities} and apply - // those for VPN network. mNetworkCapabilities.setUids(existingRanges); + updateCapabilities(); } catch (Exception e) { Log.wtf(TAG, "Failed to add restricted user to owner", e); } @@ -1290,11 +1263,6 @@ public class Vpn { } } - /** - * Updates UID ranges for this VPN and also updates its capabilities. - * - * <p>Should be called on primary ConnectivityService thread. - */ public void onUserRemoved(int userHandle) { // clean up if restricted UserInfo user = UserManager.get(mContext).getUserInfo(userHandle); @@ -1306,9 +1274,8 @@ public class Vpn { final List<UidRange> removedRanges = uidRangesForUser(userHandle, existingRanges); existingRanges.removeAll(removedRanges); - // ConnectivityService will call {@link #updateCapabilities} and - // apply those for VPN network. mNetworkCapabilities.setUids(existingRanges); + updateCapabilities(); } catch (Exception e) { Log.wtf(TAG, "Failed to remove restricted user to owner", e); } @@ -1516,12 +1483,6 @@ public class Vpn { return success; } - /** - * Updates underlying network set. - * - * <p>Note: Does not updates capabilities. Call {@link #updateCapabilities} from - * ConnectivityService thread to get updated capabilities. - */ public synchronized boolean setUnderlyingNetworks(Network[] networks) { if (!isCallerEstablishedOwnerLocked()) { return false; @@ -1538,6 +1499,7 @@ public class Vpn { } } } + updateCapabilities(); return true; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 0dcb21ab8386..c2c627d06e47 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -20,7 +20,6 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME; -import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.ConnectivityManager.TYPE_ETHERNET; import static android.net.ConnectivityManager.TYPE_MOBILE; import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA; @@ -776,14 +775,11 @@ public class ConnectivityServiceTest { public void setUids(Set<UidRange> uids) { mNetworkCapabilities.setUids(uids); - updateCapabilities(null /* defaultNetwork */); + updateCapabilities(); } @Override public int getNetId() { - if (mMockNetworkAgent == null) { - return NETID_UNSET; - } return mMockNetworkAgent.getNetwork().netId; } @@ -804,13 +800,12 @@ public class ConnectivityServiceTest { } @Override - public NetworkCapabilities updateCapabilities(Network defaultNetwork) { - if (!mConnected) return null; - super.updateCapabilities(defaultNetwork); - // Because super.updateCapabilities will update the capabilities of the agent but - // not the mock agent, the mock agent needs to know about them. + public void updateCapabilities() { + if (!mConnected) return; + super.updateCapabilities(); + // Because super.updateCapabilities will update the capabilities of the agent but not + // the mock agent, the mock agent needs to know about them. copyCapabilitiesToNetworkAgent(); - return new NetworkCapabilities(mNetworkCapabilities); } private void copyCapabilitiesToNetworkAgent() { @@ -4223,7 +4218,6 @@ public class ConnectivityServiceTest { mMockVpn.setUids(ranges); vpnNetworkAgent.connect(false); mMockVpn.connect(); - mMockVpn.setUnderlyingNetworks(new Network[0]); genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4256,7 +4250,6 @@ public class ConnectivityServiceTest { ranges.add(new UidRange(uid, uid)); mMockVpn.setUids(ranges); - vpnNetworkAgent.setUids(ranges); genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4290,11 +4283,12 @@ public class ConnectivityServiceTest { } @Test - public void testVpnWithoutInternet() { + public void testVpnWithAndWithoutInternet() { final int uid = Process.myUid(); final TestNetworkCallback defaultCallback = new TestNetworkCallback(); mCm.registerDefaultNetworkCallback(defaultCallback); + defaultCallback.assertNoCallback(); mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); @@ -4316,30 +4310,11 @@ public class ConnectivityServiceTest { vpnNetworkAgent.disconnect(); defaultCallback.assertNoCallback(); - mCm.unregisterNetworkCallback(defaultCallback); - } - - @Test - public void testVpnWithInternet() { - final int uid = Process.myUid(); - - final TestNetworkCallback defaultCallback = new TestNetworkCallback(); - mCm.registerDefaultNetworkCallback(defaultCallback); - - mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); - mWiFiNetworkAgent.connect(true); - - defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - - MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - final ArraySet<UidRange> ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); + vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); mMockVpn.setNetworkAgent(vpnNetworkAgent); mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */); mMockVpn.connect(); - defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4347,6 +4322,14 @@ public class ConnectivityServiceTest { defaultCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent); defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); + vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + ranges.clear(); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); + vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); + mMockVpn.connect(); + defaultCallback.assertNoCallback(); + mCm.unregisterNetworkCallback(defaultCallback); } @@ -4447,68 +4430,4 @@ public class ConnectivityServiceTest { mMockVpn.disconnect(); } - - @Test - public void testNullUnderlyingNetworks() { - final int uid = Process.myUid(); - - final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback(); - final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder() - .removeCapability(NET_CAPABILITY_NOT_VPN) - .addTransportType(TRANSPORT_VPN) - .build(); - NetworkCapabilities nc; - mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); - vpnNetworkCallback.assertNoCallback(); - - final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - final ArraySet<UidRange> ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */); - - vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); - nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); - assertTrue(nc.hasTransport(TRANSPORT_VPN)); - assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); - assertFalse(nc.hasTransport(TRANSPORT_WIFI)); - // By default, VPN is set to track default network (i.e. its underlying networks is null). - // In case of no default network, VPN is considered metered. - assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); - - // Connect to Cell; Cell is the default network. - mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); - mCellNetworkAgent.connect(true); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) - && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - // Connect to WiFi; WiFi is the new default. - mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); - mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); - mWiFiNetworkAgent.connect(true); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) - && caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - // Disconnect Cell. The default network did not change, so there shouldn't be any changes in - // the capabilities. - mCellNetworkAgent.disconnect(); - - // Disconnect wifi too. Now we have no default network. - mWiFiNetworkAgent.disconnect(); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) - && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - mMockVpn.disconnect(); - } } diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index a0a4ad1feb68..e377a472530e 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -457,8 +457,7 @@ public class VpnTest { final NetworkCapabilities caps = new NetworkCapabilities(); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -468,8 +467,7 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {mobile}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -479,8 +477,7 @@ public class VpnTest { assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {wifi}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { wifi }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI)); @@ -490,8 +487,7 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {mobile, wifi}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile, wifi }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI)); |