From bcaf1f959b8b5f53c138a4b53f3ced394be6e2c6 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 24 Jun 2019 13:50:45 +0900 Subject: [PATCH] Remove all static members from NetworkStatsFactory. NetworkStatsFactory is owned by NetworkStatsService, and any accesses to NSF data should go through NSS. Test: atest FrameworksNetTests Change-Id: Idbd0dbbaeb11313f63474e7ec0e01f974349fc89 --- .../android/net/INetworkStatsService.aidl | 3 +- .../server/net/NetworkStatsFactory.java | 49 ++++++++----------- .../server/net/NetworkStatsService.java | 15 ++++-- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/core/java/android/net/INetworkStatsService.aidl b/core/java/android/net/INetworkStatsService.aidl index e5f3d26667..9994f9f82b 100644 --- a/core/java/android/net/INetworkStatsService.aidl +++ b/core/java/android/net/INetworkStatsService.aidl @@ -67,7 +67,8 @@ interface INetworkStatsService { void forceUpdateIfaces( in Network[] defaultNetworks, in NetworkState[] networkStates, - in String activeIface); + in String activeIface, + in VpnInfo[] vpnInfos); /** Force update of statistics. */ @UnsupportedAppUsage void forceUpdate(); diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index 9344ef71c9..3ca1803262 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -78,17 +78,17 @@ public class NetworkStatsFactory { *

In order to prevent deadlocks, critical sections protected by this lock SHALL NOT call out * to other code that will acquire other locks within the system server. See b/134244752. */ - private static final Object sPersistentDataLock = new Object(); + private final Object mPersistentDataLock = new Object(); /** Set containing info about active VPNs and their underlying networks. */ - private static volatile VpnInfo[] sVpnInfos = new VpnInfo[0]; + private volatile VpnInfo[] mVpnInfos = new VpnInfo[0]; // A persistent snapshot of cumulative stats since device start - @GuardedBy("sPersistentDataLock") + @GuardedBy("mPersistentDataLock") private NetworkStats mPersistSnapshot; // The persistent snapshot of tun and 464xlat adjusted stats since device start - @GuardedBy("sPersistentDataLock") + @GuardedBy("mPersistentDataLock") private NetworkStats mTunAnd464xlatAdjustedStats; /** @@ -97,12 +97,13 @@ public class NetworkStatsFactory { * Because counters must never roll backwards, once a given interface is stacked on top of an * underlying interface, the stacked interface can never be stacked on top of * another interface. */ - private static final ConcurrentHashMap sStackedIfaces + private final ConcurrentHashMap mStackedIfaces = new ConcurrentHashMap<>(); - public static void noteStackedIface(String stackedIface, String baseIface) { + /** Informs the factory of a new stacked interface. */ + public void noteStackedIface(String stackedIface, String baseIface) { if (stackedIface != null && baseIface != null) { - sStackedIfaces.put(stackedIface, baseIface); + mStackedIfaces.put(stackedIface, baseIface); } } @@ -115,13 +116,8 @@ public class NetworkStatsFactory { * * @param vpnArray The snapshot of the currently-running VPNs. */ - public static void updateVpnInfos(VpnInfo[] vpnArray) { - sVpnInfos = vpnArray.clone(); - } - - @VisibleForTesting - public static VpnInfo[] getVpnInfos() { - return sVpnInfos.clone(); + public void updateVpnInfos(VpnInfo[] vpnArray) { + mVpnInfos = vpnArray.clone(); } /** @@ -132,7 +128,7 @@ public class NetworkStatsFactory { * {@link #noteStackedIface(String, String)}, but only interfaces noted before this method * is called are guaranteed to be included. */ - public static String[] augmentWithStackedInterfaces(@Nullable String[] requiredIfaces) { + public String[] augmentWithStackedInterfaces(@Nullable String[] requiredIfaces) { if (requiredIfaces == NetworkStats.INTERFACES_ALL) { return null; } @@ -142,7 +138,7 @@ public class NetworkStatsFactory { // elements as they existed upon construction exactly once, and may // (but are not guaranteed to) reflect any modifications subsequent to construction". // This is enough here. - for (Map.Entry entry : sStackedIfaces.entrySet()) { + for (Map.Entry entry : mStackedIfaces.entrySet()) { if (relatedIfaces.contains(entry.getKey())) { relatedIfaces.add(entry.getValue()); } else if (relatedIfaces.contains(entry.getValue())) { @@ -158,17 +154,12 @@ public class NetworkStatsFactory { * Applies 464xlat adjustments with ifaces noted with {@link #noteStackedIface(String, String)}. * @see NetworkStats#apply464xlatAdjustments(NetworkStats, NetworkStats, Map, boolean) */ - public static void apply464xlatAdjustments(NetworkStats baseTraffic, + public void apply464xlatAdjustments(NetworkStats baseTraffic, NetworkStats stackedTraffic, boolean useBpfStats) { - NetworkStats.apply464xlatAdjustments(baseTraffic, stackedTraffic, sStackedIfaces, + NetworkStats.apply464xlatAdjustments(baseTraffic, stackedTraffic, mStackedIfaces, useBpfStats); } - @VisibleForTesting - public static void clearStackedIfaces() { - sStackedIfaces.clear(); - } - public NetworkStatsFactory() { this(new File("/proc/"), new File("/sys/fs/bpf/map_netd_app_uid_stats_map").exists()); } @@ -179,7 +170,7 @@ public class NetworkStatsFactory { mStatsXtIfaceFmt = new File(procRoot, "net/xt_qtaguid/iface_stat_fmt"); mStatsXtUid = new File(procRoot, "net/xt_qtaguid/stats"); mUseBpfStats = useBpfStats; - synchronized (sPersistentDataLock) { + synchronized (mPersistentDataLock) { mPersistSnapshot = new NetworkStats(SystemClock.elapsedRealtime(), -1); mTunAnd464xlatAdjustedStats = new NetworkStats(SystemClock.elapsedRealtime(), -1); } @@ -304,7 +295,7 @@ public class NetworkStatsFactory { return readNetworkStatsDetail(UID_ALL, INTERFACES_ALL, TAG_ALL); } - @GuardedBy("sPersistentDataLock") + @GuardedBy("mPersistentDataLock") private void requestSwapActiveStatsMapLocked() throws RemoteException { // Ask netd to do a active map stats swap. When the binder call successfully returns, // the system server should be able to safely read and clean the inactive map @@ -328,9 +319,9 @@ public class NetworkStatsFactory { int limitUid, String[] limitIfaces, int limitTag) throws IOException { // In order to prevent deadlocks, anything protected by this lock MUST NOT call out to other // code that will acquire other locks within the system server. See b/134244752. - synchronized (sPersistentDataLock) { + synchronized (mPersistentDataLock) { // Take a reference. If this gets swapped out, we still have the old reference. - final VpnInfo[] vpnArray = sVpnInfos; + final VpnInfo[] vpnArray = mVpnInfos; // Take a defensive copy. mPersistSnapshot is mutated in some cases below final NetworkStats prev = mPersistSnapshot.clone(); @@ -379,7 +370,7 @@ public class NetworkStatsFactory { } } - @GuardedBy("sPersistentDataLock") + @GuardedBy("mPersistentDataLock") private NetworkStats adjustForTunAnd464Xlat( NetworkStats uidDetailStats, NetworkStats previousStats, VpnInfo[] vpnArray) { // Calculate delta from last snapshot @@ -389,7 +380,7 @@ public class NetworkStatsFactory { // network, the overhead is their fault. // No locking here: apply464xlatAdjustments behaves fine with an add-only // ConcurrentHashMap. - delta.apply464xlatAdjustments(sStackedIfaces, mUseBpfStats); + delta.apply464xlatAdjustments(mStackedIfaces, mUseBpfStats); // Migrate data usage over a VPN to the TUN network. for (VpnInfo info : vpnArray) { diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index e67b39137a..4f4377df09 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -130,6 +130,7 @@ import android.util.proto.ProtoOutputStream; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.net.VpnInfo; import com.android.internal.util.ArrayUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.FileRotator; @@ -777,7 +778,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { public NetworkStats getDetailedUidStats(String[] requiredIfaces) { try { final String[] ifacesToQuery = - NetworkStatsFactory.augmentWithStackedInterfaces(requiredIfaces); + mStatsFactory.augmentWithStackedInterfaces(requiredIfaces); return getNetworkStatsUidDetail(ifacesToQuery); } catch (RemoteException e) { Log.wtf(TAG, "Error compiling UID stats", e); @@ -829,7 +830,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub { public void forceUpdateIfaces( Network[] defaultNetworks, NetworkState[] networkStates, - String activeIface) { + String activeIface, + VpnInfo[] vpnInfos) { checkNetworkStackPermission(mContext); final long token = Binder.clearCallingIdentity(); @@ -838,6 +840,11 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } finally { Binder.restoreCallingIdentity(token); } + + // Update the VPN underlying interfaces only after the poll is made and tun data has been + // migrated. Otherwise the migration would use the new interfaces instead of the ones that + // were current when the polled data was transferred. + mStatsFactory.updateVpnInfos(vpnInfos); } @Override @@ -1649,7 +1656,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { // fold tethering stats and operations into uid snapshot final NetworkStats tetherSnapshot = getNetworkStatsTethering(STATS_PER_UID); tetherSnapshot.filter(UID_ALL, ifaces, TAG_ALL); - NetworkStatsFactory.apply464xlatAdjustments(uidSnapshot, tetherSnapshot, + mStatsFactory.apply464xlatAdjustments(uidSnapshot, tetherSnapshot, mUseBpfTrafficStats); uidSnapshot.combineAllValues(tetherSnapshot); @@ -1660,7 +1667,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final NetworkStats vtStats = telephonyManager.getVtDataUsage(STATS_PER_UID); if (vtStats != null) { vtStats.filter(UID_ALL, ifaces, TAG_ALL); - NetworkStatsFactory.apply464xlatAdjustments(uidSnapshot, vtStats, + mStatsFactory.apply464xlatAdjustments(uidSnapshot, vtStats, mUseBpfTrafficStats); uidSnapshot.combineAllValues(vtStats); }