From 92860f9ea0437132c761113769cdf60c63b99a33 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Wed, 23 Jun 2021 06:29:30 +0000 Subject: [PATCH 1/5] Fix the comments from aosp/1717479. 1. Increase the cleanup delay from 3s to 10s. 2. Fix the comments from aosp/1717479. Bug: 181810560 Test: atest NsdManagerTest NsdServiceTest Original-Change: https://android-review.googlesource.com/1730170 Merged-In: I71ebdd011574bd96de16a4248b0e15636418e87c Change-Id: I71ebdd011574bd96de16a4248b0e15636418e87c --- .../java/com/android/server/NsdService.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/NsdService.java b/services/core/java/com/android/server/NsdService.java index a481a6a5d3..f440ee8c4e 100644 --- a/services/core/java/com/android/server/NsdService.java +++ b/services/core/java/com/android/server/NsdService.java @@ -61,7 +61,7 @@ public class NsdService extends INsdManager.Stub { private static final String MDNS_TAG = "mDnsConnector"; private static final boolean DBG = true; - private static final long CLEANUP_DELAY_MS = 3000; + private static final long CLEANUP_DELAY_MS = 10000; private final Context mContext; private final NsdSettings mNsdSettings; @@ -94,19 +94,25 @@ public class NsdService extends INsdManager.Stub { return NsdManager.nameOf(what); } - void maybeStartDaemon() { + private void maybeStartDaemon() { mDaemon.maybeStart(); maybeScheduleStop(); } - void maybeScheduleStop() { + private boolean isAnyRequestActive() { + return mIdToClientInfoMap.size() != 0; + } + + private void scheduleStop() { + sendMessageDelayed(NsdManager.DAEMON_CLEANUP, mCleanupDelayMs); + } + private void maybeScheduleStop() { if (!isAnyRequestActive()) { - cancelStop(); - sendMessageDelayed(NsdManager.DAEMON_CLEANUP, mCleanupDelayMs); + scheduleStop(); } } - void cancelStop() { + private void cancelStop() { this.removeMessages(NsdManager.DAEMON_CLEANUP); } @@ -164,11 +170,16 @@ public class NsdService extends INsdManager.Stub { if (DBG) Slog.d(TAG, "Client connection lost with reason: " + msg.arg1); break; } + cInfo = mClients.get(msg.replyTo); if (cInfo != null) { cInfo.expungeAllRequests(); mClients.remove(msg.replyTo); } + //Last client + if (mClients.size() == 0) { + scheduleStop(); + } break; case AsyncChannel.CMD_CHANNEL_FULL_CONNECTION: AsyncChannel ac = new AsyncChannel(); @@ -235,7 +246,7 @@ public class NsdService extends INsdManager.Stub { public void exit() { // TODO: it is incorrect to stop the daemon without expunging all requests // and sending error callbacks to clients. - maybeScheduleStop(); + scheduleStop(); } private boolean requestLimitReached(ClientInfo clientInfo) { @@ -271,9 +282,6 @@ public class NsdService extends INsdManager.Stub { return NOT_HANDLED; case AsyncChannel.CMD_CHANNEL_DISCONNECTED: return NOT_HANDLED; - } - - switch (msg.what) { case NsdManager.DISABLE: //TODO: cleanup clients transitionTo(mDisabledState); @@ -531,10 +539,6 @@ public class NsdService extends INsdManager.Stub { } } - private boolean isAnyRequestActive() { - return mIdToClientInfoMap.size() != 0; - } - private String unescape(String s) { StringBuilder sb = new StringBuilder(s.length()); for (int i = 0; i < s.length(); ++i) { @@ -907,7 +911,6 @@ public class NsdService extends INsdManager.Stub { } mClientIds.clear(); mClientRequests.clear(); - mNsdStateMachine.maybeScheduleStop(); } // mClientIds is a sparse array of listener id -> mDnsClient id. For a given mDnsClient id, From 8002fe63f4b7ce44e080ac34f93f7c3c862eabe0 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 2 Jul 2021 19:12:24 +0000 Subject: [PATCH 2/5] Add support for app data accounting for in-kernel dataplanes This change ensures that app data accounting works correctly within the confines of in-kernel dataplanes, as used by platform VPNs and the VCN. Notably, the VCN MUST NOT specify the IMSI, as that would lead to double counting of the interface statistics. Bug: 175853498 Bug: 190620024 Test: atest NetworkStatsTest FrameworksVcnTests Original-Change: https://android-review.googlesource.com/1749070 Merged-In: I768907cd3dd2028c7040cddd81fc71a5ce69bbdb Change-Id: I768907cd3dd2028c7040cddd81fc71a5ce69bbdb --- core/java/android/net/NetworkStats.java | 42 ++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/core/java/android/net/NetworkStats.java b/core/java/android/net/NetworkStats.java index 6ccbab7ed8..288b06ee52 100644 --- a/core/java/android/net/NetworkStats.java +++ b/core/java/android/net/NetworkStats.java @@ -24,6 +24,7 @@ import android.compat.annotation.UnsupportedAppUsage; import android.os.Build; import android.os.Parcel; import android.os.Parcelable; +import android.os.Process; import android.os.SystemClock; import android.util.SparseBooleanArray; @@ -1487,8 +1488,31 @@ public final class NetworkStats implements Parcelable { continue; } - if (recycle.uid == tunUid) { - // Add up traffic through tunUid's underlying interfaces. + if (tunUid == Process.SYSTEM_UID) { + // Kernel-based VPN or VCN, traffic sent by apps on the VPN/VCN network + // + // Since the data is not UID-accounted on underlying networks, just use VPN/VCN + // network usage as ground truth. Encrypted traffic on the underlying networks will + // never be processed here because encrypted traffic on the underlying interfaces + // is not present in UID stats, and this method is only called on UID stats. + if (tunIface.equals(recycle.iface)) { + tunIfaceTotal.add(recycle); + underlyingIfacesTotal.add(recycle); + + // In steady state, there should always be one network, but edge cases may + // result in the network being null (network lost), and thus no underlying + // ifaces is possible. + if (perInterfaceTotal.length > 0) { + // While platform VPNs and VCNs have exactly one underlying network, that + // network may have multiple interfaces (eg for 464xlat). This layer does + // not have the required information to identify which of the interfaces + // were used. Select "any" of the interfaces. Since overhead is already + // lost, this number is an approximation anyways. + perInterfaceTotal[0].add(recycle); + } + } + } else if (recycle.uid == tunUid) { + // VpnService VPN, traffic sent by the VPN app over underlying networks for (int j = 0; j < underlyingIfaces.size(); j++) { if (Objects.equals(underlyingIfaces.get(j), recycle.iface)) { perInterfaceTotal[j].add(recycle); @@ -1497,7 +1521,7 @@ public final class NetworkStats implements Parcelable { } } } else if (tunIface.equals(recycle.iface)) { - // Add up all tunIface traffic excluding traffic from the vpn app itself. + // VpnService VPN; traffic sent by apps on the VPN network tunIfaceTotal.add(recycle); } } @@ -1532,9 +1556,13 @@ public final class NetworkStats implements Parcelable { // Consider only entries that go onto the VPN interface. continue; } - if (uid[i] == tunUid) { + + if (uid[i] == tunUid && tunUid != Process.SYSTEM_UID) { // Exclude VPN app from the redistribution, as it can choose to create packet // streams by writing to itself. + // + // However, for platform VPNs, do not exclude the system's usage of the VPN network, + // since it is never local-only, and never double counted continue; } tmpEntry.uid = uid[i]; @@ -1641,6 +1669,12 @@ public final class NetworkStats implements Parcelable { int tunUid, @NonNull List underlyingIfaces, @NonNull Entry[] moved) { + if (tunUid == Process.SYSTEM_UID) { + // No traffic recorded on a per-UID basis for in-kernel VPN/VCNs over underlying + // networks; thus no traffic to deduct. + return; + } + for (int i = 0; i < underlyingIfaces.size(); i++) { moved[i].uid = tunUid; // Add debug info From f7277ed3c45e3fb587f18c8de5edc82d83968544 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Mon, 12 Jul 2021 21:15:10 +0800 Subject: [PATCH 3/5] Keep the native mdns daemon alive for pre-S application Roll back the behavior changes by checking the target SDK to ensure that there are no compatibility issues with the pre-S application. If the target SDK of the application Date: Fri, 30 Jul 2021 02:31:26 +0000 Subject: [PATCH 4/5] Fix mMobileIfaces is not protected by lock Currently, mMobileIfaces is accessed from multiple threads, and should be protected from concurrent accessing. However, since the variable could be accessed frequently, holding the mStatsLock would make this be blocked by network stats I/O operations. Thus, protect the variable by making it volatile. Test: Wifi on/off stress test Bug: 192758557 Original-Change: https://android-review.googlesource.com/1765686 Merged-In: Ie7694a63f5203ee7c83830ca13d97219b7949fd7 Change-Id: Ie7694a63f5203ee7c83830ca13d97219b7949fd7 --- .../server/net/NetworkStatsService.java | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 4ee867b7d0..097b0711ef 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -289,8 +289,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private String mActiveIface; /** Set of any ifaces associated with mobile networks since boot. */ - @GuardedBy("mStatsLock") - private String[] mMobileIfaces = new String[0]; + private volatile String[] mMobileIfaces = new String[0]; /** Set of all ifaces currently used by traffic that does not explicitly specify a Network. */ @GuardedBy("mStatsLock") @@ -935,7 +934,12 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @Override public String[] getMobileIfaces() { - return mMobileIfaces; + // TODO (b/192758557): Remove debug log. + if (ArrayUtils.contains(mMobileIfaces, null)) { + throw new NullPointerException( + "null element in mMobileIfaces: " + Arrays.toString(mMobileIfaces)); + } + return mMobileIfaces.clone(); } @Override @@ -1084,7 +1088,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } @Override - public long getIfaceStats(String iface, int type) { + public long getIfaceStats(@NonNull String iface, int type) { + Objects.requireNonNull(iface); long nativeIfaceStats = nativeGetIfaceStat(iface, type, checkBpfStatsEnable()); if (nativeIfaceStats == -1) { return nativeIfaceStats; @@ -1382,7 +1387,12 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - mMobileIfaces = mobileIfaces.toArray(new String[mobileIfaces.size()]); + mMobileIfaces = mobileIfaces.toArray(new String[0]); + // TODO (b/192758557): Remove debug log. + if (ArrayUtils.contains(mMobileIfaces, null)) { + throw new NullPointerException( + "null element in mMobileIfaces: " + Arrays.toString(mMobileIfaces)); + } } private static int getSubIdForMobile(@NonNull NetworkStateSnapshot state) { From 9ec09dce9499b65ea3eccae2508ba939869cfe89 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Fri, 30 Jul 2021 02:31:26 +0000 Subject: [PATCH 5/5] Add debug log for tracking NPE of mMobileIfaces Test: TH Bug: 192758557 Original-Change: https://android-review.googlesource.com/1777887 Merged-In: Ib048c18b1c64627de5a9d2b04d10e084a014ff64 Change-Id: Ib048c18b1c64627de5a9d2b04d10e084a014ff64