From 0c419ca9ebee3a5664ab9f61f8b123122965fc40 Mon Sep 17 00:00:00 2001 From: Chenbo Feng Date: Wed, 10 Apr 2019 12:55:07 -0700 Subject: [PATCH 1/2] Add a lock to protect persistent stats snapshot Since the network stats could be polled from multiple services at runtime, it is not thread safe for networkStatsFactory to hold a persistent stats snapshot without any protection. Use a internal lock to prevent concurrent modification on mPersistentSnapshot to fix the problem. Bug: 124764595 Test: android.app.usage.cts.NetworkUsageStatsTest android.net.cts.TrafficStatsTest Change-Id: I22afb46f17697e8b6359d4f593802e0f4b95db8b --- .../server/net/NetworkStatsFactory.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index bf34d8fa6b..bd11d46a97 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -28,6 +28,7 @@ import android.net.NetworkStats; import android.os.StrictMode; import android.os.SystemClock; +import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.ArrayUtils; import com.android.internal.util.ProcFileReader; @@ -65,6 +66,7 @@ public class NetworkStatsFactory { private boolean mUseBpfStats; // A persistent Snapshot since device start for eBPF stats + @GuardedBy("mPersistSnapshot") private final NetworkStats mPersistSnapshot; // TODO: only do adjustments in NetworkStatsService and remove this. @@ -289,15 +291,17 @@ public class NetworkStatsFactory { stats = new NetworkStats(SystemClock.elapsedRealtime(), -1); } if (mUseBpfStats) { - if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), UID_ALL, - null, TAG_ALL, mUseBpfStats) != 0) { - throw new IOException("Failed to parse network stats"); + synchronized (mPersistSnapshot) { + if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), UID_ALL, + null, TAG_ALL, mUseBpfStats) != 0) { + throw new IOException("Failed to parse network stats"); + } + mPersistSnapshot.setElapsedRealtime(stats.getElapsedRealtime()); + mPersistSnapshot.combineAllValues(stats); + NetworkStats result = mPersistSnapshot.clone(); + result.filter(limitUid, limitIfaces, limitTag); + return result; } - mPersistSnapshot.setElapsedRealtime(stats.getElapsedRealtime()); - mPersistSnapshot.combineAllValues(stats); - NetworkStats result = mPersistSnapshot.clone(); - result.filter(limitUid, limitIfaces, limitTag); - return result; } else { if (nativeReadNetworkStatsDetail(stats, mStatsXtUid.getAbsolutePath(), limitUid, limitIfaces, limitTag, mUseBpfStats) != 0) { From 876218abcbe881aa0f62b677cb376ea66103e68b Mon Sep 17 00:00:00 2001 From: Chenbo Feng Date: Wed, 27 Feb 2019 19:07:39 -0800 Subject: [PATCH 2/2] Ask netd to swap stats map before reading To avoid protentail race problem between netd and system_server when reading the network stats map. Always inform netd before reading the stats and let netd to do a swap between active stats map and inactive stats map. So the system_server can safely remove the stats after reading. Bug: 126620214 Test: android.app.usage.cts.NetworkUsageStatsTest android.net.cts.TrafficStatsTest Change-Id: I8fa37c26bec23ffca0b29b679e72ba1189f557f1 --- .../server/net/NetworkStatsFactory.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/services/core/java/com/android/server/net/NetworkStatsFactory.java b/services/core/java/com/android/server/net/NetworkStatsFactory.java index bd11d46a97..2e64965a61 100644 --- a/services/core/java/com/android/server/net/NetworkStatsFactory.java +++ b/services/core/java/com/android/server/net/NetworkStatsFactory.java @@ -24,7 +24,10 @@ import static android.net.NetworkStats.UID_ALL; import static com.android.server.NetworkManagementSocketTagger.kernelToTag; import android.annotation.Nullable; +import android.net.INetd; import android.net.NetworkStats; +import android.net.util.NetdService; +import android.os.RemoteException; import android.os.StrictMode; import android.os.SystemClock; @@ -65,6 +68,8 @@ public class NetworkStatsFactory { private boolean mUseBpfStats; + private INetd mNetdService; + // A persistent Snapshot since device start for eBPF stats @GuardedBy("mPersistSnapshot") private final NetworkStats mPersistSnapshot; @@ -279,6 +284,19 @@ public class NetworkStatsFactory { return stats; } + @GuardedBy("mPersistSnapshot") + 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(); + } + } + // TODO: delete the lastStats parameter private NetworkStats readNetworkStatsDetailInternal(int limitUid, String[] limitIfaces, int limitTag, NetworkStats lastStats) throws IOException { @@ -292,6 +310,13 @@ public class NetworkStatsFactory { } if (mUseBpfStats) { synchronized (mPersistSnapshot) { + try { + requestSwapActiveStatsMapLocked(); + } catch (RemoteException e) { + throw new IOException(e); + } + // 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) { throw new IOException("Failed to parse network stats");