From 2cca1ef390d2f436e5ae566d868fe2063ec9d8eb Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 12 Jun 2019 17:46:15 +0000 Subject: [PATCH 1/6] Revert "Revert "Take all VPN underlying networks into account when migrating traffic for"" This reverts commit d8220c20507f0c346f517d715c7b9826b04d64e2. Reason for revert: Fix available for deadlocks. Bug: 113122541 Bug: 134244752 Merged-In: Ib65214598837289bd39dbf040b56ab7835f893ba Change-Id: Ia90bf2c72ef686e80800d113d03548e0efcadb66 (cherry picked from commit a84d9fa57247cf78a9297b0c6dbd3d81b69e235f) --- core/java/android/net/NetworkStats.java | 330 +++++++++++------- .../server/net/NetworkStatsRecorder.java | 6 +- 2 files changed, 214 insertions(+), 122 deletions(-) diff --git a/core/java/android/net/NetworkStats.java b/core/java/android/net/NetworkStats.java index e892b650bf..f09f2ee223 100644 --- a/core/java/android/net/NetworkStats.java +++ b/core/java/android/net/NetworkStats.java @@ -18,6 +18,7 @@ package android.net; import static android.os.Process.CLAT_UID; +import android.annotation.NonNull; import android.annotation.UnsupportedAppUsage; import android.os.Parcel; import android.os.Parcelable; @@ -1175,133 +1176,217 @@ public class NetworkStats implements Parcelable { /** * VPN accounting. Move some VPN's underlying traffic to other UIDs that use tun0 iface. * - * This method should only be called on delta NetworkStats. Do not call this method on a - * snapshot {@link NetworkStats} object because the tunUid and/or the underlyingIface may - * change over time. + *

This method should only be called on delta NetworkStats. Do not call this method on a + * snapshot {@link NetworkStats} object because the tunUid and/or the underlyingIface may change + * over time. * - * This method performs adjustments for one active VPN package and one VPN iface at a time. - * - * It is possible for the VPN software to use multiple underlying networks. This method - * only migrates traffic for the primary underlying network. + *

This method performs adjustments for one active VPN package and one VPN iface at a time. * * @param tunUid uid of the VPN application * @param tunIface iface of the vpn tunnel - * @param underlyingIface the primary underlying network iface used by the VPN application - * @return true if it successfully adjusts the accounting for VPN, false otherwise + * @param underlyingIfaces underlying network ifaces used by the VPN application */ - public boolean migrateTun(int tunUid, String tunIface, String underlyingIface) { - Entry tunIfaceTotal = new Entry(); - Entry underlyingIfaceTotal = new Entry(); + public void migrateTun(int tunUid, @NonNull String tunIface, + @NonNull String[] underlyingIfaces) { + // Combined usage by all apps using VPN. + final Entry tunIfaceTotal = new Entry(); + // Usage by VPN, grouped by its {@code underlyingIfaces}. + final Entry[] perInterfaceTotal = new Entry[underlyingIfaces.length]; + // Usage by VPN, summed across all its {@code underlyingIfaces}. + final Entry underlyingIfacesTotal = new Entry(); - tunAdjustmentInit(tunUid, tunIface, underlyingIface, tunIfaceTotal, underlyingIfaceTotal); + for (int i = 0; i < perInterfaceTotal.length; i++) { + perInterfaceTotal[i] = new Entry(); + } - // If tunIface < underlyingIface, it leaves the overhead traffic in the VPN app. - // If tunIface > underlyingIface, the VPN app doesn't get credit for data compression. + tunAdjustmentInit(tunUid, tunIface, underlyingIfaces, tunIfaceTotal, perInterfaceTotal, + underlyingIfacesTotal); + + // If tunIface < underlyingIfacesTotal, it leaves the overhead traffic in the VPN app. + // If tunIface > underlyingIfacesTotal, the VPN app doesn't get credit for data compression. // Negative stats should be avoided. - Entry pool = tunGetPool(tunIfaceTotal, underlyingIfaceTotal); - if (pool.isEmpty()) { - return true; - } - Entry moved = - addTrafficToApplications(tunUid, tunIface, underlyingIface, tunIfaceTotal, pool); - deductTrafficFromVpnApp(tunUid, underlyingIface, moved); - - if (!moved.isEmpty()) { - Slog.wtf(TAG, "Failed to deduct underlying network traffic from VPN package. Moved=" - + moved); - return false; - } - return true; + final Entry[] moved = + addTrafficToApplications(tunUid, tunIface, underlyingIfaces, tunIfaceTotal, + perInterfaceTotal, underlyingIfacesTotal); + deductTrafficFromVpnApp(tunUid, underlyingIfaces, moved); } /** * Initializes the data used by the migrateTun() method. * - * This is the first pass iteration which does the following work: - * (1) Adds up all the traffic through the tunUid's underlyingIface - * (both foreground and background). - * (2) Adds up all the traffic through tun0 excluding traffic from the vpn app itself. + *

This is the first pass iteration which does the following work: + * + *

+ * + * @param tunUid uid of the VPN application + * @param tunIface iface of the vpn tunnel + * @param underlyingIfaces underlying network ifaces used by the VPN application + * @param tunIfaceTotal output parameter; combined data usage by all apps using VPN + * @param perInterfaceTotal output parameter; data usage by VPN app, grouped by its {@code + * underlyingIfaces} + * @param underlyingIfacesTotal output parameter; data usage by VPN, summed across all of its + * {@code underlyingIfaces} */ - private void tunAdjustmentInit(int tunUid, String tunIface, String underlyingIface, - Entry tunIfaceTotal, Entry underlyingIfaceTotal) { - Entry recycle = new Entry(); + private void tunAdjustmentInit(int tunUid, @NonNull String tunIface, + @NonNull String[] underlyingIfaces, @NonNull Entry tunIfaceTotal, + @NonNull Entry[] perInterfaceTotal, @NonNull Entry underlyingIfacesTotal) { + final Entry recycle = new Entry(); for (int i = 0; i < size; i++) { getValues(i, recycle); if (recycle.uid == UID_ALL) { throw new IllegalStateException( "Cannot adjust VPN accounting on an iface aggregated NetworkStats."); - } if (recycle.set == SET_DBG_VPN_IN || recycle.set == SET_DBG_VPN_OUT) { + } + if (recycle.set == SET_DBG_VPN_IN || recycle.set == SET_DBG_VPN_OUT) { throw new IllegalStateException( "Cannot adjust VPN accounting on a NetworkStats containing SET_DBG_VPN_*"); } - - if (recycle.uid == tunUid && recycle.tag == TAG_NONE - && Objects.equals(underlyingIface, recycle.iface)) { - underlyingIfaceTotal.add(recycle); + if (recycle.tag != TAG_NONE) { + // TODO(b/123666283): Take all tags for tunUid into account. + continue; } - if (recycle.uid != tunUid && recycle.tag == TAG_NONE - && Objects.equals(tunIface, recycle.iface)) { + if (recycle.uid == tunUid) { + // Add up traffic through tunUid's underlying interfaces. + for (int j = 0; j < underlyingIfaces.length; j++) { + if (Objects.equals(underlyingIfaces[j], recycle.iface)) { + perInterfaceTotal[j].add(recycle); + underlyingIfacesTotal.add(recycle); + break; + } + } + } else if (tunIface.equals(recycle.iface)) { // Add up all tunIface traffic excluding traffic from the vpn app itself. tunIfaceTotal.add(recycle); } } } - private static Entry tunGetPool(Entry tunIfaceTotal, Entry underlyingIfaceTotal) { - Entry pool = new Entry(); - pool.rxBytes = Math.min(tunIfaceTotal.rxBytes, underlyingIfaceTotal.rxBytes); - pool.rxPackets = Math.min(tunIfaceTotal.rxPackets, underlyingIfaceTotal.rxPackets); - pool.txBytes = Math.min(tunIfaceTotal.txBytes, underlyingIfaceTotal.txBytes); - pool.txPackets = Math.min(tunIfaceTotal.txPackets, underlyingIfaceTotal.txPackets); - pool.operations = Math.min(tunIfaceTotal.operations, underlyingIfaceTotal.operations); - return pool; - } + /** + * Distributes traffic across apps that are using given {@code tunIface}, and returns the total + * traffic that should be moved off of {@code tunUid} grouped by {@code underlyingIfaces}. + * + * @param tunUid uid of the VPN application + * @param tunIface iface of the vpn tunnel + * @param underlyingIfaces underlying network ifaces used by the VPN application + * @param tunIfaceTotal combined data usage across all apps using {@code tunIface} + * @param perInterfaceTotal data usage by VPN app, grouped by its {@code underlyingIfaces} + * @param underlyingIfacesTotal data usage by VPN, summed across all of its {@code + * underlyingIfaces} + */ + private Entry[] addTrafficToApplications(int tunUid, @NonNull String tunIface, + @NonNull String[] underlyingIfaces, @NonNull Entry tunIfaceTotal, + @NonNull Entry[] perInterfaceTotal, @NonNull Entry underlyingIfacesTotal) { + // Traffic that should be moved off of each underlying interface for tunUid (see + // deductTrafficFromVpnApp below). + final Entry[] moved = new Entry[underlyingIfaces.length]; + for (int i = 0; i < underlyingIfaces.length; i++) { + moved[i] = new Entry(); + } - private Entry addTrafficToApplications(int tunUid, String tunIface, String underlyingIface, - Entry tunIfaceTotal, Entry pool) { - Entry moved = new Entry(); - Entry tmpEntry = new Entry(); - tmpEntry.iface = underlyingIface; + final Entry tmpEntry = new Entry(); for (int i = 0; i < size; i++) { - // the vpn app is excluded from the redistribution but all moved traffic will be - // deducted from the vpn app (see deductTrafficFromVpnApp below). - if (Objects.equals(iface[i], tunIface) && uid[i] != tunUid) { - if (tunIfaceTotal.rxBytes > 0) { - tmpEntry.rxBytes = pool.rxBytes * rxBytes[i] / tunIfaceTotal.rxBytes; - } else { - tmpEntry.rxBytes = 0; - } - if (tunIfaceTotal.rxPackets > 0) { - tmpEntry.rxPackets = pool.rxPackets * rxPackets[i] / tunIfaceTotal.rxPackets; - } else { - tmpEntry.rxPackets = 0; - } - if (tunIfaceTotal.txBytes > 0) { - tmpEntry.txBytes = pool.txBytes * txBytes[i] / tunIfaceTotal.txBytes; - } else { - tmpEntry.txBytes = 0; - } - if (tunIfaceTotal.txPackets > 0) { - tmpEntry.txPackets = pool.txPackets * txPackets[i] / tunIfaceTotal.txPackets; - } else { - tmpEntry.txPackets = 0; - } - if (tunIfaceTotal.operations > 0) { - tmpEntry.operations = - pool.operations * operations[i] / tunIfaceTotal.operations; - } else { - tmpEntry.operations = 0; - } - tmpEntry.uid = uid[i]; - tmpEntry.tag = tag[i]; + if (!Objects.equals(iface[i], tunIface)) { + // Consider only entries that go onto the VPN interface. + continue; + } + if (uid[i] == tunUid) { + // Exclude VPN app from the redistribution, as it can choose to create packet + // streams by writing to itself. + continue; + } + tmpEntry.uid = uid[i]; + tmpEntry.tag = tag[i]; + tmpEntry.metered = metered[i]; + tmpEntry.roaming = roaming[i]; + tmpEntry.defaultNetwork = defaultNetwork[i]; + + // In a first pass, compute each UID's total share of data across all underlyingIfaces. + // This is computed on the basis of the share of each UID's usage over tunIface. + // TODO: Consider refactoring first pass into a separate helper method. + long totalRxBytes = 0; + if (tunIfaceTotal.rxBytes > 0) { + // Note - The multiplication below should not overflow since NetworkStatsService + // processes this every time device has transmitted/received amount equivalent to + // global threshold alert (~ 2MB) across all interfaces. + final long rxBytesAcrossUnderlyingIfaces = + underlyingIfacesTotal.rxBytes * rxBytes[i] / tunIfaceTotal.rxBytes; + // app must not be blamed for more than it consumed on tunIface + totalRxBytes = Math.min(rxBytes[i], rxBytesAcrossUnderlyingIfaces); + } + long totalRxPackets = 0; + if (tunIfaceTotal.rxPackets > 0) { + final long rxPacketsAcrossUnderlyingIfaces = + underlyingIfacesTotal.rxPackets * rxPackets[i] / tunIfaceTotal.rxPackets; + totalRxPackets = Math.min(rxPackets[i], rxPacketsAcrossUnderlyingIfaces); + } + long totalTxBytes = 0; + if (tunIfaceTotal.txBytes > 0) { + final long txBytesAcrossUnderlyingIfaces = + underlyingIfacesTotal.txBytes * txBytes[i] / tunIfaceTotal.txBytes; + totalTxBytes = Math.min(txBytes[i], txBytesAcrossUnderlyingIfaces); + } + long totalTxPackets = 0; + if (tunIfaceTotal.txPackets > 0) { + final long txPacketsAcrossUnderlyingIfaces = + underlyingIfacesTotal.txPackets * txPackets[i] / tunIfaceTotal.txPackets; + totalTxPackets = Math.min(txPackets[i], txPacketsAcrossUnderlyingIfaces); + } + long totalOperations = 0; + if (tunIfaceTotal.operations > 0) { + final long operationsAcrossUnderlyingIfaces = + underlyingIfacesTotal.operations * operations[i] / tunIfaceTotal.operations; + totalOperations = Math.min(operations[i], operationsAcrossUnderlyingIfaces); + } + // In a second pass, distribute these values across interfaces in the proportion that + // each interface represents of the total traffic of the underlying interfaces. + for (int j = 0; j < underlyingIfaces.length; j++) { + tmpEntry.iface = underlyingIfaces[j]; + tmpEntry.rxBytes = 0; + // Reset 'set' to correct value since it gets updated when adding debug info below. tmpEntry.set = set[i]; - tmpEntry.metered = metered[i]; - tmpEntry.roaming = roaming[i]; - tmpEntry.defaultNetwork = defaultNetwork[i]; + if (underlyingIfacesTotal.rxBytes > 0) { + tmpEntry.rxBytes = + totalRxBytes + * perInterfaceTotal[j].rxBytes + / underlyingIfacesTotal.rxBytes; + } + tmpEntry.rxPackets = 0; + if (underlyingIfacesTotal.rxPackets > 0) { + tmpEntry.rxPackets = + totalRxPackets + * perInterfaceTotal[j].rxPackets + / underlyingIfacesTotal.rxPackets; + } + tmpEntry.txBytes = 0; + if (underlyingIfacesTotal.txBytes > 0) { + tmpEntry.txBytes = + totalTxBytes + * perInterfaceTotal[j].txBytes + / underlyingIfacesTotal.txBytes; + } + tmpEntry.txPackets = 0; + if (underlyingIfacesTotal.txPackets > 0) { + tmpEntry.txPackets = + totalTxPackets + * perInterfaceTotal[j].txPackets + / underlyingIfacesTotal.txPackets; + } + tmpEntry.operations = 0; + if (underlyingIfacesTotal.operations > 0) { + tmpEntry.operations = + totalOperations + * perInterfaceTotal[j].operations + / underlyingIfacesTotal.operations; + } + combineValues(tmpEntry); if (tag[i] == TAG_NONE) { - moved.add(tmpEntry); + moved[j].add(tmpEntry); // Add debug info tmpEntry.set = SET_DBG_VPN_IN; combineValues(tmpEntry); @@ -1311,38 +1396,45 @@ public class NetworkStats implements Parcelable { return moved; } - private void deductTrafficFromVpnApp(int tunUid, String underlyingIface, Entry moved) { - // Add debug info - moved.uid = tunUid; - moved.set = SET_DBG_VPN_OUT; - moved.tag = TAG_NONE; - moved.iface = underlyingIface; - moved.metered = METERED_ALL; - moved.roaming = ROAMING_ALL; - moved.defaultNetwork = DEFAULT_NETWORK_ALL; - combineValues(moved); + private void deductTrafficFromVpnApp( + int tunUid, + @NonNull String[] underlyingIfaces, + @NonNull Entry[] moved) { + for (int i = 0; i < underlyingIfaces.length; i++) { + // Add debug info + moved[i].uid = tunUid; + moved[i].set = SET_DBG_VPN_OUT; + moved[i].tag = TAG_NONE; + moved[i].iface = underlyingIfaces[i]; + moved[i].metered = METERED_ALL; + moved[i].roaming = ROAMING_ALL; + moved[i].defaultNetwork = DEFAULT_NETWORK_ALL; + combineValues(moved[i]); - // Caveat: if the vpn software uses tag, the total tagged traffic may be greater than - // the TAG_NONE traffic. - // - // Relies on the fact that the underlying traffic only has state ROAMING_NO and METERED_NO, - // which should be the case as it comes directly from the /proc file. We only blend in the - // roaming data after applying these adjustments, by checking the NetworkIdentity of the - // underlying iface. - int idxVpnBackground = findIndex(underlyingIface, tunUid, SET_DEFAULT, TAG_NONE, - METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO); - if (idxVpnBackground != -1) { - tunSubtract(idxVpnBackground, this, moved); - } + // Caveat: if the vpn software uses tag, the total tagged traffic may be greater than + // the TAG_NONE traffic. + // + // Relies on the fact that the underlying traffic only has state ROAMING_NO and + // METERED_NO, which should be the case as it comes directly from the /proc file. + // We only blend in the roaming data after applying these adjustments, by checking the + // NetworkIdentity of the underlying iface. + final int idxVpnBackground = findIndex(underlyingIfaces[i], tunUid, SET_DEFAULT, + TAG_NONE, METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO); + if (idxVpnBackground != -1) { + // Note - tunSubtract also updates moved[i]; whatever traffic that's left is removed + // from foreground usage. + tunSubtract(idxVpnBackground, this, moved[i]); + } - int idxVpnForeground = findIndex(underlyingIface, tunUid, SET_FOREGROUND, TAG_NONE, - METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO); - if (idxVpnForeground != -1) { - tunSubtract(idxVpnForeground, this, moved); + final int idxVpnForeground = findIndex(underlyingIfaces[i], tunUid, SET_FOREGROUND, + TAG_NONE, METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO); + if (idxVpnForeground != -1) { + tunSubtract(idxVpnForeground, this, moved[i]); + } } } - private static void tunSubtract(int i, NetworkStats left, Entry right) { + private static void tunSubtract(int i, @NonNull NetworkStats left, @NonNull Entry right) { long rxBytes = Math.min(left.rxBytes[i], right.rxBytes); left.rxBytes[i] -= rxBytes; right.rxBytes -= rxBytes; diff --git a/services/core/java/com/android/server/net/NetworkStatsRecorder.java b/services/core/java/com/android/server/net/NetworkStatsRecorder.java index a2e7e0cae9..bdff50053f 100644 --- a/services/core/java/com/android/server/net/NetworkStatsRecorder.java +++ b/services/core/java/com/android/server/net/NetworkStatsRecorder.java @@ -41,10 +41,10 @@ import com.android.internal.net.VpnInfo; import com.android.internal.util.FileRotator; import com.android.internal.util.IndentingPrintWriter; -import libcore.io.IoUtils; - import com.google.android.collect.Sets; +import libcore.io.IoUtils; + import java.io.ByteArrayOutputStream; import java.io.DataOutputStream; import java.io.File; @@ -234,7 +234,7 @@ public class NetworkStatsRecorder { if (vpnArray != null) { for (VpnInfo info : vpnArray) { - delta.migrateTun(info.ownerUid, info.vpnIface, info.primaryUnderlyingIface); + delta.migrateTun(info.ownerUid, info.vpnIface, info.underlyingIfaces); } } From a3da62f5a12fed130e152a5a23f5b7ff0cb622be Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 11 Jun 2019 18:21:30 -0700 Subject: [PATCH 2/6] Remove unused lastStats parameter This change removes an unused parameter that is always null in getNetworkStatsUidDetail Bug: 113122541 Bug: 134244752 Test: FrameworksNetTest passing Merged-In: I995b108ef30e1fbd6190131ed4db40a3d9327eb5 Change-Id: I575a7e4fa145f2c93537f33a2bfe952aeafd0e69 (cherry picked from commit 5823e8d3c69b10ad2a458e491c146631457ca86d) --- .../server/net/NetworkStatsFactory.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index 69efd02dea..c118bda88c 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -16,6 +16,7 @@ package com.android.server.net; +import static android.net.NetworkStats.INTERFACES_ALL; import static android.net.NetworkStats.SET_ALL; import static android.net.NetworkStats.TAG_ALL; import static android.net.NetworkStats.TAG_NONE; @@ -264,13 +265,21 @@ public class NetworkStatsFactory { } public NetworkStats readNetworkStatsDetail() throws IOException { - return readNetworkStatsDetail(UID_ALL, null, TAG_ALL, null); + return readNetworkStatsDetail(UID_ALL, INTERFACES_ALL, TAG_ALL); } - public NetworkStats readNetworkStatsDetail(int limitUid, String[] limitIfaces, int limitTag, - NetworkStats lastStats) throws IOException { - final NetworkStats stats = - readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag, lastStats); + /** + * Reads the detailed UID stats based on the provided parameters + * + * @param limitUid the UID to limit this query to + * @param limitIfaces the interfaces to limit this query to. Use {@link + * NetworkStats.INTERFACES_ALL} to select all interfaces + * @param limitTag the tags to limit this query to + * @return the NetworkStats instance containing network statistics at the present time. + */ + public NetworkStats readNetworkStatsDetail( + int limitUid, @Nullable String[] limitIfaces, int limitTag) throws IOException { + final NetworkStats stats = readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag); // No locking here: apply464xlatAdjustments behaves fine with an add-only ConcurrentHashMap. // TODO: remove this and only apply adjustments in NetworkStatsService. @@ -292,17 +301,11 @@ public class NetworkStatsFactory { } } - // TODO: delete the lastStats parameter - private NetworkStats readNetworkStatsDetailInternal(int limitUid, String[] limitIfaces, - int limitTag, NetworkStats lastStats) throws IOException { + private NetworkStats readNetworkStatsDetailInternal( + int limitUid, String[] limitIfaces, int limitTag) throws IOException { if (USE_NATIVE_PARSING) { - final NetworkStats stats; - if (lastStats != null) { - stats = lastStats; - stats.setElapsedRealtime(SystemClock.elapsedRealtime()); - } else { - stats = new NetworkStats(SystemClock.elapsedRealtime(), -1); - } + final NetworkStats stats = + new NetworkStats(SystemClock.elapsedRealtime(), 0 /* initialSize */); if (mUseBpfStats) { synchronized (mPersistSnapshot) { try { From dedb6bb0e5e9c1ad6bf800bdfb0b2c502bf78ce4 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 12 Jun 2019 17:46:31 +0000 Subject: [PATCH 3/6] 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 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) --- .../android/net/INetworkStatsService.aidl | 1 - core/java/android/net/NetworkStats.java | 44 ++++-- .../server/net/NetworkStatsFactory.java | 135 +++++++++++++----- .../server/net/NetworkStatsService.java | 28 ++-- 4 files changed, 140 insertions(+), 68 deletions(-) diff --git a/core/java/android/net/INetworkStatsService.aidl b/core/java/android/net/INetworkStatsService.aidl index 41efc50408..e5f3d26667 100644 --- a/core/java/android/net/INetworkStatsService.aidl +++ b/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. */ diff --git a/core/java/android/net/NetworkStats.java b/core/java/android/net/NetworkStats.java index f09f2ee223..6c7aa1379f 100644 --- a/core/java/android/net/NetworkStats.java +++ b/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}. + * + *

This mutates the original structure in place. + */ + public void filterDebugEntries() { + filter(e -> e.set < SET_DEBUG_START); + } + + private void filter(Predicate predicate) { Entry entry = new Entry(); int nextOutputEntry = 0; for (int i = 0; i < size; i++) { entry = getValues(i, entry); - final boolean matches = - (limitUid == UID_ALL || limitUid == entry.uid) - && (limitTag == TAG_ALL || limitTag == entry.tag) - && (limitIfaces == INTERFACES_ALL - || ArrayUtils.contains(limitIfaces, entry.iface)); - - if (matches) { - setValues(nextOutputEntry, entry); + if (predicate.test(entry)) { + if (nextOutputEntry != i) { + 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. - // This is computed on the basis of the share of each UID's usage over tunIface. + // In a first pass, compute this entry's total share of data across all + // 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]; diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index c118bda88c..2d3c66d68f 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/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") - private final NetworkStats mPersistSnapshot; + /** + * Guards persistent data access in this class + * + *

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 + * + *

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); - - // 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; + return readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag); } - @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) { - final NetworkStats stats = - new NetworkStats(SystemClock.elapsedRealtime(), 0 /* initialSize */); - if (mUseBpfStats) { - synchronized (mPersistSnapshot) { + // 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) { + // Take a reference. If this gets swapped out, we still have the old reference. + 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(); - result.filter(limitUid, limitIfaces, limitTag); - return result; + } else { + if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), UID_ALL, + 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, - limitIfaces, limitTag, mUseBpfStats) != 0) { - 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; + mPersistSnapshot = javaReadNetworkStatsDetail(mStatsXtUid, UID_ALL, INTERFACES_ALL, + TAG_ALL); } - } 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. diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index f34ace55a7..34ce8199a6 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/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 - * iface} to {@link NetworkStatsHistory}. When multiple {@link NetworkInfo} - * are active on a single {@code iface}, they are combined under a single - * {@link NetworkIdentitySet}. + * Inspect all current {@link NetworkState} to derive mapping from {@code iface} to {@link + * NetworkStatsHistory}. When multiple {@link NetworkInfo} are active on a single {@code iface}, + * they are combined under a single {@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); From 4d326f96ccd0dd6eec9d55762baf3127b620b7af Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 13 Jun 2019 10:54:38 -0700 Subject: [PATCH 4/6] Remove VPN info arrays from NetworkStats(Observer|Recorder) This change removes the now-unused VPN arrays in the network stats observer and recorder classes. These are always null values in every call site. Bug: 113122541 Bug: 120145746 Bug: 129264869 Bug: 134244752 Test: FrameworksNetTest passing Test: Manual tests show data usage fixes maintained. Merged-In: Ieb8645acc400fdaeb0df7092c5369b96f9f35af9 Change-Id: I66f263d7e12bce7668901306c0c2ecdda634abaf (cherry picked from commit 833603caabb1a850a63a970fc285b4c8ed7401f8) --- .../server/net/NetworkStatsObservers.java | 13 +++++------ .../server/net/NetworkStatsRecorder.java | 22 ++++--------------- .../server/net/NetworkStatsService.java | 14 +++++------- 3 files changed, 14 insertions(+), 35 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsObservers.java b/services/core/java/com/android/server/net/NetworkStatsObservers.java index d8408730dd..2564daeaa1 100644 --- a/services/core/java/com/android/server/net/NetworkStatsObservers.java +++ b/services/core/java/com/android/server/net/NetworkStatsObservers.java @@ -39,7 +39,6 @@ import android.util.Slog; import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; -import com.android.internal.net.VpnInfo; import java.util.concurrent.atomic.AtomicInteger; @@ -104,9 +103,9 @@ class NetworkStatsObservers { public void updateStats(NetworkStats xtSnapshot, NetworkStats uidSnapshot, ArrayMap activeIfaces, ArrayMap activeUidIfaces, - VpnInfo[] vpnArray, long currentTime) { + long currentTime) { StatsContext statsContext = new StatsContext(xtSnapshot, uidSnapshot, activeIfaces, - activeUidIfaces, vpnArray, currentTime); + activeUidIfaces, currentTime); getHandler().sendMessage(mHandler.obtainMessage(MSG_UPDATE_STATS, statsContext)); } @@ -354,7 +353,7 @@ class NetworkStatsObservers { // thread will update it. We pass a null VPN array because usage is aggregated by uid // for this snapshot, so VPN traffic can't be reattributed to responsible apps. mRecorder.recordSnapshotLocked(statsContext.mXtSnapshot, statsContext.mActiveIfaces, - null /* vpnArray */, statsContext.mCurrentTime); + statsContext.mCurrentTime); } /** @@ -396,7 +395,7 @@ class NetworkStatsObservers { // thread will update it. We pass the VPN info so VPN traffic is reattributed to // responsible apps. mRecorder.recordSnapshotLocked(statsContext.mUidSnapshot, statsContext.mActiveUidIfaces, - statsContext.mVpnArray, statsContext.mCurrentTime); + statsContext.mCurrentTime); } /** @@ -427,18 +426,16 @@ class NetworkStatsObservers { NetworkStats mUidSnapshot; ArrayMap mActiveIfaces; ArrayMap mActiveUidIfaces; - VpnInfo[] mVpnArray; long mCurrentTime; StatsContext(NetworkStats xtSnapshot, NetworkStats uidSnapshot, ArrayMap activeIfaces, ArrayMap activeUidIfaces, - VpnInfo[] vpnArray, long currentTime) { + long currentTime) { mXtSnapshot = xtSnapshot; mUidSnapshot = uidSnapshot; mActiveIfaces = activeIfaces; mActiveUidIfaces = activeUidIfaces; - mVpnArray = vpnArray; mCurrentTime = currentTime; } } diff --git a/services/core/java/com/android/server/net/NetworkStatsRecorder.java b/services/core/java/com/android/server/net/NetworkStatsRecorder.java index bdff50053f..06ec341d9e 100644 --- a/services/core/java/com/android/server/net/NetworkStatsRecorder.java +++ b/services/core/java/com/android/server/net/NetworkStatsRecorder.java @@ -23,7 +23,6 @@ import static android.text.format.DateUtils.YEAR_IN_MILLIS; import static com.android.internal.util.Preconditions.checkNotNull; -import android.annotation.Nullable; import android.net.NetworkStats; import android.net.NetworkStats.NonMonotonicObserver; import android.net.NetworkStatsHistory; @@ -37,7 +36,6 @@ import android.util.MathUtils; import android.util.Slog; import android.util.proto.ProtoOutputStream; -import com.android.internal.net.VpnInfo; import com.android.internal.util.FileRotator; import com.android.internal.util.IndentingPrintWriter; @@ -202,18 +200,12 @@ public class NetworkStatsRecorder { } /** - * Record any delta that occurred since last {@link NetworkStats} snapshot, - * using the given {@link Map} to identify network interfaces. First - * snapshot is considered bootstrap, and is not counted as delta. - * - * @param vpnArray Optional info about the currently active VPN, if any. This is used to - * redistribute traffic from the VPN app to the underlying responsible apps. - * This should always be set to null if the provided snapshot is aggregated - * across all UIDs (e.g. contains UID_ALL buckets), regardless of VPN state. + * Record any delta that occurred since last {@link NetworkStats} snapshot, using the given + * {@link Map} to identify network interfaces. First snapshot is considered bootstrap, and is + * not counted as delta. */ public void recordSnapshotLocked(NetworkStats snapshot, - Map ifaceIdent, @Nullable VpnInfo[] vpnArray, - long currentTimeMillis) { + Map ifaceIdent, long currentTimeMillis) { final HashSet unknownIfaces = Sets.newHashSet(); // skip recording when snapshot missing @@ -232,12 +224,6 @@ public class NetworkStatsRecorder { final long end = currentTimeMillis; final long start = end - delta.getElapsedRealtime(); - if (vpnArray != null) { - for (VpnInfo info : vpnArray) { - delta.migrateTun(info.ownerUid, info.vpnIface, info.underlyingIfaces); - } - } - NetworkStats.Entry entry = null; for (int i = 0; i < delta.size(); i++) { entry = delta.getValues(i, entry); diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 34ce8199a6..42802f6c3c 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -1265,28 +1265,24 @@ public class NetworkStatsService extends INetworkStatsService.Stub { // For xt/dev, we pass a null VPN array because usage is aggregated by UID, so VPN traffic // can't be reattributed to responsible apps. Trace.traceBegin(TRACE_TAG_NETWORK, "recordDev"); - mDevRecorder.recordSnapshotLocked( - devSnapshot, mActiveIfaces, null /* vpnArray */, currentTime); + mDevRecorder.recordSnapshotLocked(devSnapshot, mActiveIfaces, currentTime); Trace.traceEnd(TRACE_TAG_NETWORK); Trace.traceBegin(TRACE_TAG_NETWORK, "recordXt"); - mXtRecorder.recordSnapshotLocked( - xtSnapshot, mActiveIfaces, null /* vpnArray */, currentTime); + mXtRecorder.recordSnapshotLocked(xtSnapshot, mActiveIfaces, currentTime); Trace.traceEnd(TRACE_TAG_NETWORK); // For per-UID stats, pass the VPN info so VPN traffic is reattributed to responsible apps. Trace.traceBegin(TRACE_TAG_NETWORK, "recordUid"); - mUidRecorder.recordSnapshotLocked( - uidSnapshot, mActiveUidIfaces, null /* vpnArray */, currentTime); + mUidRecorder.recordSnapshotLocked(uidSnapshot, mActiveUidIfaces, currentTime); Trace.traceEnd(TRACE_TAG_NETWORK); Trace.traceBegin(TRACE_TAG_NETWORK, "recordUidTag"); - mUidTagRecorder.recordSnapshotLocked( - uidSnapshot, mActiveUidIfaces, null /* vpnArray */, currentTime); + mUidTagRecorder.recordSnapshotLocked(uidSnapshot, mActiveUidIfaces, 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), null /* vpnArray */, currentTime); + new ArrayMap<>(mActiveUidIfaces), currentTime); } /** From 0768aa7770af74db47b064a16a273253cd179897 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 20 Jun 2019 14:46:35 -0700 Subject: [PATCH 5/6] Inline readNetworkStatsDetailInternal, make mUseBpfStats final This change inlines the logic from readNetworkStatsDetailInternal, and reduces reundant checks in mUseBpfStats Bug: 113122541 Test: atest FrameworksNetTests run, passing Merged-In: If2ef8d8f038f32c8cf974aa02cfc1dc7e44dbad3 Change-Id: If7d41052115ed145da8a610d676f6ed33c8d5e63 (cherry picked from commit 8c9d8c5e05cd35a340c4224c61f7fa9e95b5c861) --- .../server/net/NetworkStatsFactory.java | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index 2d3c66d68f..7687718b06 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -68,7 +68,7 @@ public class NetworkStatsFactory { /** Path to {@code /proc/net/xt_qtaguid/stats}. */ private final File mStatsXtUid; - private boolean mUseBpfStats; + private final boolean mUseBpfStats; private INetd mNetdService; @@ -302,6 +302,17 @@ public class NetworkStatsFactory { return readNetworkStatsDetail(UID_ALL, INTERFACES_ALL, TAG_ALL); } + @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 + // without race problem. + if (mNetdService == null) { + mNetdService = NetdService.getInstance(); + } + mNetdService.trafficSwapActiveStatsMap(); + } + /** * Reads the detailed UID stats based on the provided parameters * @@ -312,24 +323,6 @@ public class NetworkStatsFactory { * @return the NetworkStats instance containing network statistics at the present time. */ public NetworkStats readNetworkStatsDetail( - int limitUid, @Nullable String[] limitIfaces, int limitTag) throws IOException { - return readNetworkStatsDetailInternal(limitUid, limitIfaces, limitTag); - } - - @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 - // without race problem. - if (mUseBpfStats) { - if (mNetdService == null) { - mNetdService = NetdService.getInstance(); - } - mNetdService.trafficSwapActiveStatsMap(); - } - } - - private NetworkStats readNetworkStatsDetailInternal( 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. From 0308b3971d98e3cebfe8722c86572b628d02f694 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 15 May 2019 23:38:43 +0900 Subject: [PATCH 6/6] NetworkStats: Fix race condition causing system server crashes NetworkStatsService uses an internal boolean to know when it has started for the purpose of preventing access to other internal variables before they are initialized. However that boolean is set to true in systemReady() non-atomically with respect to the initialization of the other variables it guards, which can cause the system server to crash. This patch fixes this concurrency bug by moving setting the internal boolean flag and the variable it guards in one atomic synchronized block. This patch also removes code checking if bandwidth control is enabled, because this is now always true. Bug: 132767673 Test: Compiled. Change-Id: Ia089b5767ce271d669879c975508654d4dd03429 --- .../server/net/NetworkStatsService.java | 62 +++---------------- 1 file changed, 10 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 2fc78d6f47..5505828063 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -373,14 +373,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } public void systemReady() { - mSystemReady = true; - - if (!isBandwidthControlEnabled()) { - Slog.w(TAG, "bandwidth controls disabled, unable to track stats"); - return; - } - synchronized (mStatsLock) { + mSystemReady = true; + // create data recorders along with historical rotators mDevRecorder = buildRecorder(PREFIX_DEV, mSettings.getDevConfig(), false); mXtRecorder = buildRecorder(PREFIX_XT, mSettings.getXtConfig(), false); @@ -426,7 +421,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub { // ignored; service lives in system_server } - registerPollAlarmLocked(); + // schedule periodic pall alarm based on {@link NetworkStatsSettings#getPollInterval()}. + final PendingIntent pollIntent = + PendingIntent.getBroadcast(mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0); + + final long currentRealtime = SystemClock.elapsedRealtime(); + mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime, + mSettings.getPollInterval(), pollIntent); + registerGlobalAlert(); } @@ -486,23 +488,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - /** - * Clear any existing {@link #ACTION_NETWORK_STATS_POLL} alarms, and - * reschedule based on current {@link NetworkStatsSettings#getPollInterval()}. - */ - private void registerPollAlarmLocked() { - if (mPollIntent != null) { - mAlarmManager.cancel(mPollIntent); - } - - mPollIntent = PendingIntent.getBroadcast( - mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0); - - final long currentRealtime = SystemClock.elapsedRealtime(); - mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime, - mSettings.getPollInterval(), mPollIntent); - } - /** * Register for a global alert that is delivered through * {@link INetworkManagementEventObserver} once a threshold amount of data @@ -548,8 +533,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } private INetworkStatsSession openSessionInternal(final int flags, final String callingPackage) { - assertBandwidthControlEnabled(); - final int callingUid = Binder.getCallingUid(); final int usedFlags = isRateLimitedForPoll(callingUid) ? flags & (~NetworkStatsManager.FLAG_POLL_ON_OPEN) @@ -742,7 +725,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private long getNetworkTotalBytes(NetworkTemplate template, long start, long end) { assertSystemReady(); - assertBandwidthControlEnabled(); // NOTE: if callers want to get non-augmented data, they should go // through the public API @@ -753,7 +735,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private NetworkStats getNetworkUidBytes(NetworkTemplate template, long start, long end) { assertSystemReady(); - assertBandwidthControlEnabled(); final NetworkStatsCollection uidComplete; synchronized (mStatsLock) { @@ -768,7 +749,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { if (Binder.getCallingUid() != uid) { mContext.enforceCallingOrSelfPermission(ACCESS_NETWORK_STATE, TAG); } - assertBandwidthControlEnabled(); // TODO: switch to data layer stats once kernel exports // for now, read network layer stats and flatten across all ifaces @@ -855,7 +835,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { NetworkState[] networkStates, String activeIface) { checkNetworkStackPermission(mContext); - assertBandwidthControlEnabled(); final long token = Binder.clearCallingIdentity(); try { @@ -868,7 +847,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @Override public void forceUpdate() { mContext.enforceCallingOrSelfPermission(READ_NETWORK_USAGE_HISTORY, TAG); - assertBandwidthControlEnabled(); final long token = Binder.clearCallingIdentity(); try { @@ -879,8 +857,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } private void advisePersistThreshold(long thresholdBytes) { - assertBandwidthControlEnabled(); - // clamp threshold into safe range mPersistThreshold = MathUtils.constrain(thresholdBytes, 128 * KB_IN_BYTES, 2 * MB_IN_BYTES); if (LOGV) { @@ -1739,24 +1715,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - private void assertBandwidthControlEnabled() { - if (!isBandwidthControlEnabled()) { - throw new IllegalStateException("Bandwidth module disabled"); - } - } - - private boolean isBandwidthControlEnabled() { - final long token = Binder.clearCallingIdentity(); - try { - return mNetworkManager.isBandwidthControlEnabled(); - } catch (RemoteException e) { - // ignored; service lives in system_server - return false; - } finally { - Binder.restoreCallingIdentity(token); - } - } - private class DropBoxNonMonotonicObserver implements NonMonotonicObserver { @Override public void foundNonMonotonic(NetworkStats left, int leftIndex, NetworkStats right,