From ba9778a0b58d034682fa4ed3359df14f275fafd3 Mon Sep 17 00:00:00 2001 From: Nikita Ioffe Date: Fri, 25 Jun 2021 19:48:10 +0100 Subject: [PATCH 01/17] Add CreateEmptyDevice and WaitForDevice APIs These APIs support a flow in which dm devices can be created before they are actually needed, hence minimizing the time a process will wait for ueventd to create user space paths. Bug: 190618831 Test: atest libdm_test Change-Id: I4dfa14e5271a6a13de6da73ec3c7efb1ebc0f8b8 Merged-In: I4dfa14e5271a6a13de6da73ec3c7efb1ebc0f8b8 (cherry picked from commit 15e0f5a98acbec5f00f446ac61d6f79b9ee3bd80) --- fs_mgr/libdm/dm.cpp | 32 +++++++++++++++++++++++++------- fs_mgr/libdm/dm_test.cpp | 15 +++++++++++++++ fs_mgr/libdm/include/libdm/dm.h | 13 +++++++++++++ 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index c4874b8d1..a5eda2983 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -170,19 +170,18 @@ static bool IsRecovery() { return access("/system/bin/recovery", F_OK) == 0; } -bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table, std::string* path, - const std::chrono::milliseconds& timeout_ms) { +bool DeviceMapper::CreateEmptyDevice(const std::string& name) { std::string uuid = GenerateUuid(); - if (!CreateDevice(name, uuid)) { - return false; - } + return CreateDevice(name, uuid); +} +bool DeviceMapper::WaitForDevice(const std::string& name, + const std::chrono::milliseconds& timeout_ms, std::string* path) { // We use the unique path for testing whether the device is ready. After // that, it's safe to use the dm-N path which is compatible with callers // that expect it to be formatted as such. std::string unique_path; - if (!LoadTableAndActivate(name, table) || !GetDeviceUniquePath(name, &unique_path) || - !GetDmDevicePathByName(name, path)) { + if (!GetDeviceUniquePath(name, &unique_path) || !GetDmDevicePathByName(name, path)) { DeleteDevice(name); return false; } @@ -208,6 +207,25 @@ bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table, s return true; } +bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table, std::string* path, + const std::chrono::milliseconds& timeout_ms) { + if (!CreateEmptyDevice(name)) { + return false; + } + + if (!LoadTableAndActivate(name, table)) { + DeleteDevice(name); + return false; + } + + if (!WaitForDevice(name, timeout_ms, path)) { + DeleteDevice(name); + return false; + } + + return true; +} + bool DeviceMapper::GetDeviceUniquePath(const std::string& name, std::string* path) { struct dm_ioctl io; InitIo(&io, name); diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index 8006db220..8314ec596 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -679,3 +680,17 @@ TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { ASSERT_NE(0, access(path.c_str(), F_OK)); ASSERT_EQ(ENOENT, errno); } + +TEST(libdm, CreateEmptyDevice) { + DeviceMapper& dm = DeviceMapper::Instance(); + ASSERT_TRUE(dm.CreateEmptyDevice("empty-device")); + auto guard = android::base::make_scope_guard([&]() { dm.DeleteDevice("empty-device", 5s); }); + + // Empty device should be in suspended state. + ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device")); + + std::string path; + ASSERT_TRUE(dm.WaitForDevice("empty-device", 5s, &path)); + // Path should exist. + ASSERT_EQ(0, access(path.c_str(), F_OK)); +} diff --git a/fs_mgr/libdm/include/libdm/dm.h b/fs_mgr/libdm/include/libdm/dm.h index 70b14fa46..8fcdf7459 100644 --- a/fs_mgr/libdm/include/libdm/dm.h +++ b/fs_mgr/libdm/include/libdm/dm.h @@ -115,6 +115,19 @@ class DeviceMapper final { // - ACTIVE: resumes the device. bool ChangeState(const std::string& name, DmDeviceState state); + // Creates empty device. + // This supports a use case when a caller doesn't need a device straight away, but instead + // asks kernel to create it beforehand, thus avoiding blocking itself from waiting for ueventd + // to create user space paths. + // Callers are expected to then activate their device by calling LoadTableAndActivate function. + // To avoid race conditions, callers must still synchronize with ueventd by calling + // WaitForDevice function. + bool CreateEmptyDevice(const std::string& name); + + // Waits for device paths to be created in the user space. + bool WaitForDevice(const std::string& name, const std::chrono::milliseconds& timeout_ms, + std::string* path); + // Creates a device, loads the given table, and activates it. If the device // is not able to be activated, it is destroyed, and false is returned. // After creation, |path| contains the result of calling From 3c567b28a92b9fcfacd8671c12112533912adce0 Mon Sep 17 00:00:00 2001 From: Samiul Islam Date: Thu, 26 Aug 2021 15:17:53 +0100 Subject: [PATCH 02/17] libbinder: split out PackageManagerNative aidl Very few clients of libbinder use PackageManagerNative service, as such it's a waste to couple them together. Now, user of PackageManagerNative service need to add the corresponding aidl files as shared library. Bug: 183654927 Test: builds + presubmit Ignore-AOSP-First: To avoid merge conflicts, uploading it internally first. Will be cherry-picked to AOSP later. Change-Id: Ieca32fc3c970f2b720d76071651e85459d082f02 Merged-In: Ieca32fc3c970f2b720d76071651e85459d082f02 --- storaged/Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/storaged/Android.bp b/storaged/Android.bp index 9d5cb482a..b557deed2 100644 --- a/storaged/Android.bp +++ b/storaged/Android.bp @@ -32,6 +32,7 @@ cc_defaults { "libprotobuf-cpp-lite", "libutils", "libz", + "packagemanager_aidl-cpp", ], cflags: [ From 1c496e94e0b7fb4425d05b6b9764700842723dfd Mon Sep 17 00:00:00 2001 From: Alexander Potapenko Date: Wed, 29 Sep 2021 12:14:35 +0000 Subject: [PATCH 03/17] init: introduce ro.kernel.version property This property will hold the major.minor part of the kernel version (e.g. "5.4"), allowing init scripts to act depending on that version, enabling and disabling certain features. Bug: 194156700 Test: manual on a Pixel device Signed-off-by: Alexander Potapenko Merged-In: Icec640b8a7150b344d9aa3bc0bdbcdae050c7c45 Change-Id: I5af411e39da600e5e0f6703f3a2a4930d509e29d --- init/init.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/init/init.cpp b/init/init.cpp index a7325cad9..942feb939 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ @@ -554,6 +555,19 @@ static void SetUsbController() { } } +/// Set ro.kernel.version property to contain the major.minor pair as returned +/// by uname(2). +static void SetKernelVersion() { + struct utsname uts; + unsigned int major, minor; + + if ((uname(&uts) != 0) || (sscanf(uts.release, "%u.%u", &major, &minor) != 2)) { + LOG(ERROR) << "Could not parse the kernel version from uname"; + return; + } + SetProperty("ro.kernel.version", android::base::StringPrintf("%u.%u", major, minor)); +} + static void HandleSigtermSignal(const signalfd_siginfo& siginfo) { if (siginfo.ssi_pid != 0) { // Drop any userspace SIGTERM requests. @@ -824,6 +838,7 @@ int SecondStageMain(int argc, char** argv) { export_oem_lock_status(); MountHandler mount_handler(&epoll); SetUsbController(); + SetKernelVersion(); const BuiltinFunctionMap& function_map = GetBuiltinFunctionMap(); Action::set_function_map(&function_map); From c81fec7d8eda91bdbbed87606bd3c844b5605214 Mon Sep 17 00:00:00 2001 From: Alexander Potapenko Date: Tue, 3 Aug 2021 07:47:40 +0000 Subject: [PATCH 04/17] Restrict creation of bootreceiver tracing instance to 64-bit systems. The main users of this instance are KFENCE and MTE-aided KASAN, which are only supported on arm64. Skip creation of this tracing instance on 32-bit systems to save ~6Mb memory on low-end devices. Bug: 195089948 Bug: 194719088 Bug: 194156700 Test: manual on Pixel device Merged-In: Icaf762715fed7a282b1ad738c10bcb45dc848f4d Change-Id: I61694ce174fa745ef9fd50ca7464b5a9e1d1e011 --- rootdir/init.rc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 376a678a8..d10689a01 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -590,9 +590,10 @@ on late-fs # Load trusted keys from dm-verity protected partitions exec -- /system/bin/fsverity_init --load-verified-keys +on late-fs && property:ro.product.cpu.abilist64=* # Set up a tracing instance for system_server to monitor error_report_end events. # These are sent by kernel tools like KASAN and KFENCE when a memory corruption - # is detected. + # is detected. This is only needed for 64-bit systems. mkdir /sys/kernel/tracing/instances/bootreceiver 0700 system system restorecon_recursive /sys/kernel/tracing/instances/bootreceiver write /sys/kernel/tracing/instances/bootreceiver/buffer_size_kb 1 From 17b1c428d493da1bfc4beffd90b0ac2d0401fc06 Mon Sep 17 00:00:00 2001 From: Alexander Potapenko Date: Wed, 29 Sep 2021 13:30:02 +0000 Subject: [PATCH 05/17] init.rc: disable creation of bootreceiver tracing instance for kernels >=4.9 and <= 5.4 The tracing instance takes extra RAM and is not needed on devices running older kernels. Bug: 194156700 Test: manual on a Pixel device Signed-off-by: Alexander Potapenko Merged-In: I794062741688ebea0e4bc500723a966f8f646ee1 Change-Id: Ie8614e67a89cea67bed88427820fefdf110713c9 --- rootdir/init.rc | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index d10689a01..451595092 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -590,7 +590,20 @@ on late-fs # Load trusted keys from dm-verity protected partitions exec -- /system/bin/fsverity_init --load-verified-keys -on late-fs && property:ro.product.cpu.abilist64=* +# Only enable the bootreceiver tracing instance for kernels 5.10 and above. +on late-fs && property:ro.kernel.version=4.9 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=4.14 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=4.19 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=5.4 + setprop bootreceiver.enable 0 +on late-fs + # Bootreceiver tracing instance is enabled by default. + setprop bootreceiver.enable ${bootreceiver.enable:-1} + +on property:ro.product.cpu.abilist64=* && property:bootreceiver.enable=1 # Set up a tracing instance for system_server to monitor error_report_end events. # These are sent by kernel tools like KASAN and KFENCE when a memory corruption # is detected. This is only needed for 64-bit systems. From 0e28aeb7861eaa4bae72494f8a3e6b152c889fc6 Mon Sep 17 00:00:00 2001 From: Alexander Potapenko Date: Wed, 13 Oct 2021 09:44:34 +0000 Subject: [PATCH 06/17] Revert "Revert "init.rc: disable creation of bootreceiver tracing instance for kernels >=4.9 and <= 5.4"" This reverts commit 220f604ca5f5b844b842a30fe40ad07dc570030f. Reason for revert: from bug 202436407 it looks like the initial revert wasn't needed. Change-Id: I81dba47ee4bc55da2d4c5212dfc1b6200719b8be --- rootdir/init.rc | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index d10689a01..451595092 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -590,7 +590,20 @@ on late-fs # Load trusted keys from dm-verity protected partitions exec -- /system/bin/fsverity_init --load-verified-keys -on late-fs && property:ro.product.cpu.abilist64=* +# Only enable the bootreceiver tracing instance for kernels 5.10 and above. +on late-fs && property:ro.kernel.version=4.9 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=4.14 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=4.19 + setprop bootreceiver.enable 0 +on late-fs && property:ro.kernel.version=5.4 + setprop bootreceiver.enable 0 +on late-fs + # Bootreceiver tracing instance is enabled by default. + setprop bootreceiver.enable ${bootreceiver.enable:-1} + +on property:ro.product.cpu.abilist64=* && property:bootreceiver.enable=1 # Set up a tracing instance for system_server to monitor error_report_end events. # These are sent by kernel tools like KASAN and KFENCE when a memory corruption # is detected. This is only needed for 64-bit systems. From 98e7f427e898c1f7dfe597cc4e15032c6563bbae Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 12 Oct 2021 06:27:30 +0000 Subject: [PATCH 07/17] storageproxyd: discard writes when checkpointing, if necessary If a checkpointing operation is in progress, discard any write operations that are flagged as STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT. In tandem with trusty-side changes that set the flag appropriately, this avoids the awkward case where the checkpoint is rolled back, which potentially leads to inconsistency between the data and the superblock. Based on Stephen's CL/1845477 "Add helper to check checkpoint state of mounts". Original change: https://android-review.googlesource.com/c/platform/system/core/+/1850058 Test: m storageproxyd Bug: 194313068 Change-Id: I0924084f7f0b20018cbb71f5153469c8a686e262 Merged-In: I0924084f7f0b20018cbb71f5153469c8a686e262 (cherry picked from commit 34404f4ab1326eb179f889201dcfd04eb6002d0e) --- .../include/trusty/interface/storage.h | 40 +++++----- trusty/storage/proxy/Android.bp | 3 + trusty/storage/proxy/checkpoint_handling.cpp | 77 +++++++++++++++++++ trusty/storage/proxy/checkpoint_handling.h | 37 +++++++++ trusty/storage/proxy/proxy.c | 16 ++++ 5 files changed, 155 insertions(+), 18 deletions(-) create mode 100644 trusty/storage/proxy/checkpoint_handling.cpp create mode 100644 trusty/storage/proxy/checkpoint_handling.h diff --git a/trusty/storage/interface/include/trusty/interface/storage.h b/trusty/storage/interface/include/trusty/interface/storage.h index b196d88b3..3f1dcb8c6 100644 --- a/trusty/storage/interface/include/trusty/interface/storage.h +++ b/trusty/storage/interface/include/trusty/interface/storage.h @@ -112,26 +112,30 @@ enum storage_file_open_flag { /** * enum storage_msg_flag - protocol-level flags in struct storage_msg - * @STORAGE_MSG_FLAG_BATCH: if set, command belongs to a batch transaction. - * No response will be sent by the server until - * it receives a command with this flag unset, at - * which point a cummulative result for all messages - * sent with STORAGE_MSG_FLAG_BATCH will be sent. - * This is only supported by the non-secure disk proxy - * server. - * @STORAGE_MSG_FLAG_PRE_COMMIT: if set, indicates that server need to commit - * pending changes before processing this message. - * @STORAGE_MSG_FLAG_POST_COMMIT: if set, indicates that server need to commit - * pending changes after processing this message. - * @STORAGE_MSG_FLAG_TRANSACT_COMPLETE: if set, indicates that server need to commit - * current transaction after processing this message. - * It is an alias for STORAGE_MSG_FLAG_POST_COMMIT. + * @STORAGE_MSG_FLAG_BATCH: if set, command belongs to a batch transaction. + * No response will be sent by the server until + * it receives a command with this flag unset, at + * which point a cumulative result for all messages + * sent with STORAGE_MSG_FLAG_BATCH will be sent. + * This is only supported by the non-secure disk proxy + * server. + * @STORAGE_MSG_FLAG_PRE_COMMIT: if set, indicates that server need to commit + * pending changes before processing this message. + * @STORAGE_MSG_FLAG_POST_COMMIT: if set, indicates that server need to commit + * pending changes after processing this message. + * @STORAGE_MSG_FLAG_TRANSACT_COMPLETE: if set, indicates that server need to commit + * current transaction after processing this message. + * It is an alias for STORAGE_MSG_FLAG_POST_COMMIT. + * @STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT: if set, indicates that server needs to ensure + * that there is not a pending checkpoint for + * userdata before processing this message. */ enum storage_msg_flag { - STORAGE_MSG_FLAG_BATCH = 0x1, - STORAGE_MSG_FLAG_PRE_COMMIT = 0x2, - STORAGE_MSG_FLAG_POST_COMMIT = 0x4, - STORAGE_MSG_FLAG_TRANSACT_COMPLETE = STORAGE_MSG_FLAG_POST_COMMIT, + STORAGE_MSG_FLAG_BATCH = 0x1, + STORAGE_MSG_FLAG_PRE_COMMIT = 0x2, + STORAGE_MSG_FLAG_POST_COMMIT = 0x4, + STORAGE_MSG_FLAG_TRANSACT_COMPLETE = STORAGE_MSG_FLAG_POST_COMMIT, + STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT = 0x8, }; /* diff --git a/trusty/storage/proxy/Android.bp b/trusty/storage/proxy/Android.bp index d67089fb2..38d868508 100644 --- a/trusty/storage/proxy/Android.bp +++ b/trusty/storage/proxy/Android.bp @@ -23,6 +23,7 @@ cc_binary { vendor: true, srcs: [ + "checkpoint_handling.cpp", "ipc.c", "rpmb.c", "storage.c", @@ -30,12 +31,14 @@ cc_binary { ], shared_libs: [ + "libbase", "liblog", "libhardware_legacy", ], header_libs: ["libcutils_headers"], static_libs: [ + "libfstab", "libtrustystorageinterface", "libtrusty", ], diff --git a/trusty/storage/proxy/checkpoint_handling.cpp b/trusty/storage/proxy/checkpoint_handling.cpp new file mode 100644 index 000000000..6c2fd363e --- /dev/null +++ b/trusty/storage/proxy/checkpoint_handling.cpp @@ -0,0 +1,77 @@ +/* + * Copyright (C) 2021 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 "checkpoint_handling.h" +#include "log.h" + +#include +#include +#include + +namespace { + +bool checkpointingDoneForever = false; + +} // namespace + +int is_data_checkpoint_active(bool* active) { + if (!active) { + ALOGE("active out parameter is null"); + return 0; + } + + *active = false; + + if (checkpointingDoneForever) { + return 0; + } + + android::fs_mgr::Fstab procMounts; + bool success = android::fs_mgr::ReadFstabFromFile("/proc/mounts", &procMounts); + if (!success) { + ALOGE("Could not parse /proc/mounts\n"); + /* Really bad. Tell the caller to abort the write. */ + return -1; + } + + android::fs_mgr::FstabEntry* dataEntry = + android::fs_mgr::GetEntryForMountPoint(&procMounts, "/data"); + if (dataEntry == NULL) { + ALOGE("/data is not mounted yet\n"); + return 0; + } + + /* We can't handle e.g., ext4. Nothing we can do about it for now. */ + if (dataEntry->fs_type != "f2fs") { + ALOGW("Checkpoint status not supported for filesystem %s\n", dataEntry->fs_type.c_str()); + checkpointingDoneForever = true; + return 0; + } + + /* + * The data entry looks like "... blah,checkpoint=disable:0,blah ...". + * checkpoint=disable means checkpointing is on (yes, arguably reversed). + */ + size_t checkpointPos = dataEntry->fs_options.find("checkpoint=disable"); + if (checkpointPos == std::string::npos) { + /* Assumption is that once checkpointing turns off, it stays off */ + checkpointingDoneForever = true; + } else { + *active = true; + } + + return 0; +} diff --git a/trusty/storage/proxy/checkpoint_handling.h b/trusty/storage/proxy/checkpoint_handling.h new file mode 100644 index 000000000..f1bf27c8d --- /dev/null +++ b/trusty/storage/proxy/checkpoint_handling.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2021 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 + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * is_data_checkpoint_active() - Check for an active, uncommitted checkpoint of + * /data. If a checkpoint is active, storage should not commit any + * rollback-protected writes to /data. + * @active: Out parameter that will be set to the result of the check. + * + * Return: 0 if active was set and is valid, non-zero otherwise. + */ +int is_data_checkpoint_active(bool* active); + +#ifdef __cplusplus +} +#endif diff --git a/trusty/storage/proxy/proxy.c b/trusty/storage/proxy/proxy.c index e23094183..c690a2876 100644 --- a/trusty/storage/proxy/proxy.c +++ b/trusty/storage/proxy/proxy.c @@ -26,6 +26,7 @@ #include +#include "checkpoint_handling.h" #include "ipc.h" #include "log.h" #include "rpmb.h" @@ -130,6 +131,21 @@ static int handle_req(struct storage_msg* msg, const void* req, size_t req_len) } } + if (msg->flags & STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT) { + bool is_checkpoint_active = false; + + rc = is_data_checkpoint_active(&is_checkpoint_active); + if (rc != 0) { + ALOGE("is_data_checkpoint_active failed in an unexpected way. Aborting.\n"); + msg->result = STORAGE_ERR_GENERIC; + return ipc_respond(msg, NULL, 0); + } else if (is_checkpoint_active) { + ALOGE("Checkpoint in progress, dropping write ...\n"); + msg->result = STORAGE_ERR_GENERIC; + return ipc_respond(msg, NULL, 0); + } + } + switch (msg->cmd) { case STORAGE_FILE_DELETE: rc = storage_file_delete(msg, req, req_len); From 567dd77c816b4a8d8fb17eddb8bd894e24beeb18 Mon Sep 17 00:00:00 2001 From: Rick Yiu Date: Wed, 20 Oct 2021 22:52:43 +0800 Subject: [PATCH 08/17] init.rc: Create a new group for dex2oat Create a new group for dex2oat in cpu cgroup, which is dedicated for dex2oat processes. Also modify task profiles for this change. Bug: 201223712 Test: dex2oat group created Change-Id: Ic61f4b8a64d01c03549b680970805e12b9ce4fcc Merged-In: Ic61f4b8a64d01c03549b680970805e12b9ce4fcc --- libprocessgroup/profiles/task_profiles.json | 16 ++++++++++++++-- libprocessgroup/profiles/task_profiles_28.json | 14 +++++++++++++- libprocessgroup/profiles/task_profiles_29.json | 14 +++++++++++++- libprocessgroup/profiles/task_profiles_30.json | 14 +++++++++++++- rootdir/init.rc | 4 ++++ 5 files changed, 57 insertions(+), 5 deletions(-) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index 449a50546..45d3c7c04 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -183,7 +183,19 @@ } ] }, - + { + "Name": "Dex2oatPerformance", + "Actions": [ + { + "Name": "JoinCgroup", + "Params": + { + "Controller": "cpu", + "Path": "dex2oat" + } + } + ] + }, { "Name": "CpuPolicySpread", "Actions": [ @@ -638,7 +650,7 @@ }, { "Name": "Dex2OatBootComplete", - "Profiles": [ "SCHED_SP_BACKGROUND" ] + "Profiles": [ "Dex2oatPerformance", "LowIoPriority", "TimerSlackHigh" ] } ] } diff --git a/libprocessgroup/profiles/task_profiles_28.json b/libprocessgroup/profiles/task_profiles_28.json index 9f8378590..1f32a1390 100644 --- a/libprocessgroup/profiles/task_profiles_28.json +++ b/libprocessgroup/profiles/task_profiles_28.json @@ -104,7 +104,19 @@ } ] }, - + { + "Name": "Dex2oatPerformance", + "Actions": [ + { + "Name": "JoinCgroup", + "Params": + { + "Controller": "schedtune", + "Path": "background" + } + } + ] + }, { "Name": "CpuPolicySpread", "Actions": [ diff --git a/libprocessgroup/profiles/task_profiles_29.json b/libprocessgroup/profiles/task_profiles_29.json index 9f8378590..1f32a1390 100644 --- a/libprocessgroup/profiles/task_profiles_29.json +++ b/libprocessgroup/profiles/task_profiles_29.json @@ -104,7 +104,19 @@ } ] }, - + { + "Name": "Dex2oatPerformance", + "Actions": [ + { + "Name": "JoinCgroup", + "Params": + { + "Controller": "schedtune", + "Path": "background" + } + } + ] + }, { "Name": "CpuPolicySpread", "Actions": [ diff --git a/libprocessgroup/profiles/task_profiles_30.json b/libprocessgroup/profiles/task_profiles_30.json index 9f8378590..1f32a1390 100644 --- a/libprocessgroup/profiles/task_profiles_30.json +++ b/libprocessgroup/profiles/task_profiles_30.json @@ -104,7 +104,19 @@ } ] }, - + { + "Name": "Dex2oatPerformance", + "Actions": [ + { + "Name": "JoinCgroup", + "Params": + { + "Controller": "schedtune", + "Path": "background" + } + } + ] + }, { "Name": "CpuPolicySpread", "Actions": [ diff --git a/rootdir/init.rc b/rootdir/init.rc index 376a678a8..544ea065a 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -155,6 +155,7 @@ on init mkdir /dev/cpuctl/rt mkdir /dev/cpuctl/system mkdir /dev/cpuctl/system-background + mkdir /dev/cpuctl/dex2oat chown system system /dev/cpuctl chown system system /dev/cpuctl/foreground chown system system /dev/cpuctl/background @@ -162,6 +163,7 @@ on init chown system system /dev/cpuctl/rt chown system system /dev/cpuctl/system chown system system /dev/cpuctl/system-background + chown system system /dev/cpuctl/dex2oat chown system system /dev/cpuctl/tasks chown system system /dev/cpuctl/foreground/tasks chown system system /dev/cpuctl/background/tasks @@ -169,6 +171,7 @@ on init chown system system /dev/cpuctl/rt/tasks chown system system /dev/cpuctl/system/tasks chown system system /dev/cpuctl/system-background/tasks + chown system system /dev/cpuctl/dex2oat/tasks chmod 0664 /dev/cpuctl/tasks chmod 0664 /dev/cpuctl/foreground/tasks chmod 0664 /dev/cpuctl/background/tasks @@ -176,6 +179,7 @@ on init chmod 0664 /dev/cpuctl/rt/tasks chmod 0664 /dev/cpuctl/system/tasks chmod 0664 /dev/cpuctl/system-background/tasks + chmod 0664 /dev/cpuctl/dex2oat/tasks # Create a cpu group for NNAPI HAL processes mkdir /dev/cpuctl/nnapi-hal From 4f95599b96bb6431e56cfb93ff4f7e31c4322edf Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 4 Nov 2021 15:29:01 -0700 Subject: [PATCH 09/17] llkd: Disable in userdebug builds by default While llkd helps in discovering issues in apps which leave zombies, it creates issues for dogfooders when apps are killed. Disable it by default. Bug: 202411543 Test: boot and check llkd not running Test: `setprop ro.llk.enable true` enables llkd Signed-off-by: Suren Baghdasaryan Change-Id: If93bf9e981eaa3921a9da5f3160db26c4fe17e66 Merged-In: If93bf9e981eaa3921a9da5f3160db26c4fe17e66 --- llkd/libllkd.cpp | 3 +-- llkd/llkd-debuggable.rc | 2 +- llkd/tests/llkd_test.cpp | 10 +--------- 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/llkd/libllkd.cpp b/llkd/libllkd.cpp index c4c58eef3..42602e9ee 100644 --- a/llkd/libllkd.cpp +++ b/llkd/libllkd.cpp @@ -1283,8 +1283,7 @@ bool llkInit(const char* threadname) { llkEnableSysrqT &= !llkLowRam; if (debuggable) { llkEnableSysrqT |= llkCheckEng(LLK_ENABLE_SYSRQ_T_PROPERTY); - if (!LLK_ENABLE_DEFAULT) { // NB: default is currently true ... - llkEnable |= llkCheckEng(LLK_ENABLE_PROPERTY); + if (!LLK_ENABLE_DEFAULT) { khtEnable |= llkCheckEng(KHT_ENABLE_PROPERTY); } } diff --git a/llkd/llkd-debuggable.rc b/llkd/llkd-debuggable.rc index 4b11b1c7a..c07560910 100644 --- a/llkd/llkd-debuggable.rc +++ b/llkd/llkd-debuggable.rc @@ -1,5 +1,5 @@ on property:ro.debuggable=1 - setprop llk.enable ${ro.llk.enable:-1} + setprop llk.enable ${ro.llk.enable:-0} setprop khungtask.enable ${ro.khungtask.enable:-1} on property:ro.llk.enable=eng diff --git a/llkd/tests/llkd_test.cpp b/llkd/tests/llkd_test.cpp index 475512c38..8eb9b001f 100644 --- a/llkd/tests/llkd_test.cpp +++ b/llkd/tests/llkd_test.cpp @@ -69,13 +69,9 @@ void execute(const char* command) { seconds llkdSleepPeriod(char state) { auto default_eng = android::base::GetProperty(LLK_ENABLE_PROPERTY, "eng") == "eng"; auto default_enable = LLK_ENABLE_DEFAULT; - if (!LLK_ENABLE_DEFAULT && default_eng && - android::base::GetBoolProperty("ro.debuggable", false)) { - default_enable = true; - } default_enable = android::base::GetBoolProperty(LLK_ENABLE_PROPERTY, default_enable); if (default_eng) { - GTEST_LOG_INFO << LLK_ENABLE_PROPERTY " defaults to \"eng\" thus " + GTEST_LOG_INFO << LLK_ENABLE_PROPERTY " defaults to " << (default_enable ? "true" : "false") << "\n"; } // Hail Mary hope is unconfigured. @@ -108,10 +104,6 @@ seconds llkdSleepPeriod(char state) { rest(); } default_enable = LLK_ENABLE_DEFAULT; - if (!LLK_ENABLE_DEFAULT && (android::base::GetProperty(LLK_ENABLE_PROPERTY, "eng") == "eng") && - android::base::GetBoolProperty("ro.debuggable", false)) { - default_enable = true; - } default_enable = android::base::GetBoolProperty(LLK_ENABLE_PROPERTY, default_enable); if (default_enable) { execute("start llkd-1"); From 1c1e267afccf46888e2402a229c6460c575f18f2 Mon Sep 17 00:00:00 2001 From: Yi-Yo Chiang Date: Sat, 11 Sep 2021 19:33:33 +0800 Subject: [PATCH 10/17] Add /system_ext/etc/selinux/ to the debug policy search path for GSI This change only *adds* /system_ext/etc/selinux to the debug policy search path, and does not change any preconditions to load the debug policy. The device still needs to be bootloader-unlocked and has the debug ramdisk flashed to be able to use the debug policy. The only thing changed is that now the debug policy can be loaded from /system_ext or /debug_ramdisk when system partition is compliance testing GSI. The debug policy in the boot ramdisk may be outdated if the system partition is flashed with a image built from a different revision. This happens frequently when running the compliance testing VTS, where the device is flashed with (A) GSI and (B) device vendor image & debug boot image, and (A) and (B) are built from different git revisions. To address this, we install a copy of the debug policy under /system_ext, so that the version desync between (A) & (B) wouldn't be a problem anymore because (A) no longer relies on the debug policy file from (B). Bug: 188067818 Test: Flash RQ2A.201207.001 bramble-user with debug ramdisk & flash gsi_arm64-user from master, device can boot and `adb root` works Change-Id: I4d6235c73472e4d97619b2230292e6a0bc4b3e05 Merged-In: I4d6235c73472e4d97619b2230292e6a0bc4b3e05 (cherry picked from commit 650b29d2349253a0dd6000564ccb2c19b51352db) --- init/Android.bp | 23 ++++++++++++++++++++++- init/first_stage_init.cpp | 19 +++++++++++++------ init/selinux.cpp | 30 +++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/init/Android.bp b/init/Android.bp index 7eeafa24b..a57f3a407 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -89,7 +89,19 @@ init_host_sources = [ "host_init_verifier.cpp", ] -cc_defaults { +soong_config_module_type { + name: "libinit_cc_defaults", + module_type: "cc_defaults", + config_namespace: "ANDROID", + bool_variables: [ + "PRODUCT_INSTALL_DEBUG_POLICY_TO_SYSTEM_EXT", + ], + properties: [ + "cflags", + ], +} + +libinit_cc_defaults { name: "init_defaults", sanitize: { misc_undefined: ["signed-integer-overflow"], @@ -109,6 +121,7 @@ cc_defaults { "-DDUMP_ON_UMOUNT_FAILURE=0", "-DSHUTDOWN_ZERO_TIMEOUT=0", "-DINIT_FULL_SOURCES", + "-DINSTALL_DEBUG_POLICY_TO_SYSTEM_EXT=0", ], product_variables: { debuggable: { @@ -137,6 +150,14 @@ cc_defaults { cppflags: ["-DUSER_MODE_LINUX"], }, }, + soong_config_variables: { + PRODUCT_INSTALL_DEBUG_POLICY_TO_SYSTEM_EXT: { + cflags: [ + "-UINSTALL_DEBUG_POLICY_TO_SYSTEM_EXT", + "-DINSTALL_DEBUG_POLICY_TO_SYSTEM_EXT=1", + ], + }, + }, static_libs: [ "libavb", "libc++fs", diff --git a/init/first_stage_init.cpp b/init/first_stage_init.cpp index 78e5b60a1..c7b7b0c13 100644 --- a/init/first_stage_init.cpp +++ b/init/first_stage_init.cpp @@ -330,14 +330,21 @@ int FirstStageMain(int argc, char** argv) { // If "/force_debuggable" is present, the second-stage init will use a userdebug // sepolicy and load adb_debug.prop to allow adb root, if the device is unlocked. if (access("/force_debuggable", F_OK) == 0) { + constexpr const char adb_debug_prop_src[] = "/adb_debug.prop"; + constexpr const char userdebug_plat_sepolicy_cil_src[] = "/userdebug_plat_sepolicy.cil"; std::error_code ec; // to invoke the overloaded copy_file() that won't throw. - if (!fs::copy_file("/adb_debug.prop", kDebugRamdiskProp, ec) || - !fs::copy_file("/userdebug_plat_sepolicy.cil", kDebugRamdiskSEPolicy, ec)) { - LOG(ERROR) << "Failed to setup debug ramdisk"; - } else { - // setenv for second-stage init to read above kDebugRamdisk* files. - setenv("INIT_FORCE_DEBUGGABLE", "true", 1); + if (access(adb_debug_prop_src, F_OK) == 0 && + !fs::copy_file(adb_debug_prop_src, kDebugRamdiskProp, ec)) { + LOG(WARNING) << "Can't copy " << adb_debug_prop_src << " to " << kDebugRamdiskProp + << ": " << ec.message(); } + if (access(userdebug_plat_sepolicy_cil_src, F_OK) == 0 && + !fs::copy_file(userdebug_plat_sepolicy_cil_src, kDebugRamdiskSEPolicy, ec)) { + LOG(WARNING) << "Can't copy " << userdebug_plat_sepolicy_cil_src << " to " + << kDebugRamdiskSEPolicy << ": " << ec.message(); + } + // setenv for second-stage init to read above kDebugRamdisk* files. + setenv("INIT_FORCE_DEBUGGABLE", "true", 1); } if (ForceNormalBoot(cmdline, bootconfig)) { diff --git a/init/selinux.cpp b/init/selinux.cpp index 42d302324..29c0ff3ba 100644 --- a/init/selinux.cpp +++ b/init/selinux.cpp @@ -295,6 +295,25 @@ bool IsSplitPolicyDevice() { return access(plat_policy_cil_file, R_OK) != -1; } +std::optional GetUserdebugPlatformPolicyFile() { + // See if we need to load userdebug_plat_sepolicy.cil instead of plat_sepolicy.cil. + const char* force_debuggable_env = getenv("INIT_FORCE_DEBUGGABLE"); + if (force_debuggable_env && "true"s == force_debuggable_env && AvbHandle::IsDeviceUnlocked()) { + const std::vector debug_policy_candidates = { +#if INSTALL_DEBUG_POLICY_TO_SYSTEM_EXT == 1 + "/system_ext/etc/selinux/userdebug_plat_sepolicy.cil", +#endif + kDebugRamdiskSEPolicy, + }; + for (const char* debug_policy : debug_policy_candidates) { + if (access(debug_policy, F_OK) == 0) { + return debug_policy; + } + } + } + return std::nullopt; +} + struct PolicyFile { unique_fd fd; std::string path; @@ -310,13 +329,10 @@ bool OpenSplitPolicy(PolicyFile* policy_file) { // secilc is invoked to compile the above three policy files into a single monolithic policy // file. This file is then loaded into the kernel. - // See if we need to load userdebug_plat_sepolicy.cil instead of plat_sepolicy.cil. - const char* force_debuggable_env = getenv("INIT_FORCE_DEBUGGABLE"); - bool use_userdebug_policy = - ((force_debuggable_env && "true"s == force_debuggable_env) && - AvbHandle::IsDeviceUnlocked() && access(kDebugRamdiskSEPolicy, F_OK) == 0); + const auto userdebug_plat_sepolicy = GetUserdebugPlatformPolicyFile(); + const bool use_userdebug_policy = userdebug_plat_sepolicy.has_value(); if (use_userdebug_policy) { - LOG(WARNING) << "Using userdebug system sepolicy"; + LOG(INFO) << "Using userdebug system sepolicy " << *userdebug_plat_sepolicy; } // Load precompiled policy from vendor image, if a matching policy is found there. The policy @@ -413,7 +429,7 @@ bool OpenSplitPolicy(PolicyFile* policy_file) { // clang-format off std::vector compile_args { "/system/bin/secilc", - use_userdebug_policy ? kDebugRamdiskSEPolicy: plat_policy_cil_file, + use_userdebug_policy ? *userdebug_plat_sepolicy : plat_policy_cil_file, "-m", "-M", "true", "-G", "-N", "-c", version_as_string.c_str(), plat_mapping_file.c_str(), From 437522958814ef523d62d7fa988fef495cf7f6f4 Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Wed, 1 Dec 2021 11:04:45 +0000 Subject: [PATCH 11/17] snapuserd: Address alignment fault on 32-bit systems When the scratch space is mmap'ed, the metadata buffer will be un-aligned. This may lead to alignment fault on 32-bit systems. Address this by temporarily copying it to buffer. No perf impact as this code path is not in I/O path and the copy is a for the size of metadata buffer which is 8k. Bug: 206426215 Test: Full and Incremental OTA on pixel 1: Compile snapuserd as 32 bit and reproduced the bug on pixel. 2: With fix - OTA applied successfully. 3: Reboot the device when merge was in-flight as the fix is primarily in that path. 4: Verify merge completion and data integrity post merge. Signed-off-by: Akilesh Kailash Change-Id: Icd4a21d6a61f1ab36e65994c06a4d049a2ee741c Merged-In: I63c0d862057ebf138c9d1696a942030e30598739 --- fs_mgr/libsnapshot/snapuserd_readahead.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/snapuserd_readahead.cpp b/fs_mgr/libsnapshot/snapuserd_readahead.cpp index 16d5919a5..e55fea382 100644 --- a/fs_mgr/libsnapshot/snapuserd_readahead.cpp +++ b/fs_mgr/libsnapshot/snapuserd_readahead.cpp @@ -226,9 +226,15 @@ bool ReadAheadThread::ReconstructDataFromCow() { int num_ops = 0; int total_blocks_merged = 0; + // This memcpy is important as metadata_buffer_ will be an unaligned address and will fault + // on 32-bit systems + std::unique_ptr metadata_buffer = + std::make_unique(snapuserd_->GetBufferMetadataSize()); + memcpy(metadata_buffer.get(), metadata_buffer_, snapuserd_->GetBufferMetadataSize()); + while (true) { struct ScratchMetadata* bm = reinterpret_cast( - (char*)metadata_buffer_ + metadata_offset); + (char*)metadata_buffer.get() + metadata_offset); // Done reading metadata if (bm->new_block == 0 && bm->file_offset == 0) { From d2b270c06301e70ed0b3624fb2d0d439fdc2559c Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Thu, 9 Dec 2021 18:51:28 -0800 Subject: [PATCH 12/17] libutils: do not follow process's group 1) App doesn't have cgroup access and there is no purpose of reading cgroup for app. For system_server it should be known in foreground group. So there is no benefit of reading group. 2) Reading cgroup in apps can also cause contention for other cgroup operations. 3) vendor can change cgroup setting and get_sched_policy may return incorrect information for get_sched_policy_profile_name. Test: Boot Bug: 210011562 Signed-off-by: Wei Wang Change-Id: I8e8c8b346984781c56ec93c0616121f7d5c99fe5 --- libutils/Threads.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libutils/Threads.cpp b/libutils/Threads.cpp index 6e293c741..3bf577961 100644 --- a/libutils/Threads.cpp +++ b/libutils/Threads.cpp @@ -317,10 +317,7 @@ int androidSetThreadPriority(pid_t tid, int pri) if (pri >= ANDROID_PRIORITY_BACKGROUND) { rc = SetTaskProfiles(tid, {"SCHED_SP_SYSTEM"}, true) ? 0 : -1; } else if (curr_pri >= ANDROID_PRIORITY_BACKGROUND) { - SchedPolicy policy = SP_FOREGROUND; - // Change to the sched policy group of the process. - get_sched_policy(getpid(), &policy); - rc = SetTaskProfiles(tid, {get_sched_policy_profile_name(policy)}, true) ? 0 : -1; + rc = SetTaskProfiles(tid, {"SCHED_SP_FOREGROUND"}, true) ? 0 : -1; } if (rc) { From cf4b15e61fc7ac1202c645f2daea520048629a82 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Mon, 13 Dec 2021 17:42:08 -0800 Subject: [PATCH 13/17] libprocessgroup: fall back to cpuset in get_sched_policy Since vendor has a way to override the group cpu/schedtune setup, we cannot assume the group will always return valid data. This CL let get_sched_policy to fallback to cpuset if no valid data found in cpu/schedtune cgroup. In longer term, we should find a way to cache the group or app's process state in framework other than relying on reading cgroup back. Test: /data/nativetest64/libcutils_test/libcutils_test Bug: 210066228 Signed-off-by: Wei Wang Change-Id: I8b4396365a7fc2d93e3a22746195585c140eef3c --- libprocessgroup/sched_policy.cpp | 51 +++++++++++++++++++------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/libprocessgroup/sched_policy.cpp b/libprocessgroup/sched_policy.cpp index 1a4196a4d..169b1d3e0 100644 --- a/libprocessgroup/sched_policy.cpp +++ b/libprocessgroup/sched_policy.cpp @@ -165,27 +165,7 @@ static int getCGroupSubsys(int tid, const char* subsys, std::string& subgroup) { return 0; } -int get_sched_policy(int tid, SchedPolicy* policy) { - if (tid == 0) { - tid = GetThreadId(); - } - - std::string group; - if (schedboost_enabled()) { - if ((getCGroupSubsys(tid, "schedtune", group) < 0) && - (getCGroupSubsys(tid, "cpu", group) < 0)) { - LOG(ERROR) << "Failed to find cpu cgroup for tid " << tid; - return -1; - } - } - if (group.empty() && cpusets_enabled()) { - if (getCGroupSubsys(tid, "cpuset", group) < 0) { - LOG(ERROR) << "Failed to find cpuset cgroup for tid " << tid; - return -1; - } - } - - // TODO: replace hardcoded directories +static int get_sched_policy_from_group(const std::string& group, SchedPolicy* policy) { if (group.empty()) { *policy = SP_FOREGROUND; } else if (group == "foreground") { @@ -205,6 +185,35 @@ int get_sched_policy(int tid, SchedPolicy* policy) { return 0; } +int get_sched_policy(int tid, SchedPolicy* policy) { + if (tid == 0) { + tid = GetThreadId(); + } + + std::string group; + if (schedboost_enabled()) { + if ((getCGroupSubsys(tid, "schedtune", group) < 0) && + (getCGroupSubsys(tid, "cpu", group) < 0)) { + LOG(ERROR) << "Failed to find cpu cgroup for tid " << tid; + return -1; + } + // Wipe invalid group to fallback to cpuset + if (!group.empty()) { + if (get_sched_policy_from_group(group, policy) < 0) { + group.clear(); + } else { + return 0; + } + } + } + + if (cpusets_enabled() && getCGroupSubsys(tid, "cpuset", group) < 0) { + LOG(ERROR) << "Failed to find cpuset cgroup for tid " << tid; + return -1; + } + return get_sched_policy_from_group(group, policy); +} + #else /* Stubs for non-Android targets. */ From 654bb5225bcaedb388839b60675c3751573bb174 Mon Sep 17 00:00:00 2001 From: Nikita Ioffe Date: Thu, 15 Jul 2021 15:30:09 +0100 Subject: [PATCH 14/17] Deflake libdm#CreateEmptyDevice test Judging from local experiments, it looks like device-mapper doesn't always generate a uevent after DM_DEV_CREATE ioctl. Test: presubmit Bug: 193462349 Change-Id: I8a74375631b20c14a32a41dbaf38380ebc0078e6 Merged-In: I8a74375631b20c14a32a41dbaf38380ebc0078e6 --- fs_mgr/libdm/dm_test.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index 8314ec596..541f254cb 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -684,13 +684,9 @@ TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { TEST(libdm, CreateEmptyDevice) { DeviceMapper& dm = DeviceMapper::Instance(); ASSERT_TRUE(dm.CreateEmptyDevice("empty-device")); - auto guard = android::base::make_scope_guard([&]() { dm.DeleteDevice("empty-device", 5s); }); + auto guard = + android::base::make_scope_guard([&]() { dm.DeleteDeviceIfExists("empty-device", 5s); }); // Empty device should be in suspended state. ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device")); - - std::string path; - ASSERT_TRUE(dm.WaitForDevice("empty-device", 5s, &path)); - // Path should exist. - ASSERT_EQ(0, access(path.c_str(), F_OK)); } From 6834fe66d7995b4016dcaa652c847f1948cf1571 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 16 Jul 2021 20:02:11 -0700 Subject: [PATCH 15/17] libsnapshot: Propagate merge phase across merge failures. If a merge fails we write a new snapshot status indicating that the merge failed. If this happens to occur during the second merge phase, we fail to propagate the phase counter to the new status. This means the merge is unlikely to make progress and succeed later. Bug: 213031779 Bug: 213253413 Bug: 193549218 Ignore-AOSP-First: cherry-pick from AOSP Test: inject transient failure into CheckMergeConsistency, apply OTA, reboot and complete merge. Change-Id: I31fdae6bde48e3a71b6f3fcc663541257f7ebd8f Merged-In: I31fdae6bde48e3a71b6f3fcc663541257f7ebd8f --- fs_mgr/libsnapshot/snapshot.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 4c94da28f..9a5f6902f 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2556,6 +2556,7 @@ bool SnapshotManager::WriteUpdateState(LockedFile* lock, UpdateState state, SnapshotUpdateStatus old_status = ReadSnapshotUpdateStatus(lock); status.set_compression_enabled(old_status.compression_enabled()); status.set_source_build_fingerprint(old_status.source_build_fingerprint()); + status.set_merge_phase(old_status.merge_phase()); } return WriteSnapshotUpdateStatus(lock, status); } From 739f4f5f609846f11072ec5e0a45fdd8c4225e29 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 6 Jan 2022 02:26:08 +0000 Subject: [PATCH 16/17] libsnapshot: Fix CHECK failure during second phase merge This CHECK prevents a release build from resuming a two-phase merge if the merge initially failed in the first pass. Bug: 213031779 Bug: 213253413 Bug: 193549218 Ignore-AOSP-First: cherry-pick from AOSP Test: vts_libsnapshot_test Test: update_engine_unittests Change-Id: I8bf00e3016546ef7039bb0b18eb977cc3dc1066a Merged-In: I8bf00e3016546ef7039bb0b18eb977cc3dc1066a --- .../include/libsnapshot/snapshot.h | 13 +++ fs_mgr/libsnapshot/snapshot.cpp | 11 ++- fs_mgr/libsnapshot/snapshot_test.cpp | 86 +++++++++++++++++++ 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 9bf5db18e..69d89676b 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -386,6 +386,17 @@ class SnapshotManager final : public ISnapshotManager { // first-stage to decide whether to launch snapuserd. bool IsSnapuserdRequired(); + // Add new public entries above this line. + + // Helpers for failure injection. + using MergeConsistencyChecker = + std::function; + + void set_merge_consistency_checker(MergeConsistencyChecker checker) { + merge_consistency_checker_ = checker; + } + MergeConsistencyChecker merge_consistency_checker() const { return merge_consistency_checker_; } + private: FRIEND_TEST(SnapshotTest, CleanFirstStageMount); FRIEND_TEST(SnapshotTest, CreateSnapshot); @@ -400,6 +411,7 @@ class SnapshotManager final : public ISnapshotManager { FRIEND_TEST(SnapshotTest, NoMergeBeforeReboot); FRIEND_TEST(SnapshotTest, UpdateBootControlHal); FRIEND_TEST(SnapshotUpdateTest, AddPartition); + FRIEND_TEST(SnapshotUpdateTest, ConsistencyCheckResume); FRIEND_TEST(SnapshotUpdateTest, DaemonTransition); FRIEND_TEST(SnapshotUpdateTest, DataWipeAfterRollback); FRIEND_TEST(SnapshotUpdateTest, DataWipeRollbackInRecovery); @@ -782,6 +794,7 @@ class SnapshotManager final : public ISnapshotManager { std::function uevent_regen_callback_; std::unique_ptr snapuserd_client_; std::unique_ptr old_partition_metadata_; + MergeConsistencyChecker merge_consistency_checker_; }; } // namespace snapshot diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 9a5f6902f..40cb35be1 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -87,6 +87,8 @@ static constexpr char kBootIndicatorPath[] = "/metadata/ota/snapshot-boot"; static constexpr char kRollbackIndicatorPath[] = "/metadata/ota/rollback-indicator"; static constexpr auto kUpdateStateCheckInterval = 2s; +MergeFailureCode CheckMergeConsistency(const std::string& name, const SnapshotStatus& status); + // Note: IImageManager is an incomplete type in the header, so the default // destructor doesn't work. SnapshotManager::~SnapshotManager() {} @@ -116,6 +118,7 @@ std::unique_ptr SnapshotManager::NewForFirstStageMount(IDeviceI SnapshotManager::SnapshotManager(IDeviceInfo* device) : device_(device) { metadata_dir_ = device_->GetMetadataDir(); + merge_consistency_checker_ = android::snapshot::CheckMergeConsistency; } static std::string GetCowName(const std::string& snapshot_name) { @@ -1175,6 +1178,10 @@ MergeFailureCode SnapshotManager::CheckMergeConsistency(LockedFile* lock, const const SnapshotStatus& status) { CHECK(lock); + return merge_consistency_checker_(name, status); +} + +MergeFailureCode CheckMergeConsistency(const std::string& name, const SnapshotStatus& status) { if (!status.compression_enabled()) { // Do not try to verify old-style COWs yet. return MergeFailureCode::Ok; @@ -1252,9 +1259,11 @@ MergeFailureCode SnapshotManager::MergeSecondPhaseSnapshots(LockedFile* lock) { } SnapshotUpdateStatus update_status = ReadSnapshotUpdateStatus(lock); - CHECK(update_status.state() == UpdateState::Merging); + CHECK(update_status.state() == UpdateState::Merging || + update_status.state() == UpdateState::MergeFailed); CHECK(update_status.merge_phase() == MergePhase::FIRST_PHASE); + update_status.set_state(UpdateState::Merging); update_status.set_merge_phase(MergePhase::SECOND_PHASE); if (!WriteSnapshotUpdateStatus(lock, update_status)) { return MergeFailureCode::WriteStatus; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 7630efe3f..3a3aedc57 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1297,6 +1297,92 @@ TEST_F(SnapshotUpdateTest, SpaceSwapUpdate) { } } +// Test that a transient merge consistency check failure can resume properly. +TEST_F(SnapshotUpdateTest, ConsistencyCheckResume) { + if (!IsCompressionEnabled()) { + // b/179111359 + GTEST_SKIP() << "Skipping Virtual A/B Compression test"; + } + + auto old_sys_size = GetSize(sys_); + auto old_prd_size = GetSize(prd_); + + // Grow |sys| but shrink |prd|. + SetSize(sys_, old_sys_size * 2); + sys_->set_estimate_cow_size(8_MiB); + SetSize(prd_, old_prd_size / 2); + prd_->set_estimate_cow_size(1_MiB); + + AddOperationForPartitions(); + + ASSERT_TRUE(sm->BeginUpdate()); + ASSERT_TRUE(sm->CreateUpdateSnapshots(manifest_)); + ASSERT_TRUE(WriteSnapshotAndHash("sys_b")); + ASSERT_TRUE(WriteSnapshotAndHash("vnd_b")); + ASSERT_TRUE(ShiftAllSnapshotBlocks("prd_b", old_prd_size)); + + sync(); + + // Assert that source partitions aren't affected. + for (const auto& name : {"sys_a", "vnd_a", "prd_a"}) { + ASSERT_TRUE(IsPartitionUnchanged(name)); + } + + ASSERT_TRUE(sm->FinishedSnapshotWrites(false)); + + // Simulate shutting down the device. + ASSERT_TRUE(UnmapAll()); + + // After reboot, init does first stage mount. + auto init = NewManagerForFirstStageMount("_b"); + ASSERT_NE(init, nullptr); + ASSERT_TRUE(init->NeedSnapshotsInFirstStageMount()); + ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); + + // Check that the target partitions have the same content. + for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) { + ASSERT_TRUE(IsPartitionUnchanged(name)); + } + + auto old_checker = init->merge_consistency_checker(); + + init->set_merge_consistency_checker( + [](const std::string&, const SnapshotStatus&) -> MergeFailureCode { + return MergeFailureCode::WrongMergeCountConsistencyCheck; + }); + + // Initiate the merge and wait for it to be completed. + ASSERT_TRUE(init->InitiateMerge()); + { + // Check that the merge phase is FIRST_PHASE until at least one call + // to ProcessUpdateState() occurs. + ASSERT_TRUE(AcquireLock()); + auto local_lock = std::move(lock_); + auto status = init->ReadSnapshotUpdateStatus(local_lock.get()); + ASSERT_EQ(status.merge_phase(), MergePhase::FIRST_PHASE); + } + + // Merge should have failed. + ASSERT_EQ(UpdateState::MergeFailed, init->ProcessUpdateState()); + + // Simulate shutting down the device and creating partitions again. + ASSERT_TRUE(UnmapAll()); + + // Restore the checker. + init->set_merge_consistency_checker(std::move(old_checker)); + + ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); + + // Complete the merge. + ASSERT_EQ(UpdateState::MergeCompleted, init->ProcessUpdateState()); + + // Check that the target partitions have the same content after the merge. + for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) { + ASSERT_TRUE(IsPartitionUnchanged(name)) + << "Content of " << name << " changes after the merge"; + } +} + // Test that if new system partitions uses empty space in super, that region is not snapshotted. TEST_F(SnapshotUpdateTest, DirectWriteEmptySpace) { GTEST_SKIP() << "b/141889746"; From 71429a9738471920772df0d5d50a820aa0d50fbe Mon Sep 17 00:00:00 2001 From: Kalesh Singh Date: Mon, 6 Dec 2021 16:13:06 -0800 Subject: [PATCH 17/17] Add group ID for reading tracefs Add AID_READTRACEFS and mount tracefs with gid=AID_READTRACEFS Bug: 209513178 Bug: 214061655 Test: adb shell ls -l /sys/kernel/tracing/events Change-Id: Ibbfdf8a4b771bd7520ecbaaf15a1153d6bf0e599 Merged-In: Ibbfdf8a4b771bd7520ecbaaf15a1153d6bf0e599 --- libcutils/include/private/android_filesystem_config.h | 1 + rootdir/init.rc | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libcutils/include/private/android_filesystem_config.h b/libcutils/include/private/android_filesystem_config.h index 8f22d8983..ffd4d1285 100644 --- a/libcutils/include/private/android_filesystem_config.h +++ b/libcutils/include/private/android_filesystem_config.h @@ -157,6 +157,7 @@ #define AID_READPROC 3009 /* Allow /proc read access */ #define AID_WAKELOCK 3010 /* Allow system wakelock read/write access */ #define AID_UHID 3011 /* Allow read/write to /dev/uhid node */ +#define AID_READTRACEFS 3012 /* Allow tracefs read */ /* The range 5000-5999 is also reserved for vendor partition. */ #define AID_OEM_RESERVED_2_START 5000 diff --git a/rootdir/init.rc b/rootdir/init.rc index 3f5876f48..5116c0fea 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -78,8 +78,8 @@ on early-init mkdir /dev/boringssl 0755 root root mkdir /dev/boringssl/selftest 0755 root root - # Mount tracefs - mount tracefs tracefs /sys/kernel/tracing + # Mount tracefs (with GID=AID_READTRACEFS) + mount tracefs tracefs /sys/kernel/tracing gid=3012 # create sys dirctory mkdir /dev/sys 0755 system system