From 02efdf55d2cd8224d98b3df4180184542c7672d7 Mon Sep 17 00:00:00 2001 From: Narayan Kamath Date: Wed, 27 Nov 2019 10:53:51 +0000 Subject: [PATCH] VolumeManager: limit the scope of remountUid post fork. We want to be sure we're not allocating memory, holding locks or otherwise preventing the child process from making progress. This is a temporary fix of limited scope. In the medium term, it would be preferable to exec a binary that performs this work for us as soon as we fork. Test: manual Bug: 141678467 Change-Id: I57dbd9b3c887aa27e2dd609abf0ad43c66f4ef2a --- Android.bp | 1 + VolumeManager.cpp | 103 +++++++++++++++++++++++++++++----------------- 2 files changed, 67 insertions(+), 37 deletions(-) diff --git a/Android.bp b/Android.bp index 5d93bfa..1c800fe 100644 --- a/Android.bp +++ b/Android.bp @@ -28,6 +28,7 @@ cc_defaults { name: "vold_default_libs", static_libs: [ + "libasync_safe", "libavb", "libbootloader_message", "libdm", diff --git a/VolumeManager.cpp b/VolumeManager.cpp index 3b7e6e1..f1cd232 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -516,6 +517,51 @@ int VolumeManager::setPrimary(const std::shared_ptr& return 0; } +// This code is executed after a fork so it's very important that the set of +// methods we call here is strictly limited. +// +// TODO: Get rid of this guesswork altogether and instead exec a process +// immediately after fork to do our bindding for us. +static bool childProcess(const char* storageSource, const char* userSource, int nsFd, + struct dirent* de) { + if (setns(nsFd, CLONE_NEWNS) != 0) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to setns for %s :%s", de->d_name, + strerror(errno)); + return false; + } + + // NOTE: Inlined from vold::UnmountTree here to avoid using PLOG methods and + // to also protect against future changes that may cause issues across a + // fork. + if (TEMP_FAILURE_RETRY(umount2("/storage/", MNT_DETACH)) < 0 && errno != EINVAL && + errno != ENOENT) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to unmount /storage/ :%s", + strerror(errno)); + return false; + } + + if (TEMP_FAILURE_RETRY(mount(storageSource, "/storage", NULL, MS_BIND | MS_REC, NULL)) == -1) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s for %s :%s", + storageSource, de->d_name, strerror(errno)); + return false; + } + + if (TEMP_FAILURE_RETRY(mount(NULL, "/storage", NULL, MS_REC | MS_SLAVE, NULL)) == -1) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", + "Failed to set MS_SLAVE to /storage for %s :%s", de->d_name, + strerror(errno)); + return false; + } + + if (TEMP_FAILURE_RETRY(mount(userSource, "/storage/self", NULL, MS_BIND, NULL)) == -1) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s for %s :%s", + userSource, de->d_name, strerror(errno)); + return false; + } + + return true; +} + int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { if (GetBoolProperty(android::vold::kPropFuseSnapshot, false)) { // TODO(135341433): Implement fuse specific logic. @@ -557,6 +603,8 @@ int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { int nsFd; struct stat sb; pid_t child; + std::string userSource; + std::string storageSource; static bool apexUpdatable = android::sysprop::ApexProperties::updatable().value_or(false); @@ -632,47 +680,28 @@ int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { goto next; } - if (!(child = fork())) { - if (setns(nsFd, CLONE_NEWNS) != 0) { - PLOG(ERROR) << "Failed to setns for " << de->d_name; - _exit(1); - } + if (mode == "default") { + storageSource = "/mnt/runtime/default"; + } else if (mode == "read") { + storageSource = "/mnt/runtime/read"; + } else if (mode == "write") { + storageSource = "/mnt/runtime/write"; + } else if (mode == "full") { + storageSource = "/mnt/runtime/full"; + } else { + // Sane default of no storage visible. No need to fork a child + // to remount uid. + goto next; + } - android::vold::UnmountTree("/storage/"); - - std::string storageSource; - if (mode == "default") { - storageSource = "/mnt/runtime/default"; - } else if (mode == "read") { - storageSource = "/mnt/runtime/read"; - } else if (mode == "write") { - storageSource = "/mnt/runtime/write"; - } else if (mode == "full") { - storageSource = "/mnt/runtime/full"; - } else { - // Sane default of no storage visible + // Mount user-specific symlink helper into place + userSource = StringPrintf("/mnt/user/%d", multiuser_get_user_id(uid)); + if (!(child = fork())) { + if (childProcess(storageSource.c_str(), userSource.c_str(), nsFd, de)) { _exit(0); - } - if (TEMP_FAILURE_RETRY( - mount(storageSource.c_str(), "/storage", NULL, MS_BIND | MS_REC, NULL)) == -1) { - PLOG(ERROR) << "Failed to mount " << storageSource << " for " << de->d_name; - _exit(1); - } - if (TEMP_FAILURE_RETRY(mount(NULL, "/storage", NULL, MS_REC | MS_SLAVE, NULL)) == -1) { - PLOG(ERROR) << "Failed to set MS_SLAVE to /storage for " << de->d_name; - _exit(1); - } - - // Mount user-specific symlink helper into place - userid_t user_id = multiuser_get_user_id(uid); - std::string userSource(StringPrintf("/mnt/user/%d", user_id)); - if (TEMP_FAILURE_RETRY( - mount(userSource.c_str(), "/storage/self", NULL, MS_BIND, NULL)) == -1) { - PLOG(ERROR) << "Failed to mount " << userSource << " for " << de->d_name; + } else { _exit(1); } - - _exit(0); } if (child == -1) {