Don't over-acquire NPMS locks.
We only need to hold mNetworkPoliciesSecondLock when working with
subscription plans; before this CL we could end up acquiring the two
NPMS locks out of order, resulting in a deadlock.
Also annotate objects in NSS that require mStatsLock to be held.
Test: builds, boots
Bug: 65268076
Change-Id: I06497564424316ef895dc8dceba72ae784781dc3
diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
index 0778d09..07ab417 100644
--- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
+++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
@@ -369,11 +369,14 @@
private final boolean mSuppressDefaultPolicy;
/** Defined network policies. */
+ @GuardedBy("mNetworkPoliciesSecondLock")
final ArrayMap<NetworkTemplate, NetworkPolicy> mNetworkPolicy = new ArrayMap<>();
/** Map from subId to subscription plans. */
+ @GuardedBy("mNetworkPoliciesSecondLock")
final SparseArray<SubscriptionPlan[]> mSubscriptionPlans = new SparseArray<>();
/** Map from subId to package name that owns subscription plans. */
+ @GuardedBy("mNetworkPoliciesSecondLock")
final SparseArray<String> mSubscriptionPlansOwner = new SparseArray<>();
/** Defined UID policies. */
@@ -2767,20 +2770,18 @@
return plans.toArray(new SubscriptionPlan[plans.size()]);
}
- synchronized (mUidRulesFirstLock) {
- synchronized (mNetworkPoliciesSecondLock) {
- // Only give out plan details to the package that defined them,
- // so that we don't risk leaking plans between apps. We always
- // let in core system components (like the Settings app).
- final String ownerPackage = mSubscriptionPlansOwner.get(subId);
- if (Objects.equals(ownerPackage, callingPackage)
- || (UserHandle.getCallingAppId() == android.os.Process.SYSTEM_UID)) {
- return mSubscriptionPlans.get(subId);
- } else {
- Log.w(TAG, "Not returning plans because caller " + callingPackage
- + " doesn't match owner " + ownerPackage);
- return null;
- }
+ synchronized (mNetworkPoliciesSecondLock) {
+ // Only give out plan details to the package that defined them,
+ // so that we don't risk leaking plans between apps. We always
+ // let in core system components (like the Settings app).
+ final String ownerPackage = mSubscriptionPlansOwner.get(subId);
+ if (Objects.equals(ownerPackage, callingPackage)
+ || (UserHandle.getCallingAppId() == android.os.Process.SYSTEM_UID)) {
+ return mSubscriptionPlans.get(subId);
+ } else {
+ Log.w(TAG, "Not returning plans because caller " + callingPackage
+ + " doesn't match owner " + ownerPackage);
+ return null;
}
}
}
diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java
index 4bd927d..3af5265 100644
--- a/services/core/java/com/android/server/net/NetworkStatsService.java
+++ b/services/core/java/com/android/server/net/NetworkStatsService.java
@@ -124,6 +124,7 @@
import android.util.TrustedTime;
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;
@@ -241,12 +242,17 @@
private final DropBoxNonMonotonicObserver mNonMonotonicObserver =
new DropBoxNonMonotonicObserver();
+ @GuardedBy("mStatsLock")
private NetworkStatsRecorder mDevRecorder;
+ @GuardedBy("mStatsLock")
private NetworkStatsRecorder mXtRecorder;
+ @GuardedBy("mStatsLock")
private NetworkStatsRecorder mUidRecorder;
+ @GuardedBy("mStatsLock")
private NetworkStatsRecorder mUidTagRecorder;
/** Cached {@link #mXtRecorder} stats. */
+ @GuardedBy("mStatsLock")
private NetworkStatsCollection mXtStatsCached;
/** Current counter sets for each UID. */
@@ -328,15 +334,15 @@
return;
}
- // create data recorders along with historical rotators
- mDevRecorder = buildRecorder(PREFIX_DEV, mSettings.getDevConfig(), false);
- mXtRecorder = buildRecorder(PREFIX_XT, mSettings.getXtConfig(), false);
- mUidRecorder = buildRecorder(PREFIX_UID, mSettings.getUidConfig(), false);
- mUidTagRecorder = buildRecorder(PREFIX_UID_TAG, mSettings.getUidTagConfig(), true);
-
- updatePersistThresholds();
-
synchronized (mStatsLock) {
+ // create data recorders along with historical rotators
+ mDevRecorder = buildRecorder(PREFIX_DEV, mSettings.getDevConfig(), false);
+ mXtRecorder = buildRecorder(PREFIX_XT, mSettings.getXtConfig(), false);
+ mUidRecorder = buildRecorder(PREFIX_UID, mSettings.getUidConfig(), false);
+ mUidTagRecorder = buildRecorder(PREFIX_UID_TAG, mSettings.getUidTagConfig(), true);
+
+ updatePersistThresholdsLocked();
+
// upgrade any legacy stats, migrating them to rotated files
maybeUpgradeLegacyStatsLocked();
@@ -674,9 +680,12 @@
int flags, int fields, @NetworkStatsAccess.Level int accessLevel, int callingUid) {
// We've been using pure XT stats long enough that we no longer need to
// splice DEV and XT together.
- return mXtStatsCached.getHistory(template, resolveSubscriptionPlan(template, flags),
- UID_ALL, SET_ALL, TAG_NONE, fields, Long.MIN_VALUE, Long.MAX_VALUE,
- accessLevel, callingUid);
+ final SubscriptionPlan augmentPlan = resolveSubscriptionPlan(template, flags);
+ synchronized (mStatsLock) {
+ return mXtStatsCached.getHistory(template, augmentPlan,
+ UID_ALL, SET_ALL, TAG_NONE, fields, Long.MIN_VALUE, Long.MAX_VALUE,
+ accessLevel, callingUid);
+ }
}
@Override
@@ -813,7 +822,7 @@
synchronized (mStatsLock) {
if (!mSystemReady) return;
- updatePersistThresholds();
+ updatePersistThresholdsLocked();
mDevRecorder.maybePersistLocked(currentTime);
mXtRecorder.maybePersistLocked(currentTime);
@@ -869,7 +878,7 @@
* reflect current {@link #mPersistThreshold} value. Always defers to
* {@link Global} values when defined.
*/
- private void updatePersistThresholds() {
+ private void updatePersistThresholdsLocked() {
mDevRecorder.setPersistThreshold(mSettings.getDevPersistBytes(mPersistThreshold));
mXtRecorder.setPersistThreshold(mSettings.getXtPersistBytes(mPersistThreshold));
mUidRecorder.setPersistThreshold(mSettings.getUidPersistBytes(mPersistThreshold));
@@ -1311,7 +1320,7 @@
synchronized (mStatsLock) {
if (args.length > 0 && "--proto".equals(args[0])) {
// In this case ignore all other arguments.
- dumpProto(fd);
+ dumpProtoLocked(fd);
return;
}
@@ -1387,7 +1396,7 @@
}
}
- private void dumpProto(FileDescriptor fd) {
+ private void dumpProtoLocked(FileDescriptor fd) {
final ProtoOutputStream proto = new ProtoOutputStream(fd);
// TODO Right now it writes all history. Should it limit to the "since-boot" log?