From 003c5c980765c9cc0078ff1d92f03a8b454abdd6 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 3 Oct 2019 11:09:00 -0700 Subject: [PATCH 1/2] Use TransformRecord to get SPI instead of SpiRecord IpSecService.applyTunnelModeTransform() currently does not take an SpiRecord instance, yet implicitly requires that the SpiRecord instance is still alive based on the stored SpiRecord resourceId in the TransformRecord's IpSecConfig. This check is unnecessary, as the SpiRecord has been subsumed into the TransformRecord, and the kernel resources are kept alive whether or not the SpiRecord is still held by the user. This allows users of the IpSecManager API to allocate short-lived SPIs during the creation of an IpSecTransform, without having to keep track of both of them (even though the SPI is no longer usable). The TransformRecord.getSpiRecord() call is already used in multiple other places in the same method. Bug: 142072071 Test: New tests added, passing. Change-Id: I1959f3080946267243564459ff4207647922566e Merged-In: I1959f3080946267243564459ff4207647922566e (cherry picked from commit 5258b1b82f39bf17e0751bcb94479464250aaec5) --- services/core/java/com/android/server/IpSecService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index 905c489e1d..6402e07bdd 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -1776,7 +1776,7 @@ public class IpSecService extends IIpSecService.Stub { socketRecord = userRecord.mEncapSocketRecords.getResourceOrThrow(c.getEncapSocketResourceId()); } - SpiRecord spiRecord = userRecord.mSpiRecords.getResourceOrThrow(c.getSpiResourceId()); + SpiRecord spiRecord = transformInfo.getSpiRecord(); int mark = (direction == IpSecManager.DIRECTION_OUT) @@ -1809,7 +1809,7 @@ public class IpSecService extends IIpSecService.Stub { // Set outbound SPI only. We want inbound to use any valid SA (old, new) on rekeys, // but want to guarantee outbound packets are sent over the new SA. - spi = transformInfo.getSpiRecord().getSpi(); + spi = spiRecord.getSpi(); } // Always update the policy with the relevant XFRM_IF_ID From 1d4d01a8a866de2561fd98f1738aa36e09df7d52 Mon Sep 17 00:00:00 2001 From: Treehugger Robot Date: Tue, 12 May 2020 11:06:23 +0000 Subject: [PATCH 2/2] Use CopyOnWriteArrayList to store list of NetworkStatsProviderCb In NetworkStatsService, mStatsLock will be held when iterating through the provider list. This is to protect the list from concurrent broadcast that triggered by NetworkPolicyManagerService. This is not good since the binder call is oneway, it does not make sense to block every access to the providers. This change also remove unuse variable and reduce verbose log. Test: atest FrameworksNetTests TetheringTests Bug: 150418178 Change-Id: If74e9f2ea597a0d5ae4668c3358bc687f342bbb5 Merged-In: If74e9f2ea597a0d5ae4668c3358bc687f342bbb5 (cherry picked from commit 5d8f96c6967d5b1a42ba374521078e75a9e6f187) --- .../server/net/NetworkStatsService.java | 49 +++++++------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index adf017633c..dcbdfdf7a2 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -119,7 +119,6 @@ import android.os.Looper; import android.os.Message; import android.os.Messenger; import android.os.PowerManager; -import android.os.RemoteCallbackList; import android.os.RemoteException; import android.os.SystemClock; import android.os.Trace; @@ -160,6 +159,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.Executor; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; @@ -226,12 +226,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private static final String PREFIX_UID = "uid"; private static final String PREFIX_UID_TAG = "uid_tag"; - /** - * Virtual network interface for video telephony. This is for VT data usage counting purpose. - */ - // TODO: Remove this after no one is using it. - public static final String VT_INTERFACE = NetworkStats.IFACE_VT; - /** * Settings that can be changed externally. */ @@ -304,8 +298,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub { new DropBoxNonMonotonicObserver(); private static final int MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS = 100; - private final RemoteCallbackList mStatsProviderCbList = - new RemoteCallbackList<>(); + private final CopyOnWriteArrayList mStatsProviderCbList = + new CopyOnWriteArrayList<>(); /** Semaphore used to wait for stats provider to respond to request stats update. */ private final Semaphore mStatsProviderSem = new Semaphore(0, true); @@ -1466,10 +1460,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final boolean persistForce = (flags & FLAG_PERSIST_FORCE) != 0; // Request asynchronous stats update from all providers for next poll. And wait a bit of - // time to allow providers report-in given that normally binder call should be fast. + // time to allow providers report-in given that normally binder call should be fast. Note + // that size of list might be changed because addition/removing at the same time. For + // addition, the stats of the missed provider can only be collected in next poll; + // for removal, wait might take up to MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS + // once that happened. // TODO: request with a valid token. Trace.traceBegin(TRACE_TAG_NETWORK, "provider.requestStatsUpdate"); - final int registeredCallbackCount = mStatsProviderCbList.getRegisteredCallbackCount(); + final int registeredCallbackCount = mStatsProviderCbList.size(); mStatsProviderSem.drainPermits(); invokeForAllStatsProviderCallbacks( (cb) -> cb.mProvider.onRequestStatsUpdate(0 /* unused */)); @@ -1643,7 +1641,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @Override public void setStatsProviderLimitAsync(@NonNull String iface, long quota) { - Slog.v(TAG, "setStatsProviderLimitAsync(" + iface + "," + quota + ")"); + if (LOGV) Slog.v(TAG, "setStatsProviderLimitAsync(" + iface + "," + quota + ")"); invokeForAllStatsProviderCallbacks((cb) -> cb.mProvider.onSetLimit(iface, quota)); } } @@ -1934,7 +1932,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { NetworkStatsProviderCallbackImpl callback = new NetworkStatsProviderCallbackImpl( tag, provider, mStatsProviderSem, mAlertObserver, mStatsProviderCbList); - mStatsProviderCbList.register(callback); + mStatsProviderCbList.add(callback); Log.d(TAG, "registerNetworkStatsProvider from " + callback.mTag + " uid/pid=" + getCallingUid() + "/" + getCallingPid()); return callback; @@ -1958,20 +1956,11 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private void invokeForAllStatsProviderCallbacks( @NonNull ThrowingConsumer task) { - synchronized (mStatsLock) { - final int length = mStatsProviderCbList.beginBroadcast(); + for (final NetworkStatsProviderCallbackImpl cb : mStatsProviderCbList) { try { - for (int i = 0; i < length; i++) { - final NetworkStatsProviderCallbackImpl cb = - mStatsProviderCbList.getBroadcastItem(i); - try { - task.accept(cb); - } catch (RemoteException e) { - Log.e(TAG, "Fail to broadcast to provider: " + cb.mTag, e); - } - } - } finally { - mStatsProviderCbList.finishBroadcast(); + task.accept(cb); + } catch (RemoteException e) { + Log.e(TAG, "Fail to broadcast to provider: " + cb.mTag, e); } } } @@ -1983,7 +1972,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @NonNull final INetworkStatsProvider mProvider; @NonNull private final Semaphore mSemaphore; @NonNull final INetworkManagementEventObserver mAlertObserver; - @NonNull final RemoteCallbackList mStatsProviderCbList; + @NonNull final CopyOnWriteArrayList mStatsProviderCbList; @NonNull private final Object mProviderStatsLock = new Object(); @@ -1997,7 +1986,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @NonNull String tag, @NonNull INetworkStatsProvider provider, @NonNull Semaphore semaphore, @NonNull INetworkManagementEventObserver alertObserver, - @NonNull RemoteCallbackList cbList) + @NonNull CopyOnWriteArrayList cbList) throws RemoteException { mTag = tag; mProvider = provider; @@ -2054,13 +2043,13 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @Override public void binderDied() { Log.d(TAG, mTag + ": binderDied"); - mStatsProviderCbList.unregister(this); + mStatsProviderCbList.remove(this); } @Override public void unregister() { Log.d(TAG, mTag + ": unregister"); - mStatsProviderCbList.unregister(this); + mStatsProviderCbList.remove(this); } }