From d87b6018d25cbbd33b345dc58c634718bf5d0def Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Tue, 4 Apr 2023 18:41:13 +0000 Subject: [PATCH 01/13] libprocessgroup: Add sendSignalToProcessGroup Add a function which sends signals to all members of a process group, but does not wait for the processes to exit, or for the associated cgroup to be removed. Bug: 274646058 Ignore-AOSP-First: Dependency of ActivityManager change which developed on interal git_master Test: Force-stop of chrome with 15 tabs completes ~500ms faster Test: Full Play store update causes no ANR Change-Id: I37dbdecb3394101abbee8495e71f6912b3c031f5 --- libprocessgroup/include/processgroup/processgroup.h | 5 +++++ libprocessgroup/processgroup.cpp | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 8fa9fd552328..48bc0b7f32e1 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -76,6 +76,11 @@ int killProcessGroup(uid_t uid, int initialPid, int signal, int* max_processes = // that it only returns 0 in the case that the cgroup exists and it contains no processes. int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_processes = nullptr); +// Sends the provided signal to all members of a process group, but does not wait for processes to +// exit, or for the cgroup to be removed. Callers should also ensure that killProcessGroup is called +// later to ensure the cgroup is fully removed, otherwise system resources may leak. +int sendSignalToProcessGroup(uid_t uid, int initialPid, int signal); + int createProcessGroup(uid_t uid, int initialPid, bool memControl = false); // Set various properties of a process group. For these functions to work, the process group must diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index ae9914d645ee..a02159401f88 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -542,6 +542,15 @@ int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_process return KillProcessGroup(uid, initialPid, signal, 0 /*retries*/, max_processes); } +int sendSignalToProcessGroup(uid_t uid, int initialPid, int signal) { + std::string hierarchy_root_path; + if (CgroupsAvailable()) { + CgroupGetControllerPath(CGROUPV2_CONTROLLER_NAME, &hierarchy_root_path); + } + const char* cgroup = hierarchy_root_path.c_str(); + return DoKillProcessGroupOnce(cgroup, uid, initialPid, signal); +} + static int createProcessGroupInternal(uid_t uid, int initialPid, std::string cgroup, bool activate_controllers) { auto uid_path = ConvertUidToPath(cgroup.c_str(), uid); From 0e01ffa6924d7eae988dbf16a2e280c9065cac85 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Fri, 23 Jun 2023 11:04:17 +0100 Subject: [PATCH 02/13] Make libfstab available to APEXes. The ART module needs this library to determine whether to put dexopt artifacts in dalvik-cache. Bug: 287958783 Test: m (cherry picked from https://android-review.googlesource.com/q/commit:cf16f4d794a7162e6f418d2308741c6b45806f01) Merged-In: Idf338702d4f54e9c40c0692ea29e7d83e91aca38 Change-Id: Idf338702d4f54e9c40c0692ea29e7d83e91aca38 --- fs_mgr/Android.bp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs_mgr/Android.bp b/fs_mgr/Android.bp index dd612720f26d..bbd068bacc19 100644 --- a/fs_mgr/Android.bp +++ b/fs_mgr/Android.bp @@ -181,6 +181,10 @@ cc_library_static { ramdisk_available: true, vendor_ramdisk_available: true, recovery_available: true, + apex_available: [ + "//apex_available:anyapex", + "//apex_available:platform", + ], host_supported: true, defaults: ["fs_mgr_defaults"], srcs: [ @@ -206,6 +210,7 @@ cc_library_static { "libbase_headers", "libgsi_headers", ], + min_sdk_version: "31", } cc_binary { From 5d4c782b1fedfbf339e7e8a3b5547dd5b29a9526 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Wed, 28 Jun 2023 21:28:27 +0100 Subject: [PATCH 03/13] Add a variant of ReadFstabFromFile for /proc/mounts. The variant excludes the code that is not for /proc/mounts, and therefore saves code size when being called. Also, after this change, the call to `SkipMountingPartitions` is skipped for /proc/mounts because it is not needed. Bug: 287958783 Test: atest CtsFsMgrTestCases (cherry picked from https://android-review.googlesource.com/q/commit:29ad6c2aa27384fbd44fbf520528e90ae9ce0c34) Merged-In: Ie243257fa2e87e666be7decf97ec36c806bc4524 Change-Id: Ie243257fa2e87e666be7decf97ec36c806bc4524 --- fs_mgr/fs_mgr_fstab.cpp | 35 ++++++++++++++++++++---------- fs_mgr/include_fstab/fstab/fstab.h | 1 + 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/fs_mgr/fs_mgr_fstab.cpp b/fs_mgr/fs_mgr_fstab.cpp index 598a3d2ecb4d..da9e45f4e9a2 100644 --- a/fs_mgr/fs_mgr_fstab.cpp +++ b/fs_mgr/fs_mgr_fstab.cpp @@ -51,6 +51,7 @@ namespace fs_mgr { namespace { constexpr char kDefaultAndroidDtDir[] = "/proc/device-tree/firmware/android"; +constexpr char kProcMountsPath[] = "/proc/mounts"; struct FlagList { const char *name; @@ -697,9 +698,7 @@ void EnableMandatoryFlags(Fstab* fstab) { } } -bool ReadFstabFromFile(const std::string& path, Fstab* fstab_out) { - const bool is_proc_mounts = (path == "/proc/mounts"); - +static bool ReadFstabFromFileCommon(const std::string& path, Fstab* fstab_out) { std::string fstab_str; if (!android::base::ReadFileToString(path, &fstab_str, /* follow_symlinks = */ true)) { PERROR << __FUNCTION__ << "(): failed to read file: '" << path << "'"; @@ -707,11 +706,22 @@ bool ReadFstabFromFile(const std::string& path, Fstab* fstab_out) { } Fstab fstab; - if (!ParseFstabFromString(fstab_str, is_proc_mounts, &fstab)) { + if (!ParseFstabFromString(fstab_str, path == kProcMountsPath, &fstab)) { LERROR << __FUNCTION__ << "(): failed to load fstab from : '" << path << "'"; return false; } - if (!is_proc_mounts) { + + EnableMandatoryFlags(&fstab); + + *fstab_out = std::move(fstab); + return true; +} + +bool ReadFstabFromFile(const std::string& path, Fstab* fstab) { + if (!ReadFstabFromFileCommon(path, fstab)) { + return false; + } + if (path != kProcMountsPath) { if (!access(android::gsi::kGsiBootedIndicatorFile, F_OK)) { std::string dsu_slot; if (!android::gsi::GetActiveDsu(&dsu_slot)) { @@ -723,20 +733,23 @@ bool ReadFstabFromFile(const std::string& path, Fstab* fstab_out) { PERROR << __FUNCTION__ << "(): failed to read DSU LP names"; return false; } - TransformFstabForDsu(&fstab, dsu_slot, Split(lp_names, ",")); + TransformFstabForDsu(fstab, dsu_slot, Split(lp_names, ",")); } else if (errno != ENOENT) { PERROR << __FUNCTION__ << "(): failed to access() DSU booted indicator"; return false; } - } - - SkipMountingPartitions(&fstab, false /* verbose */); - EnableMandatoryFlags(&fstab); - *fstab_out = std::move(fstab); + SkipMountingPartitions(fstab, false /* verbose */); + } return true; } +bool ReadFstabFromProcMounts(Fstab* fstab) { + // Don't call `ReadFstabFromFile` because the code for `path != kProcMountsPath` has an extra + // code size cost, even if it's never executed. + return ReadFstabFromFileCommon(kProcMountsPath, fstab); +} + // Returns fstab entries parsed from the device tree if they exist bool ReadFstabFromDt(Fstab* fstab, bool verbose) { std::string fstab_buf = ReadFstabFromDt(); diff --git a/fs_mgr/include_fstab/fstab/fstab.h b/fs_mgr/include_fstab/fstab/fstab.h index a914b53bb39c..9cb1546c5aaf 100644 --- a/fs_mgr/include_fstab/fstab/fstab.h +++ b/fs_mgr/include_fstab/fstab/fstab.h @@ -101,6 +101,7 @@ std::string GetFstabPath(); bool SkipMountWithConfig(const std::string& skip_config, Fstab* fstab, bool verbose); bool ReadFstabFromFile(const std::string& path, Fstab* fstab); +bool ReadFstabFromProcMounts(Fstab* fstab); bool ReadFstabFromDt(Fstab* fstab, bool verbose = true); bool ReadDefaultFstab(Fstab* fstab); bool SkipMountingPartitions(Fstab* fstab, bool verbose = false); From 8bb751dc8a1499f24add9e0831ab347feb2c3878 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 17 Feb 2023 01:20:52 -0800 Subject: [PATCH 04/13] snapuserd: Split the server into two classes. This splits server into two classes: one that handles the IPC requests, and one that managers snapshot handlers. This will allow embedding of the snapshot handling code, without booting up a server. It'll also make testing much easier. The handler code has been further placed behind a virtual interface, though this is likely unnecessary unless we start testing the server logic itself (or attempt to unify the S and T+ versions of snapuserd). Bug: 269361087 Test: ota Ignore-AOSP-First: cherry-pick from aosp Change-Id: I3ca3ad9c8c1fb910a1c7bf9f44165e2f44c930c9 Merged-In: I3ca3ad9c8c1fb910a1c7bf9f44165e2f44c930c9 --- fs_mgr/libsnapshot/snapuserd/Android.bp | 1 + .../snapuserd/snapuserd_daemon.cpp | 2 +- .../user-space-merge/handler_manager.cpp | 371 ++++++++++++++++++ .../user-space-merge/handler_manager.h | 131 +++++++ .../user-space-merge/snapuserd_server.cpp | 338 +--------------- .../user-space-merge/snapuserd_server.h | 58 +-- 6 files changed, 526 insertions(+), 375 deletions(-) create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp create mode 100644 fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h diff --git a/fs_mgr/libsnapshot/snapuserd/Android.bp b/fs_mgr/libsnapshot/snapuserd/Android.bp index dd11a33daf28..65f3d6d297ed 100644 --- a/fs_mgr/libsnapshot/snapuserd/Android.bp +++ b/fs_mgr/libsnapshot/snapuserd/Android.bp @@ -62,6 +62,7 @@ cc_library_static { "dm-snapshot-merge/snapuserd_worker.cpp", "dm-snapshot-merge/snapuserd_readahead.cpp", "snapuserd_buffer.cpp", + "user-space-merge/handler_manager.cpp", "user-space-merge/snapuserd_core.cpp", "user-space-merge/snapuserd_dm_user.cpp", "user-space-merge/snapuserd_merge.cpp", diff --git a/fs_mgr/libsnapshot/snapuserd/snapuserd_daemon.cpp b/fs_mgr/libsnapshot/snapuserd/snapuserd_daemon.cpp index ae20e1fa479e..36dad3343d3e 100644 --- a/fs_mgr/libsnapshot/snapuserd/snapuserd_daemon.cpp +++ b/fs_mgr/libsnapshot/snapuserd/snapuserd_daemon.cpp @@ -114,7 +114,7 @@ bool Daemon::StartServerForUserspaceSnapshots(int arg_start, int argc, char** ar return false; } auto handler = user_server_.AddHandler(parts[0], parts[1], parts[2], parts[3]); - if (!handler || !user_server_.StartHandler(handler)) { + if (!handler || !user_server_.StartHandler(parts[0])) { return false; } } diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp new file mode 100644 index 000000000000..c5150c411d02 --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp @@ -0,0 +1,371 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "handler_manager.h" + +#include + +#include + +#include "snapuserd_core.h" + +namespace android { +namespace snapshot { + +static constexpr uint8_t kMaxMergeThreads = 2; + +void HandlerThread::FreeResources() { + // Each worker thread holds a reference to snapuserd. + // Clear them so that all the resources + // held by snapuserd is released + if (snapuserd_) { + snapuserd_->FreeResources(); + snapuserd_ = nullptr; + } +} + +SnapshotHandlerManager::SnapshotHandlerManager() { + monitor_merge_event_fd_.reset(eventfd(0, EFD_CLOEXEC)); + if (monitor_merge_event_fd_ == -1) { + PLOG(FATAL) << "monitor_merge_event_fd_: failed to create eventfd"; + } +} + +std::shared_ptr SnapshotHandlerManager::AddHandler( + const std::string& misc_name, const std::string& cow_device_path, + const std::string& backing_device, const std::string& base_path_merge, + int num_worker_threads, bool use_iouring, bool perform_verification) { + auto snapuserd = std::make_shared(misc_name, cow_device_path, backing_device, + base_path_merge, num_worker_threads, + use_iouring, perform_verification); + if (!snapuserd->InitCowDevice()) { + LOG(ERROR) << "Failed to initialize Snapuserd"; + return nullptr; + } + + if (!snapuserd->InitializeWorkers()) { + LOG(ERROR) << "Failed to initialize workers"; + return nullptr; + } + + auto handler = std::make_shared(snapuserd); + { + std::lock_guard lock(lock_); + if (FindHandler(&lock, misc_name) != dm_users_.end()) { + LOG(ERROR) << "Handler already exists: " << misc_name; + return nullptr; + } + dm_users_.push_back(handler); + } + return handler; +} + +bool SnapshotHandlerManager::StartHandler(const std::string& misc_name) { + std::lock_guard lock(lock_); + auto iter = FindHandler(&lock, misc_name); + if (iter == dm_users_.end()) { + LOG(ERROR) << "Could not find handler: " << misc_name; + return false; + } + if (!(*iter)->snapuserd() || (*iter)->snapuserd()->IsAttached()) { + LOG(ERROR) << "Tried to re-attach control device: " << misc_name; + return false; + } + if (!StartHandler(*iter)) { + return false; + } + return true; +} + +bool SnapshotHandlerManager::StartHandler(const std::shared_ptr& handler) { + if (handler->snapuserd()->IsAttached()) { + LOG(ERROR) << "Handler already attached"; + return false; + } + + handler->snapuserd()->AttachControlDevice(); + + handler->thread() = std::thread(std::bind(&SnapshotHandlerManager::RunThread, this, handler)); + return true; +} + +bool SnapshotHandlerManager::DeleteHandler(const std::string& misc_name) { + { + std::lock_guard lock(lock_); + auto iter = FindHandler(&lock, misc_name); + if (iter == dm_users_.end()) { + // After merge is completed, we swap dm-user table with + // the underlying dm-linear base device. Hence, worker + // threads would have terminted and was removed from + // the list. + LOG(DEBUG) << "Could not find handler: " << misc_name; + return true; + } + + if (!(*iter)->ThreadTerminated()) { + (*iter)->snapuserd()->NotifyIOTerminated(); + } + } + if (!RemoveAndJoinHandler(misc_name)) { + return false; + } + return true; +} + +void SnapshotHandlerManager::RunThread(std::shared_ptr handler) { + LOG(INFO) << "Entering thread for handler: " << handler->misc_name(); + + if (!handler->snapuserd()->Start()) { + LOG(ERROR) << " Failed to launch all worker threads"; + } + + handler->snapuserd()->CloseFds(); + bool merge_completed = handler->snapuserd()->CheckMergeCompletionStatus(); + handler->snapuserd()->UnmapBufferRegion(); + + auto misc_name = handler->misc_name(); + LOG(INFO) << "Handler thread about to exit: " << misc_name; + + { + std::lock_guard lock(lock_); + if (merge_completed) { + num_partitions_merge_complete_ += 1; + active_merge_threads_ -= 1; + WakeupMonitorMergeThread(); + } + handler->SetThreadTerminated(); + auto iter = FindHandler(&lock, handler->misc_name()); + if (iter == dm_users_.end()) { + // RemoveAndJoinHandler() already removed us from the list, and is + // now waiting on a join(), so just return. Additionally, release + // all the resources held by snapuserd object which are shared + // by worker threads. This should be done when the last reference + // of "handler" is released; but we will explicitly release here + // to make sure snapuserd object is freed as it is the biggest + // consumer of memory in the daemon. + handler->FreeResources(); + LOG(INFO) << "Exiting handler thread to allow for join: " << misc_name; + return; + } + + LOG(INFO) << "Exiting handler thread and freeing resources: " << misc_name; + + if (handler->snapuserd()->IsAttached()) { + handler->thread().detach(); + } + + // Important: free resources within the lock. This ensures that if + // WaitForDelete() is called, the handler is either in the list, or + // it's not and its resources are guaranteed to be freed. + handler->FreeResources(); + dm_users_.erase(iter); + } +} + +bool SnapshotHandlerManager::InitiateMerge(const std::string& misc_name) { + std::lock_guard lock(lock_); + auto iter = FindHandler(&lock, misc_name); + if (iter == dm_users_.end()) { + LOG(ERROR) << "Could not find handler: " << misc_name; + return false; + } + + return StartMerge(&lock, *iter); +} + +bool SnapshotHandlerManager::StartMerge(std::lock_guard* proof_of_lock, + const std::shared_ptr& handler) { + CHECK(proof_of_lock); + + if (!handler->snapuserd()->IsAttached()) { + LOG(ERROR) << "Handler not attached to dm-user - Merge thread cannot be started"; + return false; + } + + handler->snapuserd()->MonitorMerge(); + + if (!is_merge_monitor_started_) { + std::thread(&SnapshotHandlerManager::MonitorMerge, this).detach(); + is_merge_monitor_started_ = true; + } + + merge_handlers_.push(handler); + WakeupMonitorMergeThread(); + return true; +} + +void SnapshotHandlerManager::WakeupMonitorMergeThread() { + uint64_t notify = 1; + ssize_t rc = TEMP_FAILURE_RETRY(write(monitor_merge_event_fd_.get(), ¬ify, sizeof(notify))); + if (rc < 0) { + PLOG(FATAL) << "failed to notify monitor merge thread"; + } +} + +void SnapshotHandlerManager::MonitorMerge() { + while (!stop_monitor_merge_thread_) { + uint64_t testVal; + ssize_t ret = + TEMP_FAILURE_RETRY(read(monitor_merge_event_fd_.get(), &testVal, sizeof(testVal))); + if (ret == -1) { + PLOG(FATAL) << "Failed to read from eventfd"; + } else if (ret == 0) { + LOG(FATAL) << "Hit EOF on eventfd"; + } + + LOG(INFO) << "MonitorMerge: active-merge-threads: " << active_merge_threads_; + { + std::lock_guard lock(lock_); + while (active_merge_threads_ < kMaxMergeThreads && merge_handlers_.size() > 0) { + auto handler = merge_handlers_.front(); + merge_handlers_.pop(); + + if (!handler->snapuserd()) { + LOG(INFO) << "MonitorMerge: skipping deleted handler: " << handler->misc_name(); + continue; + } + + LOG(INFO) << "Starting merge for partition: " + << handler->snapuserd()->GetMiscName(); + handler->snapuserd()->InitiateMerge(); + active_merge_threads_ += 1; + } + } + } + + LOG(INFO) << "Exiting MonitorMerge: size: " << merge_handlers_.size(); +} + +std::string SnapshotHandlerManager::GetMergeStatus(const std::string& misc_name) { + std::lock_guard lock(lock_); + auto iter = FindHandler(&lock, misc_name); + if (iter == dm_users_.end()) { + LOG(ERROR) << "Could not find handler: " << misc_name; + return {}; + } + + return (*iter)->snapuserd()->GetMergeStatus(); +} + +double SnapshotHandlerManager::GetMergePercentage() { + std::lock_guard lock(lock_); + + double percentage = 0.0; + int n = 0; + + for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { + auto& th = (*iter)->thread(); + if (th.joinable()) { + // Merge percentage by individual partitions wherein merge is still + // in-progress + percentage += (*iter)->snapuserd()->GetMergePercentage(); + n += 1; + } + } + + // Calculate final merge including those partitions where merge was already + // completed - num_partitions_merge_complete_ will track them when each + // thread exists in RunThread. + int total_partitions = n + num_partitions_merge_complete_; + + if (total_partitions) { + percentage = ((num_partitions_merge_complete_ * 100.0) + percentage) / total_partitions; + } + + LOG(DEBUG) << "Merge %: " << percentage + << " num_partitions_merge_complete_: " << num_partitions_merge_complete_ + << " total_partitions: " << total_partitions << " n: " << n; + return percentage; +} + +bool SnapshotHandlerManager::GetVerificationStatus() { + std::lock_guard lock(lock_); + + bool status = true; + for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { + auto& th = (*iter)->thread(); + if (th.joinable() && status) { + status = (*iter)->snapuserd()->CheckPartitionVerification() && status; + } else { + // return immediately if there is a failure + return false; + } + } + + return status; +} + +bool SnapshotHandlerManager::RemoveAndJoinHandler(const std::string& misc_name) { + std::shared_ptr handler; + { + std::lock_guard lock(lock_); + + auto iter = FindHandler(&lock, misc_name); + if (iter == dm_users_.end()) { + // Client already deleted. + return true; + } + handler = std::move(*iter); + dm_users_.erase(iter); + } + + auto& th = handler->thread(); + if (th.joinable()) { + th.join(); + } + return true; +} + +void SnapshotHandlerManager::TerminateMergeThreads() { + std::lock_guard guard(lock_); + + for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { + if (!(*iter)->ThreadTerminated()) { + (*iter)->snapuserd()->NotifyIOTerminated(); + } + } +} + +void SnapshotHandlerManager::JoinAllThreads() { + // Acquire the thread list within the lock. + std::vector> dm_users; + { + std::lock_guard guard(lock_); + dm_users = std::move(dm_users_); + } + + for (auto& client : dm_users) { + auto& th = client->thread(); + + if (th.joinable()) th.join(); + } + + stop_monitor_merge_thread_ = true; + WakeupMonitorMergeThread(); +} + +auto SnapshotHandlerManager::FindHandler(std::lock_guard* proof_of_lock, + const std::string& misc_name) -> HandlerList::iterator { + CHECK(proof_of_lock); + + for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { + if ((*iter)->misc_name() == misc_name) { + return iter; + } + } + return dm_users_.end(); +} + +} // namespace snapshot +} // namespace android diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h new file mode 100644 index 000000000000..b7ddac19f28a --- /dev/null +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h @@ -0,0 +1,131 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include +#include +#include + +#include + +namespace android { +namespace snapshot { + +class SnapshotHandler; + +class HandlerThread { + public: + explicit HandlerThread(std::shared_ptr snapuserd); + + void FreeResources(); + const std::shared_ptr& snapuserd() const { return snapuserd_; } + std::thread& thread() { return thread_; } + + const std::string& misc_name() const { return misc_name_; } + bool ThreadTerminated() { return thread_terminated_; } + void SetThreadTerminated() { thread_terminated_ = true; } + + private: + std::thread thread_; + std::shared_ptr snapuserd_; + std::string misc_name_; + bool thread_terminated_ = false; +}; + +class ISnapshotHandlerManager { + public: + virtual ~ISnapshotHandlerManager() {} + + // Add a new snapshot handler but do not start serving requests yet. + virtual std::shared_ptr AddHandler(const std::string& misc_name, + const std::string& cow_device_path, + const std::string& backing_device, + const std::string& base_path_merge, + int num_worker_threads, bool use_iouring, + bool perform_verification) = 0; + + // Start serving requests on a snapshot handler. + virtual bool StartHandler(const std::string& misc_name) = 0; + + // Stop serving requests on a snapshot handler and remove it. + virtual bool DeleteHandler(const std::string& misc_name) = 0; + + // Begin merging blocks on the given snapshot handler. + virtual bool InitiateMerge(const std::string& misc_name) = 0; + + // Return a string containing a status code indicating the merge status + // on the handler. Returns empty on error. + virtual std::string GetMergeStatus(const std::string& misc_name) = 0; + + // Wait until all handlers have terminated. + virtual void JoinAllThreads() = 0; + + // Stop any in-progress merge threads. + virtual void TerminateMergeThreads() = 0; + + // Returns the merge progress across all merging snapshot handlers. + virtual double GetMergePercentage() = 0; + + // Returns whether all snapshots have verified. + virtual bool GetVerificationStatus() = 0; +}; + +class SnapshotHandlerManager final : public ISnapshotHandlerManager { + public: + SnapshotHandlerManager(); + std::shared_ptr AddHandler(const std::string& misc_name, + const std::string& cow_device_path, + const std::string& backing_device, + const std::string& base_path_merge, + int num_worker_threads, bool use_iouring, + bool perform_verification) override; + bool StartHandler(const std::string& misc_name) override; + bool DeleteHandler(const std::string& misc_name) override; + bool InitiateMerge(const std::string& misc_name) override; + std::string GetMergeStatus(const std::string& misc_name) override; + void JoinAllThreads() override; + void TerminateMergeThreads() override; + double GetMergePercentage() override; + bool GetVerificationStatus() override; + + private: + bool StartHandler(const std::shared_ptr& handler); + void RunThread(std::shared_ptr handler); + bool StartMerge(std::lock_guard* proof_of_lock, + const std::shared_ptr& handler); + void MonitorMerge(); + void WakeupMonitorMergeThread(); + bool RemoveAndJoinHandler(const std::string& misc_name); + + // Find a HandlerThread within a lock. + using HandlerList = std::vector>; + HandlerList::iterator FindHandler(std::lock_guard* proof_of_lock, + const std::string& misc_name); + + std::mutex lock_; + HandlerList dm_users_; + + bool is_merge_monitor_started_ = false; + bool stop_monitor_merge_thread_ = false; + int active_merge_threads_ = 0; + int num_partitions_merge_complete_ = 0; + std::queue> merge_handlers_; + android::base::unique_fd monitor_merge_event_fd_; +}; + +} // namespace snapshot +} // namespace android diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp index b7ce2109adfe..cc8115f5f800 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -62,11 +62,8 @@ DaemonOps UserSnapshotServer::Resolveop(std::string& input) { } UserSnapshotServer::UserSnapshotServer() { - monitor_merge_event_fd_.reset(eventfd(0, EFD_CLOEXEC)); - if (monitor_merge_event_fd_ == -1) { - PLOG(FATAL) << "monitor_merge_event_fd_: failed to create eventfd"; - } terminating_ = false; + handlers_ = std::make_unique(); } UserSnapshotServer::~UserSnapshotServer() { @@ -99,7 +96,7 @@ void UserSnapshotServer::Parsemsg(std::string const& msg, const char delim, void UserSnapshotServer::ShutdownThreads() { terminating_ = true; - JoinAllThreads(); + handlers_->JoinAllThreads(); } HandlerThread::HandlerThread(std::shared_ptr snapuserd) @@ -166,17 +163,7 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st return Sendmsg(fd, "fail"); } - std::lock_guard lock(lock_); - auto iter = FindHandler(&lock, out[1]); - if (iter == dm_users_.end()) { - LOG(ERROR) << "Could not find handler: " << out[1]; - return Sendmsg(fd, "fail"); - } - if (!(*iter)->snapuserd() || (*iter)->snapuserd()->IsAttached()) { - LOG(ERROR) << "Tried to re-attach control device: " << out[1]; - return Sendmsg(fd, "fail"); - } - if (!StartHandler(*iter)) { + if (!handlers_->StartHandler(out[1])) { return Sendmsg(fd, "fail"); } return Sendmsg(fd, "success"); @@ -209,30 +196,13 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; return Sendmsg(fd, "fail"); } - { - std::lock_guard lock(lock_); - auto iter = FindHandler(&lock, out[1]); - if (iter == dm_users_.end()) { - // After merge is completed, we swap dm-user table with - // the underlying dm-linear base device. Hence, worker - // threads would have terminted and was removed from - // the list. - LOG(DEBUG) << "Could not find handler: " << out[1]; - return Sendmsg(fd, "success"); - } - - if (!(*iter)->ThreadTerminated()) { - (*iter)->snapuserd()->NotifyIOTerminated(); - } - } - if (!RemoveAndJoinHandler(out[1])) { + if (!handlers_->DeleteHandler(out[1])) { return Sendmsg(fd, "fail"); } return Sendmsg(fd, "success"); } case DaemonOps::DETACH: { - std::lock_guard lock(lock_); - TerminateMergeThreads(&lock); + handlers_->TerminateMergeThreads(); terminating_ = true; return true; } @@ -252,24 +222,15 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st return Sendmsg(fd, "fail"); } if (out[0] == "initiate_merge") { - std::lock_guard lock(lock_); - auto iter = FindHandler(&lock, out[1]); - if (iter == dm_users_.end()) { - LOG(ERROR) << "Could not find handler: " << out[1]; + if (!handlers_->InitiateMerge(out[1])) { return Sendmsg(fd, "fail"); } - - if (!StartMerge(&lock, *iter)) { - return Sendmsg(fd, "fail"); - } - return Sendmsg(fd, "success"); } return Sendmsg(fd, "fail"); } case DaemonOps::PERCENTAGE: { - std::lock_guard lock(lock_); - double percentage = GetMergePercentage(&lock); + double percentage = handlers_->GetMergePercentage(); return Sendmsg(fd, std::to_string(percentage)); } @@ -280,24 +241,16 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; return Sendmsg(fd, "snapshot-merge-failed"); } - { - std::lock_guard lock(lock_); - auto iter = FindHandler(&lock, out[1]); - if (iter == dm_users_.end()) { - LOG(ERROR) << "Could not find handler: " << out[1]; - return Sendmsg(fd, "snapshot-merge-failed"); - } - - std::string merge_status = GetMergeStatus(*iter); - return Sendmsg(fd, merge_status); + auto status = handlers_->GetMergeStatus(out[1]); + if (status.empty()) { + return Sendmsg(fd, "snapshot-merge-failed"); } + return Sendmsg(fd, status); } case DaemonOps::UPDATE_VERIFY: { - std::lock_guard lock(lock_); - if (!UpdateVerification(&lock)) { + if (!handlers_->GetVerificationStatus()) { return Sendmsg(fd, "fail"); } - return Sendmsg(fd, "success"); } default: { @@ -308,56 +261,6 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st } } -void UserSnapshotServer::RunThread(std::shared_ptr handler) { - LOG(INFO) << "Entering thread for handler: " << handler->misc_name(); - - if (!handler->snapuserd()->Start()) { - LOG(ERROR) << " Failed to launch all worker threads"; - } - - handler->snapuserd()->CloseFds(); - bool merge_completed = handler->snapuserd()->CheckMergeCompletionStatus(); - handler->snapuserd()->UnmapBufferRegion(); - - auto misc_name = handler->misc_name(); - LOG(INFO) << "Handler thread about to exit: " << misc_name; - - { - std::lock_guard lock(lock_); - if (merge_completed) { - num_partitions_merge_complete_ += 1; - active_merge_threads_ -= 1; - WakeupMonitorMergeThread(); - } - handler->SetThreadTerminated(); - auto iter = FindHandler(&lock, handler->misc_name()); - if (iter == dm_users_.end()) { - // RemoveAndJoinHandler() already removed us from the list, and is - // now waiting on a join(), so just return. Additionally, release - // all the resources held by snapuserd object which are shared - // by worker threads. This should be done when the last reference - // of "handler" is released; but we will explicitly release here - // to make sure snapuserd object is freed as it is the biggest - // consumer of memory in the daemon. - handler->FreeResources(); - LOG(INFO) << "Exiting handler thread to allow for join: " << misc_name; - return; - } - - LOG(INFO) << "Exiting handler thread and freeing resources: " << misc_name; - - if (handler->snapuserd()->IsAttached()) { - handler->thread().detach(); - } - - // Important: free resources within the lock. This ensures that if - // WaitForDelete() is called, the handler is either in the list, or - // it's not and its resources are guaranteed to be freed. - handler->FreeResources(); - dm_users_.erase(iter); - } -} - bool UserSnapshotServer::Start(const std::string& socketname) { bool start_listening = true; @@ -423,28 +326,10 @@ bool UserSnapshotServer::Run() { } } - JoinAllThreads(); + handlers_->JoinAllThreads(); return true; } -void UserSnapshotServer::JoinAllThreads() { - // Acquire the thread list within the lock. - std::vector> dm_users; - { - std::lock_guard guard(lock_); - dm_users = std::move(dm_users_); - } - - for (auto& client : dm_users) { - auto& th = client->thread(); - - if (th.joinable()) th.join(); - } - - stop_monitor_merge_thread_ = true; - WakeupMonitorMergeThread(); -} - void UserSnapshotServer::AddWatchedFd(android::base::borrowed_fd fd, int events) { struct pollfd p = {}; p.fd = fd.get(); @@ -506,185 +391,13 @@ std::shared_ptr UserSnapshotServer::AddHandler(const std::string& perform_verification = false; } - auto snapuserd = std::make_shared(misc_name, cow_device_path, backing_device, - base_path_merge, num_worker_threads, - io_uring_enabled_, perform_verification); - if (!snapuserd->InitCowDevice()) { - LOG(ERROR) << "Failed to initialize Snapuserd"; - return nullptr; - } - - if (!snapuserd->InitializeWorkers()) { - LOG(ERROR) << "Failed to initialize workers"; - return nullptr; - } - - auto handler = std::make_shared(snapuserd); - { - std::lock_guard lock(lock_); - if (FindHandler(&lock, misc_name) != dm_users_.end()) { - LOG(ERROR) << "Handler already exists: " << misc_name; - return nullptr; - } - dm_users_.push_back(handler); - } - return handler; -} - -bool UserSnapshotServer::StartHandler(const std::shared_ptr& handler) { - if (handler->snapuserd()->IsAttached()) { - LOG(ERROR) << "Handler already attached"; - return false; - } - - handler->snapuserd()->AttachControlDevice(); - - handler->thread() = std::thread(std::bind(&UserSnapshotServer::RunThread, this, handler)); - return true; -} - -bool UserSnapshotServer::StartMerge(std::lock_guard* proof_of_lock, - const std::shared_ptr& handler) { - CHECK(proof_of_lock); - - if (!handler->snapuserd()->IsAttached()) { - LOG(ERROR) << "Handler not attached to dm-user - Merge thread cannot be started"; - return false; - } - - handler->snapuserd()->MonitorMerge(); - - if (!is_merge_monitor_started_.has_value()) { - std::thread(&UserSnapshotServer::MonitorMerge, this).detach(); - is_merge_monitor_started_ = true; - } - - merge_handlers_.push(handler); - WakeupMonitorMergeThread(); - return true; -} - -auto UserSnapshotServer::FindHandler(std::lock_guard* proof_of_lock, - const std::string& misc_name) -> HandlerList::iterator { - CHECK(proof_of_lock); - - for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { - if ((*iter)->misc_name() == misc_name) { - return iter; - } - } - return dm_users_.end(); -} - -void UserSnapshotServer::TerminateMergeThreads(std::lock_guard* proof_of_lock) { - CHECK(proof_of_lock); - - for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { - if (!(*iter)->ThreadTerminated()) { - (*iter)->snapuserd()->NotifyIOTerminated(); - } - } -} - -std::string UserSnapshotServer::GetMergeStatus(const std::shared_ptr& handler) { - return handler->snapuserd()->GetMergeStatus(); -} - -double UserSnapshotServer::GetMergePercentage(std::lock_guard* proof_of_lock) { - CHECK(proof_of_lock); - double percentage = 0.0; - int n = 0; - - for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { - auto& th = (*iter)->thread(); - if (th.joinable()) { - // Merge percentage by individual partitions wherein merge is still - // in-progress - percentage += (*iter)->snapuserd()->GetMergePercentage(); - n += 1; - } - } - - // Calculate final merge including those partitions where merge was already - // completed - num_partitions_merge_complete_ will track them when each - // thread exists in RunThread. - int total_partitions = n + num_partitions_merge_complete_; - - if (total_partitions) { - percentage = ((num_partitions_merge_complete_ * 100.0) + percentage) / total_partitions; - } - - LOG(DEBUG) << "Merge %: " << percentage - << " num_partitions_merge_complete_: " << num_partitions_merge_complete_ - << " total_partitions: " << total_partitions << " n: " << n; - return percentage; -} - -bool UserSnapshotServer::RemoveAndJoinHandler(const std::string& misc_name) { - std::shared_ptr handler; - { - std::lock_guard lock(lock_); - - auto iter = FindHandler(&lock, misc_name); - if (iter == dm_users_.end()) { - // Client already deleted. - return true; - } - handler = std::move(*iter); - dm_users_.erase(iter); - } - - auto& th = handler->thread(); - if (th.joinable()) { - th.join(); - } - return true; -} - -void UserSnapshotServer::WakeupMonitorMergeThread() { - uint64_t notify = 1; - ssize_t rc = TEMP_FAILURE_RETRY(write(monitor_merge_event_fd_.get(), ¬ify, sizeof(notify))); - if (rc < 0) { - PLOG(FATAL) << "failed to notify monitor merge thread"; - } -} - -void UserSnapshotServer::MonitorMerge() { - while (!stop_monitor_merge_thread_) { - uint64_t testVal; - ssize_t ret = - TEMP_FAILURE_RETRY(read(monitor_merge_event_fd_.get(), &testVal, sizeof(testVal))); - if (ret == -1) { - PLOG(FATAL) << "Failed to read from eventfd"; - } else if (ret == 0) { - LOG(FATAL) << "Hit EOF on eventfd"; - } - - LOG(INFO) << "MonitorMerge: active-merge-threads: " << active_merge_threads_; - { - std::lock_guard lock(lock_); - while (active_merge_threads_ < kMaxMergeThreads && merge_handlers_.size() > 0) { - auto handler = merge_handlers_.front(); - merge_handlers_.pop(); - - if (!handler->snapuserd()) { - LOG(INFO) << "MonitorMerge: skipping deleted handler: " << handler->misc_name(); - continue; - } - - LOG(INFO) << "Starting merge for partition: " - << handler->snapuserd()->GetMiscName(); - handler->snapuserd()->InitiateMerge(); - active_merge_threads_ += 1; - } - } - } - - LOG(INFO) << "Exiting MonitorMerge: size: " << merge_handlers_.size(); + return handlers_->AddHandler(misc_name, cow_device_path, backing_device, base_path_merge, + num_worker_threads, io_uring_enabled_, perform_verification); } bool UserSnapshotServer::WaitForSocket() { - auto scope_guard = android::base::make_scope_guard([this]() -> void { JoinAllThreads(); }); + auto scope_guard = + android::base::make_scope_guard([this]() -> void { handlers_->JoinAllThreads(); }); auto socket_path = ANDROID_SOCKET_DIR "/"s + kSnapuserdSocketProxy; @@ -781,21 +494,8 @@ bool UserSnapshotServer::RunForSocketHandoff() { return true; } -bool UserSnapshotServer::UpdateVerification(std::lock_guard* proof_of_lock) { - CHECK(proof_of_lock); - - bool status = true; - for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) { - auto& th = (*iter)->thread(); - if (th.joinable() && status) { - status = (*iter)->snapuserd()->CheckPartitionVerification() && status; - } else { - // return immediately if there is a failure - return false; - } - } - - return status; +bool UserSnapshotServer::StartHandler(const std::string& misc_name) { + return handlers_->StartHandler(misc_name); } } // namespace snapshot diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h index 12c3903c495c..ae5190391dfa 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h @@ -31,6 +31,7 @@ #include #include +#include "handler_manager.h" #include "snapuserd_core.h" namespace android { @@ -54,33 +55,6 @@ enum class DaemonOps { INVALID, }; -class HandlerThread { - public: - explicit HandlerThread(std::shared_ptr snapuserd); - - void FreeResources() { - // Each worker thread holds a reference to snapuserd. - // Clear them so that all the resources - // held by snapuserd is released - if (snapuserd_) { - snapuserd_->FreeResources(); - snapuserd_ = nullptr; - } - } - const std::shared_ptr& snapuserd() const { return snapuserd_; } - std::thread& thread() { return thread_; } - - const std::string& misc_name() const { return misc_name_; } - bool ThreadTerminated() { return thread_terminated_; } - void SetThreadTerminated() { thread_terminated_ = true; } - - private: - std::thread thread_; - std::shared_ptr snapuserd_; - std::string misc_name_; - bool thread_terminated_ = false; -}; - class UserSnapshotServer { private: android::base::unique_fd sockfd_; @@ -88,21 +62,12 @@ class UserSnapshotServer { volatile bool received_socket_signal_ = false; std::vector watched_fds_; bool is_socket_present_ = false; - int num_partitions_merge_complete_ = 0; - int active_merge_threads_ = 0; - bool stop_monitor_merge_thread_ = false; bool is_server_running_ = false; bool io_uring_enabled_ = false; - std::optional is_merge_monitor_started_; - - android::base::unique_fd monitor_merge_event_fd_; + std::unique_ptr handlers_; std::mutex lock_; - using HandlerList = std::vector>; - HandlerList dm_users_; - std::queue> merge_handlers_; - void AddWatchedFd(android::base::borrowed_fd fd, int events); void AcceptClient(); bool HandleClient(android::base::borrowed_fd fd, int revents); @@ -111,28 +76,15 @@ class UserSnapshotServer { bool Receivemsg(android::base::borrowed_fd fd, const std::string& str); void ShutdownThreads(); - bool RemoveAndJoinHandler(const std::string& control_device); DaemonOps Resolveop(std::string& input); std::string GetDaemonStatus(); void Parsemsg(std::string const& msg, const char delim, std::vector& out); bool IsTerminating() { return terminating_; } - void RunThread(std::shared_ptr handler); - void MonitorMerge(); - void JoinAllThreads(); bool StartWithSocket(bool start_listening); - // Find a HandlerThread within a lock. - HandlerList::iterator FindHandler(std::lock_guard* proof_of_lock, - const std::string& misc_name); - - double GetMergePercentage(std::lock_guard* proof_of_lock); - void TerminateMergeThreads(std::lock_guard* proof_of_lock); - - bool UpdateVerification(std::lock_guard* proof_of_lock); - public: UserSnapshotServer(); ~UserSnapshotServer(); @@ -147,12 +99,8 @@ class UserSnapshotServer { const std::string& cow_device_path, const std::string& backing_device, const std::string& base_path_merge); - bool StartHandler(const std::shared_ptr& handler); - bool StartMerge(std::lock_guard* proof_of_lock, - const std::shared_ptr& handler); - std::string GetMergeStatus(const std::shared_ptr& handler); + bool StartHandler(const std::string& misc_name); - void WakeupMonitorMergeThread(); void SetTerminating() { terminating_ = true; } void ReceivedSocketSignal() { received_socket_signal_ = true; } void SetServerRunning() { is_server_running_ = true; } From f84f9c319e5a05fa6bfed9730ea6b9d28407a84b Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 14 Mar 2023 09:09:48 -0700 Subject: [PATCH 05/13] snapuserd: Remove DaemonOps. These are only used for a single switch statement. Just compare the input strings instead. Bug: 269361087 Test: builds, ota applies Ignore-AOSP-First: cherry-pick from aosp Change-Id: Ib90ae55309ea99c181585c64ed2ac814e5ed6c72 Merged-In: Ib90ae55309ea99c181585c64ed2ac814e5ed6c72 --- .../user-space-merge/snapuserd_server.cpp | 218 ++++++++---------- .../user-space-merge/snapuserd_server.h | 16 -- 2 files changed, 94 insertions(+), 140 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp index cc8115f5f800..d87990aca2a8 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -45,22 +45,6 @@ using namespace std::string_literals; using android::base::borrowed_fd; using android::base::unique_fd; -DaemonOps UserSnapshotServer::Resolveop(std::string& input) { - if (input == "init") return DaemonOps::INIT; - if (input == "start") return DaemonOps::START; - if (input == "stop") return DaemonOps::STOP; - if (input == "query") return DaemonOps::QUERY; - if (input == "delete") return DaemonOps::DELETE; - if (input == "detach") return DaemonOps::DETACH; - if (input == "supports") return DaemonOps::SUPPORTS; - if (input == "initiate_merge") return DaemonOps::INITIATE; - if (input == "merge_percent") return DaemonOps::PERCENTAGE; - if (input == "getstatus") return DaemonOps::GETSTATUS; - if (input == "update-verify") return DaemonOps::UPDATE_VERIFY; - - return DaemonOps::INVALID; -} - UserSnapshotServer::UserSnapshotServer() { terminating_ = false; handlers_ = std::make_unique(); @@ -132,132 +116,118 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st std::vector out; Parsemsg(str, delim, out); - DaemonOps op = Resolveop(out[0]); - - switch (op) { - case DaemonOps::INIT: { - // Message format: - // init,,,, - // - // Reads the metadata and send the number of sectors - if (out.size() != 5) { - LOG(ERROR) << "Malformed init message, " << out.size() << " parts"; - return Sendmsg(fd, "fail"); - } - auto handler = AddHandler(out[1], out[2], out[3], out[4]); - if (!handler) { - return Sendmsg(fd, "fail"); - } - - auto retval = "success," + std::to_string(handler->snapuserd()->GetNumSectors()); - return Sendmsg(fd, retval); + const auto& cmd = out[0]; + if (cmd == "init") { + // Message format: + // init,,,, + // + // Reads the metadata and send the number of sectors + if (out.size() != 5) { + LOG(ERROR) << "Malformed init message, " << out.size() << " parts"; + return Sendmsg(fd, "fail"); } - case DaemonOps::START: { - // Message format: - // start, - // - // Start the new thread which binds to dm-user misc device - if (out.size() != 2) { - LOG(ERROR) << "Malformed start message, " << out.size() << " parts"; - return Sendmsg(fd, "fail"); - } - if (!handlers_->StartHandler(out[1])) { - return Sendmsg(fd, "fail"); - } - return Sendmsg(fd, "success"); - } - case DaemonOps::STOP: { - // Message format: stop - // - // Stop all the threads gracefully and then shutdown the - // main thread - SetTerminating(); - ShutdownThreads(); - return true; + auto handler = AddHandler(out[1], out[2], out[3], out[4]); + if (!handler) { + return Sendmsg(fd, "fail"); } - case DaemonOps::QUERY: { - // Message format: query - // - // As part of transition, Second stage daemon will be - // created before terminating the first stage daemon. Hence, - // for a brief period client may have to distiguish between - // first stage daemon and second stage daemon. - // - // Second stage daemon is marked as active and hence will - // be ready to receive control message. - return Sendmsg(fd, GetDaemonStatus()); + + auto retval = "success," + std::to_string(handler->snapuserd()->GetNumSectors()); + return Sendmsg(fd, retval); + } else if (cmd == "start") { + // Message format: + // start, + // + // Start the new thread which binds to dm-user misc device + if (out.size() != 2) { + LOG(ERROR) << "Malformed start message, " << out.size() << " parts"; + return Sendmsg(fd, "fail"); } - case DaemonOps::DELETE: { - // Message format: - // delete, - if (out.size() != 2) { - LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; - return Sendmsg(fd, "fail"); - } - if (!handlers_->DeleteHandler(out[1])) { - return Sendmsg(fd, "fail"); - } - return Sendmsg(fd, "success"); + + if (!handlers_->StartHandler(out[1])) { + return Sendmsg(fd, "fail"); } - case DaemonOps::DETACH: { - handlers_->TerminateMergeThreads(); - terminating_ = true; - return true; + return Sendmsg(fd, "success"); + } else if (cmd == "stop") { + // Message format: stop + // + // Stop all the threads gracefully and then shutdown the + // main thread + SetTerminating(); + ShutdownThreads(); + return true; + } else if (cmd == "query") { + // Message format: query + // + // As part of transition, Second stage daemon will be + // created before terminating the first stage daemon. Hence, + // for a brief period client may have to distiguish between + // first stage daemon and second stage daemon. + // + // Second stage daemon is marked as active and hence will + // be ready to receive control message. + return Sendmsg(fd, GetDaemonStatus()); + } else if (cmd == "delete") { + // Message format: + // delete, + if (out.size() != 2) { + LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; + return Sendmsg(fd, "fail"); } - case DaemonOps::SUPPORTS: { - if (out.size() != 2) { - LOG(ERROR) << "Malformed supports message, " << out.size() << " parts"; - return Sendmsg(fd, "fail"); - } - if (out[1] == "second_stage_socket_handoff") { - return Sendmsg(fd, "success"); - } + if (!handlers_->DeleteHandler(out[1])) { return Sendmsg(fd, "fail"); } - case DaemonOps::INITIATE: { - if (out.size() != 2) { - LOG(ERROR) << "Malformed initiate-merge message, " << out.size() << " parts"; - return Sendmsg(fd, "fail"); - } - if (out[0] == "initiate_merge") { - if (!handlers_->InitiateMerge(out[1])) { - return Sendmsg(fd, "fail"); - } - return Sendmsg(fd, "success"); - } + return Sendmsg(fd, "success"); + } else if (cmd == "detach") { + handlers_->TerminateMergeThreads(); + terminating_ = true; + return true; + } else if (cmd == "supports") { + if (out.size() != 2) { + LOG(ERROR) << "Malformed supports message, " << out.size() << " parts"; return Sendmsg(fd, "fail"); } - case DaemonOps::PERCENTAGE: { - double percentage = handlers_->GetMergePercentage(); - - return Sendmsg(fd, std::to_string(percentage)); + if (out[1] == "second_stage_socket_handoff") { + return Sendmsg(fd, "success"); } - case DaemonOps::GETSTATUS: { - // Message format: - // getstatus, - if (out.size() != 2) { - LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; - return Sendmsg(fd, "snapshot-merge-failed"); - } - auto status = handlers_->GetMergeStatus(out[1]); - if (status.empty()) { - return Sendmsg(fd, "snapshot-merge-failed"); - } - return Sendmsg(fd, status); + return Sendmsg(fd, "fail"); + } else if (cmd == "initiate_merge") { + if (out.size() != 2) { + LOG(ERROR) << "Malformed initiate-merge message, " << out.size() << " parts"; + return Sendmsg(fd, "fail"); } - case DaemonOps::UPDATE_VERIFY: { - if (!handlers_->GetVerificationStatus()) { + if (out[0] == "initiate_merge") { + if (!handlers_->InitiateMerge(out[1])) { return Sendmsg(fd, "fail"); } return Sendmsg(fd, "success"); } - default: { - LOG(ERROR) << "Received unknown message type from client"; - Sendmsg(fd, "fail"); - return false; + return Sendmsg(fd, "fail"); + } else if (cmd == "merge_percent") { + double percentage = handlers_->GetMergePercentage(); + return Sendmsg(fd, std::to_string(percentage)); + } else if (cmd == "getstatus") { + // Message format: + // getstatus, + if (out.size() != 2) { + LOG(ERROR) << "Malformed delete message, " << out.size() << " parts"; + return Sendmsg(fd, "snapshot-merge-failed"); + } + auto status = handlers_->GetMergeStatus(out[1]); + if (status.empty()) { + return Sendmsg(fd, "snapshot-merge-failed"); } + return Sendmsg(fd, status); + } else if (cmd == "update-verify") { + if (!handlers_->GetVerificationStatus()) { + return Sendmsg(fd, "fail"); + } + return Sendmsg(fd, "success"); + } else { + LOG(ERROR) << "Received unknown message type from client"; + Sendmsg(fd, "fail"); + return false; } } diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h index ae5190391dfa..988c01a99ba3 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h @@ -40,21 +40,6 @@ namespace snapshot { static constexpr uint32_t kMaxPacketSize = 512; static constexpr uint8_t kMaxMergeThreads = 2; -enum class DaemonOps { - INIT, - START, - QUERY, - STOP, - DELETE, - DETACH, - SUPPORTS, - INITIATE, - PERCENTAGE, - GETSTATUS, - UPDATE_VERIFY, - INVALID, -}; - class UserSnapshotServer { private: android::base::unique_fd sockfd_; @@ -76,7 +61,6 @@ class UserSnapshotServer { bool Receivemsg(android::base::borrowed_fd fd, const std::string& str); void ShutdownThreads(); - DaemonOps Resolveop(std::string& input); std::string GetDaemonStatus(); void Parsemsg(std::string const& msg, const char delim, std::vector& out); From 269e99f9558ae332e10329451fc51dc6698375a0 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 14 Mar 2023 10:57:47 -0700 Subject: [PATCH 06/13] snapuserd: Run snapuserd threads in-process. This removes the temporary daemon and socket and instead directly embeds SnapshotHandlerManager. We can also remove the test flags for io_uring as with this they're no longer necessary. Bug: 269361087 Test: snapuserd_test Ignore-AOSP-First: cherry-pick from aosp Change-Id: I728186d9bc90c98fabd76d3f86569840488ada96 Merged-In: I728186d9bc90c98fabd76d3f86569840488ada96 --- fs_mgr/libsnapshot/snapuserd/Android.bp | 15 ++-- .../user-space-merge/handler_manager.cpp | 3 + .../user-space-merge/snapuserd_core.cpp | 5 -- .../user-space-merge/snapuserd_server.cpp | 3 - .../user-space-merge/snapuserd_test.cpp | 75 ++++++------------- 5 files changed, 33 insertions(+), 68 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/Android.bp b/fs_mgr/libsnapshot/snapuserd/Android.bp index 65f3d6d297ed..9fe567acbc7e 100644 --- a/fs_mgr/libsnapshot/snapuserd/Android.bp +++ b/fs_mgr/libsnapshot/snapuserd/Android.bp @@ -165,7 +165,7 @@ cc_binary { } cc_test { - name: "cow_snapuserd_test", + name: "snapuserd_test_legacy", defaults: [ "fs_mgr_defaults", "libsnapshot_cow_defaults", @@ -217,16 +217,17 @@ cc_test { ], static_libs: [ "libbrotli", - "libgtest", - "libsnapshot_cow", - "libsnapshot_snapuserd", "libcutils_sockets", - "libz", - "libfs_mgr", "libdm", "libext4_utils", - "liburing", + "libfs_mgr", "libgflags", + "libgtest", + "libsnapshot_cow", + "libsnapshot_snapuserd", + "libsnapuserd", + "liburing", + "libz", ], include_dirs: ["bionic/libc/kernel"], header_libs: [ diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp index c5150c411d02..bdba5c0cc3ab 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp @@ -25,6 +25,9 @@ namespace snapshot { static constexpr uint8_t kMaxMergeThreads = 2; +HandlerThread::HandlerThread(std::shared_ptr snapuserd) + : snapuserd_(snapuserd), misc_name_(snapuserd_->GetMiscName()) {} + void HandlerThread::FreeResources() { // Each worker thread holds a reference to snapuserd. // Clear them so that all the resources diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp index c8a5cc584a28..8e1212b7b3ce 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp @@ -421,11 +421,6 @@ bool SnapshotHandler::IsIouringSupported() { struct utsname uts; unsigned int major, minor; - if (android::base::GetBoolProperty("snapuserd.test.io_uring.force_disable", false)) { - SNAP_LOG(INFO) << "io_uring disabled for testing"; - return false; - } - if ((uname(&uts) != 0) || (sscanf(uts.release, "%u.%u", &major, &minor) != 2)) { SNAP_LOG(ERROR) << "Could not parse the kernel version from uname. " << " io_uring not supported"; diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp index d87990aca2a8..c953f1a053bd 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -83,9 +83,6 @@ void UserSnapshotServer::ShutdownThreads() { handlers_->JoinAllThreads(); } -HandlerThread::HandlerThread(std::shared_ptr snapuserd) - : snapuserd_(snapuserd), misc_name_(snapuserd_->GetMiscName()) {} - bool UserSnapshotServer::Sendmsg(android::base::borrowed_fd fd, const std::string& msg) { ssize_t ret = TEMP_FAILURE_RETRY(send(fd.get(), msg.data(), msg.size(), MSG_NOSIGNAL)); if (ret < 0) { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp index ef3689f61f6b..efe0c1443215 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp @@ -37,9 +37,9 @@ #include #include #include -#include #include +#include "handler_manager.h" #include "snapuserd_core.h" DEFINE_string(force_config, "", "Force testing mode with iouring disabled"); @@ -54,8 +54,6 @@ using namespace std::chrono_literals; using namespace android::dm; using namespace std; -static constexpr char kSnapuserdSocketTest[] = "snapuserdTest"; - class Tempdevice { public: Tempdevice(const std::string& name, const DmTable& table) @@ -68,15 +66,15 @@ class Tempdevice { } ~Tempdevice() { if (valid_) { - dm_.DeleteDevice(name_); + dm_.DeleteDeviceIfExists(name_); } } bool Destroy() { if (!valid_) { - return false; + return true; } valid_ = false; - return dm_.DeleteDevice(name_); + return dm_.DeleteDeviceIfExists(name_); } const std::string& path() const { return path_; } const std::string& name() const { return name_; } @@ -139,7 +137,6 @@ class SnapuserdTest : public ::testing::Test { void SetDeviceControlName(); void InitDaemon(); void CreateDmUserDevice(); - void StartSnapuserdDaemon(); unique_ptr base_loop_; unique_ptr dmuser_dev_; @@ -149,9 +146,9 @@ class SnapuserdTest : public ::testing::Test { unique_fd base_fd_; std::unique_ptr cow_system_; - std::unique_ptr client_; std::unique_ptr orig_buffer_; std::unique_ptr merged_buffer_; + SnapshotHandlerManager handlers_; bool setup_ok_ = false; bool merge_ok_ = false; size_t size_ = 100_MiB; @@ -181,9 +178,9 @@ void SnapuserdTest::Shutdown() { ASSERT_TRUE(dmuser_dev_->Destroy()); auto misc_device = "/dev/dm-user/" + system_device_ctrl_name_; - ASSERT_TRUE(client_->WaitForDeviceDelete(system_device_ctrl_name_)); + ASSERT_TRUE(handlers_.DeleteHandler(system_device_ctrl_name_)); ASSERT_TRUE(android::fs_mgr::WaitForFileDeleted(misc_device, 10s)); - ASSERT_TRUE(client_->DetachSnapuserd()); + handlers_.TerminateMergeThreads(); } bool SnapuserdTest::SetupDefault() { @@ -218,8 +215,6 @@ bool SnapuserdTest::SetupCopyOverlap_2() { bool SnapuserdTest::SetupDaemon() { SetDeviceControlName(); - StartSnapuserdDaemon(); - CreateDmUserDevice(); InitCowDevice(); InitDaemon(); @@ -229,20 +224,6 @@ bool SnapuserdTest::SetupDaemon() { return setup_ok_; } -void SnapuserdTest::StartSnapuserdDaemon() { - pid_t pid = fork(); - ASSERT_GE(pid, 0); - if (pid == 0) { - std::string arg0 = "/system/bin/snapuserd"; - std::string arg1 = "-socket="s + kSnapuserdSocketTest; - char* const argv[] = {arg0.data(), arg1.data(), nullptr}; - ASSERT_GE(execv(arg0.c_str(), argv), 0); - } else { - client_ = SnapuserdClient::Connect(kSnapuserdSocketTest, 10s); - ASSERT_NE(client_, nullptr); - } -} - void SnapuserdTest::CreateBaseDevice() { unique_fd rnd_fd; @@ -591,9 +572,17 @@ void SnapuserdTest::CreateCowDevice() { } void SnapuserdTest::InitCowDevice() { - uint64_t num_sectors = client_->InitDmUserCow(system_device_ctrl_name_, cow_system_->path, - base_loop_->device(), base_loop_->device()); - ASSERT_NE(num_sectors, 0); + bool use_iouring = true; + if (FLAGS_force_config == "iouring_disabled") { + use_iouring = false; + } + + auto handler = + handlers_.AddHandler(system_device_ctrl_name_, cow_system_->path, base_loop_->device(), + base_loop_->device(), 1, use_iouring, false); + ASSERT_NE(handler, nullptr); + ASSERT_NE(handler->snapuserd(), nullptr); + ASSERT_NE(handler->snapuserd()->GetNumSectors(), 0); } void SnapuserdTest::SetDeviceControlName() { @@ -631,13 +620,12 @@ void SnapuserdTest::CreateDmUserDevice() { } void SnapuserdTest::InitDaemon() { - bool ok = client_->AttachDmUser(system_device_ctrl_name_); - ASSERT_TRUE(ok); + ASSERT_TRUE(handlers_.StartHandler(system_device_ctrl_name_)); } void SnapuserdTest::CheckMergeCompletion() { while (true) { - double percentage = client_->GetMergePercent(); + double percentage = handlers_.GetMergePercentage(); if ((int)percentage == 100) { break; } @@ -652,8 +640,6 @@ void SnapuserdTest::SetupImpl() { SetDeviceControlName(); - StartSnapuserdDaemon(); - CreateDmUserDevice(); InitCowDevice(); InitDaemon(); @@ -669,8 +655,7 @@ bool SnapuserdTest::Merge() { } void SnapuserdTest::StartMerge() { - bool ok = client_->InitiateMerge(system_device_ctrl_name_); - ASSERT_TRUE(ok); + ASSERT_TRUE(handlers_.InitiateMerge(system_device_ctrl_name_)); } void SnapuserdTest::ValidateMerge() { @@ -684,7 +669,6 @@ void SnapuserdTest::SimulateDaemonRestart() { Shutdown(); std::this_thread::sleep_for(500ms); SetDeviceControlName(); - StartSnapuserdDaemon(); CreateDmUserDevice(); InitCowDevice(); InitDaemon(); @@ -844,20 +828,5 @@ int main(int argc, char** argv) { gflags::ParseCommandLineFlags(&argc, &argv, false); - android::base::SetProperty("ctl.stop", "snapuserd"); - - if (FLAGS_force_config == "iouring_disabled") { - if (!android::base::SetProperty("snapuserd.test.io_uring.force_disable", "1")) { - return testing::AssertionFailure() - << "Failed to disable property: snapuserd.test.io_uring.disabled"; - } - } - - int ret = RUN_ALL_TESTS(); - - if (FLAGS_force_config == "iouring_disabled") { - android::base::SetProperty("snapuserd.test.io_uring.force_disable", "0"); - } - - return ret; + return RUN_ALL_TESTS(); } From 689adfad37bb2fc98fd9b687c3e087901255328e Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Tue, 18 Jul 2023 08:39:10 -0700 Subject: [PATCH 07/13] Listen on property_service_for_system socket for powerctl messages It is easy to dos the property_service socket, since it will wait for a complete data packet from one command before moving on to the next one. To prevent low privilege apps interfering with system and root apps, add a second property_service socket that only they can use. However, since writes to properties are not thread-safe, limit use of this second socket to just sys.powerctl messages. These are the messages that this security issue is concerned about, and they do not actually write to the properties, rather they are acted upon immediately. Bug: 262208935 Test: Builds, boots Ignore-AOSP-First: Security fix Change-Id: I32835de31bb42c91b6479051ddf4b26b5c0b163f --- init/property_service.cpp | 108 ++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 46 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 8da69822ccb9..3e9a6d9f342e 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -57,12 +57,12 @@ #include #include #include +#include #include #include #include #include #include - #include "debug_ramdisk.h" #include "epoll.h" #include "init.h" @@ -111,12 +111,13 @@ constexpr auto API_LEVEL_CURRENT = 10000; static bool persistent_properties_loaded = false; -static int property_set_fd = -1; static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; static std::mutex accept_messages_lock; +static std::mutex selinux_check_access_lock; static std::thread property_service_thread; +static std::thread property_service_for_system_thread; static std::unique_ptr persist_write_thread; @@ -161,6 +162,7 @@ bool CanReadProperty(const std::string& source_context, const std::string& name) ucred cr = {.pid = 0, .uid = 0, .gid = 0}; audit_data.cr = &cr; + auto lock = std::lock_guard{selinux_check_access_lock}; return selinux_check_access(source_context.c_str(), target_context, "file", "read", &audit_data) == 0; } @@ -176,10 +178,9 @@ static bool CheckMacPerms(const std::string& name, const char* target_context, audit_data.name = name.c_str(); audit_data.cr = &cr; - bool has_access = (selinux_check_access(source_context, target_context, "property_service", - "set", &audit_data) == 0); - - return has_access; + auto lock = std::lock_guard{selinux_check_access_lock}; + return selinux_check_access(source_context, target_context, "property_service", "set", + &audit_data) == 0; } void NotifyPropertyChange(const std::string& name, const std::string& value) { @@ -394,31 +395,37 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - prop_info* pi = (prop_info*)__system_property_find(name.c_str()); - if (pi != nullptr) { - // ro.* properties are actually "write-once". - if (StartsWith(name, "ro.")) { - *error = "Read-only property was already set"; - return {PROP_ERROR_READ_ONLY_PROPERTY}; - } - - __system_property_update(pi, value.c_str(), valuelen); + if (name == "sys.powerctl") { + // No action here - NotifyPropertyChange will trigger the appropriate action, and since this + // can come to the second thread, we mustn't call out to the __system_property_* functions + // which support multiple readers but only one mutator. } else { - int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); - if (rc < 0) { - *error = "__system_property_add failed"; - return {PROP_ERROR_SET_FAILED}; + prop_info* pi = (prop_info*)__system_property_find(name.c_str()); + if (pi != nullptr) { + // ro.* properties are actually "write-once". + if (StartsWith(name, "ro.")) { + *error = "Read-only property was already set"; + return {PROP_ERROR_READ_ONLY_PROPERTY}; + } + + __system_property_update(pi, value.c_str(), valuelen); + } else { + int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); + if (rc < 0) { + *error = "__system_property_add failed"; + return {PROP_ERROR_SET_FAILED}; + } } - } - // Don't write properties to disk until after we have read all default - // properties to prevent them from being overwritten by default values. - if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { - if (persist_write_thread) { - persist_write_thread->Write(name, value, std::move(*socket)); - return {}; + // Don't write properties to disk until after we have read all default + // properties to prevent them from being overwritten by default values. + if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { + if (persist_write_thread) { + persist_write_thread->Write(name, value, std::move(*socket)); + return {}; + } + WritePersistentProperty(name, value); } - WritePersistentProperty(name, value); } NotifyPropertyChange(name, value); @@ -579,10 +586,10 @@ uint32_t HandlePropertySetNoSocket(const std::string& name, const std::string& v return *ret; } -static void handle_property_set_fd() { +static void handle_property_set_fd(int fd) { static constexpr uint32_t kDefaultSocketTimeout = 2000; /* ms */ - int s = accept4(property_set_fd, nullptr, nullptr, SOCK_CLOEXEC); + int s = accept4(fd, nullptr, nullptr, SOCK_CLOEXEC); if (s == -1) { return; } @@ -1419,19 +1426,21 @@ static void HandleInitSocket() { } } -static void PropertyServiceThread() { +static void PropertyServiceThread(int fd, bool listen_init) { Epoll epoll; if (auto result = epoll.Open(); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(property_set_fd, handle_property_set_fd); + if (auto result = epoll.RegisterHandler(fd, std::bind(handle_property_set_fd, fd)); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { - LOG(FATAL) << result.error(); + if (listen_init) { + if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { + LOG(FATAL) << result.error(); + } } while (true) { @@ -1482,6 +1491,23 @@ void PersistWriteThread::Write(std::string name, std::string value, SocketConnec cv_.notify_all(); } +void StartThread(const char* name, int mode, int gid, std::thread& t, bool listen_init) { + int fd = -1; + if (auto result = CreateSocket(name, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, + /*passcred=*/false, /*should_listen=*/false, mode, /*uid=*/0, + /*gid=*/gid, /*socketcon=*/{}); + result.ok()) { + fd = *result; + } else { + LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); + } + + listen(fd, 8); + + auto new_thread = std::thread(PropertyServiceThread, fd, listen_init); + t.swap(new_thread); +} + void StartPropertyService(int* epoll_socket) { InitPropertySet("ro.property_service.version", "2"); @@ -1493,19 +1519,9 @@ void StartPropertyService(int* epoll_socket) { init_socket = sockets[1]; StartSendingMessages(); - if (auto result = CreateSocket(PROP_SERVICE_NAME, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, - /*passcred=*/false, /*should_listen=*/false, 0666, /*uid=*/0, - /*gid=*/0, /*socketcon=*/{}); - result.ok()) { - property_set_fd = *result; - } else { - LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); - } - - listen(property_set_fd, 8); - - auto new_thread = std::thread{PropertyServiceThread}; - property_service_thread.swap(new_thread); + StartThread(PROP_SERVICE_FOR_SYSTEM_NAME, 0660, AID_SYSTEM, property_service_for_system_thread, + true); + StartThread(PROP_SERVICE_NAME, 0666, 0, property_service_thread, false); auto async_persist_writes = android::base::GetBoolProperty("ro.property_service.async_persist_writes", false); From f4b1d698b1d3fd4592c68464923912d902460509 Mon Sep 17 00:00:00 2001 From: Yi-Yo Chiang Date: Tue, 25 Jul 2023 22:08:55 +0800 Subject: [PATCH 08/13] init: Unify duplicated get_android_dt_dir with libfs_mgr init and libfs_mgr both defines get_android_dt_dir() with subtle differences. Merge the two implementations into libfs_mgr to reduce code duplication (in terms of source code and code gen) Note: init's implementation checks the kernel cmdline first and then the kernel bootconfig, while libfs_mgr's order is the opposite. Realistically I don't think this order matter much though. If any, we should prioritize bootconfig over kernel cmdline most of the time. Bug: 293695109 Test: Presubmit Change-Id: Ic8d2c965c62f9e873ccdaf77d67c7708f25a7b56 (cherry picked from https://android-review.googlesource.com/q/commit:d7c67b40a9b6a5e72d54adf37da22238381182f7) Ignore-AOSP-First: Fix merge conflict --- fs_mgr/libfstab/boot_config.cpp | 29 +++++++++++++++++++- fs_mgr/libfstab/fstab.cpp | 29 ++++---------------- fs_mgr/libfstab/fstab_priv.h | 1 - fs_mgr/libfstab/include/fstab/fstab.h | 4 +++ init/Android.bp | 1 + init/property_service.cpp | 6 +++-- init/util.cpp | 39 +++++---------------------- init/util.h | 3 --- 8 files changed, 49 insertions(+), 63 deletions(-) diff --git a/fs_mgr/libfstab/boot_config.cpp b/fs_mgr/libfstab/boot_config.cpp index 8fb28c607cb9..fee40158b709 100644 --- a/fs_mgr/libfstab/boot_config.cpp +++ b/fs_mgr/libfstab/boot_config.cpp @@ -27,6 +27,33 @@ #include "fstab_priv.h" #include "logging_macros.h" +namespace android { +namespace fs_mgr { + +const std::string& GetAndroidDtDir() { + // Set once and saves time for subsequent calls to this function + static const std::string kAndroidDtDir = [] { + std::string android_dt_dir; + if ((fs_mgr_get_boot_config_from_bootconfig_source("android_dt_dir", &android_dt_dir) || + fs_mgr_get_boot_config_from_kernel_cmdline("android_dt_dir", &android_dt_dir)) && + !android_dt_dir.empty()) { + // Ensure the returned path ends with a / + if (android_dt_dir.back() != '/') { + android_dt_dir.push_back('/'); + } + } else { + // Fall back to the standard procfs-based path + android_dt_dir = "/proc/device-tree/firmware/android/"; + } + LINFO << "Using Android DT directory " << android_dt_dir; + return android_dt_dir; + }(); + return kAndroidDtDir; +} + +} // namespace fs_mgr +} // namespace android + std::vector> fs_mgr_parse_cmdline(const std::string& cmdline) { static constexpr char quote = '"'; @@ -145,7 +172,7 @@ bool fs_mgr_get_boot_config(const std::string& key, std::string* out_val) { // firstly, check the device tree if (is_dt_compatible()) { - std::string file_name = get_android_dt_dir() + "/" + key; + std::string file_name = android::fs_mgr::GetAndroidDtDir() + key; if (android::base::ReadFileToString(file_name, out_val)) { if (!out_val->empty()) { out_val->pop_back(); // Trims the trailing '\0' out. diff --git a/fs_mgr/libfstab/fstab.cpp b/fs_mgr/libfstab/fstab.cpp index 5b5c3d26c450..86ba03148838 100644 --- a/fs_mgr/libfstab/fstab.cpp +++ b/fs_mgr/libfstab/fstab.cpp @@ -51,7 +51,6 @@ namespace android { namespace fs_mgr { namespace { -constexpr char kDefaultAndroidDtDir[] = "/proc/device-tree/firmware/android"; constexpr char kProcMountsPath[] = "/proc/mounts"; struct FlagList { @@ -337,25 +336,14 @@ bool ParseFsMgrFlags(const std::string& flags, FstabEntry* entry) { return true; } -std::string InitAndroidDtDir() { - std::string android_dt_dir; - // The platform may specify a custom Android DT path in kernel cmdline - if (!fs_mgr_get_boot_config_from_bootconfig_source("android_dt_dir", &android_dt_dir) && - !fs_mgr_get_boot_config_from_kernel_cmdline("android_dt_dir", &android_dt_dir)) { - // Fall back to the standard procfs-based path - android_dt_dir = kDefaultAndroidDtDir; - } - return android_dt_dir; -} - bool IsDtFstabCompatible() { std::string dt_value; - std::string file_name = get_android_dt_dir() + "/fstab/compatible"; + std::string file_name = GetAndroidDtDir() + "fstab/compatible"; if (ReadDtFile(file_name, &dt_value) && dt_value == "android,fstab") { // If there's no status property or its set to "ok" or "okay", then we use the DT fstab. std::string status_value; - std::string status_file_name = get_android_dt_dir() + "/fstab/status"; + std::string status_file_name = GetAndroidDtDir() + "fstab/status"; return !ReadDtFile(status_file_name, &status_value) || status_value == "ok" || status_value == "okay"; } @@ -368,7 +356,7 @@ std::string ReadFstabFromDt() { return {}; } - std::string fstabdir_name = get_android_dt_dir() + "/fstab"; + std::string fstabdir_name = GetAndroidDtDir() + "fstab"; std::unique_ptr fstabdir(opendir(fstabdir_name.c_str()), closedir); if (!fstabdir) return {}; @@ -876,7 +864,7 @@ const FstabEntry* GetEntryForMountPoint(const Fstab* fstab, const std::string& p std::set GetBootDevices() { // First check bootconfig, then kernel commandline, then the device tree - std::string dt_file_name = get_android_dt_dir() + "/boot_devices"; + std::string dt_file_name = GetAndroidDtDir() + "boot_devices"; std::string value; if (fs_mgr_get_boot_config_from_bootconfig_source("boot_devices", &value) || fs_mgr_get_boot_config_from_bootconfig_source("boot_device", &value)) { @@ -948,15 +936,8 @@ bool InRecovery() { } // namespace fs_mgr } // namespace android -// FIXME: The same logic is duplicated in system/core/init/ -const std::string& get_android_dt_dir() { - // Set once and saves time for subsequent calls to this function - static const std::string kAndroidDtDir = android::fs_mgr::InitAndroidDtDir(); - return kAndroidDtDir; -} - bool is_dt_compatible() { - std::string file_name = get_android_dt_dir() + "/compatible"; + std::string file_name = android::fs_mgr::GetAndroidDtDir() + "compatible"; std::string dt_value; if (android::fs_mgr::ReadDtFile(file_name, &dt_value)) { if (dt_value == "android,firmware") { diff --git a/fs_mgr/libfstab/fstab_priv.h b/fs_mgr/libfstab/fstab_priv.h index fb12b9fb1d1c..5d226f8798f6 100644 --- a/fs_mgr/libfstab/fstab_priv.h +++ b/fs_mgr/libfstab/fstab_priv.h @@ -36,7 +36,6 @@ bool fs_mgr_get_boot_config_from_bootconfig(const std::string& bootconfig, const bool fs_mgr_get_boot_config_from_bootconfig_source(const std::string& key, std::string* out_val); bool fs_mgr_update_for_slotselect(android::fs_mgr::Fstab* fstab); -const std::string& get_android_dt_dir(); bool is_dt_compatible(); namespace android { diff --git a/fs_mgr/libfstab/include/fstab/fstab.h b/fs_mgr/libfstab/include/fstab/fstab.h index e0683ac284cb..0a45fe87ad3e 100644 --- a/fs_mgr/libfstab/include/fstab/fstab.h +++ b/fs_mgr/libfstab/include/fstab/fstab.h @@ -124,5 +124,9 @@ std::set GetBootDevices(); // expected name. std::string GetVerityDeviceName(const FstabEntry& entry); +// Returns the Android Device Tree directory as specified in the kernel bootconfig or cmdline. +// If the platform does not configure a custom DT path, returns the standard one (based in procfs). +const std::string& GetAndroidDtDir(); + } // namespace fs_mgr } // namespace android diff --git a/init/Android.bp b/init/Android.bp index ee34215d750e..d4852d6483be 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -539,6 +539,7 @@ cc_defaults { "libprotobuf-cpp-lite", ], static_libs: [ + "libfs_mgr", "libhidl-gen-utils", ], } diff --git a/init/property_service.cpp b/init/property_service.cpp index 3e9a6d9f342e..87ab13b3c3c5 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -57,6 +57,7 @@ #include #include #include +#include #include #include #include @@ -1324,7 +1325,8 @@ static void ProcessKernelDt() { return; } - std::unique_ptr dir(opendir(get_android_dt_dir().c_str()), closedir); + std::unique_ptr dir(opendir(android::fs_mgr::GetAndroidDtDir().c_str()), + closedir); if (!dir) return; std::string dt_file; @@ -1335,7 +1337,7 @@ static void ProcessKernelDt() { continue; } - std::string file_name = get_android_dt_dir() + dp->d_name; + std::string file_name = android::fs_mgr::GetAndroidDtDir() + dp->d_name; android::base::ReadFileToString(file_name, &dt_file); std::replace(dt_file.begin(), dt_file.end(), ',', '.'); diff --git a/init/util.cpp b/init/util.cpp index d0478e80d005..61d59a4801d0 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -42,6 +42,10 @@ #include #include +#if defined(__ANDROID__) +#include +#endif + #ifdef INIT_FULL_SOURCES #include #include @@ -60,8 +64,6 @@ using namespace std::literals::string_literals; namespace android { namespace init { -const std::string kDefaultAndroidDtDir("/proc/device-tree/firmware/android/"); - const std::string kDataDirPrefix("/data/"); void (*trigger_shutdown)(const std::string& command) = nullptr; @@ -375,45 +377,18 @@ Result ExpandProps(const std::string& src) { return dst; } -static std::string init_android_dt_dir() { - // Use the standard procfs-based path by default - std::string android_dt_dir = kDefaultAndroidDtDir; - // The platform may specify a custom Android DT path in kernel cmdline - ImportKernelCmdline([&](const std::string& key, const std::string& value) { - if (key == "androidboot.android_dt_dir") { - android_dt_dir = value; - } - }); - // ..Or bootconfig - if (android_dt_dir == kDefaultAndroidDtDir) { - ImportBootconfig([&](const std::string& key, const std::string& value) { - if (key == "androidboot.android_dt_dir") { - android_dt_dir = value; - } - }); - } - - LOG(INFO) << "Using Android DT directory " << android_dt_dir; - return android_dt_dir; -} - -// FIXME: The same logic is duplicated in system/core/fs_mgr/ -const std::string& get_android_dt_dir() { - // Set once and saves time for subsequent calls to this function - static const std::string kAndroidDtDir = init_android_dt_dir(); - return kAndroidDtDir; -} - // Reads the content of device tree file under the platform's Android DT directory. // Returns true if the read is success, false otherwise. bool read_android_dt_file(const std::string& sub_path, std::string* dt_content) { - const std::string file_name = get_android_dt_dir() + sub_path; +#if defined(__ANDROID__) + const std::string file_name = android::fs_mgr::GetAndroidDtDir() + sub_path; if (android::base::ReadFileToString(file_name, dt_content)) { if (!dt_content->empty()) { dt_content->pop_back(); // Trims the trailing '\0' out. return true; } } +#endif return false; } diff --git a/init/util.h b/init/util.h index 3f0a4e023f08..1c00a3efc0b1 100644 --- a/init/util.h +++ b/init/util.h @@ -60,9 +60,6 @@ bool make_dir(const std::string& path, mode_t mode); bool is_dir(const char* pathname); Result ExpandProps(const std::string& src); -// Returns the platform's Android DT directory as specified in the kernel cmdline. -// If the platform does not configure a custom DT path, returns the standard one (based in procfs). -const std::string& get_android_dt_dir(); // Reads or compares the content of device tree file under the platform's Android DT directory. bool read_android_dt_file(const std::string& sub_path, std::string* dt_content); bool is_android_dt_value_expected(const std::string& sub_path, const std::string& expected_content); From f83c5c8fecf89d9315945368aa20350c2f235cc0 Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Thu, 31 Aug 2023 00:31:35 +0000 Subject: [PATCH 09/13] Add seal if ashmem-dev is backed by memfd Need to seal the buffer size in align with ashmem if set to PROT_READ only to prevent untrusted remote process to shrink the buffer size and crash it. Bug: 294609150 Test: build Ignore-AOSP-First: Security Change-Id: I9288cf30b41e84ad8d3247c204e20482912bff69 --- libcutils/ashmem-dev.cpp | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/libcutils/ashmem-dev.cpp b/libcutils/ashmem-dev.cpp index 6a27f9a2004b..56d68759f348 100644 --- a/libcutils/ashmem-dev.cpp +++ b/libcutils/ashmem-dev.cpp @@ -349,6 +349,12 @@ static int memfd_create_region(const char* name, size_t size) { return -1; } + // forbid size changes to match ashmem behaviour + if (fcntl(fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK) == -1) { + ALOGE("memfd_create(%s, %zd) F_ADD_SEALS failed: %m", name, size); + return -1; + } + if (debug_log) { ALOGE("memfd_create(%s, %zd) success. fd=%d\n", name, size, fd.get()); } @@ -400,14 +406,29 @@ int ashmem_create_region(const char *name, size_t size) } static int memfd_set_prot_region(int fd, int prot) { - /* Only proceed if an fd needs to be write-protected */ + int seals = fcntl(fd, F_GET_SEALS); + if (seals == -1) { + ALOGE("memfd_set_prot_region(%d, %d): F_GET_SEALS failed: %s\n", fd, prot, strerror(errno)); + return -1; + } + if (prot & PROT_WRITE) { + /* Now we want the buffer to be read-write, let's check if the buffer + * has been previously marked as read-only before, if so return error + */ + if (seals & F_SEAL_FUTURE_WRITE) { + ALOGE("memfd_set_prot_region(%d, %d): region is write protected\n", fd, prot); + errno = EINVAL; // inline with ashmem error code, if already in + // read-only mode + return -1; + } return 0; } - if (fcntl(fd, F_ADD_SEALS, F_SEAL_FUTURE_WRITE) == -1) { - ALOGE("memfd_set_prot_region(%d, %d): F_SEAL_FUTURE_WRITE seal failed: %s\n", fd, prot, - strerror(errno)); + /* We would only allow read-only for any future file operations */ + if (fcntl(fd, F_ADD_SEALS, F_SEAL_FUTURE_WRITE | F_SEAL_SEAL) == -1) { + ALOGE("memfd_set_prot_region(%d, %d): F_SEAL_FUTURE_WRITE | F_SEAL_SEAL seal failed: %s\n", + fd, prot, strerror(errno)); return -1; } From 61a2897733e15a12b7aa2dfd99957e83cbe59351 Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Thu, 31 Aug 2023 00:31:35 +0000 Subject: [PATCH 10/13] Add seal if ashmem-dev is backed by memfd Need to seal the buffer size in align with ashmem if set to PROT_READ only to prevent untrusted remote process to shrink the buffer size and crash it. Bug: 294609150 Test: build Ignore-AOSP-First: Security Change-Id: I9288cf30b41e84ad8d3247c204e20482912bff69 Merged-In: I9288cf30b41e84ad8d3247c204e20482912bff69 (cherry picked from commit f83c5c8fecf89d9315945368aa20350c2f235cc0) --- libcutils/ashmem-dev.cpp | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/libcutils/ashmem-dev.cpp b/libcutils/ashmem-dev.cpp index 8c232f0cd0dc..501a4acd7256 100644 --- a/libcutils/ashmem-dev.cpp +++ b/libcutils/ashmem-dev.cpp @@ -341,6 +341,12 @@ static int memfd_create_region(const char* name, size_t size) { return -1; } + // forbid size changes to match ashmem behaviour + if (fcntl(fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK) == -1) { + ALOGE("memfd_create(%s, %zd) F_ADD_SEALS failed: %m", name, size); + return -1; + } + if (debug_log) { ALOGE("memfd_create(%s, %zd) success. fd=%d\n", name, size, fd.get()); } @@ -392,14 +398,29 @@ int ashmem_create_region(const char *name, size_t size) } static int memfd_set_prot_region(int fd, int prot) { - /* Only proceed if an fd needs to be write-protected */ + int seals = fcntl(fd, F_GET_SEALS); + if (seals == -1) { + ALOGE("memfd_set_prot_region(%d, %d): F_GET_SEALS failed: %s\n", fd, prot, strerror(errno)); + return -1; + } + if (prot & PROT_WRITE) { + /* Now we want the buffer to be read-write, let's check if the buffer + * has been previously marked as read-only before, if so return error + */ + if (seals & F_SEAL_FUTURE_WRITE) { + ALOGE("memfd_set_prot_region(%d, %d): region is write protected\n", fd, prot); + errno = EINVAL; // inline with ashmem error code, if already in + // read-only mode + return -1; + } return 0; } - if (fcntl(fd, F_ADD_SEALS, F_SEAL_FUTURE_WRITE) == -1) { - ALOGE("memfd_set_prot_region(%d, %d): F_SEAL_FUTURE_WRITE seal failed: %s\n", fd, prot, - strerror(errno)); + /* We would only allow read-only for any future file operations */ + if (fcntl(fd, F_ADD_SEALS, F_SEAL_FUTURE_WRITE | F_SEAL_SEAL) == -1) { + ALOGE("memfd_set_prot_region(%d, %d): F_SEAL_FUTURE_WRITE | F_SEAL_SEAL seal failed: %s\n", + fd, prot, strerror(errno)); return -1; } From ecf8e1cf2fc907bfd5f747cd8fb456076dd48d66 Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Mon, 18 Sep 2023 19:52:52 +0000 Subject: [PATCH 11/13] apply staged property value when loading persistent props Bug: b/300111812 Change-Id: I81b6bd984aad8f7ddec93ce74f4543e4f71be508 Ignore-AOSP-First: see cherry pick to aosp/2781147 --- init/persistent_properties.cpp | 18 +++++++-------- init/persistent_properties.h | 2 ++ init/property_service.cpp | 41 ++++++++++++++++++++++++++++++---- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/init/persistent_properties.cpp b/init/persistent_properties.cpp index 8db72679ee75..8efb72c7ea94 100644 --- a/init/persistent_properties.cpp +++ b/init/persistent_properties.cpp @@ -46,13 +46,6 @@ namespace { constexpr const char kLegacyPersistentPropertyDir[] = "/data/property"; -void AddPersistentProperty(const std::string& name, const std::string& value, - PersistentProperties* persistent_properties) { - auto persistent_property_record = persistent_properties->add_properties(); - persistent_property_record->set_name(name); - persistent_property_record->set_value(value); -} - Result LoadLegacyPersistentProperties() { std::unique_ptr dir(opendir(kLegacyPersistentPropertyDir), closedir); if (!dir) { @@ -161,9 +154,9 @@ Result ParsePersistentPropertyFile(const std::string& file return Error() << "Unable to parse persistent property file: Could not parse protobuf"; } for (auto& prop : persistent_properties.properties()) { - if (!StartsWith(prop.name(), "persist.")) { + if (!StartsWith(prop.name(), "persist.") && !StartsWith(prop.name(), "next_boot.")) { return Error() << "Unable to load persistent property file: property '" << prop.name() - << "' doesn't start with 'persist.'"; + << "' doesn't start with 'persist.' or 'next_boot.'"; } } return persistent_properties; @@ -171,6 +164,13 @@ Result ParsePersistentPropertyFile(const std::string& file } // namespace +void AddPersistentProperty(const std::string& name, const std::string& value, + PersistentProperties* persistent_properties) { + auto persistent_property_record = persistent_properties->add_properties(); + persistent_property_record->set_name(name); + persistent_property_record->set_value(value); +} + Result LoadPersistentPropertyFile() { auto file_contents = ReadPersistentPropertyFile(); if (!file_contents.ok()) return file_contents.error(); diff --git a/init/persistent_properties.h b/init/persistent_properties.h index 3845a0d86714..a6f80e64f51d 100644 --- a/init/persistent_properties.h +++ b/init/persistent_properties.h @@ -25,6 +25,8 @@ namespace android { namespace init { +void AddPersistentProperty(const std::string& name, const std::string& value, + PersistentProperties* persistent_properties); PersistentProperties LoadPersistentProperties(); void WritePersistentProperty(const std::string& name, const std::string& value); diff --git a/init/property_service.cpp b/init/property_service.cpp index a1b0b8ed51d0..ec18694f52c8 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -420,7 +420,8 @@ static std::optional PropertySet(const std::string& name, const std::s // Don't write properties to disk until after we have read all default // properties to prevent them from being overwritten by default values. - if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { + bool need_persist = StartsWith(name, "persist.") || StartsWith(name, "next_boot."); + if (socket && persistent_properties_loaded && need_persist) { if (persist_write_thread) { persist_write_thread->Write(name, value, std::move(*socket)); return {}; @@ -1405,11 +1406,43 @@ static void HandleInitSocket() { case InitMessage::kLoadPersistentProperties: { load_override_properties(); // Read persistent properties after all default values have been loaded. + // Apply staged and persistent properties + bool has_staged_prop = false; + auto const staged_prefix = std::string_view("next_boot."); + auto const staged_persist_prefix = std::string_view("next_boot.persist."); + auto persist_props_map = std::unordered_map(); + auto persistent_properties = LoadPersistentProperties(); - for (const auto& persistent_property_record : persistent_properties.properties()) { - InitPropertySet(persistent_property_record.name(), - persistent_property_record.value()); + for (const auto& property_record : persistent_properties.properties()) { + auto const& prop_name = property_record.name(); + auto const& prop_value = property_record.value(); + + if (StartsWith(prop_name, staged_prefix)) { + has_staged_prop = true; + auto actual_prop_name = prop_name.substr(staged_prefix.size()); + InitPropertySet(actual_prop_name, prop_value); + if (StartsWith(prop_name, staged_persist_prefix)) { + persist_props_map[actual_prop_name] = prop_value; + } + } else if (!persist_props_map.count(prop_name)) { + InitPropertySet(prop_name, prop_value); + } + } + + // Update persist prop file if there are staged props + if (has_staged_prop) { + PersistentProperties updated_persist_props; + for (auto const& [prop_name, prop_value] : persist_props_map) { + AddPersistentProperty(prop_name, prop_value, &updated_persist_props); + } + + // write current updated persist prop file + auto result = WritePersistentPropertyFile(updated_persist_props); + if (!result.ok()) { + LOG(ERROR) << "Could not store persistent property: " << result.error(); + } } + // Apply debug ramdisk special settings after persistent properties are loaded. if (android::base::GetBoolProperty("ro.force.debuggable", false)) { // Always enable usb adb if device is booted with debug ramdisk. From 5ae7a8053af634487a3abff927af97e9c1656e70 Mon Sep 17 00:00:00 2001 From: Nate Myren Date: Fri, 25 Aug 2023 14:30:50 -0700 Subject: [PATCH 12/13] Initialize the appcompat system property folder Certain applications may have their system properties overlaid with the contents overlaid for appcompat purposes. Init must initialize the appcompat folder, same as it does the standard folder. Bug: 291814949 Ignore-AOSP-First: must be merged with ag/24558703, which has an Aosp -> internal merge conflict Test: manual Change-Id: I0c6a0f66dc543c6e861bc86a417e4feb5ecd7789 --- init/property_service.cpp | 16 +++++++++++++--- .../property_info_parser/property_info_parser.h | 2 ++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index a1b0b8ed51d0..dc25a9d24b12 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -76,6 +76,10 @@ #include "system/core/init/property_service.pb.h" #include "util.h" +static constexpr char APPCOMPAT_OVERRIDE_PROP_FOLDERNAME[] = + "/dev/__properties__/appcompat_override"; +static constexpr char APPCOMPAT_OVERRIDE_PROP_TREE_FILE[] = + "/dev/__properties__/appcompat_override/property_info"; using namespace std::literals; using android::base::ErrnoError; @@ -1287,11 +1291,17 @@ void CreateSerializedPropertyInfo() { return; } - constexpr static const char kPropertyInfosPath[] = "/dev/__properties__/property_info"; - if (!WriteStringToFile(serialized_contexts, kPropertyInfosPath, 0444, 0, 0, false)) { + if (!WriteStringToFile(serialized_contexts, PROP_TREE_FILE, 0444, 0, 0, false)) { PLOG(ERROR) << "Unable to write serialized property infos to file"; } - selinux_android_restorecon(kPropertyInfosPath, 0); + selinux_android_restorecon(PROP_TREE_FILE, 0); + + mkdir(APPCOMPAT_OVERRIDE_PROP_FOLDERNAME, S_IRWXU | S_IXGRP | S_IXOTH); + if (!WriteStringToFile(serialized_contexts, APPCOMPAT_OVERRIDE_PROP_TREE_FILE, 0444, 0, 0, + false)) { + PLOG(ERROR) << "Unable to write appcompat override property infos to file"; + } + selinux_android_restorecon(APPCOMPAT_OVERRIDE_PROP_TREE_FILE, 0); } static void ExportKernelBootProps() { diff --git a/property_service/libpropertyinfoparser/include/property_info_parser/property_info_parser.h b/property_service/libpropertyinfoparser/include/property_info_parser/property_info_parser.h index 054802197a5f..65705accbc78 100644 --- a/property_service/libpropertyinfoparser/include/property_info_parser/property_info_parser.h +++ b/property_service/libpropertyinfoparser/include/property_info_parser/property_info_parser.h @@ -20,6 +20,8 @@ #include #include +static constexpr char PROP_TREE_FILE[] = "/dev/__properties__/property_info"; + namespace android { namespace properties { From 6c625e5d2e33bdaed7b9933b237502968c4b16e1 Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Mon, 23 Oct 2023 14:33:14 -0700 Subject: [PATCH 13/13] Run fsck to resolve possible data corruption Trigger fsck on mount of /data if the value of ro.preventative_fsck is not equal to the contents of /metadata/vold/preventative_fsck, then set the file to the property to prevent future runs See b/305658663 for context Bug: 305658663 Test: Make sure fsck run after first boot and not after second Ignore-AOSP-First: Critical UDC only bug Change-Id: I856c812d22363cc1d1e8aa88706d4d3b89044f52 --- fs_mgr/fs_mgr.cpp | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index 742cdfab012a..8b9bce1dbb5d 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -699,6 +699,29 @@ static void SetReadAheadSize(const std::string& entry_block_device, off64_t size } } +// +// Mechanism to allow fsck to be triggered by setting ro.preventative_fsck +// Introduced to address b/305658663 +// If the property value is not equal to the flag file contents, trigger +// fsck and store the property value in the flag file +// If we want to trigger again, simply change the property value +// +static bool check_if_preventative_fsck_needed(const FstabEntry& entry) { + const char* flag_file = "/metadata/vold/preventative_fsck"; + if (entry.mount_point != "/data") return false; + + // Don't error check - both default to empty string, which is OK + std::string prop = android::base::GetProperty("ro.preventative_fsck", ""); + std::string flag; + android::base::ReadFileToString(flag_file, &flag); + if (prop == flag) return false; + // fsck is run immediately, so assume it runs or there is some deeper problem + if (!android::base::WriteStringToFile(prop, flag_file)) + PERROR << "Failed to write file " << flag_file; + LINFO << "Run preventative fsck on /data"; + return true; +} + // // Prepare the filesystem on the given block device to be mounted. // @@ -749,7 +772,7 @@ static int prepare_fs_for_mount(const std::string& blk_device, const FstabEntry& } } - if (entry.fs_mgr_flags.check || + if (check_if_preventative_fsck_needed(entry) || entry.fs_mgr_flags.check || (fs_stat & (FS_STAT_UNCLEAN_SHUTDOWN | FS_STAT_QUOTA_ENABLED))) { check_fs(blk_device, entry.fs_type, mount_point, &fs_stat); }