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)
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 1c66c5a..c9f9ab6 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -867,8 +867,7 @@
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 @@
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 @@
* 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 @@
}
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 @@
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 @@
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 @@
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 @@
// 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 @@
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 56510b7e..2a80f0e 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 @@
mNetworkCapabilities = new NetworkCapabilities();
mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
- updateCapabilities(null /* defaultNetwork */);
+ updateCapabilities();
loadAlwaysOnPackage();
}
@@ -300,39 +300,18 @@
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 @@
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 @@
}
/**
- * 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 @@
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 @@
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 @@
}
}
- /**
- * 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 @@
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 @@
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 @@
}
}
}
+ updateCapabilities();
return true;
}
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index 0dcb21a..c2c627d 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.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 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 @@
}
@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 @@
mMockVpn.setUids(ranges);
vpnNetworkAgent.connect(false);
mMockVpn.connect();
- mMockVpn.setUnderlyingNetworks(new Network[0]);
genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent);
genericNotVpnNetworkCallback.assertNoCallback();
@@ -4256,7 +4250,6 @@
ranges.add(new UidRange(uid, uid));
mMockVpn.setUids(ranges);
- vpnNetworkAgent.setUids(ranges);
genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent);
genericNotVpnNetworkCallback.assertNoCallback();
@@ -4290,11 +4283,12 @@
}
@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 @@
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 @@
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 @@
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 a0a4ad1..e377a47 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 @@
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 @@
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 @@
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 @@
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));
