From e7927da1b6dc4f96714aa9bc4fbb71b3659f8cea Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Wed, 25 Nov 2015 10:05:17 -0800 Subject: [PATCH] Don't call DPM from UserManager to avoid lock inversion - Also make sure DPMS.mOwners is always guarded with DPMS.this. (and remove synchronization from Owners.) Bug 25796840 Change-Id: I83f7b78e7b437d9c2a2b1d6e714346cd15f95330 --- core/java/android/os/UserManagerInternal.java | 12 +++ .../android/server/pm/UserManagerService.java | 59 +++++++++++---- .../DevicePolicyManagerService.java | 72 ++++++++++-------- .../android/server/devicepolicy/Owners.java | 74 +++++++++++-------- .../DevicePolicyManagerServiceTestable.java | 2 +- 5 files changed, 142 insertions(+), 77 deletions(-) diff --git a/core/java/android/os/UserManagerInternal.java b/core/java/android/os/UserManagerInternal.java index 26c7a1e2d0ecb..898b6cffa9d4c 100644 --- a/core/java/android/os/UserManagerInternal.java +++ b/core/java/android/os/UserManagerInternal.java @@ -69,4 +69,16 @@ public abstract class UserManagerInternal { /** Remove a {@link UserRestrictionsListener}. */ public abstract void removeUserRestrictionsListener(UserRestrictionsListener listener); + + /** + * Called by {@link com.android.server.devicepolicy.DevicePolicyManagerService} to update + * whether the device is managed by device owner. + */ + public abstract void setDeviceManaged(boolean isManaged); + + /** + * Called by {@link com.android.server.devicepolicy.DevicePolicyManagerService} to update + * whether the user is managed by profile owner. + */ + public abstract void setUserManaged(int userId, boolean isManaged); } diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index ff829ff321d5b..424b90234ca2b 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -24,7 +24,6 @@ import android.app.ActivityManagerInternal; import android.app.ActivityManagerNative; import android.app.IActivityManager; import android.app.IStopUserCallback; -import android.app.admin.DevicePolicyManager; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -216,14 +215,15 @@ public class UserManagerService extends IUserManager.Stub { private final SparseArray mAppliedUserRestrictions = new SparseArray<>(); /** - * User restrictions set by {@link DevicePolicyManager} that should be applied to all users, - * including guests. + * User restrictions set by {@link com.android.server.devicepolicy.DevicePolicyManagerService} + * that should be applied to all users, including guests. */ @GuardedBy("mRestrictionsLock") private Bundle mDevicePolicyGlobalUserRestrictions; /** - * User restrictions set by {@link DevicePolicyManager} for each user. + * User restrictions set by {@link com.android.server.devicepolicy.DevicePolicyManagerService} + * for each user. */ @GuardedBy("mRestrictionsLock") private final SparseArray mDevicePolicyLocalUserRestrictions = new SparseArray<>(); @@ -248,6 +248,12 @@ public class UserManagerService extends IUserManager.Stub { private final LocalService mLocalService; + @GuardedBy("mUsersLock") + private boolean mIsDeviceManaged; + + @GuardedBy("mUsersLock") + private final SparseBooleanArray mIsUserManaged = new SparseBooleanArray(); + @GuardedBy("mUserRestrictionsListeners") private final ArrayList mUserRestrictionsListeners = new ArrayList<>(); @@ -509,11 +515,9 @@ public class UserManagerService extends IUserManager.Stub { if (!userInfo.isAdmin()) { return false; } + // restricted profile can be created if there is no DO set and the admin user has no PO; + return !mIsDeviceManaged && !mIsUserManaged.get(userId); } - DevicePolicyManager dpm = (DevicePolicyManager) mContext.getSystemService( - Context.DEVICE_POLICY_SERVICE); - // restricted profile can be created if there is no DO set and the admin user has no PO - return !dpm.isDeviceManaged() && dpm.getProfileOwnerAsUser(userId) == null; } /* @@ -1596,11 +1600,10 @@ public class UserManagerService extends IUserManager.Stub { if (UserManager.isSplitSystemUser() && !isGuest && !isManagedProfile && getPrimaryUser() == null) { flags |= UserInfo.FLAG_PRIMARY; - DevicePolicyManager devicePolicyManager = (DevicePolicyManager) - mContext.getSystemService(Context.DEVICE_POLICY_SERVICE); - if (devicePolicyManager == null - || !devicePolicyManager.isDeviceManaged()) { - flags |= UserInfo.FLAG_ADMIN; + synchronized (mUsersLock) { + if (!mIsDeviceManaged) { + flags |= UserInfo.FLAG_ADMIN; + } } } userId = getNextAvailableId(); @@ -1865,6 +1868,13 @@ public class UserManagerService extends IUserManager.Stub { // Remove this user from the list synchronized (mUsersLock) { mUsers.remove(userHandle); + mIsUserManaged.delete(userHandle); + } + synchronized (mRestrictionsLock) { + mBaseUserRestrictions.remove(userHandle); + mAppliedUserRestrictions.remove(userHandle); + mCachedEffectiveUserRestrictions.remove(userHandle); + mDevicePolicyLocalUserRestrictions.remove(userHandle); } // Remove user file AtomicFile userFile = new AtomicFile(new File(mUsersDir, userHandle + XML_SUFFIX)); @@ -2376,9 +2386,10 @@ public class UserManagerService extends IUserManager.Stub { if (user == null) { continue; } + final int userId = user.id; pw.print(" "); pw.print(user); pw.print(" serialNo="); pw.print(user.serialNumber); - if (mRemovingUserIds.get(mUsers.keyAt(i))) { + if (mRemovingUserIds.get(userId)) { pw.print(" "); } if (user.partial) { @@ -2403,6 +2414,8 @@ public class UserManagerService extends IUserManager.Stub { sb.append(" ago"); pw.println(sb); } + pw.print(" Has profile owner: "); + pw.println(mIsUserManaged.get(userId)); pw.println(" Restrictions:"); synchronized (mRestrictionsLock) { UserRestrictionsUtils.dumpRestrictions( @@ -2427,6 +2440,10 @@ public class UserManagerService extends IUserManager.Stub { synchronized (mGuestRestrictions) { UserRestrictionsUtils.dumpRestrictions(pw, " ", mGuestRestrictions); } + synchronized (mUsersLock) { + pw.println(); + pw.println(" Device managed: " + mIsDeviceManaged); + } } } @@ -2507,6 +2524,20 @@ public class UserManagerService extends IUserManager.Stub { mUserRestrictionsListeners.remove(listener); } } + + @Override + public void setDeviceManaged(boolean isManaged) { + synchronized (mUsersLock) { + mIsDeviceManaged = isManaged; + } + } + + @Override + public void setUserManaged(int userId, boolean isManaged) { + synchronized (mUsersLock) { + mIsUserManaged.put(userId, isManaged); + } + } } private class Shell extends ShellCommand { diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index f80a61163cdf4..819925080ea37 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -1071,7 +1071,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } Owners newOwners() { - return new Owners(mContext); + return new Owners(mContext, getUserManager(), getUserManagerInternal()); } UserManager getUserManager() { @@ -2150,20 +2150,24 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } private void ensureDeviceOwnerUserStarted() { - if (mOwners.hasDeviceOwner()) { - final int userId = mOwners.getDeviceOwnerUserId(); - if (VERBOSE_LOG) { - Log.v(LOG_TAG, "Starting non-system DO user: " + userId); + final int userId; + synchronized (this) { + if (!mOwners.hasDeviceOwner()) { + return; } - if (userId != UserHandle.USER_SYSTEM) { - try { - mInjector.getIActivityManager().startUserInBackground(userId); + userId = mOwners.getDeviceOwnerUserId(); + } + if (VERBOSE_LOG) { + Log.v(LOG_TAG, "Starting non-system DO user: " + userId); + } + if (userId != UserHandle.USER_SYSTEM) { + try { + mInjector.getIActivityManager().startUserInBackground(userId); - // STOPSHIP Prevent the DO user from being killed. + // STOPSHIP Prevent the DO user from being killed. - } catch (RemoteException e) { - Slog.w(LOG_TAG, "Exception starting user", e); - } + } catch (RemoteException e) { + Slog.w(LOG_TAG, "Exception starting user", e); } } } @@ -4584,7 +4588,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { + " for device owner"); } synchronized (this) { - enforceCanSetDeviceOwner(userId); + enforceCanSetDeviceOwnerLocked(userId); // Shutting down backup manager service permanently. long ident = mInjector.binderClearCallingIdentity(); @@ -4751,7 +4755,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { + " not installed for userId:" + userHandle); } synchronized (this) { - enforceCanSetProfileOwner(userHandle); + enforceCanSetProfileOwnerLocked(userHandle); mOwners.setProfileOwner(who, ownerName, userHandle); mOwners.writeProfileOwner(userHandle); return true; @@ -4953,7 +4957,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * - SYSTEM_UID * - adb if there are not accounts. */ - private void enforceCanSetProfileOwner(int userHandle) { + private void enforceCanSetProfileOwnerLocked(int userHandle) { UserInfo info = mUserManager.getUserInfo(userHandle); if (info == null) { // User doesn't exist. @@ -4995,7 +4999,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * The device owner can only be set before the setup phase of the primary user has completed, * except for adb if no accounts or additional users are present on the device. */ - private void enforceCanSetDeviceOwner(int userId) { + private void enforceCanSetDeviceOwnerLocked(int userId) { if (mOwners.hasDeviceOwner()) { throw new IllegalStateException("Trying to set the device owner, but device owner " + "is already set."); @@ -6819,20 +6823,22 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { public boolean isProvisioningAllowed(String action) { final int callingUserId = mInjector.userHandleGetCallingUserId(); if (DevicePolicyManager.ACTION_PROVISION_MANAGED_PROFILE.equals(action)) { - if (mOwners.hasDeviceOwner()) { - if (!mInjector.userManagerIsSplitSystemUser()) { - // Only split-system-user systems support managed-profiles in combination with - // device-owner. - return false; - } - if (mOwners.getDeviceOwnerUserId() != UserHandle.USER_SYSTEM) { - // Only system device-owner supports managed-profiles. Non-system device-owner - // doesn't. - return false; - } - if (callingUserId == UserHandle.USER_SYSTEM) { - // Managed-profiles cannot be setup on the system user, only regular users. - return false; + synchronized (this) { + if (mOwners.hasDeviceOwner()) { + if (!mInjector.userManagerIsSplitSystemUser()) { + // Only split-system-user systems support managed-profiles in combination with + // device-owner. + return false; + } + if (mOwners.getDeviceOwnerUserId() != UserHandle.USER_SYSTEM) { + // Only system device-owner supports managed-profiles. Non-system device-owner + // doesn't. + return false; + } + if (callingUserId == UserHandle.USER_SYSTEM) { + // Managed-profiles cannot be setup on the system user, only regular users. + return false; + } } } if (getProfileOwner(callingUserId) != null) { @@ -6877,8 +6883,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } private boolean isDeviceOwnerProvisioningAllowed(int callingUserId) { - if (mOwners.hasDeviceOwner()) { - return false; + synchronized (this) { + if (mOwners.hasDeviceOwner()) { + return false; + } } if (getProfileOwner(callingUserId) != null) { return false; diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/Owners.java b/services/devicepolicy/java/com/android/server/devicepolicy/Owners.java index 435de7ac3f3d7..f7de0b3896f7b 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/Owners.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/Owners.java @@ -23,6 +23,7 @@ import android.content.pm.UserInfo; import android.os.Environment; import android.os.UserHandle; import android.os.UserManager; +import android.os.UserManagerInternal; import android.util.ArrayMap; import android.util.AtomicFile; import android.util.Log; @@ -50,6 +51,9 @@ import libcore.io.IoUtils; /** * Stores and restores state for the Device and Profile owners. By definition there can be * only one device owner, but there may be a profile owner for each user. + * + *

This class is not thread safe. (i.e. access to this class must always be synchronized + * in the caller side.) */ class Owners { private static final String TAG = "DevicePolicyManagerService"; @@ -78,8 +82,8 @@ class Owners { private static final String TAG_SYSTEM_UPDATE_POLICY = "system-update-policy"; - private final Context mContext; private final UserManager mUserManager; + private final UserManagerInternal mUserManagerInternal; // Internal state for the device owner package. private OwnerInfo mDeviceOwner; @@ -92,44 +96,48 @@ class Owners { // Local system update policy controllable by device owner. private SystemUpdatePolicy mSystemUpdatePolicy; - public Owners(Context context) { - mContext = context; - mUserManager = context.getSystemService(UserManager.class); + public Owners(Context context, UserManager userManager, + UserManagerInternal userManagerInternal) { + mUserManager = userManager; + mUserManagerInternal = userManagerInternal; } /** * Load configuration from the disk. */ void load() { - synchronized (this) { - // First, try to read from the legacy file. - final File legacy = getLegacyConfigFileWithTestOverride(); + // First, try to read from the legacy file. + final File legacy = getLegacyConfigFileWithTestOverride(); - if (readLegacyOwnerFile(legacy)) { - if (DEBUG) { - Log.d(TAG, "Legacy config file found."); - } + final List users = mUserManager.getUsers(); - // Legacy file exists, write to new files and remove the legacy one. - writeDeviceOwner(); - for (int userId : getProfileOwnerKeys()) { - writeProfileOwner(userId); - } - if (DEBUG) { - Log.d(TAG, "Deleting legacy config file"); - } - if (!legacy.delete()) { - Slog.e(TAG, "Failed to remove the legacy setting file"); - } - } else { - // No legacy file, read from the new format files. - new DeviceOwnerReadWriter().readFromFileLocked(); - - final List users = mUserManager.getUsers(); - for (UserInfo ui : users) { - new ProfileOwnerReadWriter(ui.id).readFromFileLocked(); - } + if (readLegacyOwnerFile(legacy)) { + if (DEBUG) { + Log.d(TAG, "Legacy config file found."); } + + // Legacy file exists, write to new files and remove the legacy one. + writeDeviceOwner(); + for (int userId : getProfileOwnerKeys()) { + writeProfileOwner(userId); + } + if (DEBUG) { + Log.d(TAG, "Deleting legacy config file"); + } + if (!legacy.delete()) { + Slog.e(TAG, "Failed to remove the legacy setting file"); + } + } else { + // No legacy file, read from the new format files. + new DeviceOwnerReadWriter().readFromFileLocked(); + + for (UserInfo ui : users) { + new ProfileOwnerReadWriter(ui.id).readFromFileLocked(); + } + } + mUserManagerInternal.setDeviceManaged(hasDeviceOwner()); + for (UserInfo ui : users) { + mUserManagerInternal.setUserManaged(ui.id, hasProfileOwner(ui.id)); } if (hasDeviceOwner() && hasProfileOwner(getDeviceOwnerUserId())) { Slog.w(TAG, String.format("User %d has both DO and PO, which is not supported", @@ -169,21 +177,27 @@ class Owners { boolean userRestrictionsMigrated) { mDeviceOwner = new OwnerInfo(ownerName, admin, userRestrictionsMigrated); mDeviceOwnerUserId = userId; + + mUserManagerInternal.setDeviceManaged(true); } void clearDeviceOwner() { mDeviceOwner = null; mDeviceOwnerUserId = UserHandle.USER_NULL; + + mUserManagerInternal.setDeviceManaged(false); } void setProfileOwner(ComponentName admin, String ownerName, int userId) { // For a newly set PO, there's no need for migration. mProfileOwners.put(userId, new OwnerInfo(ownerName, admin, /* userRestrictionsMigrated =*/ true)); + mUserManagerInternal.setUserManaged(userId, true); } void removeProfileOwner(int userId) { mProfileOwners.remove(userId); + mUserManagerInternal.setUserManaged(userId, false); } ComponentName getProfileOwnerComponent(int userId) { diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java index 56d6fc05d6b52..435b6026830ed 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java @@ -52,7 +52,7 @@ public class DevicePolicyManagerServiceTestable extends DevicePolicyManagerServi private final File mProfileOwnerBase; public OwnersTestable(DpmMockContext context) { - super(context); + super(context, context.userManager, context.userManagerInternal); mLegacyFile = new File(context.dataDir, LEGACY_FILE); mDeviceOwnerFile = new File(context.dataDir, DEVICE_OWNER_FILE); mProfileOwnerBase = new File(context.dataDir, PROFILE_OWNER_FILE_BASE);