From dc97f26ef9a4e327f8bad18a354789769053fa02 Mon Sep 17 00:00:00 2001 From: Lucas Wei Date: Mon, 28 Mar 2022 21:57:58 +0800 Subject: [PATCH 01/13] fastboot: Add vendor_kernel_boot Bug: 234474672 Bug: 214409109 Signed-off-by: Lucas Wei Merged-In: I8b0baa887e5e2309a1cb4a602fe8f6ca9e22526b Change-Id: I60b7ec0c160d38e01b15ed547d51a2a007bfd7f5 --- fastboot/fastboot.bash | 2 +- fastboot/fastboot.cpp | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/fastboot/fastboot.bash b/fastboot/fastboot.bash index e9bf9e940..911071aee 100644 --- a/fastboot/fastboot.bash +++ b/fastboot/fastboot.bash @@ -109,7 +109,7 @@ _fastboot_cmd_flash() { cur="${COMP_WORDS[COMP_CWORD]}" if [[ $i -eq $COMP_CWORD ]]; then - partitions="boot bootloader dtbo init_boot modem odm odm_dlkm oem product pvmfw radio recovery system system_dlkm vbmeta vendor vendor_dlkm" + partitions="boot bootloader dtbo init_boot modem odm odm_dlkm oem product pvmfw radio recovery system system_dlkm vbmeta vendor vendor_dlkm vendor_kernel_boot" COMPREPLY=( $(compgen -W "$partitions" -- $cur) ) else _fastboot_util_complete_local_file "${cur}" '!*.img' diff --git a/fastboot/fastboot.cpp b/fastboot/fastboot.cpp index 39d86f9b3..79c3ac746 100644 --- a/fastboot/fastboot.cpp +++ b/fastboot/fastboot.cpp @@ -186,6 +186,11 @@ static Image images[] = { "vendor_dlkm.img", "vendor_dlkm.sig", "vendor_dlkm", true, ImageType::Normal }, + { "vendor_kernel_boot", + "vendor_kernel_boot.img", + "vendor_kernel_boot.sig", + "vendor_kernel_boot", + true, ImageType::BootCritical }, { nullptr, "vendor_other.img", "vendor.sig", "vendor", true, ImageType::Normal }, // clang-format on }; From 09ab353773ba9e242c93ad4c379d9cf66832c145 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Tue, 28 Jun 2022 17:39:45 +0800 Subject: [PATCH 02/13] bootstat: add more bootreasons add more bootreasons for new projects Bug: 219902794 Test: trigger apc watchdog then "adb root; adb shell bootstat -p" Signed-off-by: Jason Chiu Merged-In: I7e19c4d48fb3d5b1c49dc8688936cf1d6eea6e9c Change-Id: If90861e8460db8bb7125097e32315c18feb327da --- bootstat/bootstat.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bootstat/bootstat.cpp b/bootstat/bootstat.cpp index 2c878f024..af0c89e9f 100644 --- a/bootstat/bootstat.cpp +++ b/bootstat/bootstat.cpp @@ -463,6 +463,11 @@ const std::map kBootReasonMap = { {"watchdog,gsa,hard", 215}, {"watchdog,gsa,soft", 216}, {"watchdog,pmucal", 217}, + {"reboot,early,bl", 218}, + {"watchdog,apc,gsa,crashed", 219}, + {"watchdog,apc,bl31,crashed", 220}, + {"watchdog,apc,pbl,crashed", 221}, + {"reboot,memory_protect,hyp", 222}, }; // Converts a string value representing the reason the system booted to an From 70052ec848d763b8d92d89252cfda834ed55a3c3 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 8 Apr 2022 15:08:48 -0700 Subject: [PATCH 03/13] Add support for only starting 64 bit zygote. This is part of the changes that will allow creating a single system image but a different set of properties will either start or not start the secondary zygote. Bug: 227482437 Test: Verified that secondary doesn't start with same system image Test: with ro.zygote set to zygote64 and abilists set appropriately. Test: Verified that secondary does not start when restarting netd. Test: Verified that secondary does start with same system image Test: with ro.zygote set to zygote64_32 and abilists set appropriately. Test: Verified that secondary does start when restarting netd. Test: Verified that a 64 bit device only starts the primary. Test: Verified that a 32 bit device only starts the primary. Change-Id: Id37a223c73f9a61868b2e26450ef4b6964f7b496 Merged-In: Id37a223c73f9a61868b2e26450ef4b6964f7b496 --- rootdir/init.rc | 16 +++++++++++----- rootdir/init.zygote64_32.rc | 1 + 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index cd71aa8aa..870a97b55 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -823,7 +823,6 @@ on post-fs-data mkdir /data/misc/odsign 0710 root system # directory used for odsign metrics mkdir /data/misc/odsign/metrics 0770 root system - # Directory for VirtualizationService temporary image files. # Delete any stale files owned by the old virtualizationservice uid (b/230056726). chmod 0770 /data/misc/virtualizationservice @@ -1030,8 +1029,7 @@ on zygote-start && property:ro.crypto.state=unencrypted exec_start update_verifier_nonencrypted start statsd start netd - start zygote - start zygote_secondary + trigger zygote-run on zygote-start && property:ro.crypto.state=unsupported wait_for_prop odsign.verification.done 1 @@ -1039,8 +1037,7 @@ on zygote-start && property:ro.crypto.state=unsupported exec_start update_verifier_nonencrypted start statsd start netd - start zygote - start zygote_secondary + trigger zygote-run on zygote-start && property:ro.crypto.state=encrypted && property:ro.crypto.type=file wait_for_prop odsign.verification.done 1 @@ -1048,6 +1045,15 @@ on zygote-start && property:ro.crypto.state=encrypted && property:ro.crypto.type exec_start update_verifier_nonencrypted start statsd start netd + trigger zygote-run + +on zygote-run && property:ro.zygote=zygote32 + start zygote + +on zygote-run && property:ro.zygote=zygote64 + start zygote + +on zygote-run && property:ro.zygote=zygote64_32 start zygote start zygote_secondary diff --git a/rootdir/init.zygote64_32.rc b/rootdir/init.zygote64_32.rc index efb30d664..dfe16454c 100644 --- a/rootdir/init.zygote64_32.rc +++ b/rootdir/init.zygote64_32.rc @@ -25,3 +25,4 @@ service zygote_secondary /system/bin/app_process32 -Xzygote /system/bin --zygote socket usap_pool_secondary stream 660 root system onrestart restart zygote task_profiles ProcessCapacityHigh MaxPerformance + disabled From c5ad522fef9c02c918387c44e2b86c227630eee0 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Thu, 14 Jul 2022 21:53:47 +0000 Subject: [PATCH 04/13] Revert "Add support for only starting 64 bit zygote." This reverts commit da94c7f6501158de734171c0f62a486ac69ac8a9. Reason for revert: It appears this change slows down boot on normal devices. Technically, this change is not necessary, but it prevents starting the secondary and having it throw an error in the only run 64 bit zygote config. But it's easier to throw the error than slow down boot up. Bug: 238971179 Test: Verified that on a 64 with 32 config, the secondary zygote Test: starts but exits. Change-Id: I7ab0496a402db83e70168d52e5d5911b82a3b06a Merged-In: I7ab0496a402db83e70168d52e5d5911b82a3b06a (cherry picked from commit 3fa3f861d4c091aa59a6b217a3306a5ba80284f6) --- rootdir/init.rc | 16 +++++----------- rootdir/init.zygote64_32.rc | 1 - 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 870a97b55..cd71aa8aa 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -823,6 +823,7 @@ on post-fs-data mkdir /data/misc/odsign 0710 root system # directory used for odsign metrics mkdir /data/misc/odsign/metrics 0770 root system + # Directory for VirtualizationService temporary image files. # Delete any stale files owned by the old virtualizationservice uid (b/230056726). chmod 0770 /data/misc/virtualizationservice @@ -1029,7 +1030,8 @@ on zygote-start && property:ro.crypto.state=unencrypted exec_start update_verifier_nonencrypted start statsd start netd - trigger zygote-run + start zygote + start zygote_secondary on zygote-start && property:ro.crypto.state=unsupported wait_for_prop odsign.verification.done 1 @@ -1037,7 +1039,8 @@ on zygote-start && property:ro.crypto.state=unsupported exec_start update_verifier_nonencrypted start statsd start netd - trigger zygote-run + start zygote + start zygote_secondary on zygote-start && property:ro.crypto.state=encrypted && property:ro.crypto.type=file wait_for_prop odsign.verification.done 1 @@ -1045,15 +1048,6 @@ on zygote-start && property:ro.crypto.state=encrypted && property:ro.crypto.type exec_start update_verifier_nonencrypted start statsd start netd - trigger zygote-run - -on zygote-run && property:ro.zygote=zygote32 - start zygote - -on zygote-run && property:ro.zygote=zygote64 - start zygote - -on zygote-run && property:ro.zygote=zygote64_32 start zygote start zygote_secondary diff --git a/rootdir/init.zygote64_32.rc b/rootdir/init.zygote64_32.rc index dfe16454c..efb30d664 100644 --- a/rootdir/init.zygote64_32.rc +++ b/rootdir/init.zygote64_32.rc @@ -25,4 +25,3 @@ service zygote_secondary /system/bin/app_process32 -Xzygote /system/bin --zygote socket usap_pool_secondary stream 660 root system onrestart restart zygote task_profiles ProcessCapacityHigh MaxPerformance - disabled From 2c0161be6a3f09c63ec88dd0b367daed49cefec7 Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Wed, 29 Jun 2022 05:41:19 +0000 Subject: [PATCH 05/13] Tune snapshot-merge performance Currently, there is one thread per partition for snapshot merge. When all these threads are run in parallel, this may stress the system as the merge threads are both CPU and I/O bound. Allow only two merge threads to be in-flight at any point in time. This will ensure that there is forward progress done with respect to snapshot-merge and only two cores are used as against using 5-6 cores. Additionally, system and prodcut partitions are merged first. This is primarily because /root is mounted of system partition and faster the merge completes on /system partition, we can switch the dm tables immediately. There is no change in the merge phase from libsnapshot perspective. This prioritization is based on each merge phase. If the system partition merge is in second phase, then it takes priority in that phase. As a side benefit, this should also reduce the memory usage when merge is in-flight given that we now limit the threads. There is slight delay in overall merge time as we now throttle the merge. No boot time regressions observed. Full OTA: Merge time (Without this patch): 42 seconds Merge time (With this patch): 46 seconds Incremental OTA: Merge time (Without this patch): 52 seconds Merge time (With this patch): 57 seconds system partition merge completes in the first ~12-16 seconds. App-launch (COLD) on Pixel: Baseline (After snapshot-merge is completed when there is no daemon): ========================== Chrome: 250 youtube: 631 camera: 230 ========================== Without this patch when snapshot-merge is in-progress (in ms): Full - OTA Chrome: 1729 youtube: 3126 camera: 1525 ========================== With this patch when snapshot-merge is in-progress (in ms): Full - OTA Chrome: 1061 youtube: 820 camera: 1378 Incremental - OTA (350M) Chrome: 495 youtube: 1442 camera: 641 ===================== Bug: 237490659 Ignore-AOSP-First: cherry-pick from aosp Test: Full and incremental OTA Signed-off-by: Akilesh Kailash Change-Id: I887d5073dba88e9a8a85ac10c771e4ccee7c84ff --- fs_mgr/libsnapshot/snapshot.cpp | 11 ++- .../user-space-merge/snapuserd_core.cpp | 5 +- .../user-space-merge/snapuserd_core.h | 6 +- .../user-space-merge/snapuserd_server.cpp | 73 +++++++++++++++++-- .../user-space-merge/snapuserd_server.h | 18 ++++- .../snapuserd_transitions.cpp | 19 +++++ 6 files changed, 121 insertions(+), 11 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 019b64a44..870801ef1 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2151,8 +2151,17 @@ bool SnapshotManager::ListSnapshots(LockedFile* lock, std::vector* if (!suffix.empty() && !android::base::EndsWith(name, suffix)) { continue; } - snapshots->emplace_back(std::move(name)); + + // Insert system and product partition at the beginning so that + // during snapshot-merge, these partitions are merged first. + if (name == "system_a" || name == "system_b" || name == "product_a" || + name == "product_b") { + snapshots->insert(snapshots->begin(), std::move(name)); + } else { + snapshots->emplace_back(std::move(name)); + } } + return true; } 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 692cb740c..718c13ce0 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.cpp @@ -143,17 +143,18 @@ void SnapshotHandler::PrepareReadAhead() { NotifyRAForMergeReady(); } -void SnapshotHandler::CheckMergeCompletionStatus() { +bool SnapshotHandler::CheckMergeCompletionStatus() { if (!merge_initiated_) { SNAP_LOG(INFO) << "Merge was not initiated. Total-data-ops: " << reader_->get_num_total_data_ops(); - return; + return false; } struct CowHeader* ch = reinterpret_cast(mapped_addr_); SNAP_LOG(INFO) << "Merge-status: Total-Merged-ops: " << ch->num_merge_ops << " Total-data-ops: " << reader_->get_num_total_data_ops(); + return true; } bool SnapshotHandler::ReadMetadata() { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index 83d40f635..dd2627eee 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -274,7 +274,7 @@ class SnapshotHandler : public std::enable_shared_from_this { const bool& IsAttached() const { return attached_; } void AttachControlDevice() { attached_ = true; } - void CheckMergeCompletionStatus(); + bool CheckMergeCompletionStatus(); bool CommitMerge(int num_merge_ops); void CloseFds() { cow_fd_ = {}; } @@ -305,6 +305,8 @@ class SnapshotHandler : public std::enable_shared_from_this { // State transitions for merge void InitiateMerge(); + void MonitorMerge(); + void WakeupMonitorMergeThread(); void WaitForMergeComplete(); bool WaitForMergeBegin(); void NotifyRAForMergeReady(); @@ -333,6 +335,7 @@ class SnapshotHandler : public std::enable_shared_from_this { void SetSocketPresent(bool socket) { is_socket_present_ = socket; } void SetIouringEnabled(bool io_uring_enabled) { is_io_uring_enabled_ = io_uring_enabled; } bool MergeInitiated() { return merge_initiated_; } + bool MergeMonitored() { return merge_monitored_; } double GetMergePercentage() { return merge_completion_percentage_; } // Merge Block State Transitions @@ -407,6 +410,7 @@ class SnapshotHandler : public std::enable_shared_from_this { double merge_completion_percentage_; bool merge_initiated_ = false; + bool merge_monitored_ = false; bool attached_ = false; bool is_socket_present_; bool is_io_uring_enabled_ = false; 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 82b2b25dd..5d93f01a7 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -59,6 +59,14 @@ DaemonOps UserSnapshotServer::Resolveop(std::string& input) { return DaemonOps::INVALID; } +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; +} + UserSnapshotServer::~UserSnapshotServer() { // Close any client sockets that were added via AcceptClient(). for (size_t i = 1; i < watched_fds_.size(); i++) { @@ -249,7 +257,7 @@ bool UserSnapshotServer::Receivemsg(android::base::borrowed_fd fd, const std::st return Sendmsg(fd, "fail"); } - if (!StartMerge(*iter)) { + if (!StartMerge(&lock, *iter)) { return Sendmsg(fd, "fail"); } @@ -298,7 +306,7 @@ void UserSnapshotServer::RunThread(std::shared_ptr ha } handler->snapuserd()->CloseFds(); - handler->snapuserd()->CheckMergeCompletionStatus(); + bool merge_completed = handler->snapuserd()->CheckMergeCompletionStatus(); handler->snapuserd()->UnmapBufferRegion(); auto misc_name = handler->misc_name(); @@ -306,7 +314,11 @@ void UserSnapshotServer::RunThread(std::shared_ptr ha { std::lock_guard lock(lock_); - num_partitions_merge_complete_ += 1; + 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()) { @@ -418,6 +430,9 @@ void UserSnapshotServer::JoinAllThreads() { if (th.joinable()) th.join(); } + + stop_monitor_merge_thread_ = true; + WakeupMonitorMergeThread(); } void UserSnapshotServer::AddWatchedFd(android::base::borrowed_fd fd, int events) { @@ -502,13 +517,24 @@ bool UserSnapshotServer::StartHandler(const std::shared_ptr& handler) { +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()->InitiateMerge(); + 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; } @@ -590,6 +616,42 @@ bool UserSnapshotServer::RemoveAndJoinHandler(const std::string& misc_name) { 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(); + LOG(INFO) << "Starting merge for partition: " + << handler->snapuserd()->GetMiscName(); + handler->snapuserd()->InitiateMerge(); + active_merge_threads_ += 1; + } + } + } + + LOG(INFO) << "Exiting MonitorMerge: size: " << merge_handlers_.size(); +} + bool UserSnapshotServer::WaitForSocket() { auto scope_guard = android::base::make_scope_guard([this]() -> void { JoinAllThreads(); }); @@ -646,6 +708,7 @@ bool UserSnapshotServer::WaitForSocket() { if (!StartWithSocket(true)) { return false; } + return Run(); } 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 34e7941bc..e0844aeb1 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.h @@ -15,6 +15,7 @@ #pragma once #include +#include #include #include @@ -22,6 +23,8 @@ #include #include #include +#include +#include #include #include #include @@ -34,6 +37,7 @@ namespace android { namespace snapshot { static constexpr uint32_t kMaxPacketSize = 512; +static constexpr uint8_t kMaxMergeThreads = 2; enum class DaemonOps { INIT, @@ -84,13 +88,19 @@ class UserSnapshotServer { 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::mutex lock_; using HandlerList = std::vector>; HandlerList dm_users_; + std::queue> merge_handlers_; void AddWatchedFd(android::base::borrowed_fd fd, int events); void AcceptClient(); @@ -108,6 +118,8 @@ class UserSnapshotServer { bool IsTerminating() { return terminating_; } void RunThread(std::shared_ptr handler); + void MonitorMerge(); + void JoinAllThreads(); bool StartWithSocket(bool start_listening); @@ -119,7 +131,7 @@ class UserSnapshotServer { void TerminateMergeThreads(std::lock_guard* proof_of_lock); public: - UserSnapshotServer() { terminating_ = false; } + UserSnapshotServer(); ~UserSnapshotServer(); bool Start(const std::string& socketname); @@ -133,9 +145,11 @@ class UserSnapshotServer { const std::string& backing_device, const std::string& base_path_merge); bool StartHandler(const std::shared_ptr& handler); - bool StartMerge(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); + void WakeupMonitorMergeThread(); void SetTerminating() { terminating_ = true; } void ReceivedSocketSignal() { received_socket_signal_ = true; } void SetServerRunning() { is_server_running_ = true; } diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_transitions.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_transitions.cpp index d4e1d7c7e..28c9f688a 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_transitions.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_transitions.cpp @@ -165,6 +165,13 @@ using namespace android; using namespace android::dm; using android::base::unique_fd; +void SnapshotHandler::MonitorMerge() { + { + std::lock_guard lock(lock_); + merge_monitored_ = true; + } +} + // This is invoked once primarily by update-engine to initiate // the merge void SnapshotHandler::InitiateMerge() { @@ -361,10 +368,16 @@ void SnapshotHandler::WaitForMergeComplete() { std::string SnapshotHandler::GetMergeStatus() { bool merge_not_initiated = false; + bool merge_monitored = false; bool merge_failed = false; { std::lock_guard lock(lock_); + + if (MergeMonitored()) { + merge_monitored = true; + } + if (!MergeInitiated()) { merge_not_initiated = true; } @@ -387,6 +400,12 @@ std::string SnapshotHandler::GetMergeStatus() { return "snapshot-merge-complete"; } + // Merge monitor thread is tracking the merge but the merge thread + // is not started yet. + if (merge_monitored) { + return "snapshot-merge"; + } + // Return the state as "snapshot". If the device was rebooted during // merge, we will return the status as "snapshot". This is ok, as // libsnapshot will explicitly resume the merge. This is slightly From 365ac4e34858dbd6ae3319d7e201c55e71e3284b Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Thu, 30 Jun 2022 19:01:23 +0000 Subject: [PATCH 06/13] Reduce priority of merge threads Currently, when daemon is spin up, it runs at the highest priority with nice value set to -20. This can potetially lead to a problem in a busy system especially after OTA reboot when all the merge threads are running in parallel. Now that we reduced the number of merge threads in-flight to 2, we reduce the priority as well by setting the nice value to -5. The other threads which serve I/O's from dm-user (from root filesystem) still runs at higher priority. We need this because post OTA reboot, these threads serve I/O's until merge is completed. Merge threads on the other hand can run at a relatively lower priority. We need to make sure that there is always forward progress even in a busy system and hence set the priority to -5 as compared to default value of 0. No boot time regressions observed. Output of NICE value of merge and worker threads post OTA reboot: 1 S 0 427 451 1 0 39 -20 64 2314640 dev_r+ ? 00:00:00 8 1 S 0 427 486 1 4 39 -20 64 2314640 dev_r+ ? 00:00:02 8 1 S 0 427 487 1 4 39 -20 64 2314640 dev_r+ ? 00:00:02 8 1 S 0 427 488 1 3 39 -20 64 2314640 dev_r+ ? 00:00:02 8 5 R 0 427 634 1 1 24 -5 64 2314640 0 ? 00:00:00 8 5 R 0 427 935 1 5 24 -5 64 2314640 0 ? 00:00:02 8 Bug: 237490659 Test: Full and incremental OTA Ignore-AOSP-First: cherry-pick from aosp Signed-off-by: Akilesh Kailash Change-Id: I6791dd72ccd8cd5bba6eff663bb3f9598bce7ed2 --- .../libsnapshot/snapuserd/user-space-merge/snapuserd_core.h | 5 +++++ .../snapuserd/user-space-merge/snapuserd_merge.cpp | 4 ++++ .../snapuserd/user-space-merge/snapuserd_readahead.cpp | 4 ++++ 3 files changed, 13 insertions(+) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h index dd2627eee..c6805e5d9 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_core.h @@ -18,6 +18,9 @@ #include #include #include +#include +#include +#include #include #include @@ -54,6 +57,8 @@ static_assert(PAYLOAD_BUFFER_SZ >= BLOCK_SZ); static constexpr int kNumWorkerThreads = 4; +static constexpr int kNiceValueForMergeThreads = -5; + #define SNAP_LOG(level) LOG(level) << misc_name_ << ": " #define SNAP_PLOG(level) PLOG(level) << misc_name_ << ": " diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp index c26a2cd5c..17f3c4fe5 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp @@ -543,6 +543,10 @@ bool Worker::RunMergeThread() { return true; } + if (setpriority(PRIO_PROCESS, gettid(), kNiceValueForMergeThreads)) { + SNAP_PLOG(ERROR) << "Failed to set priority for TID: " << gettid(); + } + SNAP_LOG(INFO) << "Merge starting.."; if (!Init()) { diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_readahead.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_readahead.cpp index fa2866f39..b9e4255b2 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_readahead.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_readahead.cpp @@ -727,6 +727,10 @@ bool ReadAhead::RunThread() { InitializeIouring(); + if (setpriority(PRIO_PROCESS, gettid(), kNiceValueForMergeThreads)) { + SNAP_PLOG(ERROR) << "Failed to set priority for TID: " << gettid(); + } + while (!RAIterDone()) { if (!ReadAheadIOStart()) { break; From a70049022b53abca4e1b94a0fa0da140436f31ee Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Mon, 25 Jul 2022 19:08:46 +0000 Subject: [PATCH 07/13] Flush after every 2MB merge of replace ops. This will be in sync with incremental OTA's where the sync is done every 2MB. This improves performance on devices with low memory. Merge times for full OTA may increase by couple of seconds but that is ok given it decreases the memory footprint. Bug: 237490659 Test: OTA Ignore-AOSP-First: cherry-pick from aosp Signed-off-by: Akilesh Kailash Change-Id: Ic2c8d2ffdbdb677e0c4d44e5de68ce8ccf86df34 --- .../user-space-merge/snapuserd_merge.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp index 17f3c4fe5..63f47d6f0 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_merge.cpp @@ -71,16 +71,16 @@ int Worker::PrepareMerge(uint64_t* source_offset, int* pending_ops, } bool Worker::MergeReplaceZeroOps() { - // Flush every 8192 ops. Since all ops are independent and there is no + // Flush after merging 2MB. Since all ops are independent and there is no // dependency between COW ops, we will flush the data and the number - // of ops merged in COW file for every 8192 ops. If there is a crash, - // we will end up replaying some of the COW ops which were already merged. - // That is ok. + // of ops merged in COW block device. If there is a crash, we will + // end up replaying some of the COW ops which were already merged. That is + // ok. // - // Why 8192 ops ? Increasing this may improve merge time 3-4 seconds but - // we need to make sure that we checkpoint; 8k ops seems optimal. In-case - // if there is a crash merge should always make forward progress. - int total_ops_merged_per_commit = (PAYLOAD_BUFFER_SZ / BLOCK_SZ) * 32; + // Although increasing this greater than 2MB may help in improving merge + // times; however, on devices with low memory, this can be problematic + // when there are multiple merge threads in parallel. + int total_ops_merged_per_commit = (PAYLOAD_BUFFER_SZ / BLOCK_SZ) * 2; int num_ops_merged = 0; SNAP_LOG(INFO) << "MergeReplaceZeroOps started...."; From 5b02ed521ccbb52e727364769aa4514f58055f82 Mon Sep 17 00:00:00 2001 From: Stephen Crane Date: Wed, 27 Jul 2022 17:16:05 -0700 Subject: [PATCH 08/13] storageproxy: Support POST_COMMIT sync for all commands Previously we did not support STORAGE_MSG_FLAG_POST_COMMIT for anything but RPMB operations (in which case it was a no-op). We need to support this flag in order to store a superblock in non-secure storage, as we need that write to commit atomically wrt all other writes. Test: com.android.storage-unittest.nsp Bug: 228793975 Change-Id: Ia453c1916970e0b65a91e42f18b920ac4e1f01db Merged-In: Ia453c1916970e0b65a91e42f18b920ac4e1f01db (cherry picked from commit 57770a531885cea930f8c56a311c0a0c530abdad) --- trusty/storage/proxy/proxy.c | 7 ++++--- trusty/storage/proxy/storage.c | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/trusty/storage/proxy/proxy.c b/trusty/storage/proxy/proxy.c index 262003427..ed670b7fe 100644 --- a/trusty/storage/proxy/proxy.c +++ b/trusty/storage/proxy/proxy.c @@ -116,10 +116,11 @@ static int drop_privs(void) { static int handle_req(struct storage_msg* msg, const void* req, size_t req_len) { int rc; - if ((msg->flags & STORAGE_MSG_FLAG_POST_COMMIT) && (msg->cmd != STORAGE_RPMB_SEND)) { + if ((msg->flags & STORAGE_MSG_FLAG_POST_COMMIT) && msg->cmd != STORAGE_RPMB_SEND && + msg->cmd != STORAGE_FILE_WRITE) { /* - * handling post commit messages on non rpmb commands are not - * implemented as there is no use case for this yet. + * handling post commit messages on commands other than rpmb and write + * operations are not implemented as there is no use case for this yet. */ ALOGE("cmd 0x%x: post commit option is not implemented\n", msg->cmd); msg->result = STORAGE_ERR_UNIMPLEMENTED; diff --git a/trusty/storage/proxy/storage.c b/trusty/storage/proxy/storage.c index c00c399d9..2fedcce6d 100644 --- a/trusty/storage/proxy/storage.c +++ b/trusty/storage/proxy/storage.c @@ -407,6 +407,14 @@ int storage_file_write(struct storage_msg *msg, goto err_response; } + if (msg->flags & STORAGE_MSG_FLAG_POST_COMMIT) { + rc = storage_sync_checkpoint(); + if (rc < 0) { + msg->result = STORAGE_ERR_GENERIC; + goto err_response; + } + } + msg->result = STORAGE_NO_ERROR; err_response: From cf458bae1e9cbd7014114d14223886daa5be7d98 Mon Sep 17 00:00:00 2001 From: Stephen Crane Date: Fri, 29 Jul 2022 16:18:58 -0700 Subject: [PATCH 09/13] storageproxy: Report fsync failures with a distinct error code Fsync failures are special because they may indicate a failure of an operation before the current operation. Report these cases as a new, distinct error. Test: Cause fsync failure and check error response Bug: 239105007 Change-Id: Ie9d4a1949586e90006256c975786e21ced655e66 Merged-In: Ie9d4a1949586e90006256c975786e21ced655e66 (cherry picked from commit 1c75d1e3a7a70881879ab751747eeba39da98d7b) --- trusty/storage/interface/include/trusty/interface/storage.h | 4 ++++ trusty/storage/proxy/proxy.c | 2 +- trusty/storage/proxy/storage.c | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/trusty/storage/interface/include/trusty/interface/storage.h b/trusty/storage/interface/include/trusty/interface/storage.h index 3f1dcb8c6..255ade127 100644 --- a/trusty/storage/interface/include/trusty/interface/storage.h +++ b/trusty/storage/interface/include/trusty/interface/storage.h @@ -70,6 +70,9 @@ enum storage_cmd { * @STORAGE_ERR_TRANSACT returned by various operations to indicate that current transaction * is in error state. Such state could be only cleared by sending * STORAGE_END_TRANSACTION message. + * @STORAGE_ERR_SYNC_FAILURE indicates that the current operation failed to sync + * to disk. Only returned if STORAGE_MSG_FLAG_PRE_COMMIT or + * STORAGE_MSG_FLAG_POST_COMMIT was set for the request. */ enum storage_err { STORAGE_NO_ERROR = 0, @@ -80,6 +83,7 @@ enum storage_err { STORAGE_ERR_NOT_FOUND = 5, STORAGE_ERR_EXIST = 6, STORAGE_ERR_TRANSACT = 7, + STORAGE_ERR_SYNC_FAILURE = 8, }; /** diff --git a/trusty/storage/proxy/proxy.c b/trusty/storage/proxy/proxy.c index ed670b7fe..f01589287 100644 --- a/trusty/storage/proxy/proxy.c +++ b/trusty/storage/proxy/proxy.c @@ -130,7 +130,7 @@ static int handle_req(struct storage_msg* msg, const void* req, size_t req_len) if (msg->flags & STORAGE_MSG_FLAG_PRE_COMMIT) { rc = storage_sync_checkpoint(); if (rc < 0) { - msg->result = STORAGE_ERR_GENERIC; + msg->result = STORAGE_ERR_SYNC_FAILURE; return ipc_respond(msg, NULL, 0); } } diff --git a/trusty/storage/proxy/storage.c b/trusty/storage/proxy/storage.c index 2fedcce6d..c531cfd13 100644 --- a/trusty/storage/proxy/storage.c +++ b/trusty/storage/proxy/storage.c @@ -410,7 +410,7 @@ int storage_file_write(struct storage_msg *msg, if (msg->flags & STORAGE_MSG_FLAG_POST_COMMIT) { rc = storage_sync_checkpoint(); if (rc < 0) { - msg->result = STORAGE_ERR_GENERIC; + msg->result = STORAGE_ERR_SYNC_FAILURE; goto err_response; } } From 06b537addaaa2c40d24a5b9a89206149b377edc8 Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Thu, 4 Aug 2022 15:51:49 +0000 Subject: [PATCH 10/13] libsnapshot: Store index of COW ops vector Using vector + unordered_map to retrieve the index in a COW op vector consumes significant memory; this is a problem especially when there are hundreds of thousands of operations. Instead, just store the index of the COW op vector during pre-processing. On Pixel, peak memory usage when all the partitions are mapped: Without patch: RssAnon: 118804 kB With path: RssAnon: 55772 kB Additionally, post OTA reboot, memory usage further goes down as the partition merge completes. Bug: 237490659 Test: OTA on Pixel Ignore-AOSP-First: cherry-pick from aosp Signed-off-by: Akilesh Kailash Change-Id: Icc68a9688ceb89572821cee2dac689779f5e7c11 --- fs_mgr/libsnapshot/cow_reader.cpp | 70 ++++++++----------- .../include/libsnapshot/cow_reader.h | 3 +- 2 files changed, 32 insertions(+), 41 deletions(-) diff --git a/fs_mgr/libsnapshot/cow_reader.cpp b/fs_mgr/libsnapshot/cow_reader.cpp index 746feeb8d..75a58a6fc 100644 --- a/fs_mgr/libsnapshot/cow_reader.cpp +++ b/fs_mgr/libsnapshot/cow_reader.cpp @@ -38,7 +38,7 @@ CowReader::CowReader(ReaderFlags reader_flag) : fd_(-1), header_(), fd_size_(0), - merge_op_blocks_(std::make_shared>()), + block_pos_index_(std::make_shared>()), reader_flag_(reader_flag) {} static void SHA256(const void*, size_t, uint8_t[]) { @@ -58,13 +58,12 @@ std::unique_ptr CowReader::CloneCowReader() { cow->fd_size_ = fd_size_; cow->last_label_ = last_label_; cow->ops_ = ops_; - cow->merge_op_blocks_ = merge_op_blocks_; cow->merge_op_start_ = merge_op_start_; - cow->block_map_ = block_map_; cow->num_total_data_ops_ = num_total_data_ops_; cow->num_ordered_ops_to_merge_ = num_ordered_ops_to_merge_; cow->has_seq_ops_ = has_seq_ops_; cow->data_loc_ = data_loc_; + cow->block_pos_index_ = block_pos_index_; return cow; } @@ -415,10 +414,10 @@ bool CowReader::ParseOps(std::optional label) { // Replace-op-4, Zero-op-9, Replace-op-5 } //============================================================== bool CowReader::PrepMergeOps() { - auto merge_op_blocks = std::make_shared>(); + auto merge_op_blocks = std::make_unique>(); std::vector other_ops; auto seq_ops_set = std::unordered_set(); - auto block_map = std::make_shared>(); + auto block_map = std::make_unique>(); size_t num_seqs = 0; size_t read; @@ -477,13 +476,18 @@ bool CowReader::PrepMergeOps() { merge_op_blocks->insert(merge_op_blocks->end(), other_ops.begin(), other_ops.end()); + for (auto block : *merge_op_blocks) { + block_pos_index_->push_back(block_map->at(block)); + } + num_total_data_ops_ = merge_op_blocks->size(); if (header_.num_merge_ops > 0) { merge_op_start_ = header_.num_merge_ops; } - block_map_ = block_map; - merge_op_blocks_ = merge_op_blocks; + block_map->clear(); + merge_op_blocks->clear(); + return true; } @@ -589,9 +593,7 @@ const CowOperation& CowOpIter::Get() { class CowRevMergeOpIter final : public ICowOpIter { public: explicit CowRevMergeOpIter(std::shared_ptr> ops, - std::shared_ptr> merge_op_blocks, - std::shared_ptr> map, - uint64_t start); + std::shared_ptr> block_pos_index, uint64_t start); bool Done() override; const CowOperation& Get() override; @@ -602,17 +604,15 @@ class CowRevMergeOpIter final : public ICowOpIter { private: std::shared_ptr> ops_; - std::shared_ptr> merge_op_blocks_; - std::shared_ptr> map_; - std::vector::reverse_iterator block_riter_; + std::vector::reverse_iterator block_riter_; + std::shared_ptr> cow_op_index_vec_; uint64_t start_; }; class CowMergeOpIter final : public ICowOpIter { public: explicit CowMergeOpIter(std::shared_ptr> ops, - std::shared_ptr> merge_op_blocks, - std::shared_ptr> map, uint64_t start); + std::shared_ptr> block_pos_index, uint64_t start); bool Done() override; const CowOperation& Get() override; @@ -623,26 +623,21 @@ class CowMergeOpIter final : public ICowOpIter { private: std::shared_ptr> ops_; - std::shared_ptr> merge_op_blocks_; - std::shared_ptr> map_; - std::vector::iterator block_iter_; + std::vector::iterator block_iter_; + std::shared_ptr> cow_op_index_vec_; uint64_t start_; }; CowMergeOpIter::CowMergeOpIter(std::shared_ptr> ops, - std::shared_ptr> merge_op_blocks, - std::shared_ptr> map, - uint64_t start) { + std::shared_ptr> block_pos_index, uint64_t start) { ops_ = ops; - merge_op_blocks_ = merge_op_blocks; - map_ = map; start_ = start; - - block_iter_ = merge_op_blocks->begin() + start; + cow_op_index_vec_ = block_pos_index; + block_iter_ = cow_op_index_vec_->begin() + start; } bool CowMergeOpIter::RDone() { - return block_iter_ == merge_op_blocks_->begin(); + return block_iter_ == cow_op_index_vec_->begin(); } void CowMergeOpIter::Prev() { @@ -651,7 +646,7 @@ void CowMergeOpIter::Prev() { } bool CowMergeOpIter::Done() { - return block_iter_ == merge_op_blocks_->end(); + return block_iter_ == cow_op_index_vec_->end(); } void CowMergeOpIter::Next() { @@ -661,23 +656,20 @@ void CowMergeOpIter::Next() { const CowOperation& CowMergeOpIter::Get() { CHECK(!Done()); - return ops_->data()[map_->at(*block_iter_)]; + return ops_->data()[*block_iter_]; } CowRevMergeOpIter::CowRevMergeOpIter(std::shared_ptr> ops, - std::shared_ptr> merge_op_blocks, - std::shared_ptr> map, + std::shared_ptr> block_pos_index, uint64_t start) { ops_ = ops; - merge_op_blocks_ = merge_op_blocks; - map_ = map; start_ = start; - - block_riter_ = merge_op_blocks->rbegin(); + cow_op_index_vec_ = block_pos_index; + block_riter_ = cow_op_index_vec_->rbegin(); } bool CowRevMergeOpIter::RDone() { - return block_riter_ == merge_op_blocks_->rbegin(); + return block_riter_ == cow_op_index_vec_->rbegin(); } void CowRevMergeOpIter::Prev() { @@ -686,7 +678,7 @@ void CowRevMergeOpIter::Prev() { } bool CowRevMergeOpIter::Done() { - return block_riter_ == merge_op_blocks_->rend() - start_; + return block_riter_ == cow_op_index_vec_->rend() - start_; } void CowRevMergeOpIter::Next() { @@ -696,7 +688,7 @@ void CowRevMergeOpIter::Next() { const CowOperation& CowRevMergeOpIter::Get() { CHECK(!Done()); - return ops_->data()[map_->at(*block_riter_)]; + return ops_->data()[*block_riter_]; } std::unique_ptr CowReader::GetOpIter() { @@ -704,12 +696,12 @@ std::unique_ptr CowReader::GetOpIter() { } std::unique_ptr CowReader::GetRevMergeOpIter(bool ignore_progress) { - return std::make_unique(ops_, merge_op_blocks_, block_map_, + return std::make_unique(ops_, block_pos_index_, ignore_progress ? 0 : merge_op_start_); } std::unique_ptr CowReader::GetMergeOpIter(bool ignore_progress) { - return std::make_unique(ops_, merge_op_blocks_, block_map_, + return std::make_unique(ops_, block_pos_index_, ignore_progress ? 0 : merge_op_start_); } diff --git a/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h b/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h index f4d5c72f3..fbdd6b98b 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/cow_reader.h @@ -170,9 +170,8 @@ class CowReader final : public ICowReader { uint64_t fd_size_; std::optional last_label_; std::shared_ptr> ops_; - std::shared_ptr> merge_op_blocks_; uint64_t merge_op_start_{}; - std::shared_ptr> block_map_; + std::shared_ptr> block_pos_index_; uint64_t num_total_data_ops_{}; uint64_t num_ordered_ops_to_merge_{}; bool has_seq_ops_{}; From b0bb60cb16e2553d1a478c091c4fcf7b61e9ec17 Mon Sep 17 00:00:00 2001 From: Jooyung Han Date: Thu, 18 Aug 2022 09:56:18 +0900 Subject: [PATCH 11/13] Prepare /data/property before load_persist_props Without the directory (this happens on the very first boot), load_persist_props can't create an initial version of /data/property/persistent_properties (probably empty). This leads to persisting all in-memory "persist.*" properties later when a persistent property is set. This is regression from Android S because persistent props from, for example, build.prop will be persisted even when there's no process to explicitly setprop. Bug: 242264580 Bug: 243958304 Test: launch cuttlefish and verify that there's no props from build.prop Merged-In: I5819a97750e4d5d1ee5a7c308bf944c7aeab2f90 Change-Id: I5819a97750e4d5d1ee5a7c308bf944c7aeab2f90 (cherry picked from commit 95614963039be8500c6f2a81194e97e01b35bc7b) --- rootdir/init.rc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index cd71aa8aa..c56f86089 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -720,9 +720,13 @@ on post-fs-data # Multi-installed APEXes are selected using persist props. # Load persist properties and override properties (if enabled) from /data, # before starting apexd. + # /data/property should be created before `load_persist_props` + mkdir /data/property 0700 root root encryption=Require load_persist_props + start logd start logd-reinit + # Some existing vendor rc files use 'on load_persist_props_action' to know # when persist props are ready. These are difficult to change due to GRF, # so continue triggering this action here even though props are already loaded @@ -842,7 +846,6 @@ on post-fs-data mkdir /data/app-asec 0700 root root encryption=Require mkdir /data/app-lib 0771 system system encryption=Require mkdir /data/app 0771 system system encryption=Require - mkdir /data/property 0700 root root encryption=Require # create directory for updated font files. mkdir /data/fonts/ 0771 root root encryption=Require From 63818fc16d664a76650ad4f0b457cbd2500d9c5c Mon Sep 17 00:00:00 2001 From: Per Larsen Date: Sat, 20 Aug 2022 23:13:54 -0700 Subject: [PATCH 12/13] trusty/apploader: Add missing doc for enum value Document the APPLOADER_ERR_POLICY_VIOLATION value in enum apploader_error. Bug: 208968719 Change-Id: Ia9b17f4ea705d13567b2ba74f2dcd6df5a0c7d73 (cherry picked from commit c5253819f85486ee40ae13f59742afeba8f8ef58) Merged-In: Ia9b17f4ea705d13567b2ba74f2dcd6df5a0c7d73 --- trusty/apploader/apploader_ipc.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/trusty/apploader/apploader_ipc.h b/trusty/apploader/apploader_ipc.h index 306596eba..ed5cbde1a 100644 --- a/trusty/apploader/apploader_ipc.h +++ b/trusty/apploader/apploader_ipc.h @@ -45,6 +45,9 @@ enum apploader_command : uint32_t { * @APPLOADER_ERR_INTERNAL: miscellaneous or internal apploader * error not covered by the above * @APPLOADER_ERR_INVALID_VERSION: invalid application version + * @APPLOADER_ERR_POLICY_VIOLATION: signature verification succeeded but + * key+manifest combination not allowed + * by app loader policy engine */ enum apploader_error : uint32_t { APPLOADER_NO_ERROR = 0, From c3bda9f3edb1aefea44a3fe4d741aca63c57c9bc Mon Sep 17 00:00:00 2001 From: Per Larsen Date: Sat, 20 Aug 2022 23:16:53 -0700 Subject: [PATCH 13/13] trusty/apploader: Handle APPLOADER_ERR_NOT_ENCRYPTED Add a specific error message to the Android CLI tool for the case where the apploader rejected an attempt to load an application which requested encryption of its ELF image via its manifest while containing an unencrypted ELF image. Bug: 241824652 Change-Id: Ib2a3c881015700492b8166be38c41753bf51b3b2 (cherry picked from commit db9a554a2f30c1592f8825303a4d5131d6b32a2e) Merged-In: Ib2a3c881015700492b8166be38c41753bf51b3b2 --- trusty/apploader/apploader.cpp | 3 +++ trusty/apploader/apploader_ipc.h | 2 ++ 2 files changed, 5 insertions(+) diff --git a/trusty/apploader/apploader.cpp b/trusty/apploader/apploader.cpp index 278499f17..17d083c73 100644 --- a/trusty/apploader/apploader.cpp +++ b/trusty/apploader/apploader.cpp @@ -226,6 +226,9 @@ static ssize_t read_response(int tipc_fd) { case APPLOADER_ERR_POLICY_VIOLATION: LOG(ERROR) << "Error: loading denied by policy engine"; break; + case APPLOADER_ERR_NOT_ENCRYPTED: + LOG(ERROR) << "Error: unmet application encryption requirement"; + break; default: LOG(ERROR) << "Unrecognized error: " << resp.error; break; diff --git a/trusty/apploader/apploader_ipc.h b/trusty/apploader/apploader_ipc.h index ed5cbde1a..f0376929c 100644 --- a/trusty/apploader/apploader_ipc.h +++ b/trusty/apploader/apploader_ipc.h @@ -48,6 +48,7 @@ enum apploader_command : uint32_t { * @APPLOADER_ERR_POLICY_VIOLATION: signature verification succeeded but * key+manifest combination not allowed * by app loader policy engine + * @APPLOADER_ERR_NOT_ENCRYPTED: unmet application encryption requirement */ enum apploader_error : uint32_t { APPLOADER_NO_ERROR = 0, @@ -60,6 +61,7 @@ enum apploader_error : uint32_t { APPLOADER_ERR_INTERNAL, APPLOADER_ERR_INVALID_VERSION, APPLOADER_ERR_POLICY_VIOLATION, + APPLOADER_ERR_NOT_ENCRYPTED, }; /**