From e6f6699ef988e6146a168f7be9b9c0f9a6632f37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Thu, 28 May 2020 01:06:28 -0700 Subject: [PATCH 1/2] NetworkStats: apply464xlatAdjustments - remove useBpfStats parameter. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test: atest NetworkStatsTest NetworkStatsFactoryTest Bug: 150738490 Signed-off-by: Maciej Żenczykowski Change-Id: I0121a4ac7ee824adc5930bab786d550b2f00b05b --- core/java/android/net/NetworkStats.java | 7 +++--- .../server/net/NetworkStatsFactory.java | 10 ++++---- .../server/net/NetworkStatsService.java | 5 ++-- .../java/android/net/NetworkStatsTest.java | 24 ++++++------------- 4 files changed, 16 insertions(+), 30 deletions(-) diff --git a/core/java/android/net/NetworkStats.java b/core/java/android/net/NetworkStats.java index 3141320a167d1..ea0889276c5c9 100644 --- a/core/java/android/net/NetworkStats.java +++ b/core/java/android/net/NetworkStats.java @@ -1069,11 +1069,10 @@ public final class NetworkStats implements Parcelable { * @param baseTraffic Traffic on the base interfaces. Will be mutated. * @param stackedTraffic Stats with traffic stacked on top of our ifaces. Will also be mutated. * @param stackedIfaces Mapping ipv6if -> ipv4if interface where traffic is counted on both. - * @param useBpfStats True if eBPF is in use. * @hide */ public static void apply464xlatAdjustments(NetworkStats baseTraffic, - NetworkStats stackedTraffic, Map stackedIfaces, boolean useBpfStats) { + NetworkStats stackedTraffic, Map stackedIfaces) { // For recycling Entry entry = null; for (int i = 0; i < stackedTraffic.size; i++) { @@ -1113,8 +1112,8 @@ public final class NetworkStats implements Parcelable { * @param stackedIfaces Mapping ipv6if -> ipv4if interface where traffic is counted on both. * @hide */ - public void apply464xlatAdjustments(Map stackedIfaces, boolean useBpfStats) { - apply464xlatAdjustments(this, this, stackedIfaces, useBpfStats); + public void apply464xlatAdjustments(Map stackedIfaces) { + apply464xlatAdjustments(this, this, stackedIfaces); } /** diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index 3dac106418c52..86ad0b3086763 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -152,12 +152,10 @@ public class NetworkStatsFactory { /** * Applies 464xlat adjustments with ifaces noted with {@link #noteStackedIface(String, String)}. - * @see NetworkStats#apply464xlatAdjustments(NetworkStats, NetworkStats, Map, boolean) + * @see NetworkStats#apply464xlatAdjustments(NetworkStats, NetworkStats, Map) */ - public void apply464xlatAdjustments(NetworkStats baseTraffic, - NetworkStats stackedTraffic, boolean useBpfStats) { - NetworkStats.apply464xlatAdjustments(baseTraffic, stackedTraffic, mStackedIfaces, - useBpfStats); + public void apply464xlatAdjustments(NetworkStats baseTraffic, NetworkStats stackedTraffic) { + NetworkStats.apply464xlatAdjustments(baseTraffic, stackedTraffic, mStackedIfaces); } public NetworkStatsFactory() { @@ -380,7 +378,7 @@ public class NetworkStatsFactory { // network, the overhead is their fault. // No locking here: apply464xlatAdjustments behaves fine with an add-only // ConcurrentHashMap. - delta.apply464xlatAdjustments(mStackedIfaces, mUseBpfStats); + delta.apply464xlatAdjustments(mStackedIfaces); // 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 823717c7d2b4b..ba538e1cd8263 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -1877,14 +1877,13 @@ 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); - mStatsFactory.apply464xlatAdjustments(uidSnapshot, tetherSnapshot, - mUseBpfTrafficStats); + mStatsFactory.apply464xlatAdjustments(uidSnapshot, tetherSnapshot); uidSnapshot.combineAllValues(tetherSnapshot); // get a stale copy of uid stats snapshot provided by providers. final NetworkStats providerStats = getNetworkStatsFromProviders(STATS_PER_UID); providerStats.filter(UID_ALL, ifaces, TAG_ALL); - mStatsFactory.apply464xlatAdjustments(uidSnapshot, providerStats, mUseBpfTrafficStats); + mStatsFactory.apply464xlatAdjustments(uidSnapshot, providerStats); uidSnapshot.combineAllValues(providerStats); uidSnapshot.combineAllValues(mUidOperations); diff --git a/tests/net/java/android/net/NetworkStatsTest.java b/tests/net/java/android/net/NetworkStatsTest.java index f28b1864c11c1..735fa7cf37514 100644 --- a/tests/net/java/android/net/NetworkStatsTest.java +++ b/tests/net/java/android/net/NetworkStatsTest.java @@ -926,21 +926,14 @@ public class NetworkStatsTest { 3 /* txPackets */, 0 /* operations */); - final NetworkStats statsXt = new NetworkStats(TEST_START, 3) + final NetworkStats stats = new NetworkStats(TEST_START, 3) .insertEntry(appEntry) .insertEntry(rootUidEntry) .insertEntry(otherEntry); - final NetworkStats statsEbpf = new NetworkStats(TEST_START, 3) - .insertEntry(appEntry) - .insertEntry(rootUidEntry) - .insertEntry(otherEntry); + stats.apply464xlatAdjustments(stackedIface); - statsXt.apply464xlatAdjustments(stackedIface, false); - statsEbpf.apply464xlatAdjustments(stackedIface, true); - - assertEquals(3, statsXt.size()); - assertEquals(3, statsEbpf.size()); + assertEquals(3, stats.size()); final NetworkStats.Entry expectedAppUid = new NetworkStats.Entry( v4Iface, appUid, SET_DEFAULT, TAG_NONE, 30949510, @@ -955,12 +948,9 @@ public class NetworkStatsTest { 17607, 97, 0); - assertEquals(expectedAppUid, statsXt.getValues(0, null)); - assertEquals(expectedRootUid, statsXt.getValues(1, null)); - assertEquals(otherEntry, statsXt.getValues(2, null)); - assertEquals(expectedAppUid, statsEbpf.getValues(0, null)); - assertEquals(expectedRootUid, statsEbpf.getValues(1, null)); - assertEquals(otherEntry, statsEbpf.getValues(2, null)); + assertEquals(expectedAppUid, stats.getValues(0, null)); + assertEquals(expectedRootUid, stats.getValues(1, null)); + assertEquals(otherEntry, stats.getValues(2, null)); } @Test @@ -985,7 +975,7 @@ public class NetworkStatsTest { .insertEntry(secondEntry); // Empty map: no adjustment - stats.apply464xlatAdjustments(new ArrayMap<>(), false); + stats.apply464xlatAdjustments(new ArrayMap<>()); assertEquals(2, stats.size()); assertEquals(firstEntry, stats.getValues(0, null)); From 8de903184a4f0312a744012e4059e51c79a67a46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Thu, 28 May 2020 01:33:52 -0700 Subject: [PATCH 2/2] NetworkStats: apply464xlatAdjustments - don't remove CLAT_UID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should no longer be needed after all the cleanups. Test: atest NetworkStatsTest NetworkStatsFactoryTest Bug: 150738490 Signed-off-by: Maciej Żenczykowski Change-Id: I289d935f84b616ed857ef4c5a7427d57c282d00c --- core/java/android/net/NetworkStats.java | 8 -------- tests/net/res/raw/xt_qtaguid_with_clat | 2 -- tests/net/res/raw/xt_qtaguid_with_clat_simple | 1 - 3 files changed, 11 deletions(-) diff --git a/core/java/android/net/NetworkStats.java b/core/java/android/net/NetworkStats.java index ea0889276c5c9..34e48eb44f327 100644 --- a/core/java/android/net/NetworkStats.java +++ b/core/java/android/net/NetworkStats.java @@ -16,8 +16,6 @@ package android.net; -import static android.os.Process.CLAT_UID; - import android.annotation.IntDef; import android.annotation.NonNull; import android.annotation.Nullable; @@ -1095,12 +1093,6 @@ public final class NetworkStats implements Parcelable { entry.txBytes += entry.txPackets * IPV4V6_HEADER_DELTA; stackedTraffic.setValues(i, entry); } - - // Theoretically there should be no traffic accounted to the clat daemon's uid: - // see ebpf program 'netd.c's early returns - // and iptables '-m owner --uid-owner clat -j RETURN' rules prior to accounting - // TODO: remove this - should definitely be safe once ebpf only. - baseTraffic.removeUids(new int[] {CLAT_UID}); } /** diff --git a/tests/net/res/raw/xt_qtaguid_with_clat b/tests/net/res/raw/xt_qtaguid_with_clat index 8967d4f1ac4a9..f04b32f08332d 100644 --- a/tests/net/res/raw/xt_qtaguid_with_clat +++ b/tests/net/res/raw/xt_qtaguid_with_clat @@ -41,5 +41,3 @@ idx iface acct_tag_hex uid_tag_int cnt_set rx_bytes rx_packets tx_bytes tx_packe 41 dummy0 0x0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 42 lo 0x0 0 0 1288 16 1288 16 0 0 532 8 756 8 0 0 532 8 756 8 43 lo 0x0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 -44 wlan0 0x0 1029 0 0 0 312046 5113 0 0 0 0 0 0 306544 5046 3230 38 2272 29 -45 wlan0 0x0 1029 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \ No newline at end of file diff --git a/tests/net/res/raw/xt_qtaguid_with_clat_simple b/tests/net/res/raw/xt_qtaguid_with_clat_simple index 2c9b9f768dd72..a1d6d411bad81 100644 --- a/tests/net/res/raw/xt_qtaguid_with_clat_simple +++ b/tests/net/res/raw/xt_qtaguid_with_clat_simple @@ -2,4 +2,3 @@ idx iface acct_tag_hex uid_tag_int cnt_set rx_bytes rx_packets tx_bytes tx_packe 2 v4-wlan0 0x0 10060 0 42600 213 4100 41 42600 213 0 0 0 0 4100 41 0 0 0 0 3 v4-wlan0 0x0 10060 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 4 wlan0 0x0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 -5 wlan0 0x0 1029 0 0 0 4920 41 0 0 0 0 0 0 4920 41 0 0 0 0