From 7a8bb0d2836df3ec9fe930d9f0b8f672be0becf2 Mon Sep 17 00:00:00 2001 From: Martijn Coenen Date: Mon, 6 Sep 2021 11:15:08 +0200 Subject: [PATCH] Fix race condition around CE storage becoming available. This fixes a race condition that exists around preparing CE storage, and a package being reinstalled at the same time. The normal CE data unlock procedure is something like this: 1. User state is RUNNING_LOCKED 2. UserController calls onBeforeUnlockUser() 3. onBeforeUnlockUser() prepares CE storage 4. onBeforeUnlockUser() calls PackageManagerService.reconcileAppsData() 5. reconcileAppsData() prepares app CE data directories, creating dirs when needed 6. UserController changes state to RUNNING_UNLOCKING The race comes into the picture when an app is installed right between step 5 and step 6; when a new app is installed, PMS does have a function to create the CE app data directory; but that function only creates CE data directories when the user state is RUNNING_UNLOCKING (or later); so even though technically CE storage became available in step 3, other parts of PMS will only use it after step 6 has completed. To fix this, we use StorageManagerService to record the fact that CE storage for a particular user is prepared; then, PackageManagerService can query StorageManager to ask whether CE storage is prepared. That in combination with the user key being unlocked is then used as a condition for creating CE data directories from PackageManagerService. Bug: 187103629 Test: manual Change-Id: Ice81ae98441b9ed287e8db4196a041c3d47afce9 --- .../os/storage/StorageManagerInternal.java | 16 ++++++++++++++++ .../android/server/StorageManagerService.java | 17 +++++++++++++++++ .../server/pm/PackageManagerService.java | 6 ++++-- .../android/server/pm/StorageEventHelper.java | 6 +++++- .../android/server/pm/UserManagerService.java | 4 ++++ 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/core/java/android/os/storage/StorageManagerInternal.java b/core/java/android/os/storage/StorageManagerInternal.java index 54905ec6eaeb6..8928a423c6cb9 100644 --- a/core/java/android/os/storage/StorageManagerInternal.java +++ b/core/java/android/os/storage/StorageManagerInternal.java @@ -18,6 +18,7 @@ package android.os.storage; import android.annotation.NonNull; import android.annotation.Nullable; +import android.annotation.UserIdInt; import android.os.IVold; import java.util.List; @@ -135,4 +136,19 @@ public abstract class StorageManagerInternal { * {@link VolumeInfo#isPrimary()} */ public abstract List getPrimaryVolumeIds(); + + /** + * Tells StorageManager that CE storage for this user has been prepared. + * + * @param userId userId for which CE storage has been prepared + */ + public abstract void markCeStoragePrepared(@UserIdInt int userId); + + /** + * Returns true when CE storage for this user has been prepared. + * + * When the user key is unlocked and CE storage has been prepared, + * it's ok to access and modify CE directories on volumes for this user. + */ + public abstract boolean isCeStoragePrepared(@UserIdInt int userId); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 3510501358121..cb1f744b61630 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -221,6 +221,9 @@ class StorageManagerService extends IStorageManager.Stub @GuardedBy("mLock") private final Set mFuseMountedUser = new ArraySet<>(); + @GuardedBy("mLock") + private final Set mCeStoragePreparedUsers = new ArraySet<>(); + public static class Lifecycle extends SystemService { private StorageManagerService mStorageManagerService; @@ -4854,5 +4857,19 @@ class StorageManagerService extends IStorageManager.Stub } return primaryVolumeIds; } + + @Override + public void markCeStoragePrepared(int userId) { + synchronized (mLock) { + mCeStoragePreparedUsers.add(userId); + } + } + + @Override + public boolean isCeStoragePrepared(int userId) { + synchronized (mLock) { + return mCeStoragePreparedUsers.contains(userId); + } + } } } diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index d3bd279eeacab..5118048cbd465 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -15622,8 +15622,9 @@ public class PackageManagerService extends IPackageManager.Stub removeKeystoreDataIfNeeded(mInjector.getUserManagerInternal(), userId, appId); UserManagerInternal umInternal = mInjector.getUserManagerInternal(); + StorageManagerInternal smInternal = mInjector.getLocalService(StorageManagerInternal.class); final int flags; - if (umInternal.isUserUnlockingOrUnlocked(userId)) { + if (StorageManager.isUserKeyUnlocked(userId) && smInternal.isCeStoragePrepared(userId)) { flags = StorageManager.FLAG_STORAGE_DE | StorageManager.FLAG_STORAGE_CE; } else if (umInternal.isUserRunning(userId)) { flags = StorageManager.FLAG_STORAGE_DE; @@ -18724,7 +18725,8 @@ public class PackageManagerService extends IPackageManager.Stub StorageManagerInternal smInternal = mInjector.getLocalService(StorageManagerInternal.class); for (UserInfo user : mUserManager.getUsers(false /*excludeDying*/)) { final int flags; - if (umInternal.isUserUnlockingOrUnlocked(user.id)) { + if (StorageManager.isUserKeyUnlocked(user.id) + && smInternal.isCeStoragePrepared(user.id)) { flags = StorageManager.FLAG_STORAGE_DE | StorageManager.FLAG_STORAGE_CE; } else if (umInternal.isUserRunning(user.id)) { flags = StorageManager.FLAG_STORAGE_DE; diff --git a/services/core/java/com/android/server/pm/StorageEventHelper.java b/services/core/java/com/android/server/pm/StorageEventHelper.java index 486d160d97620..a70df912406fe 100644 --- a/services/core/java/com/android/server/pm/StorageEventHelper.java +++ b/services/core/java/com/android/server/pm/StorageEventHelper.java @@ -38,6 +38,7 @@ import android.os.FileUtils; import android.os.UserHandle; import android.os.storage.StorageEventListener; import android.os.storage.StorageManager; +import android.os.storage.StorageManagerInternal; import android.os.storage.VolumeInfo; import android.text.TextUtils; import android.util.Log; @@ -157,9 +158,12 @@ public class StorageEventHelper extends StorageEventListener { // Reconcile app data for all started/unlocked users final StorageManager sm = mPm.mInjector.getSystemService(StorageManager.class); UserManagerInternal umInternal = mPm.mInjector.getUserManagerInternal(); + StorageManagerInternal smInternal = mPm.mInjector.getLocalService( + StorageManagerInternal.class); for (UserInfo user : mPm.mUserManager.getUsers(false /* includeDying */)) { final int flags; - if (umInternal.isUserUnlockingOrUnlocked(user.id)) { + if (StorageManager.isUserKeyUnlocked(user.id) + && smInternal.isCeStoragePrepared(user.id)) { flags = StorageManager.FLAG_STORAGE_DE | StorageManager.FLAG_STORAGE_CE; } else if (umInternal.isUserRunning(user.id)) { flags = StorageManager.FLAG_STORAGE_DE; diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index e8182e07ba4d2..d02d301af9fcb 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -80,6 +80,7 @@ import android.os.UserManager; import android.os.UserManager.EnforcingUser; import android.os.UserManager.QuietModeFlag; import android.os.storage.StorageManager; +import android.os.storage.StorageManagerInternal; import android.provider.Settings; import android.security.GateKeeper; import android.service.gatekeeper.IGateKeeperService; @@ -4836,6 +4837,9 @@ public class UserManagerService extends IUserManager.Stub { mUserDataPreparer.prepareUserData(userId, userSerial, StorageManager.FLAG_STORAGE_CE); t.traceEnd(); + StorageManagerInternal smInternal = LocalServices.getService(StorageManagerInternal.class); + smInternal.markCeStoragePrepared(userId); + t.traceBegin("reconcileAppsData-" + userId); mPm.reconcileAppsData(userId, StorageManager.FLAG_STORAGE_CE, migrateAppsData); t.traceEnd();