LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Remove ConnectivityManager and its usages from NetworkStatsService.

Browse Source 
NSS needed it for getting VpnInfo[], NetworkState[] and
activeLinkProperties which it used to query via ConnectivityManager.

For VpnInfo[], this was racy as NSS may ignore intermediate changes to a
VPN's underlying networks. See http://b/123961098 for more context.

It may also lead to deadlocks b/w ConnectivityService and
NetworkStatsService. See http://b/126245192 for more info.

This change will ensure that NSS is never contending on any of
ConnectivityService locks.

Bug: 123961098
Bug: 126245192
Bug: 120145746
Test: atest FrameworksNetTests
Change-Id: I57e117bb4e9efe491b19d6b5a479f2d58d1c58e6
This commit is contained in:
Varun Anand
2019-02-07 14:13:13 -08:00
parent bd51b3057a
commit 45406bff69
4 changed files with 108 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
core/java/android/net/IConnectivityManager.aidl
View File

@@ -122,8 +122,6 @@ interface IConnectivityManager
LegacyVpnInfo getLegacyVpnInfo(int userId);
VpnInfo[] getAllVpnInfo();
boolean updateLockdownVpn();
boolean isAlwaysOnVpnPackageSupported(int userId, String packageName);
boolean setAlwaysOnVpnPackage(int userId, String packageName, boolean lockdown,

22
services/core/java/com/android/server/ConnectivityService.java
View File

@@ -4094,12 +4094,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
}
/**
* Return the information of all ongoing VPNs. This method is used by NetworkStatsService
* and not available in ConnectivityManager.
* Return the information of all ongoing VPNs.
*
* <p>This method is used to update NetworkStatsService.
*
* <p>Must be called on the handler thread.
*/
@Override
public VpnInfo[] getAllVpnInfo() {
enforceConnectivityInternalPermission();
private VpnInfo[] getAllVpnInfo() {
ensureRunningOnConnectivityServiceThread();
synchronized (mVpns) {
if (mLockdownEnabled) {
return new VpnInfo[0];
@@ -6434,6 +6436,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
* Must be called on the handler thread.
*/
private Network[] getDefaultNetworks() {
ensureRunningOnConnectivityServiceThread();
ArrayList<Network> defaultNetworks = new ArrayList<>();
NetworkAgentInfo defaultNetwork = getDefaultNetwork();
for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) {
@@ -6449,8 +6452,15 @@ public class ConnectivityService extends IConnectivityManager.Stub
* properties tracked by NetworkStatsService on an active iface has changed.
*/
private void notifyIfacesChangedForNetworkStats() {
ensureRunningOnConnectivityServiceThread();
String activeIface = null;
LinkProperties activeLinkProperties = getActiveLinkProperties();
if (activeLinkProperties != null) {
activeIface = activeLinkProperties.getInterfaceName();
}
try {
mStatsService.forceUpdateIfaces(getDefaultNetworks());
mStatsService.forceUpdateIfaces(
getDefaultNetworks(), getAllVpnInfo(), getAllNetworkState(), activeIface);
} catch (Exception ignored) {
}
}

63
tests/net/java/com/android/server/ConnectivityServiceTest.java
View File

@@ -125,6 +125,7 @@ import android.net.NetworkParcelable;
import android.net.NetworkRequest;
import android.net.NetworkSpecifier;
import android.net.NetworkStackClient;
import android.net.NetworkState;
import android.net.NetworkUtils;
import android.net.ProxyInfo;
import android.net.RouteInfo;
@@ -157,6 +158,7 @@ import android.util.Log;
import android.util.SparseArray;
import com.android.internal.net.VpnConfig;
import com.android.internal.net.VpnInfo;
import com.android.internal.util.ArrayUtils;
import com.android.internal.util.WakeupMessage;
import com.android.internal.util.test.BroadcastInterceptingContext;
@@ -4356,48 +4358,91 @@ public class ConnectivityServiceTest {
mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
Network[] onlyCell = new Network[]{mCellNetworkAgent.getNetwork()};
Network[] onlyWifi = new Network[]{mWiFiNetworkAgent.getNetwork()};
Network[] onlyCell = new Network[] {mCellNetworkAgent.getNetwork()};
Network[] onlyWifi = new Network[] {mWiFiNetworkAgent.getNetwork()};
LinkProperties cellLp = new LinkProperties();
cellLp.setInterfaceName(MOBILE_IFNAME);
LinkProperties wifiLp = new LinkProperties();
wifiLp.setInterfaceName(WIFI_IFNAME);
// Simple connection should have updated ifaces
mCellNetworkAgent.connect(false);
mCellNetworkAgent.sendLinkProperties(cellLp);
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyCell);
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
// Default network switch should update ifaces.
mWiFiNetworkAgent.connect(false);
mWiFiNetworkAgent.sendLinkProperties(wifiLp);
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyWifi);
assertEquals(wifiLp, mService.getActiveLinkProperties());
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyWifi),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(WIFI_IFNAME));
reset(mStatsService);
// Disconnect should update ifaces.
mWiFiNetworkAgent.disconnect();
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyCell);
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
// Metered change should update ifaces
mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED);
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyCell);
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
mCellNetworkAgent.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED);
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyCell);
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
// Captive portal change shouldn't update ifaces
mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_CAPTIVE_PORTAL);
waitForIdle();
verify(mStatsService, never()).forceUpdateIfaces(onlyCell);
verify(mStatsService, never())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
// Roaming change should update ifaces
mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING);
waitForIdle();
verify(mStatsService, atLeastOnce()).forceUpdateIfaces(onlyCell);
verify(mStatsService, atLeastOnce())
.forceUpdateIfaces(
eq(onlyCell),
eq(new VpnInfo[0]),
any(NetworkState[].class),
eq(MOBILE_IFNAME));
reset(mStatsService);
}

81
tests/net/java/com/android/server/net/NetworkStatsServiceTest.java
View File

@@ -70,7 +70,6 @@ import android.app.usage.NetworkStatsManager;
import android.content.Context;
import android.content.Intent;
import android.net.DataUsageRequest;
import android.net.IConnectivityManager;
import android.net.INetworkManagementEventObserver;
import android.net.INetworkStatsSession;
import android.net.LinkProperties;
@@ -163,7 +162,6 @@ public class NetworkStatsServiceTest {
private @Mock INetworkManagementService mNetManager;
private @Mock NetworkStatsSettings mSettings;
private @Mock IConnectivityManager mConnManager;
private @Mock IBinder mBinder;
private @Mock AlarmManager mAlarmManager;
private HandlerThread mHandlerThread;
@@ -205,7 +203,6 @@ public class NetworkStatsServiceTest {
Handler.Callback callback = new NetworkStatsService.HandlerCallback(mService);
mHandler = new Handler(mHandlerThread.getLooper(), callback);
mService.setHandler(mHandler, callback);
mService.bindConnectivityManager(mConnManager);
mElapsedRealtime = 0L;
@@ -234,7 +231,6 @@ public class NetworkStatsServiceTest {
mNetManager = null;
mSettings = null;
mConnManager = null;
mSession.close();
mService = null;
@@ -245,12 +241,12 @@ public class NetworkStatsServiceTest {
// pretend that wifi network comes online; service should ask about full
// network state, and poll any existing interfaces before updating.
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// verify service has empty history for wifi
assertNetworkTotal(sTemplateWifi, 0L, 0L, 0L, 0L, 0);
@@ -289,12 +285,12 @@ public class NetworkStatsServiceTest {
// pretend that wifi network comes online; service should ask about full
// network state, and poll any existing interfaces before updating.
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// verify service has empty history for wifi
assertNetworkTotal(sTemplateWifi, 0L, 0L, 0L, 0L, 0);
@@ -363,12 +359,12 @@ public class NetworkStatsServiceTest {
// pretend that wifi network comes online; service should ask about full
// network state, and poll any existing interfaces before updating.
expectSettings(0L, HOUR_IN_MILLIS, WEEK_IN_MILLIS);
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// modify some number on wifi, and trigger poll event
@@ -405,12 +401,12 @@ public class NetworkStatsServiceTest {
public void testUidStatsAcrossNetworks() throws Exception {
// pretend first mobile network comes online
expectDefaultSettings();
expectNetworkState(buildMobile3gState(IMSI_1));
NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
// create some traffic on first network
@@ -437,7 +433,7 @@ public class NetworkStatsServiceTest {
// disappearing, to verify we don't count backwards.
incrementCurrentTime(HOUR_IN_MILLIS);
expectDefaultSettings();
expectNetworkState(buildMobile3gState(IMSI_2));
states = new NetworkState[] {buildMobile3gState(IMSI_2)};
expectNetworkStatsSummary(new NetworkStats(getElapsedRealtime(), 1)
.addIfaceValues(TEST_IFACE, 2048L, 16L, 512L, 4L));
expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 3)
@@ -446,7 +442,7 @@ public class NetworkStatsServiceTest {
.addValues(TEST_IFACE, UID_BLUE, SET_DEFAULT, TAG_NONE, 512L, 4L, 0L, 0L, 0L));
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
forcePollAndWaitForIdle();
@@ -481,12 +477,12 @@ public class NetworkStatsServiceTest {
public void testUidRemovedIsMoved() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// create some traffic
@@ -540,12 +536,12 @@ public class NetworkStatsServiceTest {
public void testUid3g4gCombinedByTemplate() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildMobile3gState(IMSI_1));
NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
// create some traffic
@@ -566,14 +562,14 @@ public class NetworkStatsServiceTest {
// now switch over to 4g network
incrementCurrentTime(HOUR_IN_MILLIS);
expectDefaultSettings();
expectNetworkState(buildMobile4gState(TEST_IFACE2));
states = new NetworkState[] {buildMobile4gState(TEST_IFACE2)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 1)
.addValues(TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1024L, 8L, 1024L, 8L, 0L)
.addValues(TEST_IFACE, UID_RED, SET_DEFAULT, 0xF00D, 512L, 4L, 512L, 4L, 0L));
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
forcePollAndWaitForIdle();
@@ -598,12 +594,12 @@ public class NetworkStatsServiceTest {
public void testSummaryForAllUid() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// create some traffic for two apps
@@ -657,12 +653,12 @@ public class NetworkStatsServiceTest {
public void testDetailedUidStats() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
NetworkStats.Entry entry1 = new NetworkStats.Entry(
TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 50L, 5L, 50L, 5L, 0L);
@@ -700,13 +696,13 @@ public class NetworkStatsServiceTest {
stackedProp.setInterfaceName(stackedIface);
final NetworkState wifiState = buildWifiState();
wifiState.linkProperties.addStackedLink(stackedProp);
expectNetworkState(wifiState);
NetworkState[] states = new NetworkState[] {wifiState};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
NetworkStats.Entry uidStats = new NetworkStats.Entry(
TEST_IFACE, UID_BLUE, SET_DEFAULT, 0xF00D, 1024L, 8L, 512L, 4L, 0L);
@@ -745,12 +741,12 @@ public class NetworkStatsServiceTest {
public void testForegroundBackground() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// create some initial traffic
@@ -803,12 +799,12 @@ public class NetworkStatsServiceTest {
public void testMetered() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildWifiState(true /* isMetered */));
NetworkState[] states = new NetworkState[] {buildWifiState(true /* isMetered */)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// create some initial traffic
@@ -843,12 +839,13 @@ public class NetworkStatsServiceTest {
public void testRoaming() throws Exception {
// pretend that network comes online
expectDefaultSettings();
expectNetworkState(buildMobile3gState(IMSI_1, true /* isRoaming */));
NetworkState[] states =
new NetworkState[] {buildMobile3gState(IMSI_1, true /* isRoaming */)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
// Create some traffic
@@ -882,12 +879,12 @@ public class NetworkStatsServiceTest {
public void testTethering() throws Exception {
// pretend first mobile network comes online
expectDefaultSettings();
expectNetworkState(buildMobile3gState(IMSI_1));
NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_MOBILE);
mService.forceUpdateIfaces(NETWORKS_MOBILE, new VpnInfo[0], states, getActiveIface(states));
// create some tethering traffic
@@ -925,12 +922,12 @@ public class NetworkStatsServiceTest {
// pretend that wifi network comes online; service should ask about full
// network state, and poll any existing interfaces before updating.
expectDefaultSettings();
expectNetworkState(buildWifiState());
NetworkState[] states = new NetworkState[] {buildWifiState()};
expectNetworkStatsSummary(buildEmptyStats());
expectNetworkStatsUidDetail(buildEmptyStats());
expectBandwidthControlCheck();
mService.forceUpdateIfaces(NETWORKS_WIFI);
mService.forceUpdateIfaces(NETWORKS_WIFI, new VpnInfo[0], states, getActiveIface(states));
// verify service has empty history for wifi
assertNetworkTotal(sTemplateWifi, 0L, 0L, 0L, 0L, 0);
@@ -1077,11 +1074,11 @@ public class NetworkStatsServiceTest {
expectBandwidthControlCheck();
}
private void expectNetworkState(NetworkState... state) throws Exception {
when(mConnManager.getAllNetworkState()).thenReturn(state);
final LinkProperties linkProp = state.length > 0 ? state[0].linkProperties : null;
when(mConnManager.getActiveLinkProperties()).thenReturn(linkProp);
private String getActiveIface(NetworkState... states) throws Exception {
if (states == null || states.length == 0 || states[0].linkProperties == null) {
return null;
}
return states[0].linkProperties.getInterfaceName();
}
private void expectNetworkStatsSummary(NetworkStats summary) throws Exception {
@@ -1090,8 +1087,6 @@ public class NetworkStatsServiceTest {
private void expectNetworkStatsSummary(NetworkStats summary, NetworkStats tetherStats)
throws Exception {
when(mConnManager.getAllVpnInfo()).thenReturn(new VpnInfo[0]);
expectNetworkStatsTethering(STATS_PER_IFACE, tetherStats);
expectNetworkStatsSummaryDev(summary.clone());
expectNetworkStatsSummaryXt(summary.clone());