From 090ae07bc2eb5c053825ea3f1e98be39e50d54bd Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Mon, 18 Oct 2021 22:33:15 -0700 Subject: [PATCH 01/12] [vold] Check incremental paths before mounting Vold was trusting system_server too much and allowed for pretty much any path in mount()/bindMount() calls for incremental. This CL adds validation to make sure it's only accessing own directories. This includes enforcing no symlinks in the paths Ignore-AOSP-First: security fix Bug: 198657657 Test: manual Change-Id: I6035447f94ef44c4ae3294c3ae47de2d7210683a --- Utils.cpp | 50 +++++++++++++++++++++++++++++++++ Utils.h | 13 +++++++++ VoldNativeService.cpp | 49 ++++++++++++++++++++++++++------ VoldNativeServiceValidation.cpp | 27 ++++++++++++++++++ VoldNativeServiceValidation.h | 5 ++++ 5 files changed, 136 insertions(+), 8 deletions(-) diff --git a/Utils.cpp b/Utils.cpp index f9f3058..f059c67 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -1695,5 +1695,55 @@ status_t PrepareAndroidDirs(const std::string& volumeRoot) { return OK; } + +namespace ab = android::base; + +static ab::unique_fd openDirFd(int parentFd, const char* name) { + return ab::unique_fd{::openat(parentFd, name, O_CLOEXEC | O_DIRECTORY | O_PATH | O_NOFOLLOW)}; +} + +static ab::unique_fd openAbsolutePathFd(std::string_view path) { + if (path.empty() || path[0] != '/') { + errno = EINVAL; + return {}; + } + if (path == "/") { + return openDirFd(-1, "/"); + } + + // first component is special - it includes the leading slash + auto next = path.find('/', 1); + auto component = std::string(path.substr(0, next)); + if (component == "..") { + errno = EINVAL; + return {}; + } + auto fd = openDirFd(-1, component.c_str()); + if (!fd.ok()) { + return fd; + } + path.remove_prefix(std::min(next + 1, path.size())); + while (next != path.npos && !path.empty()) { + next = path.find('/'); + component.assign(path.substr(0, next)); + fd = openDirFd(fd, component.c_str()); + if (!fd.ok()) { + return fd; + } + path.remove_prefix(std::min(next + 1, path.size())); + } + return fd; +} + +std::pair OpenDirInProcfs(std::string_view path) { + auto fd = openAbsolutePathFd(path); + if (!fd.ok()) { + return {}; + } + + auto linkPath = std::string("/proc/self/fd/") += std::to_string(fd.get()); + return {std::move(fd), std::move(linkPath)}; +} + } // namespace vold } // namespace android diff --git a/Utils.h b/Utils.h index bb6615c..8120db7 100644 --- a/Utils.h +++ b/Utils.h @@ -27,6 +27,7 @@ #include #include +#include #include struct DIR; @@ -200,6 +201,18 @@ status_t UnmountUserFuse(userid_t userId, const std::string& absolute_lower_path const std::string& relative_upper_path); status_t PrepareAndroidDirs(const std::string& volumeRoot); + +// Open a given directory as an FD, and return that and the corresponding procfs virtual +// symlink path that can be used in any API that accepts a path string. Path stays valid until +// the directory FD is closed. +// +// This may be useful when an API wants to restrict a path passed from an untrusted process, +// and do it without any TOCTOU attacks possible (e.g. where an attacker replaces one of +// the components with a symlink after the check passed). In that case opening a path through +// this function guarantees that the target directory stays the same, and that it can be +// referenced inside the current process via the virtual procfs symlink returned here. +std::pair OpenDirInProcfs(std::string_view path); + } // namespace vold } // namespace android diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 291a0d9..757354c 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -993,10 +993,25 @@ binder::Status VoldNativeService::mountIncFs( const std::string& sysfsName, ::android::os::incremental::IncrementalFileSystemControlParcel* _aidl_return) { ENFORCE_SYSTEM_OR_ROOT; - CHECK_ARGUMENT_PATH(backingPath); - CHECK_ARGUMENT_PATH(targetDir); + if (auto status = CheckIncrementalPath(IncrementalPathKind::MountTarget, targetDir); + !status.isOk()) { + return status; + } + if (auto status = CheckIncrementalPath(IncrementalPathKind::MountSource, backingPath); + !status.isOk()) { + return status; + } - auto control = incfs::mount(backingPath, targetDir, + auto [backingFd, backingSymlink] = OpenDirInProcfs(backingPath); + if (!backingFd.ok()) { + return translate(-errno); + } + auto [targetFd, targetSymlink] = OpenDirInProcfs(targetDir); + if (!targetFd.ok()) { + return translate(-errno); + } + + auto control = incfs::mount(backingSymlink, targetSymlink, {.flags = IncFsMountFlags(flags), // Mount with read timeouts. .defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS, @@ -1019,9 +1034,15 @@ binder::Status VoldNativeService::mountIncFs( binder::Status VoldNativeService::unmountIncFs(const std::string& dir) { ENFORCE_SYSTEM_OR_ROOT; - CHECK_ARGUMENT_PATH(dir); + if (auto status = CheckIncrementalPath(IncrementalPathKind::Any, dir); !status.isOk()) { + return status; + } - return translate(incfs::unmount(dir)); + auto [fd, symLink] = OpenDirInProcfs(dir); + if (!fd.ok()) { + return translate(-errno); + } + return translate(incfs::unmount(symLink)); } binder::Status VoldNativeService::setIncFsMountOptions( @@ -1069,10 +1090,22 @@ binder::Status VoldNativeService::setIncFsMountOptions( binder::Status VoldNativeService::bindMount(const std::string& sourceDir, const std::string& targetDir) { ENFORCE_SYSTEM_OR_ROOT; - CHECK_ARGUMENT_PATH(sourceDir); - CHECK_ARGUMENT_PATH(targetDir); + if (auto status = CheckIncrementalPath(IncrementalPathKind::Any, sourceDir); !status.isOk()) { + return status; + } + if (auto status = CheckIncrementalPath(IncrementalPathKind::Bind, targetDir); !status.isOk()) { + return status; + } - return translate(incfs::bindMount(sourceDir, targetDir)); + auto [sourceFd, sourceSymlink] = OpenDirInProcfs(sourceDir); + if (!sourceFd.ok()) { + return translate(-errno); + } + auto [targetFd, targetSymlink] = OpenDirInProcfs(targetDir); + if (!targetFd.ok()) { + return translate(-errno); + } + return translate(incfs::bindMount(sourceSymlink, targetSymlink)); } binder::Status VoldNativeService::destroyDsuMetadataKey(const std::string& dsuSlot) { diff --git a/VoldNativeServiceValidation.cpp b/VoldNativeServiceValidation.cpp index ee1e65a..1d19141 100644 --- a/VoldNativeServiceValidation.cpp +++ b/VoldNativeServiceValidation.cpp @@ -105,4 +105,31 @@ binder::Status CheckArgumentHex(const std::string& hex) { return Ok(); } +binder::Status CheckIncrementalPath(IncrementalPathKind kind, const std::string& path) { + if (auto status = CheckArgumentPath(path); !status.isOk()) { + return status; + } + if (kind == IncrementalPathKind::MountSource || kind == IncrementalPathKind::MountTarget || + kind == IncrementalPathKind::Any) { + if (android::base::StartsWith(path, "/data/incremental/MT_")) { + if (kind != IncrementalPathKind::MountSource && + (android::base::EndsWith(path, "/mount") || path.find("/mount/") != path.npos)) { + return Ok(); + } + if (kind != IncrementalPathKind::MountTarget && + (android::base::EndsWith(path, "/backing_store") || + path.find("/backing_store/") != path.npos)) { + return Ok(); + } + } + } + if (kind == IncrementalPathKind::Bind || kind == IncrementalPathKind::Any) { + if (android::base::StartsWith(path, "/data/app/")) { + return Ok(); + } + } + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("Path '%s' is not allowed", path.c_str())); +} + } // namespace android::vold diff --git a/VoldNativeServiceValidation.h b/VoldNativeServiceValidation.h index d2fc9e0..7fcb738 100644 --- a/VoldNativeServiceValidation.h +++ b/VoldNativeServiceValidation.h @@ -34,4 +34,9 @@ binder::Status CheckArgumentId(const std::string& id); binder::Status CheckArgumentPath(const std::string& path); binder::Status CheckArgumentHex(const std::string& hex); +// Incremental service is only allowed to touch its own directory, and the installed apps dir. +// This function ensures the caller isn't doing anything tricky. +enum class IncrementalPathKind { MountSource, MountTarget, Bind, Any }; +binder::Status CheckIncrementalPath(IncrementalPathKind kind, const std::string& path); + } // namespace android::vold From 4461c5a1764c150e6598a6f20f2987ecb11e958a Mon Sep 17 00:00:00 2001 From: Youkichi Hosoi Date: Sat, 13 Nov 2021 16:27:02 +0900 Subject: [PATCH 02/12] Split MOUNT_FLAG_VISIBLE into MOUNT_FLAG_VISIBLE_FOR_{READ, WRITE} IVold.MOUNT_FLAG_VISIBLE is split into MOUNT_FLAG_VISIBLE_FOR_READ and MOUNT_FLAG_VISIBLE_FOR_WRITE. Accordingly, VolumeBase::MountFlags::kVisible is split into kVisibleForRead and kVisibleForWrite. Bug: 206019156 Test: m Ignore-AOSP-First: Cherry-pick to AOSP later to avoid merge conflicts. Change-Id: Ia55673400d9f713f221650e1335a46ba11f6f027 --- VolumeManager.cpp | 8 ++++---- binder/android/os/IVold.aidl | 3 ++- model/EmulatedVolume.cpp | 2 +- model/PublicVolume.cpp | 2 +- model/VolumeBase.h | 13 +++++++++++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/VolumeManager.cpp b/VolumeManager.cpp index d299593..02025b7 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -1002,8 +1002,8 @@ int VolumeManager::setupAppDir(const std::string& path, int32_t appUid, bool fix // The volume must be mounted return false; } - if ((vol.getMountFlags() & VolumeBase::MountFlags::kVisible) == 0) { - // and visible + if (!vol.isVisibleForWrite()) { + // App dirs should only be created for writable volumes. return false; } if (vol.getInternalPath().empty()) { @@ -1077,8 +1077,8 @@ int VolumeManager::createObb(const std::string& sourcePath, const std::string& s // The volume must be mounted return false; } - if ((vol.getMountFlags() & VolumeBase::MountFlags::kVisible) == 0) { - // and visible + if (!vol.isVisibleForWrite()) { + // Obb volume should only be created for writable volumes. return false; } if (vol.getInternalPath().empty()) { diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index 606f473..8dd7860 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -159,7 +159,8 @@ interface IVold { const int FSTRIM_FLAG_DEEP_TRIM = 1; const int MOUNT_FLAG_PRIMARY = 1; - const int MOUNT_FLAG_VISIBLE = 2; + const int MOUNT_FLAG_VISIBLE_FOR_READ = 2; + const int MOUNT_FLAG_VISIBLE_FOR_WRITE = 4; const int PARTITION_TYPE_PUBLIC = 0; const int PARTITION_TYPE_PRIVATE = 1; diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 6f21ff8..b686437 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -246,7 +246,7 @@ status_t EmulatedVolume::unmountSdcardFs() { status_t EmulatedVolume::doMount() { std::string label = getLabel(); - bool isVisible = getMountFlags() & MountFlags::kVisible; + bool isVisible = isVisibleForWrite(); mSdcardFsDefault = StringPrintf("/mnt/runtime/default/%s", label.c_str()); mSdcardFsRead = StringPrintf("/mnt/runtime/read/%s", label.c_str()); diff --git a/model/PublicVolume.cpp b/model/PublicVolume.cpp index 12e31ff..bf54c95 100644 --- a/model/PublicVolume.cpp +++ b/model/PublicVolume.cpp @@ -97,7 +97,7 @@ status_t PublicVolume::doDestroy() { } status_t PublicVolume::doMount() { - bool isVisible = getMountFlags() & MountFlags::kVisible; + bool isVisible = isVisibleForWrite(); readMetadata(); if (mFsType == "vfat" && vfat::IsSupported()) { diff --git a/model/VolumeBase.h b/model/VolumeBase.h index 689750d..f29df65 100644 --- a/model/VolumeBase.h +++ b/model/VolumeBase.h @@ -63,8 +63,14 @@ class VolumeBase { enum MountFlags { /* Flag that volume is primary external storage */ kPrimary = 1 << 0, - /* Flag that volume is visible to normal apps */ - kVisible = 1 << 1, + /* + * Flags indicating that volume is visible to normal apps. + * kVisibleForRead and kVisibleForWrite correspond to + * VolumeInfo.MOUNT_FLAG_VISIBLE_FOR_READ and + * VolumeInfo.MOUNT_FLAG_VISIBLE_FOR_WRITE, respectively. + */ + kVisibleForRead = 1 << 1, + kVisibleForWrite = 1 << 2, }; enum class State { @@ -103,6 +109,9 @@ class VolumeBase { std::shared_ptr findVolume(const std::string& id); bool isEmulated() { return mType == Type::kEmulated; } + bool isVisibleForRead() const { return (mMountFlags & MountFlags::kVisibleForRead) != 0; } + bool isVisibleForWrite() const { return (mMountFlags & MountFlags::kVisibleForWrite) != 0; } + bool isVisible() const { return isVisibleForRead() || isVisibleForWrite(); } status_t create(); status_t destroy(); From 5bb9faab49e2bd7872e2d8b4ada6be89df7758fb Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 25 Jan 2022 19:21:37 +0000 Subject: [PATCH 03/12] Remove HardwareAuthToken parameters from binder interface These are no longer used. Test: atest com.android.server.locksettings Bug: 184723544 Ignore-AOSP-First: prerequisite changes in frameworks/base have merge conflicts between AOSP and internal master Change-Id: I6160d30deb138a5366532de84cbf6f02cbc69b8c --- VoldNativeService.cpp | 23 ----------------------- VoldNativeService.h | 9 +++------ binder/android/os/IVold.aidl | 9 +++------ 3 files changed, 6 insertions(+), 35 deletions(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 1033af9..1903063 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -731,36 +731,19 @@ binder::Status VoldNativeService::destroyUserKey(int32_t userId) { return translateBool(fscrypt_destroy_user_key(userId)); } -static bool token_empty(const std::string& token) { - return token.size() == 0 || token == "!"; -} - binder::Status VoldNativeService::addUserKeyAuth(int32_t userId, int32_t userSerial, - const std::string& token, const std::string& secret) { ENFORCE_SYSTEM_OR_ROOT; ACQUIRE_CRYPT_LOCK; - if (!token_empty(token)) { - LOG(ERROR) << "Vold doesn't use auth tokens, but non-empty token passed to addUserKeyAuth."; - return binder::Status::fromServiceSpecificError(-EINVAL); - } - return translateBool(fscrypt_add_user_key_auth(userId, userSerial, secret)); } binder::Status VoldNativeService::clearUserKeyAuth(int32_t userId, int32_t userSerial, - const std::string& token, const std::string& secret) { ENFORCE_SYSTEM_OR_ROOT; ACQUIRE_CRYPT_LOCK; - if (!token_empty(token)) { - LOG(ERROR) - << "Vold doesn't use auth tokens, but non-empty token passed to clearUserKeyAuth."; - return binder::Status::fromServiceSpecificError(-EINVAL); - } - return translateBool(fscrypt_clear_user_key_auth(userId, userSerial, secret)); } @@ -780,16 +763,10 @@ binder::Status VoldNativeService::getUnlockedUsers(std::vector* _aidl_retur } binder::Status VoldNativeService::unlockUserKey(int32_t userId, int32_t userSerial, - const std::string& token, const std::string& secret) { ENFORCE_SYSTEM_OR_ROOT; ACQUIRE_CRYPT_LOCK; - if (!token_empty(token)) { - LOG(ERROR) << "Vold doesn't use auth tokens, but non-empty token passed to unlockUserKey."; - return binder::Status::fromServiceSpecificError(-EINVAL); - } - return translateBool(fscrypt_unlock_user_key(userId, userSerial, secret)); } diff --git a/VoldNativeService.h b/VoldNativeService.h index 49bcbaa..653165f 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -127,15 +127,12 @@ class VoldNativeService : public BinderService, public os::Bn binder::Status createUserKey(int32_t userId, int32_t userSerial, bool ephemeral); binder::Status destroyUserKey(int32_t userId); - binder::Status addUserKeyAuth(int32_t userId, int32_t userSerial, const std::string& token, - const std::string& secret); - binder::Status clearUserKeyAuth(int32_t userId, int32_t userSerial, const std::string& token, - const std::string& secret); + binder::Status addUserKeyAuth(int32_t userId, int32_t userSerial, const std::string& secret); + binder::Status clearUserKeyAuth(int32_t userId, int32_t userSerial, const std::string& secret); binder::Status fixateNewestUserKeyAuth(int32_t userId); binder::Status getUnlockedUsers(std::vector* _aidl_return); - binder::Status unlockUserKey(int32_t userId, int32_t userSerial, const std::string& token, - const std::string& secret); + binder::Status unlockUserKey(int32_t userId, int32_t userSerial, const std::string& secret); binder::Status lockUserKey(int32_t userId); binder::Status prepareUserStorage(const std::optional& uuid, int32_t userId, diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index c72ceea..7c2e88b 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -101,15 +101,12 @@ interface IVold { void createUserKey(int userId, int userSerial, boolean ephemeral); void destroyUserKey(int userId); - void addUserKeyAuth(int userId, int userSerial, @utf8InCpp String token, - @utf8InCpp String secret); - void clearUserKeyAuth(int userId, int userSerial, @utf8InCpp String token, - @utf8InCpp String secret); + void addUserKeyAuth(int userId, int userSerial, @utf8InCpp String secret); + void clearUserKeyAuth(int userId, int userSerial, @utf8InCpp String secret); void fixateNewestUserKeyAuth(int userId); int[] getUnlockedUsers(); - void unlockUserKey(int userId, int userSerial, @utf8InCpp String token, - @utf8InCpp String secret); + void unlockUserKey(int userId, int userSerial, @utf8InCpp String secret); void lockUserKey(int userId); void prepareUserStorage(@nullable @utf8InCpp String uuid, int userId, int userSerial, From 0cf90d7ca07ea94a7a318aba6e4e8872e63337dd Mon Sep 17 00:00:00 2001 From: Samiul Islam Date: Fri, 11 Feb 2022 16:17:49 +0000 Subject: [PATCH 04/12] Create root directory for supplemental data during user creation In order to store supplemental data for apps, we want to create a root directory at location `/data/misc_ce//supplmental` and `/data/misc_de/supplemental`. These directories will then host supplemental data for each app based on package name, e.g, `/data/misc_ce/0/supplemental/`. Since these are sub-directories of misc directory, vold should prepare them for consistency. Bug: 217543371 Test: atest SupplementalProcessStorageHostTest Test: see ag/16681989 Ignore-AOSP-First: Feature is being developed in internal branch Change-Id: I66ef7a7241c9f82cecedaeb6c9a91f127668300a --- vold_prepare_subdirs.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/vold_prepare_subdirs.cpp b/vold_prepare_subdirs.cpp index 0d58e4d..0fe8b01 100644 --- a/vold_prepare_subdirs.cpp +++ b/vold_prepare_subdirs.cpp @@ -172,8 +172,13 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla return false; } + auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); + if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, + misc_de_path + "/supplemental", user_id)) { + return false; + } + if (volume_uuid.empty()) { - auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); if (!prepare_dir(sehandle, 0700, 0, 0, misc_de_path + "/vold")) return false; if (!prepare_dir(sehandle, 0700, 0, 0, misc_de_path + "/storaged")) return false; if (!prepare_dir(sehandle, 0700, 0, 0, misc_de_path + "/rollback")) return false; @@ -203,8 +208,13 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla return false; } + auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); + if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, + misc_ce_path + "/supplemental", user_id)) { + return false; + } + if (volume_uuid.empty()) { - auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); if (!prepare_dir(sehandle, 0700, 0, 0, misc_ce_path + "/vold")) return false; if (!prepare_dir(sehandle, 0700, 0, 0, misc_ce_path + "/storaged")) return false; if (!prepare_dir(sehandle, 0700, 0, 0, misc_ce_path + "/rollback")) return false; From bad7cd0fd7ef149a801c165dc3ca4a3743054a6c Mon Sep 17 00:00:00 2001 From: Nikita Ioffe Date: Mon, 21 Feb 2022 19:03:26 +0000 Subject: [PATCH 05/12] Rename SupplementalProcess to SdkSandbox Ignore-AOSP-First: code not in AOSP yet Bug: 220320098 Test: presubmit Change-Id: I727342675f6817d4dced431b4ef57e909c02eb5a --- vold_prepare_subdirs.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vold_prepare_subdirs.cpp b/vold_prepare_subdirs.cpp index 0fe8b01..692c500 100644 --- a/vold_prepare_subdirs.cpp +++ b/vold_prepare_subdirs.cpp @@ -174,7 +174,7 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, - misc_de_path + "/supplemental", user_id)) { + misc_de_path + "/sdksandbox", user_id)) { return false; } @@ -210,7 +210,7 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, - misc_ce_path + "/supplemental", user_id)) { + misc_ce_path + "/sdksandbox", user_id)) { return false; } From b459591fd1a46729e6203a021563989fc7c21b17 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Mon, 7 Mar 2022 20:27:06 +0000 Subject: [PATCH 06/12] Create misc_ce and misc_de directories on /mnt/expand We want to store sdk data on the same volume as app data. Since sdk data is stored in misc_ce and misc_de directory, we need to ensure they exist on adopted storage mounted at /mnt/expand/. This CL creates `/mnt/expand//misc_{ce,de}` directories when disk is mouted and then when user storage is prepared, the sdk root directory is created. By having these directories, we can now move the sdk data to other volume when app data is moved. Bug: b/222034645 Test: atest SdkSandboxStorageHostTest (see ag/17120883) Ignore-AOSP-First: End to end test added which exists in internal branch only. Will cherry-pick this CL to aosp standalone once it is safely merged to internal branch. Change-Id: I0e73d9ce105abec4b77c378cde58aa7365258f01 --- FsCrypt.cpp | 41 ++++++++++++++++++++++++---------------- Utils.cpp | 16 ++++++++-------- Utils.h | 4 ++-- model/PrivateVolume.cpp | 2 ++ vold_prepare_subdirs.cpp | 20 +++++++++++--------- 5 files changed, 48 insertions(+), 35 deletions(-) diff --git a/FsCrypt.cpp b/FsCrypt.cpp index be68222..49e7bd0 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -764,7 +764,7 @@ bool fscrypt_unlock_user_key(userid_t user_id, int serial, const std::string& se // unlock directories when not in emulation mode, to bring devices // back into a known-good state. if (!emulated_unlock(android::vold::BuildDataSystemCePath(user_id), 0771) || - !emulated_unlock(android::vold::BuildDataMiscCePath(user_id), 01771) || + !emulated_unlock(android::vold::BuildDataMiscCePath("", user_id), 01771) || !emulated_unlock(android::vold::BuildDataMediaCePath("", user_id), 0770) || !emulated_unlock(android::vold::BuildDataUserCePath("", user_id), 0771)) { LOG(ERROR) << "Failed to unlock user " << user_id; @@ -782,7 +782,7 @@ bool fscrypt_lock_user_key(userid_t user_id) { } else if (fscrypt_is_emulated()) { // When in emulation mode, we just use chmod if (!emulated_lock(android::vold::BuildDataSystemCePath(user_id)) || - !emulated_lock(android::vold::BuildDataMiscCePath(user_id)) || + !emulated_lock(android::vold::BuildDataMiscCePath("", user_id)) || !emulated_lock(android::vold::BuildDataMediaCePath("", user_id)) || !emulated_lock(android::vold::BuildDataUserCePath("", user_id))) { LOG(ERROR) << "Failed to lock user " << user_id; @@ -817,7 +817,7 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ // DE_n key auto system_de_path = android::vold::BuildDataSystemDePath(user_id); - auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); + auto misc_de_path = android::vold::BuildDataMiscDePath(volume_uuid, user_id); auto vendor_de_path = android::vold::BuildDataVendorDePath(user_id); auto user_de_path = android::vold::BuildDataUserDePath(volume_uuid, user_id); @@ -831,9 +831,10 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ if (!prepare_dir(profiles_de_path, 0771, AID_SYSTEM, AID_SYSTEM)) return false; if (!prepare_dir(system_de_path, 0770, AID_SYSTEM, AID_SYSTEM)) return false; - if (!prepare_dir(misc_de_path, 01771, AID_SYSTEM, AID_MISC)) return false; if (!prepare_dir(vendor_de_path, 0771, AID_ROOT, AID_ROOT)) return false; } + + if (!prepare_dir(misc_de_path, 01771, AID_SYSTEM, AID_MISC)) return false; if (!prepare_dir(user_de_path, 0771, AID_SYSTEM, AID_SYSTEM)) return false; if (fscrypt_is_native()) { @@ -841,11 +842,14 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ if (volume_uuid.empty()) { if (!lookup_policy(s_de_policies, user_id, &de_policy)) return false; if (!EnsurePolicy(de_policy, system_de_path)) return false; - if (!EnsurePolicy(de_policy, misc_de_path)) return false; if (!EnsurePolicy(de_policy, vendor_de_path)) return false; } else { - if (!read_or_create_volkey(misc_de_path, volume_uuid, &de_policy)) return false; + auto misc_de_empty_volume_path = android::vold::BuildDataMiscDePath("", user_id); + if (!read_or_create_volkey(misc_de_empty_volume_path, volume_uuid, &de_policy)) { + return false; + } } + if (!EnsurePolicy(de_policy, misc_de_path)) return false; if (!EnsurePolicy(de_policy, user_de_path)) return false; } } @@ -853,14 +857,13 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ if (flags & android::os::IVold::STORAGE_FLAG_CE) { // CE_n key auto system_ce_path = android::vold::BuildDataSystemCePath(user_id); - auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); + auto misc_ce_path = android::vold::BuildDataMiscCePath(volume_uuid, user_id); auto vendor_ce_path = android::vold::BuildDataVendorCePath(user_id); auto media_ce_path = android::vold::BuildDataMediaCePath(volume_uuid, user_id); auto user_ce_path = android::vold::BuildDataUserCePath(volume_uuid, user_id); if (volume_uuid.empty()) { if (!prepare_dir(system_ce_path, 0770, AID_SYSTEM, AID_SYSTEM)) return false; - if (!prepare_dir(misc_ce_path, 01771, AID_SYSTEM, AID_MISC)) return false; if (!prepare_dir(vendor_ce_path, 0771, AID_ROOT, AID_ROOT)) return false; } if (!prepare_dir(media_ce_path, 02770, AID_MEDIA_RW, AID_MEDIA_RW)) return false; @@ -873,6 +876,7 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ return false; } + if (!prepare_dir(misc_ce_path, 01771, AID_SYSTEM, AID_MISC)) return false; if (!prepare_dir(user_ce_path, 0771, AID_SYSTEM, AID_SYSTEM)) return false; if (fscrypt_is_native()) { @@ -880,12 +884,15 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ if (volume_uuid.empty()) { if (!lookup_policy(s_ce_policies, user_id, &ce_policy)) return false; if (!EnsurePolicy(ce_policy, system_ce_path)) return false; - if (!EnsurePolicy(ce_policy, misc_ce_path)) return false; if (!EnsurePolicy(ce_policy, vendor_ce_path)) return false; } else { - if (!read_or_create_volkey(misc_ce_path, volume_uuid, &ce_policy)) return false; + auto misc_ce_empty_volume_path = android::vold::BuildDataMiscCePath("", user_id); + if (!read_or_create_volkey(misc_ce_empty_volume_path, volume_uuid, &ce_policy)) { + return false; + } } if (!EnsurePolicy(ce_policy, media_ce_path)) return false; + if (!EnsurePolicy(ce_policy, misc_ce_path)) return false; if (!EnsurePolicy(ce_policy, user_ce_path)) return false; } @@ -913,20 +920,21 @@ bool fscrypt_destroy_user_storage(const std::string& volume_uuid, userid_t user_ if (flags & android::os::IVold::STORAGE_FLAG_CE) { // CE_n key auto system_ce_path = android::vold::BuildDataSystemCePath(user_id); - auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); + auto misc_ce_path = android::vold::BuildDataMiscCePath(volume_uuid, user_id); auto vendor_ce_path = android::vold::BuildDataVendorCePath(user_id); auto media_ce_path = android::vold::BuildDataMediaCePath(volume_uuid, user_id); auto user_ce_path = android::vold::BuildDataUserCePath(volume_uuid, user_id); res &= destroy_dir(media_ce_path); + res &= destroy_dir(misc_ce_path); res &= destroy_dir(user_ce_path); if (volume_uuid.empty()) { res &= destroy_dir(system_ce_path); - res &= destroy_dir(misc_ce_path); res &= destroy_dir(vendor_ce_path); } else { if (fscrypt_is_native()) { - res &= destroy_volkey(misc_ce_path, volume_uuid); + auto misc_ce_empty_volume_path = android::vold::BuildDataMiscCePath("", user_id); + res &= destroy_volkey(misc_ce_empty_volume_path, volume_uuid); } } } @@ -939,11 +947,12 @@ bool fscrypt_destroy_user_storage(const std::string& volume_uuid, userid_t user_ // DE_n key auto system_de_path = android::vold::BuildDataSystemDePath(user_id); - auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); + auto misc_de_path = android::vold::BuildDataMiscDePath(volume_uuid, user_id); auto vendor_de_path = android::vold::BuildDataVendorDePath(user_id); auto user_de_path = android::vold::BuildDataUserDePath(volume_uuid, user_id); res &= destroy_dir(user_de_path); + res &= destroy_dir(misc_de_path); if (volume_uuid.empty()) { res &= destroy_dir(system_legacy_path); #if MANAGE_MISC_DIRS @@ -951,11 +960,11 @@ bool fscrypt_destroy_user_storage(const std::string& volume_uuid, userid_t user_ #endif res &= destroy_dir(profiles_de_path); res &= destroy_dir(system_de_path); - res &= destroy_dir(misc_de_path); res &= destroy_dir(vendor_de_path); } else { if (fscrypt_is_native()) { - res &= destroy_volkey(misc_de_path, volume_uuid); + auto misc_de_empty_volume_path = android::vold::BuildDataMiscDePath("", user_id); + res &= destroy_volkey(misc_de_empty_volume_path, volume_uuid); } } } diff --git a/Utils.cpp b/Utils.cpp index 864cbf8..70b70f4 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -1120,14 +1120,6 @@ std::string BuildDataMiscLegacyPath(userid_t userId) { return StringPrintf("%s/misc/user/%u", BuildDataPath("").c_str(), userId); } -std::string BuildDataMiscCePath(userid_t userId) { - return StringPrintf("%s/misc_ce/%u", BuildDataPath("").c_str(), userId); -} - -std::string BuildDataMiscDePath(userid_t userId) { - return StringPrintf("%s/misc_de/%u", BuildDataPath("").c_str(), userId); -} - // Keep in sync with installd (frameworks/native/cmds/installd/utils.h) std::string BuildDataProfilesDePath(userid_t userId) { return StringPrintf("%s/misc/profiles/cur/%u", BuildDataPath("").c_str(), userId); @@ -1157,6 +1149,14 @@ std::string BuildDataMediaCePath(const std::string& volumeUuid, userid_t userId) return StringPrintf("%s/media/%u", data.c_str(), userId); } +std::string BuildDataMiscCePath(const std::string& volumeUuid, userid_t userId) { + return StringPrintf("%s/misc_ce/%u", BuildDataPath(volumeUuid).c_str(), userId); +} + +std::string BuildDataMiscDePath(const std::string& volumeUuid, userid_t userId) { + return StringPrintf("%s/misc_de/%u", BuildDataPath(volumeUuid).c_str(), userId); +} + std::string BuildDataUserCePath(const std::string& volumeUuid, userid_t userId) { // TODO: unify with installd path generation logic std::string data(BuildDataPath(volumeUuid)); diff --git a/Utils.h b/Utils.h index 2d54639..9facb35 100644 --- a/Utils.h +++ b/Utils.h @@ -150,14 +150,14 @@ std::string BuildDataSystemLegacyPath(userid_t userid); std::string BuildDataSystemCePath(userid_t userid); std::string BuildDataSystemDePath(userid_t userid); std::string BuildDataMiscLegacyPath(userid_t userid); -std::string BuildDataMiscCePath(userid_t userid); -std::string BuildDataMiscDePath(userid_t userid); std::string BuildDataProfilesDePath(userid_t userid); std::string BuildDataVendorCePath(userid_t userid); std::string BuildDataVendorDePath(userid_t userid); std::string BuildDataPath(const std::string& volumeUuid); std::string BuildDataMediaCePath(const std::string& volumeUuid, userid_t userid); +std::string BuildDataMiscCePath(const std::string& volumeUuid, userid_t userid); +std::string BuildDataMiscDePath(const std::string& volumeUuid, userid_t userid); std::string BuildDataUserCePath(const std::string& volumeUuid, userid_t userid); std::string BuildDataUserDePath(const std::string& volumeUuid, userid_t userid); diff --git a/model/PrivateVolume.cpp b/model/PrivateVolume.cpp index 1875b7b..a692ea9 100644 --- a/model/PrivateVolume.cpp +++ b/model/PrivateVolume.cpp @@ -173,6 +173,8 @@ status_t PrivateVolume::doMount() { if (PrepareDir(mPath + "/app", 0771, AID_SYSTEM, AID_SYSTEM) || PrepareDir(mPath + "/user", 0711, AID_SYSTEM, AID_SYSTEM) || PrepareDir(mPath + "/user_de", 0711, AID_SYSTEM, AID_SYSTEM) || + PrepareDir(mPath + "/misc_ce", 0711, AID_SYSTEM, AID_SYSTEM) || + PrepareDir(mPath + "/misc_de", 0711, AID_SYSTEM, AID_SYSTEM) || PrepareDir(mPath + "/media", 0770, AID_MEDIA_RW, AID_MEDIA_RW, attrs) || PrepareDir(mPath + "/media/0", 0770, AID_MEDIA_RW, AID_MEDIA_RW) || PrepareDir(mPath + "/local", 0751, AID_ROOT, AID_ROOT) || diff --git a/vold_prepare_subdirs.cpp b/vold_prepare_subdirs.cpp index 692c500..94d7f15 100644 --- a/vold_prepare_subdirs.cpp +++ b/vold_prepare_subdirs.cpp @@ -172,7 +172,7 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla return false; } - auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); + auto misc_de_path = android::vold::BuildDataMiscDePath(volume_uuid, user_id); if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, misc_de_path + "/sdksandbox", user_id)) { return false; @@ -208,7 +208,7 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla return false; } - auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); + auto misc_ce_path = android::vold::BuildDataMiscCePath(volume_uuid, user_id); if (!prepare_dir_for_user(sehandle, 0771, AID_SYSTEM, AID_SYSTEM, misc_ce_path + "/sdksandbox", user_id)) { return false; @@ -256,18 +256,20 @@ static bool prepare_subdirs(const std::string& volume_uuid, int user_id, int fla static bool destroy_subdirs(const std::string& volume_uuid, int user_id, int flags) { bool res = true; - if (volume_uuid.empty()) { - if (flags & android::os::IVold::STORAGE_FLAG_CE) { - auto misc_ce_path = android::vold::BuildDataMiscCePath(user_id); - res &= rmrf_contents(misc_ce_path); + if (flags & android::os::IVold::STORAGE_FLAG_CE) { + auto misc_ce_path = android::vold::BuildDataMiscCePath(volume_uuid, user_id); + res &= rmrf_contents(misc_ce_path); + if (volume_uuid.empty()) { auto vendor_ce_path = android::vold::BuildDataVendorCePath(user_id); res &= rmrf_contents(vendor_ce_path); } - if (flags & android::os::IVold::STORAGE_FLAG_DE) { - auto misc_de_path = android::vold::BuildDataMiscDePath(user_id); - res &= rmrf_contents(misc_de_path); + } + if (flags & android::os::IVold::STORAGE_FLAG_DE) { + auto misc_de_path = android::vold::BuildDataMiscDePath(volume_uuid, user_id); + res &= rmrf_contents(misc_de_path); + if (volume_uuid.empty()) { auto vendor_de_path = android::vold::BuildDataVendorDePath(user_id); res &= rmrf_contents(vendor_de_path); } From 1269ae8d6844d111fa58a280f7f11e21a56d8935 Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Fri, 11 Mar 2022 21:12:44 +0000 Subject: [PATCH 07/12] Disable fuse-bpf ag/17002484 does not disable fuse-bpf as hoped when the device has once booted with fuse-bpf enabled, since the persistent property persists Change name of property as read to disable feature regardless of current state Bug: 221892618 Ignore-AOSP-First: This change has topic dependencies. aosp/2022395 will be merged right after this one. Test: fuse-bpf is disabled even if persist.sys.fuse.bpf.enable is true Change-Id: I423d05d24809b097d02ca5845ab16283edc953b0 --- Utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Utils.h b/Utils.h index 029b35d..71eb5eb 100644 --- a/Utils.h +++ b/Utils.h @@ -37,7 +37,7 @@ namespace vold { static const char* kVoldAppDataIsolationEnabled = "persist.sys.vold_app_data_isolation_enabled"; static const char* kExternalStorageSdcardfs = "external_storage.sdcardfs.enabled"; -static const char* kFuseBpfEnabled = "persist.sys.fuse.bpf.enable"; +static const char* kFuseBpfEnabled = "persist.sys.fuse.bpf.override"; static constexpr std::chrono::seconds kUntrustedFsckSleepTime(45); From 583ae3e55da03f0f1d80f6067cf74471bdeda180 Mon Sep 17 00:00:00 2001 From: Alessio Balsini Date: Wed, 23 Mar 2022 18:27:59 +0000 Subject: [PATCH 08/12] FUSE-BPF: use both ro and persist properties persist.sys.fuse.bpf.enable and ro.fuse.bpf.enabled are both used to decide if FUSE-BPF must be enabled or not. - ro.fuse.bpf.enabled is a read-only property that is set in the device makefile and would allow dogfooding devices to turn the feature on/off. - persist.sys.fuse.bpf.enable is a system property that overrides ro.fuse.bpf.enabled and can only be set manually during the development to simplify the testing of FUSE-BPF, mostly to compare if those tests that are failing with FUSE-BPF were failing also without the feature. Bug: 202785178 Test: adb logcat | grep "FuseDaemon" | grep BPF Ignore-AOSP-First: FUSE-BPF is not available in AOSP Signed-off-by: Alessio Balsini Change-Id: I23f9d27172907f6c72c73bea22e4a7e0ac643888 --- Utils.cpp | 10 ++++++++++ Utils.h | 3 ++- model/EmulatedVolume.cpp | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Utils.cpp b/Utils.cpp index ba6afd8..e8049ed 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -1765,5 +1765,15 @@ std::pair OpenDirInProcfs(std::string_vie return {std::move(fd), std::move(linkPath)}; } +bool IsFuseBpfEnabled() { + std::string bpf_override = android::base::GetProperty("persist.sys.fuse.bpf.override", ""); + if (bpf_override == "true") { + return true; + } else if (bpf_override == "false") { + return false; + } + return base::GetBoolProperty("ro.fuse.bpf.enabled", false); +} + } // namespace vold } // namespace android diff --git a/Utils.h b/Utils.h index 71eb5eb..429669b 100644 --- a/Utils.h +++ b/Utils.h @@ -37,7 +37,6 @@ namespace vold { static const char* kVoldAppDataIsolationEnabled = "persist.sys.vold_app_data_isolation_enabled"; static const char* kExternalStorageSdcardfs = "external_storage.sdcardfs.enabled"; -static const char* kFuseBpfEnabled = "persist.sys.fuse.bpf.override"; static constexpr std::chrono::seconds kUntrustedFsckSleepTime(45); @@ -206,6 +205,8 @@ status_t UnmountUserFuse(userid_t userId, const std::string& absolute_lower_path status_t PrepareAndroidDirs(const std::string& volumeRoot); +bool IsFuseBpfEnabled(); + // Open a given directory as an FD, and return that and the corresponding procfs virtual // symlink path that can be used in any API that accepts a path string. Path stays valid until // the directory FD is closed. diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 7c8a4e0..270d097 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -49,7 +49,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mRawPath = rawPath; mLabel = "emulated"; mFuseMounted = false; - mFuseBpfEnabled = base::GetBoolProperty(kFuseBpfEnabled, false); + mFuseBpfEnabled = IsFuseBpfEnabled(); mUseSdcardFs = IsSdcardfsUsed(); mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mRawPath = rawPath; mLabel = fsUuid; mFuseMounted = false; - mFuseBpfEnabled = base::GetBoolProperty(kFuseBpfEnabled, false); + mFuseBpfEnabled = IsFuseBpfEnabled(); mUseSdcardFs = IsSdcardfsUsed(); mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } From c193c3fbb827e3aa53b65e5e3dbd96fc11b75701 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 4 May 2022 04:39:50 +0000 Subject: [PATCH 09/12] Enforce that internal storage is prepared first Before doing anything else in fscrypt_prepare_user_storage(), error out if adoptable storage is being prepared before internal storage. Without this explicit check, making this mistake results in a sequence of weird errors that is hard to trace back to the actual problem. Bug: 231387956 Change-Id: Ib26cc1bd46ffa2578f6f0156dfacc5496dae3178 (cherry picked from commit c66c2e306dd9c253f5d25c377e6133909d44b86d) Merged-In: Ib26cc1bd46ffa2578f6f0156dfacc5496dae3178 --- FsCrypt.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/FsCrypt.cpp b/FsCrypt.cpp index 42df78b..6c08177 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -812,6 +812,23 @@ bool fscrypt_prepare_user_storage(const std::string& volume_uuid, userid_t user_ LOG(DEBUG) << "fscrypt_prepare_user_storage for volume " << escape_empty(volume_uuid) << ", user " << user_id << ", serial " << serial << ", flags " << flags; + // Internal storage must be prepared before adoptable storage, since the + // user's volume keys are stored in their internal storage. + if (!volume_uuid.empty()) { + if ((flags & android::os::IVold::STORAGE_FLAG_DE) && + !android::vold::pathExists(android::vold::BuildDataMiscDePath("", user_id))) { + LOG(ERROR) << "Cannot prepare DE storage for user " << user_id << " on volume " + << volume_uuid << " before internal storage"; + return false; + } + if ((flags & android::os::IVold::STORAGE_FLAG_CE) && + !android::vold::pathExists(android::vold::BuildDataMiscCePath("", user_id))) { + LOG(ERROR) << "Cannot prepare CE storage for user " << user_id << " on volume " + << volume_uuid << " before internal storage"; + return false; + } + } + if (flags & android::os::IVold::STORAGE_FLAG_DE) { // DE_sys key auto system_legacy_path = android::vold::BuildDataSystemLegacyPath(user_id); From 7667d64ab88a28c6c21a7363b3a2ea1407df644e Mon Sep 17 00:00:00 2001 From: Daeho Jeong Date: Wed, 18 May 2022 09:03:15 -0700 Subject: [PATCH 10/12] vold: fix the range of stopped state of idleMaint When it is stuck in runDevGC() for an unexpected reason, now it cannot prevent the other callers from entering into it again. Fix it. Bug: 232297944 Test: run "sm idle-maint run" twice & check whether they are overlapped Signed-off-by: Daeho Jeong Change-Id: I785c8aeebd8fcf58c34d9be9968d99634d0b420a Merged-In: I785c8aeebd8fcf58c34d9be9968d99634d0b420a --- IdleMaint.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/IdleMaint.cpp b/IdleMaint.cpp index 2bfe3d9..426be58 100644 --- a/IdleMaint.cpp +++ b/IdleMaint.cpp @@ -450,17 +450,17 @@ int RunIdleMaint(bool needGC, const android::sp& stopGc(paths); } + if (!gc_aborted) { + Trim(nullptr); + runDevGc(); + } + lk.lock(); idle_maint_stat = IdleMaintStats::kStopped; lk.unlock(); cv_stop.notify_one(); - if (!gc_aborted) { - Trim(nullptr); - runDevGc(); - } - if (listener) { android::os::PersistableBundle extras; listener->onFinished(0, extras); From 11a666b0925efda40282a5d9df52fca93c787c6b Mon Sep 17 00:00:00 2001 From: Zim Date: Thu, 19 May 2022 16:53:22 +0100 Subject: [PATCH 11/12] Abort FUSE as part of volume reset This fixes a bug in Android T where MediaProvider leaked FUSE fds in it's process preveventing it from dying after being killed. This resulted in the MP in a zombie state. Even though, this bug was more prevalent in Android T due to a change in the Parcel lifecycle (see b/233216232), this bug could have always occurred in theory. This fix should be harmless since after volume reset, all FUSE volumes should be unmounted and aborting the FUSE connections will either no-op or actually prevent the FUSE daemon from getting wedged in a zombie state. Test: Manually trigger a FUSE fd leak in the MediaProvider, kill it and verify that it is restarted without zombie. Bug: 233216232 Bug: 231792374 Change-Id: I9e559a48b9a72e6ecbc3a277a09ea5d34c9ec499 Merged-In: I9e559a48b9a72e6ecbc3a277a09ea5d34c9ec499 --- VolumeManager.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/VolumeManager.cpp b/VolumeManager.cpp index bc556ef..a7d39c1 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -914,6 +914,10 @@ int VolumeManager::reset() { updateVirtualDisk(); mAddedUsers.clear(); mStartedUsers.clear(); + + // Abort all FUSE connections to avoid deadlocks if the FUSE daemon was killed + // with FUSE fds open. + abortFuse(); return 0; } From d96b2ac076f0d82d3c2068cf4dda134bedb11dfe Mon Sep 17 00:00:00 2001 From: Daeho Jeong Date: Fri, 17 Jun 2022 11:06:37 -0700 Subject: [PATCH 12/12] Use sysfs control for storage device GC Sometimes, waiting for the HAL makes infinite calls to HAL and ending up with power consuming issues. While tracking the root cause, we will temporally turn off HAL for storage device GC. Bug: 235470321 Test: run "sm idle-maint run" Ignore-AOSP-First: This is a temporal fix for Android TM devices. Signed-off-by: Daeho Jeong Change-Id: Ieb371b7fdebfe938206a45547bb24dfbf2c2e7be --- IdleMaint.cpp | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/IdleMaint.cpp b/IdleMaint.cpp index 426be58..9d3450a 100644 --- a/IdleMaint.cpp +++ b/IdleMaint.cpp @@ -391,28 +391,6 @@ static void runDevGcOnHal(Service service, GcCallbackImpl cb, GetDescription get } static void runDevGc(void) { - auto aidl_service_name = AStorage::descriptor + "/default"s; - if (AServiceManager_isDeclared(aidl_service_name.c_str())) { - ndk::SpAIBinder binder(AServiceManager_waitForService(aidl_service_name.c_str())); - if (binder.get() != nullptr) { - std::shared_ptr aidl_service = AStorage::fromBinder(binder); - if (aidl_service != nullptr) { - runDevGcOnHal(aidl_service, ndk::SharedRefBase::make(), - &ndk::ScopedAStatus::getDescription); - return; - } - } - LOG(WARNING) << "Device declares " << aidl_service_name - << " but it is not running, skip dev GC on AIDL HAL"; - return; - } - auto hidl_service = HStorage::getService(); - if (hidl_service != nullptr) { - runDevGcOnHal(hidl_service, sp(new HGcCallbackImpl()), - &Return::description); - return; - } - // fallback to legacy code path runDevGcFstab(); }