NetworkStatsFactory: Take VPNs into account for network/battery stats

This change fixes detailed UID stats to ensure network and battery stats
both take VPNs into account. NetworkStatsFactory is being made aware of
VPNs enabled, and the full set of underlying networks present.

Since traffic can only be migrated over a NetworkStats delta, NSF
maintains a NetworkStats snapshot across all UIDs/ifaces/tags.

This snapshot gets updated whenever NSF records a new snapshot
(based on various hooks such as VPN updating its underlying networks,
network getting lost, etc.), or NetworkStatsService's
getDetailedUidStats() method being called.

This change widens the scope of the existing mPersistentSnapshot lock,
renaming it to mPersistentDataLock, and ensures that TUN migrations are
not done in parallel. Additionally, mVpnInfos is updated via
pointer-swapping, to reduce the scope of the mPersistentDataLock.

The safety of this change is predicated on:
1. NetworkStatsFactory lock not held, so services cannot deadlock through
the cyclical lock.

2. The broadening of the scope of the lock in NetworkStatsFactory has no
threading implications, as it is always the last (leaf node) lock held,
and therefore is impossible to have lock inversion.

Additionally, to ensure VPNs work with 464xlat, the VPN info passed to
the NetworkStatsFactory includes all underlying interfaces, instead of
only passing the first one.

This (partially) re-applies changes from:
aosp/972848: Add one more test for VPN usage stats.
aosp/972847: Addressing comments for http://ag/7700679.
aosp/885338: NetworkStatsService: Fix getDetailedUidStats to take VPNs
             into account.
Co-developed with: Varun Anand <vaanand@google.com>

Bug: 113122541
Bug: 120145746
Bug: 129264869
Bug: 134244752
Test: FrameworksNetTest passing
Test: Manual tests show data usage fixes maintained.
Merged-In: I6466ec1411fc5ed6954125d27d353b6cd1be719e
Change-Id: Id45ae956ad7165be346ecc010e17d260563ac1c0
(cherry picked from commit 9fbbdebc61513982a6775460e1d400956f803bde)

core/java/android/net/INetworkStatsService.aidl

@@ -66,7 +66,6 @@ interface INetworkStatsService {
     /** Force update of ifaces. */
     void forceUpdateIfaces(
          in Network[] defaultNetworks,
-         in VpnInfo[] vpnArray,
          in NetworkState[] networkStates,
          in String activeIface);
     /** Force update of statistics. */

core/java/android/net/NetworkStats.java

@@ -23,7 +23,6 @@ import android.annotation.UnsupportedAppUsage;
 import android.os.Parcel;
 import android.os.Parcelable;
 import android.os.SystemClock;
-import android.util.Slog;
 import android.util.SparseBooleanArray;
 
 import com.android.internal.annotations.VisibleForTesting;
@@ -37,6 +36,7 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
+import java.util.function.Predicate;
 
 /**
  * Collection of active network statistics. Can contain summary details across
@@ -994,23 +994,33 @@ public class NetworkStats implements Parcelable {
         if (limitUid == UID_ALL && limitTag == TAG_ALL && limitIfaces == INTERFACES_ALL) {
             return;
         }
+        filter(e -> (limitUid == UID_ALL || limitUid == e.uid)
+                && (limitTag == TAG_ALL || limitTag == e.tag)
+                && (limitIfaces == INTERFACES_ALL
+                    || ArrayUtils.contains(limitIfaces, e.iface)));
+    }
 
+    /**
+     * Only keep entries with {@link #set} value less than {@link #SET_DEBUG_START}.
+     *
+     * <p>This mutates the original structure in place.
+     */
+    public void filterDebugEntries() {
+        filter(e -> e.set < SET_DEBUG_START);
+    }
+
+    private void filter(Predicate<Entry> predicate) {
         Entry entry = new Entry();
         int nextOutputEntry = 0;
         for (int i = 0; i < size; i++) {
             entry = getValues(i, entry);
-            final boolean matches =
+            if (predicate.test(entry)) {
-                    (limitUid == UID_ALL || limitUid == entry.uid)
+                if (nextOutputEntry != i) {
-                    && (limitTag == TAG_ALL || limitTag == entry.tag)
+                    setValues(nextOutputEntry, entry);
-                    && (limitIfaces == INTERFACES_ALL
+                }
-                            || ArrayUtils.contains(limitIfaces, entry.iface));
-
-            if (matches) {
-                setValues(nextOutputEntry, entry);
                 nextOutputEntry++;
             }
         }
-
         size = nextOutputEntry;
     }
 
@@ -1289,7 +1299,8 @@ public class NetworkStats implements Parcelable {
         }
 
         final Entry tmpEntry = new Entry();
-        for (int i = 0; i < size; i++) {
+        final int origSize = size;
+        for (int i = 0; i < origSize; i++) {
             if (!Objects.equals(iface[i], tunIface)) {
                 // Consider only entries that go onto the VPN interface.
                 continue;
@@ -1305,8 +1316,9 @@ public class NetworkStats implements Parcelable {
             tmpEntry.roaming = roaming[i];
             tmpEntry.defaultNetwork = defaultNetwork[i];
 
-            // In a first pass, compute each UID's total share of data across all underlyingIfaces.
+            // In a first pass, compute this entry's total share of data across all
-            // This is computed on the basis of the share of each UID's usage over tunIface.
+            // underlyingIfaces. This is computed on the basis of the share of this entry's usage
+            // over tunIface.
             // TODO: Consider refactoring first pass into a separate helper method.
             long totalRxBytes = 0;
             if (tunIfaceTotal.rxBytes > 0) {
@@ -1383,9 +1395,11 @@ public class NetworkStats implements Parcelable {
                                     * perInterfaceTotal[j].operations
                                     / underlyingIfacesTotal.operations;
                 }
-
+                // tmpEntry now contains the migrated data of the i-th entry for the j-th underlying
+                // interface. Add that data usage to this object.
                 combineValues(tmpEntry);
                 if (tag[i] == TAG_NONE) {
+                    // Add the migrated data to moved so it is deducted from the VPN app later.
                     moved[j].add(tmpEntry);
                     // Add debug info
                     tmpEntry.set = SET_DBG_VPN_IN;
@@ -1401,8 +1415,8 @@ public class NetworkStats implements Parcelable {
             @NonNull String[] underlyingIfaces,
             @NonNull Entry[] moved) {
         for (int i = 0; i < underlyingIfaces.length; i++) {
-            // Add debug info
             moved[i].uid = tunUid;
+            // Add debug info
             moved[i].set = SET_DBG_VPN_OUT;
             moved[i].tag = TAG_NONE;
             moved[i].iface = underlyingIfaces[i];

services/core/java/com/android/server/net/NetworkStatsFactory.java

@@ -34,6 +34,7 @@ import android.os.SystemClock;
 
 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.ProcFileReader;
 
@@ -71,11 +72,25 @@ public class NetworkStatsFactory {
 
     private INetd mNetdService;
 
-    // A persistent Snapshot since device start for eBPF stats
+    /**
-    @GuardedBy("mPersistSnapshot")
+     * Guards persistent data access in this class
-    private final NetworkStats mPersistSnapshot;
+     *
+     * <p>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();
+
+    /** Set containing info about active VPNs and their underlying networks. */
+    private static volatile VpnInfo[] sVpnInfos = new VpnInfo[0];
+
+    // A persistent snapshot of cumulative stats since device start
+    @GuardedBy("sPersistentDataLock")
+    private NetworkStats mPersistSnapshot;
+
+    // The persistent snapshot of tun and 464xlat adjusted stats since device start
+    @GuardedBy("sPersistentDataLock")
+    private NetworkStats mTunAnd464xlatAdjustedStats;
 
-    // TODO: only do adjustments in NetworkStatsService and remove this.
     /**
      * (Stacked interface) -> (base interface) association for all connected ifaces since boot.
      *
@@ -91,6 +106,24 @@ public class NetworkStatsFactory {
         }
     }
 
+    /**
+     * Set active VPN information for data usage migration purposes
+     *
+     * <p>Traffic on TUN-based VPNs inherently all appear to be originated from the VPN providing
+     * app's UID. This method is used to support migration of VPN data usage, ensuring data is
+     * accurately billed to the real owner of the traffic.
+     *
+     * @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();
+    }
+
     /**
      * Get a set of interfaces containing specified ifaces and stacked interfaces.
      *
@@ -147,6 +180,7 @@ public class NetworkStatsFactory {
         mStatsXtUid = new File(procRoot, "net/xt_qtaguid/stats");
         mUseBpfStats = useBpfStats;
         mPersistSnapshot = new NetworkStats(SystemClock.elapsedRealtime(), -1);
+        mTunAnd464xlatAdjustedStats = new NetworkStats(SystemClock.elapsedRealtime(), -1);
     }
 
     public NetworkStats readBpfNetworkStatsDev() throws IOException {
@@ -279,16 +313,10 @@ public class NetworkStatsFactory {
      */
     public NetworkStats readNetworkStatsDetail(
             int limitUid, @Nullable String[] limitIfaces, int limitTag) throws IOException {
-        final NetworkStats stats = readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag);
+        return readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag);
-
-        // No locking here: apply464xlatAdjustments behaves fine with an add-only ConcurrentHashMap.
-        // TODO: remove this and only apply adjustments in NetworkStatsService.
-        stats.apply464xlatAdjustments(sStackedIfaces, mUseBpfStats);
-
-        return stats;
     }
 
-    @GuardedBy("mPersistSnapshot")
+    @GuardedBy("sPersistentDataLock")
     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
@@ -303,11 +331,18 @@ public class NetworkStatsFactory {
 
     private NetworkStats readNetworkStatsDetailInternal(
             int limitUid, String[] limitIfaces, int limitTag) throws IOException {
-        if (USE_NATIVE_PARSING) {
+        // In order to prevent deadlocks, anything protected by this lock MUST NOT call out to other
-            final NetworkStats stats =
+        // code that will acquire other locks within the system server. See b/134244752.
-                    new NetworkStats(SystemClock.elapsedRealtime(), 0 /* initialSize */);
+        synchronized (sPersistentDataLock) {
-            if (mUseBpfStats) {
+            // Take a reference. If this gets swapped out, we still have the old reference.
-                synchronized (mPersistSnapshot) {
+            final VpnInfo[] vpnArray = sVpnInfos;
+            // Take a defensive copy. mPersistSnapshot is mutated in some cases below
+            final NetworkStats prev = mPersistSnapshot.clone();
+
+            if (USE_NATIVE_PARSING) {
+                final NetworkStats stats =
+                        new NetworkStats(SystemClock.elapsedRealtime(), 0 /* initialSize */);
+                if (mUseBpfStats) {
                     try {
                         requestSwapActiveStatsMapLocked();
                     } catch (RemoteException e) {
@@ -316,32 +351,66 @@ public class NetworkStatsFactory {
                     // Stats are always read from the inactive map, so they must be read after the
                     // swap
                     if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), UID_ALL,
-                            null, TAG_ALL, mUseBpfStats) != 0) {
+                            INTERFACES_ALL, TAG_ALL, mUseBpfStats) != 0) {
                         throw new IOException("Failed to parse network stats");
                     }
+
+                    // BPF stats are incremental; fold into mPersistSnapshot.
                     mPersistSnapshot.setElapsedRealtime(stats.getElapsedRealtime());
                     mPersistSnapshot.combineAllValues(stats);
-                    NetworkStats result = mPersistSnapshot.clone();
+                } else {
-                    result.filter(limitUid, limitIfaces, limitTag);
+                    if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), UID_ALL,
-                    return result;
+                            INTERFACES_ALL, TAG_ALL, mUseBpfStats) != 0) {
+                        throw new IOException("Failed to parse network stats");
+                    }
+                    if (SANITY_CHECK_NATIVE) {
+                        final NetworkStats javaStats = javaReadNetworkStatsDetail(mStatsXtUid,
+                                UID_ALL, INTERFACES_ALL, TAG_ALL);
+                        assertEquals(javaStats, stats);
+                    }
+
+                    mPersistSnapshot = stats;
                 }
             } else {
-                if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), limitUid,
+                mPersistSnapshot = javaReadNetworkStatsDetail(mStatsXtUid, UID_ALL, INTERFACES_ALL,
-                        limitIfaces, limitTag, mUseBpfStats) != 0) {
+                        TAG_ALL);
-                    throw new IOException("Failed to parse network stats");
-                }
-                if (SANITY_CHECK_NATIVE) {
-                    final NetworkStats javaStats = javaReadNetworkStatsDetail(mStatsXtUid, limitUid,
-                            limitIfaces, limitTag);
-                    assertEquals(javaStats, stats);
-                }
-                return stats;
             }
-        } else {
+
-            return javaReadNetworkStatsDetail(mStatsXtUid, limitUid, limitIfaces, limitTag);
+            NetworkStats adjustedStats = adjustForTunAnd464Xlat(mPersistSnapshot, prev, vpnArray);
+
+            // Filter return values
+            adjustedStats.filter(limitUid, limitIfaces, limitTag);
+            return adjustedStats;
         }
     }
 
+    @GuardedBy("sPersistentDataLock")
+    private NetworkStats adjustForTunAnd464Xlat(
+            NetworkStats uidDetailStats, NetworkStats previousStats, VpnInfo[] vpnArray) {
+        // Calculate delta from last snapshot
+        final NetworkStats delta = uidDetailStats.subtract(previousStats);
+
+        // Apply 464xlat adjustments before VPN adjustments. If VPNs are using v4 on a v6 only
+        // network, the overhead is their fault.
+        // No locking here: apply464xlatAdjustments behaves fine with an add-only
+        // ConcurrentHashMap.
+        delta.apply464xlatAdjustments(sStackedIfaces, mUseBpfStats);
+
+        // Migrate data usage over a VPN to the TUN network.
+        for (VpnInfo info : vpnArray) {
+            delta.migrateTun(info.ownerUid, info.vpnIface, info.underlyingIfaces);
+        }
+
+        // Filter out debug entries as that may lead to over counting.
+        delta.filterDebugEntries();
+
+        // Update mTunAnd464xlatAdjustedStats with migrated delta.
+        mTunAnd464xlatAdjustedStats.combineAllValues(delta);
+        mTunAnd464xlatAdjustedStats.setElapsedRealtime(uidDetailStats.getElapsedRealtime());
+
+        return mTunAnd464xlatAdjustedStats.clone();
+    }
+
     /**
      * Parse and return {@link NetworkStats} with UID-level details. Values are
      * expected to monotonically increase since device boot.

services/core/java/com/android/server/net/NetworkStatsService.java

@@ -131,7 +131,6 @@ 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;
@@ -267,10 +266,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     @GuardedBy("mStatsLock")
     private Network[] mDefaultNetworks = new Network[0];
 
-    /** Set containing info about active VPNs and their underlying networks. */
-    @GuardedBy("mStatsLock")
-    private VpnInfo[] mVpnInfos = new VpnInfo[0];
-
     private final DropBoxNonMonotonicObserver mNonMonotonicObserver =
             new DropBoxNonMonotonicObserver();
 
@@ -864,7 +859,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     @Override
     public void forceUpdateIfaces(
             Network[] defaultNetworks,
-            VpnInfo[] vpnArray,
             NetworkState[] networkStates,
             String activeIface) {
         checkNetworkStackPermission(mContext);
@@ -872,7 +866,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
         final long token = Binder.clearCallingIdentity();
         try {
-            updateIfaces(defaultNetworks, vpnArray, networkStates, activeIface);
+            updateIfaces(defaultNetworks, networkStates, activeIface);
         } finally {
             Binder.restoreCallingIdentity(token);
         }
@@ -1138,13 +1132,11 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
     private void updateIfaces(
             Network[] defaultNetworks,
-            VpnInfo[] vpnArray,
             NetworkState[] networkStates,
             String activeIface) {
         synchronized (mStatsLock) {
             mWakeLock.acquire();
             try {
-                mVpnInfos = vpnArray;
                 mActiveIface = activeIface;
                 updateIfacesLocked(defaultNetworks, networkStates);
             } finally {
@@ -1154,10 +1146,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     /**
-     * Inspect all current {@link NetworkState} to derive mapping from {@code
+     * Inspect all current {@link NetworkState} to derive mapping from {@code iface} to {@link
-     * iface} to {@link NetworkStatsHistory}. When multiple {@link NetworkInfo}
+     * NetworkStatsHistory}. When multiple {@link NetworkInfo} are active on a single {@code iface},
-     * are active on a single {@code iface}, they are combined under a single
+     * they are combined under a single {@link NetworkIdentitySet}.
-     * {@link NetworkIdentitySet}.
      */
     @GuardedBy("mStatsLock")
     private void updateIfacesLocked(Network[] defaultNetworks, NetworkState[] states) {
@@ -1283,18 +1274,19 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         Trace.traceEnd(TRACE_TAG_NETWORK);
 
         // For per-UID stats, pass the VPN info so VPN traffic is reattributed to responsible apps.
-        VpnInfo[] vpnArray = mVpnInfos;
         Trace.traceBegin(TRACE_TAG_NETWORK, "recordUid");
-        mUidRecorder.recordSnapshotLocked(uidSnapshot, mActiveUidIfaces, vpnArray, currentTime);
+        mUidRecorder.recordSnapshotLocked(
+                uidSnapshot, mActiveUidIfaces, null /* vpnArray */, currentTime);
         Trace.traceEnd(TRACE_TAG_NETWORK);
         Trace.traceBegin(TRACE_TAG_NETWORK, "recordUidTag");
-        mUidTagRecorder.recordSnapshotLocked(uidSnapshot, mActiveUidIfaces, vpnArray, currentTime);
+        mUidTagRecorder.recordSnapshotLocked(
+                uidSnapshot, mActiveUidIfaces, null /* vpnArray */, currentTime);
         Trace.traceEnd(TRACE_TAG_NETWORK);
 
         // We need to make copies of member fields that are sent to the observer to avoid
         // a race condition between the service handler thread and the observer's
         mStatsObservers.updateStats(xtSnapshot, uidSnapshot, new ArrayMap<>(mActiveIfaces),
-                new ArrayMap<>(mActiveUidIfaces), vpnArray, currentTime);
+                new ArrayMap<>(mActiveUidIfaces), null /* vpnArray */, currentTime);
     }
 
     /**
@@ -1667,8 +1659,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
      */
     private NetworkStats getNetworkStatsUidDetail(String[] ifaces)
             throws RemoteException {
-
-        // TODO: remove 464xlat adjustments from NetworkStatsFactory and apply all at once here.
         final NetworkStats uidSnapshot = mNetworkManager.getNetworkStatsUidDetail(UID_ALL,
                 ifaces);