From ff61ef9ef7fe5377e36a0e5551fcd5b595a864e9 Mon Sep 17 00:00:00 2001 From: Shawn Willden Date: Thu, 17 Feb 2022 15:49:27 -0700 Subject: [PATCH 01/45] Provide alternate SE RoT provisioning path. On some devices it is infeasible to provision the KeyMint RoT bits in the Android Bootloader. This provides an alternate path to provision them from the TEE during early boot. Bug: 219076736 Test: VtsAidlKeyMintTargetTest Change-Id: Ibae9050b9a102dad3710f9495d3dfa43fa1d1b3f --- .../trusty_keymaster/TrustyKeyMintDevice.h | 10 ++++++++-- trusty/keymaster/keymint/TrustyKeyMintDevice.cpp | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/trusty/keymaster/include/trusty_keymaster/TrustyKeyMintDevice.h b/trusty/keymaster/include/trusty_keymaster/TrustyKeyMintDevice.h index 5fd628f3c..c8d8932c4 100644 --- a/trusty/keymaster/include/trusty_keymaster/TrustyKeyMintDevice.h +++ b/trusty/keymaster/include/trusty_keymaster/TrustyKeyMintDevice.h @@ -27,6 +27,7 @@ namespace aidl::android::hardware::security::keymint::trusty { using ::keymaster::TrustyKeymaster; using ::ndk::ScopedAStatus; using secureclock::TimeStampToken; +using ::std::array; using ::std::optional; using ::std::shared_ptr; using ::std::vector; @@ -77,8 +78,13 @@ class TrustyKeyMintDevice : public BnKeyMintDevice { const optional& timestampToken) override; ScopedAStatus earlyBootEnded() override; - ScopedAStatus convertStorageKeyToEphemeral(const std::vector& storageKeyBlob, - std::vector* ephemeralKeyBlob) override; + ScopedAStatus convertStorageKeyToEphemeral(const vector& storageKeyBlob, + vector* ephemeralKeyBlob) override; + + ScopedAStatus getRootOfTrustChallenge(array* challenge) override; + ScopedAStatus getRootOfTrust(const array& challenge, + vector* rootOfTrust) override; + ScopedAStatus sendRootOfTrust(const vector& rootOfTrust) override; protected: std::shared_ptr impl_; diff --git a/trusty/keymaster/keymint/TrustyKeyMintDevice.cpp b/trusty/keymaster/keymint/TrustyKeyMintDevice.cpp index 68a791259..44780e835 100644 --- a/trusty/keymaster/keymint/TrustyKeyMintDevice.cpp +++ b/trusty/keymaster/keymint/TrustyKeyMintDevice.cpp @@ -306,7 +306,7 @@ ScopedAStatus TrustyKeyMintDevice::earlyBootEnded() { } ScopedAStatus TrustyKeyMintDevice::convertStorageKeyToEphemeral( - const std::vector& storageKeyBlob, std::vector* ephemeralKeyBlob) { + const vector& storageKeyBlob, vector* ephemeralKeyBlob) { keymaster::ExportKeyRequest request(impl_->message_version()); request.SetKeyMaterial(storageKeyBlob.data(), storageKeyBlob.size()); request.key_format = KM_KEY_FORMAT_RAW; @@ -321,4 +321,17 @@ ScopedAStatus TrustyKeyMintDevice::convertStorageKeyToEphemeral( return ScopedAStatus::ok(); } +ScopedAStatus TrustyKeyMintDevice::getRootOfTrustChallenge(array* /* challenge */) { + return kmError2ScopedAStatus(KM_ERROR_UNIMPLEMENTED); +} + +ScopedAStatus TrustyKeyMintDevice::getRootOfTrust(const array& /* challenge */, + vector* /* rootOfTrust */) { + return kmError2ScopedAStatus(KM_ERROR_UNIMPLEMENTED); +} + +ScopedAStatus TrustyKeyMintDevice::sendRootOfTrust(const vector& /* rootOfTrust */) { + return kmError2ScopedAStatus(KM_ERROR_UNIMPLEMENTED); +} + } // namespace aidl::android::hardware::security::keymint::trusty From 24d3ac349a98350ce12b5f8a3bc1917dd4d16a7a Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Fri, 11 Feb 2022 17:26:28 -0500 Subject: [PATCH 02/45] Fix OkOrFail conversion ambiguities OkOrFail has specialized conversions for Result to avoid ambiguous implicit conversion sequences. Since user conversion operators sequences can be followed by integral promotion, specializing for integral types is necessary. Specialize ResultError so calling code() returns a status_t instead of a StatusT and message() is implemented even when not carrying a string. Eventually, these classes should be combined. Add equality operators for ResultError. Bug: 219580167 Test: atest Errors_test.cpp Ignore-AOSP-First: Internal topic dependencies Change-Id: I14acecfd2aef33c40e79ddb091e2f4af9291d837 --- libutils/Errors_test.cpp | 62 +++++++++++++++ libutils/include/utils/ErrorsMacros.h | 108 +++++++++++++++++++++++++- 2 files changed, 166 insertions(+), 4 deletions(-) diff --git a/libutils/Errors_test.cpp b/libutils/Errors_test.cpp index 873c99490..0d13bb03c 100644 --- a/libutils/Errors_test.cpp +++ b/libutils/Errors_test.cpp @@ -108,3 +108,65 @@ TEST(errors, result_in_status) { status_t b = g(false); EXPECT_EQ(PERMISSION_DENIED, b); } + +TEST(errors, conversion_promotion) { + constexpr size_t successVal = 10ull; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +TEST(errors, conversion_promotion_bool) { + constexpr size_t successVal = true; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +TEST(errors, conversion_promotion_char) { + constexpr char successVal = 'a'; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +struct IntContainer { + // Implicit conversion from int is desired + IntContainer(int val) : val_(val) {} + int val_; +}; + +TEST(errors, conversion_construct) { + constexpr int successVal = 10; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value().val_, successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} diff --git a/libutils/include/utils/ErrorsMacros.h b/libutils/include/utils/ErrorsMacros.h index 048c5386f..fdc46e617 100644 --- a/libutils/include/utils/ErrorsMacros.h +++ b/libutils/include/utils/ErrorsMacros.h @@ -25,6 +25,7 @@ // [1] build/soong/cc/config/global.go#commonGlobalIncludes #include #include +#include #include @@ -44,13 +45,58 @@ struct StatusT { status_t val_; }; + namespace base { +// TODO(b/221235365) StatusT fulfill ResultError contract and cleanup. + +// Unlike typical ResultError types, the underlying code should be a status_t +// instead of a StatusT. We also special-case message generation. +template<> +struct ResultError { + ResultError(status_t s) : val_(s) { + LOG_FATAL_IF(s == OK, "Result error should not hold success"); + } + + template + operator expected>() const { + return unexpected(*this); + } + + std::string message() const { return statusToString(val_); } + status_t code() const { return val_; } + + private: + const status_t val_; +}; + +template<> +struct ResultError { + template + ResultError(T&& message, status_t s) : val_(s), message_(std::forward(message)) { + LOG_FATAL_IF(s == OK, "Result error should not hold success"); + } + + ResultError(status_t s) : val_(s) {} + + template + operator expected>() const { + return unexpected(*this); + } + + status_t code() const { return val_; } + + std::string message() const { return statusToString(val_) + message_; } + private: + const status_t val_; + std::string message_; +}; // Specialization of android::base::OkOrFail for V = status_t. This is used to use the OR_RETURN // and OR_FATAL macros with statements that yields a value of status_t. See android-base/errors.h // for the detailed contract. template <> struct OkOrFail { + static_assert(std::is_same_v); // Tests if status_t is a success value of not. static bool IsOk(const status_t& s) { return s == OK; } @@ -71,16 +117,70 @@ struct OkOrFail { // Or converts into Result. This is used when OR_RETURN is used in a function whose // return type is Result. - template >> + + template operator Result() && { - return Error(std::move(val_)); + return ResultError(std::move(val_)); } - operator Result() && { return Error(std::move(val_)); } + template + operator Result() && { + return ResultError(std::move(val_)); + } + // Since user defined conversion can be followed by numeric conversion, + // we have to specialize all conversions to results holding numeric types to + // avoid conversion ambiguities with the constructor of expected. +#pragma push_macro("SPECIALIZED_CONVERSION") +#define SPECIALIZED_CONVERSION(type)\ + operator Result() && { return ResultError(std::move(val_)); }\ + operator Result() && { return ResultError(std::move(val_));} + + SPECIALIZED_CONVERSION(int) + SPECIALIZED_CONVERSION(short int) + SPECIALIZED_CONVERSION(unsigned short int) + SPECIALIZED_CONVERSION(unsigned int) + SPECIALIZED_CONVERSION(long int) + SPECIALIZED_CONVERSION(unsigned long int) + SPECIALIZED_CONVERSION(long long int) + SPECIALIZED_CONVERSION(unsigned long long int) + SPECIALIZED_CONVERSION(bool) + SPECIALIZED_CONVERSION(char) + SPECIALIZED_CONVERSION(unsigned char) + SPECIALIZED_CONVERSION(signed char) + SPECIALIZED_CONVERSION(wchar_t) + SPECIALIZED_CONVERSION(char16_t) + SPECIALIZED_CONVERSION(char32_t) + SPECIALIZED_CONVERSION(float) + SPECIALIZED_CONVERSION(double) + SPECIALIZED_CONVERSION(long double) +#undef SPECIALIZED_CONVERSION +#pragma pop_macro("SPECIALIZED_CONVERSION") // String representation of the error value. static std::string ErrorMessage(const status_t& s) { return statusToString(s); } }; - } // namespace base + + +// These conversions make StatusT directly comparable to status_t in order to +// avoid calling code whenever comparisons are desired. + +template +bool operator==(const base::ResultError& l, const status_t& r) { + return (l.code() == r); +} +template +bool operator==(const status_t& l, const base::ResultError& r) { + return (l == r.code()); +} + +template +bool operator!=(const base::ResultError& l, const status_t& r) { + return (l.code() != r); +} +template +bool operator!=(const status_t& l, const base::ResultError& r) { + return (l != r.code()); +} + } // namespace android From 206e8a4f827254173ea04461a772cccf2f0c558c Mon Sep 17 00:00:00 2001 From: Almaz Mingaleev Date: Tue, 31 May 2022 09:25:17 +0100 Subject: [PATCH 03/45] Remove TZUvA feature. The feature was superseded by tzdata mainline module(s). Ignore-AOSP-First: merge conflict on aosp/master -> goog/master Bug: 148144561 Test: see system/timezone Change-Id: If87e9a71a725f665bfc977d95e52c04668447081 --- rootdir/init.rc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 70a3736cb..b811e9773 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -1021,10 +1021,6 @@ on post-fs-data # completed and apexd.status becomes "ready". exec_start apexd-snapshotde - # Check any timezone data in /data is newer than the copy in the time zone data - # module, delete if not. - exec - system system -- /system/bin/tzdatacheck /apex/com.android.tzdata/etc/tz /data/misc/zoneinfo - # sys.memfd_use set to false by default, which keeps it disabled # until it is confirmed that apps and vendor processes don't make # IOCTLs on ashmem fds any more. From 123d45e1d3825a0e54f84c9d7c2895155f69873f Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Fri, 8 Jul 2022 18:22:34 -0700 Subject: [PATCH 04/45] libsnapshot: fix integer overflow when calculating sector id. Ignore-AOSP-First: fuzzer-detected overflow Test: WIP Bug: 237639416 Change-Id: I91dedc1ccfab8dcf40e5b4756e199697e62b1f3c --- fs_mgr/libsnapshot/partition_cow_creator.cpp | 21 ++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/fs_mgr/libsnapshot/partition_cow_creator.cpp b/fs_mgr/libsnapshot/partition_cow_creator.cpp index 5569da038..acbcbe219 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.cpp +++ b/fs_mgr/libsnapshot/partition_cow_creator.cpp @@ -131,15 +131,28 @@ bool OptimizeSourceCopyOperation(const InstallOperation& operation, InstallOpera return is_optimized; } -void WriteExtent(DmSnapCowSizeCalculator* sc, const chromeos_update_engine::Extent& de, +bool WriteExtent(DmSnapCowSizeCalculator* sc, const chromeos_update_engine::Extent& de, unsigned int sectors_per_block) { const auto block_boundary = de.start_block() + de.num_blocks(); for (auto b = de.start_block(); b < block_boundary; ++b) { for (unsigned int s = 0; s < sectors_per_block; ++s) { - const auto sector_id = b * sectors_per_block + s; + // sector_id = b * sectors_per_block + s; + uint64_t block_start_sector_id; + if (__builtin_mul_overflow(b, sectors_per_block, &block_start_sector_id)) { + LOG(ERROR) << "Integer overflow when calculating sector id (" << b << " * " + << sectors_per_block << ")"; + return false; + } + uint64_t sector_id; + if (__builtin_add_overflow(block_start_sector_id, s, §or_id)) { + LOG(ERROR) << "Integer overflow when calculating sector id (" + << block_start_sector_id << " + " << s << ")"; + return false; + } sc->WriteSector(sector_id); } } + return true; } std::optional PartitionCowCreator::GetCowSize() { @@ -167,7 +180,7 @@ std::optional PartitionCowCreator::GetCowSize() { // Allocate space for extra extents (if any). These extents are those that can be // used for error corrections or to store verity hash trees. for (const auto& de : extra_extents) { - WriteExtent(&sc, de, sectors_per_block); + if (!WriteExtent(&sc, de, sectors_per_block)) return std::nullopt; } if (update == nullptr) return sc.cow_size_bytes(); @@ -182,7 +195,7 @@ std::optional PartitionCowCreator::GetCowSize() { } for (const auto& de : written_op->dst_extents()) { - WriteExtent(&sc, de, sectors_per_block); + if (!WriteExtent(&sc, de, sectors_per_block)) return std::nullopt; } } From e6281db539133e7958ec77e37f6c545a6185a587 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Tue, 26 Jul 2022 16:37:58 +0000 Subject: [PATCH 05/45] libprocessgroup: Add support for SetUserProfiles Provide SetUserProfiles API to apply profiles at UID level. This enables applying the freezer profile to an entire UID instead of just individual process cgroups. Test: bash arg: -p Test: bash arg: com.haok.nirvana Test: bash arg: 1 Test: args: [-p, com.haok.nirvana, 1] Test: arg: "-p" Test: arg: "com.haok.nirvana" Test: arg: "1" Test: data="com.haok.nirvana" Test: Events injected: 1 Test: ## Network stats: elapsed time=14ms (0ms mobile, 0ms wifi, 14ms not connected) Test: raven:/ $ ps -eo pid,uid,ppid,name|grep 10266 Test: 2499 10266 823 com.haok.nirvana Test: 2577 10266 823 com.haok.nirvana:resident Test: 2584 10266 823 android.media Test: 2669 10266 1 app_d Test: 2672 10266 1 app_d Test: raven:/ $ am force-stop com.haok.nirvana Test: raven:/ $ ps -eo pid,uid,ppid,name|grep 10266 Test: 1|raven:/ $ Ignore-AOSP-First: Topic with AMS changes which is developed on git_master Bug: 236708592 Change-Id: I45e34244f9943c217757cf346c9410672a1ce365 --- .../include/processgroup/processgroup.h | 1 + libprocessgroup/processgroup.cpp | 4 ++ libprocessgroup/task_profiles.cpp | 66 +++++++++++++++++++ libprocessgroup/task_profiles.h | 6 ++ libprocessgroup/task_profiles_test.cpp | 4 ++ 5 files changed, 81 insertions(+) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 39b9f3fc0..32479e458 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -32,6 +32,7 @@ bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::s bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); +bool SetUserProfiles(uid_t uid, const std::vector& profiles); #ifndef __ANDROID_VNDK__ diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 51c810e98..b1ce8af1f 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -174,6 +174,10 @@ extern "C" bool android_set_process_profiles(uid_t uid, pid_t pid, size_t num_pr return SetProcessProfiles(uid, pid, profiles_); } +bool SetUserProfiles(uid_t uid, const std::vector& profiles) { + return TaskProfiles::GetInstance().SetUserProfiles(uid, profiles, false); +} + static std::string ConvertUidToPath(const char* cgroup, uid_t uid) { return StringPrintf("%s/uid_%d", cgroup, uid); } diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index e1c593407..9dfc0ba9a 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -139,6 +139,19 @@ bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { return true; } +bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const +{ + if (path == nullptr) { + return true; + } + + const std::string& file_name = + controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; + *path = StringPrintf("%s/uid_%d/%s", controller()->path(), uid, file_name.c_str()); + return true; +} + + bool SetClampsAction::ExecuteForProcess(uid_t, pid_t) const { // TODO: add support when kernel supports util_clamp LOG(WARNING) << "SetClampsAction::ExecuteForProcess is not supported"; @@ -225,6 +238,31 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } +bool SetAttributeAction::ExecuteForUID(uid_t uid) const +{ + std::string path; + + if (!attribute_->GetPathForUID(uid, &path)) { + LOG(ERROR) << "Failed to find cgroup for uid " << uid; + return false; + } + + if (!WriteStringToFile(value_, path)) { + if (access(path.c_str(), F_OK) < 0) { + if (optional_) { + return true; + } else { + LOG(ERROR) << "No such cgroup attribute: " << path; + return false; + } + } + PLOG(ERROR) << "Failed to write '" << value_ << "' to " << path; + return false; + } + return true; +} + + SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_[ProfileAction::RCT_TASK]); @@ -552,6 +590,16 @@ bool TaskProfile::ExecuteForTask(int tid) const { return true; } +bool TaskProfile::ExecuteForUID(uid_t uid) const { + for (const auto& element : elements_) { + if (!element->ExecuteForUID(uid)) { + LOG(VERBOSE) << "Applying profile action " << element->Name() << " failed"; + return false; + } + } + return true; +} + void TaskProfile::EnableResourceCaching(ProfileAction::ResourceCacheType cache_type) { if (res_cached_) { return; @@ -804,6 +852,24 @@ const IProfileAttribute* TaskProfiles::GetAttribute(const std::string& name) con return nullptr; } +bool TaskProfiles::SetUserProfiles(uid_t uid, const std::vector& profiles, + bool use_fd_cache) { + for (const auto& name : profiles) { + TaskProfile* profile = GetProfile(name); + if (profile != nullptr) { + if (use_fd_cache) { + profile->EnableResourceCaching(ProfileAction::RCT_PROCESS); + } + if (!profile->ExecuteForUID(uid)) { + PLOG(WARNING) << "Failed to apply " << name << " process profile"; + } + } else { + PLOG(WARNING) << "Failed to find " << name << "process profile"; + } + } + return true; +} + bool TaskProfiles::SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, bool use_fd_cache) { bool success = true; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index df08f65c7..cdd9b42a2 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -33,6 +33,7 @@ class IProfileAttribute { virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; + virtual bool GetPathForUID(uid_t uid, std::string* path) const = 0; }; class ProfileAttribute : public IProfileAttribute { @@ -50,6 +51,7 @@ class ProfileAttribute : public IProfileAttribute { void Reset(const CgroupController& controller, const std::string& file_name) override; bool GetPathForTask(int tid, std::string* path) const override; + bool GetPathForUID(uid_t uid, std::string* path) const override; private: CgroupController controller_; @@ -69,6 +71,7 @@ class ProfileAction { // Default implementations will fail virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; }; virtual bool ExecuteForTask(int) const { return false; }; + virtual bool ExecuteForUID(uid_t) const { return false; }; virtual void EnableResourceCaching(ResourceCacheType) {} virtual void DropResourceCaching(ResourceCacheType) {} @@ -113,6 +116,7 @@ class SetAttributeAction : public ProfileAction { const char* Name() const override { return "SetAttribute"; } bool ExecuteForProcess(uid_t uid, pid_t pid) const override; bool ExecuteForTask(int tid) const override; + bool ExecuteForUID(uid_t uid) const override; private: const IProfileAttribute* attribute_; @@ -176,6 +180,7 @@ class TaskProfile { bool ExecuteForProcess(uid_t uid, pid_t pid) const; bool ExecuteForTask(int tid) const; + bool ExecuteForUID(uid_t uid) const; void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); @@ -212,6 +217,7 @@ class TaskProfiles { bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, bool use_fd_cache); bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache); + bool SetUserProfiles(uid_t uid, const std::vector& profiles, bool use_fd_cache); private: std::map> profiles_; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index 09ac44c6b..fed819e53 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -121,6 +121,10 @@ class ProfileAttributeMock : public IProfileAttribute { return true; }; + bool GetPathForUID(uid_t, std::string *) const override { + return false; + } + private: const std::string file_name_; }; From aee11b0a3da5bbfd09d258278700b358a8691151 Mon Sep 17 00:00:00 2001 From: Dan Shi Date: Fri, 19 Aug 2022 18:40:14 +0000 Subject: [PATCH 06/45] Revert "libprocessgroup: Add support for SetUserProfiles" Revert "Freeze package cgroup before killing" Revert submission 19436294-freeze_kill Reason for revert: b/243096961 Reverted Changes: I06aaa5a08:Freeze package cgroup before killing I45e34244f:libprocessgroup: Add support for SetUserProfiles Bug: 243096961 Change-Id: I32162341d6a38f458a9c59d63e0cdc56a97f1efe --- .../include/processgroup/processgroup.h | 1 - libprocessgroup/processgroup.cpp | 4 -- libprocessgroup/task_profiles.cpp | 66 ------------------- libprocessgroup/task_profiles.h | 6 -- libprocessgroup/task_profiles_test.cpp | 4 -- 5 files changed, 81 deletions(-) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 32479e458..39b9f3fc0 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -32,7 +32,6 @@ bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::s bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); -bool SetUserProfiles(uid_t uid, const std::vector& profiles); #ifndef __ANDROID_VNDK__ diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index b1ce8af1f..51c810e98 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -174,10 +174,6 @@ extern "C" bool android_set_process_profiles(uid_t uid, pid_t pid, size_t num_pr return SetProcessProfiles(uid, pid, profiles_); } -bool SetUserProfiles(uid_t uid, const std::vector& profiles) { - return TaskProfiles::GetInstance().SetUserProfiles(uid, profiles, false); -} - static std::string ConvertUidToPath(const char* cgroup, uid_t uid) { return StringPrintf("%s/uid_%d", cgroup, uid); } diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 9dfc0ba9a..e1c593407 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -139,19 +139,6 @@ bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { return true; } -bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const -{ - if (path == nullptr) { - return true; - } - - const std::string& file_name = - controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; - *path = StringPrintf("%s/uid_%d/%s", controller()->path(), uid, file_name.c_str()); - return true; -} - - bool SetClampsAction::ExecuteForProcess(uid_t, pid_t) const { // TODO: add support when kernel supports util_clamp LOG(WARNING) << "SetClampsAction::ExecuteForProcess is not supported"; @@ -238,31 +225,6 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } -bool SetAttributeAction::ExecuteForUID(uid_t uid) const -{ - std::string path; - - if (!attribute_->GetPathForUID(uid, &path)) { - LOG(ERROR) << "Failed to find cgroup for uid " << uid; - return false; - } - - if (!WriteStringToFile(value_, path)) { - if (access(path.c_str(), F_OK) < 0) { - if (optional_) { - return true; - } else { - LOG(ERROR) << "No such cgroup attribute: " << path; - return false; - } - } - PLOG(ERROR) << "Failed to write '" << value_ << "' to " << path; - return false; - } - return true; -} - - SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_[ProfileAction::RCT_TASK]); @@ -590,16 +552,6 @@ bool TaskProfile::ExecuteForTask(int tid) const { return true; } -bool TaskProfile::ExecuteForUID(uid_t uid) const { - for (const auto& element : elements_) { - if (!element->ExecuteForUID(uid)) { - LOG(VERBOSE) << "Applying profile action " << element->Name() << " failed"; - return false; - } - } - return true; -} - void TaskProfile::EnableResourceCaching(ProfileAction::ResourceCacheType cache_type) { if (res_cached_) { return; @@ -852,24 +804,6 @@ const IProfileAttribute* TaskProfiles::GetAttribute(const std::string& name) con return nullptr; } -bool TaskProfiles::SetUserProfiles(uid_t uid, const std::vector& profiles, - bool use_fd_cache) { - for (const auto& name : profiles) { - TaskProfile* profile = GetProfile(name); - if (profile != nullptr) { - if (use_fd_cache) { - profile->EnableResourceCaching(ProfileAction::RCT_PROCESS); - } - if (!profile->ExecuteForUID(uid)) { - PLOG(WARNING) << "Failed to apply " << name << " process profile"; - } - } else { - PLOG(WARNING) << "Failed to find " << name << "process profile"; - } - } - return true; -} - bool TaskProfiles::SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, bool use_fd_cache) { bool success = true; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index cdd9b42a2..df08f65c7 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -33,7 +33,6 @@ class IProfileAttribute { virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; - virtual bool GetPathForUID(uid_t uid, std::string* path) const = 0; }; class ProfileAttribute : public IProfileAttribute { @@ -51,7 +50,6 @@ class ProfileAttribute : public IProfileAttribute { void Reset(const CgroupController& controller, const std::string& file_name) override; bool GetPathForTask(int tid, std::string* path) const override; - bool GetPathForUID(uid_t uid, std::string* path) const override; private: CgroupController controller_; @@ -71,7 +69,6 @@ class ProfileAction { // Default implementations will fail virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; }; virtual bool ExecuteForTask(int) const { return false; }; - virtual bool ExecuteForUID(uid_t) const { return false; }; virtual void EnableResourceCaching(ResourceCacheType) {} virtual void DropResourceCaching(ResourceCacheType) {} @@ -116,7 +113,6 @@ class SetAttributeAction : public ProfileAction { const char* Name() const override { return "SetAttribute"; } bool ExecuteForProcess(uid_t uid, pid_t pid) const override; bool ExecuteForTask(int tid) const override; - bool ExecuteForUID(uid_t uid) const override; private: const IProfileAttribute* attribute_; @@ -180,7 +176,6 @@ class TaskProfile { bool ExecuteForProcess(uid_t uid, pid_t pid) const; bool ExecuteForTask(int tid) const; - bool ExecuteForUID(uid_t uid) const; void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); @@ -217,7 +212,6 @@ class TaskProfiles { bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, bool use_fd_cache); bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache); - bool SetUserProfiles(uid_t uid, const std::vector& profiles, bool use_fd_cache); private: std::map> profiles_; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index fed819e53..09ac44c6b 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -121,10 +121,6 @@ class ProfileAttributeMock : public IProfileAttribute { return true; }; - bool GetPathForUID(uid_t, std::string *) const override { - return false; - } - private: const std::string file_name_; }; From 7f47b5204180a6f98e3d64280eb8142b43392668 Mon Sep 17 00:00:00 2001 From: Sanjana Sunil Date: Mon, 28 Mar 2022 19:05:36 +0000 Subject: [PATCH 07/45] Create misc_ce and misc_de mirror storage Create a mirror directory for misc_ce and misc_de storage by bind mounting the respective directories. This is done for the defaul null volume only, and other volumes are handled at a later staged. When an SDK sandbox process is spawned and data isolation needs to occur, the sdksandbox directories present in the misc directories will be used to bind mount from, after tmpfs is mounted on the original. Bug: 214241165 Test: atest SdkSandboxStorageHostTest Ignore-AOSP-First: Will cherry pick based on other CLs in the topic Change-Id: Icb1dc7d7fbd53a5c3853acf2f9d4d75b278d7295 --- rootdir/init.rc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 60bf57bef..e791b31ed 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -939,22 +939,28 @@ on post-fs-data restorecon /data/media exec - media_rw media_rw -- /system/bin/chattr +F /data/media - # A tmpfs directory, which will contain all apps CE DE data directory that - # bind mount from the original source. + # A tmpfs directory, which will contain all apps and sdk sandbox CE and DE + # data directory that bind mount from the original source. mount tmpfs tmpfs /data_mirror nodev noexec nosuid mode=0700,uid=0,gid=1000 restorecon /data_mirror mkdir /data_mirror/data_ce 0700 root root mkdir /data_mirror/data_de 0700 root root + mkdir /data_mirror/misc_ce 0700 root root + mkdir /data_mirror/misc_de 0700 root root # Create CE and DE data directory for default volume mkdir /data_mirror/data_ce/null 0700 root root mkdir /data_mirror/data_de/null 0700 root root + mkdir /data_mirror/misc_ce/null 0700 root root + mkdir /data_mirror/misc_de/null 0700 root root # Bind mount CE and DE data directory to mirror's default volume directory. # The 'slave' option (MS_SLAVE) is needed to cause the later bind mount of # /data/data onto /data/user/0 to propagate to /data_mirror/data_ce/null/0. mount none /data/user /data_mirror/data_ce/null bind rec slave mount none /data/user_de /data_mirror/data_de/null bind rec + mount none /data/misc_ce /data_mirror/misc_ce/null bind rec + mount none /data/misc_de /data_mirror/misc_de/null bind rec # Create mirror directory for jit profiles mkdir /data_mirror/cur_profiles 0700 root root From 673da5b972e751b723b735a63ae57bd54d956b7e Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Mon, 22 Aug 2022 21:25:09 +0000 Subject: [PATCH 08/45] Revert "Revert "libprocessgroup: Add support for SetUserProfiles"" This reverts commit aee11b0a3da5bbfd09d258278700b358a8691151. This change was originally reverted because its only user was reverted under b/243096961 at ag/19679188. We bring it back now with a fixed user. Bug: 236708592 Bug: 148425913 Ignore-AOSP-First: Topic with AMS changes which is developed on git_master Change-Id: I2a8ae0d9faabe7950b758a09870d128889be4d0a --- .../include/processgroup/processgroup.h | 1 + libprocessgroup/processgroup.cpp | 5 ++ libprocessgroup/task_profiles.cpp | 64 +++++++++++++++++++ libprocessgroup/task_profiles.h | 7 ++ libprocessgroup/task_profiles_test.cpp | 4 ++ 5 files changed, 81 insertions(+) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 45a723f74..f3cd421dd 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -35,6 +35,7 @@ bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::s bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); +bool SetUserProfiles(uid_t uid, const std::vector& profiles); __END_DECLS diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index bdda1020c..393936ca7 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -195,6 +195,11 @@ extern "C" bool android_set_process_profiles(uid_t uid, pid_t pid, size_t num_pr return SetProcessProfiles(uid, pid, std::span(profiles_)); } +bool SetUserProfiles(uid_t uid, const std::vector& profiles) { + return TaskProfiles::GetInstance().SetUserProfiles(uid, std::span(profiles), + false); +} + static std::string ConvertUidToPath(const char* cgroup, uid_t uid) { return StringPrintf("%s/uid_%d", cgroup, uid); } diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 744710f3b..0fbfc8c07 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -139,6 +139,17 @@ bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { return true; } +bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const { + if (path == nullptr) { + return true; + } + + const std::string& file_name = + controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; + *path = StringPrintf("%s/uid_%d/%s", controller()->path(), uid, file_name.c_str()); + return true; +} + bool SetClampsAction::ExecuteForProcess(uid_t, pid_t) const { // TODO: add support when kernel supports util_clamp LOG(WARNING) << "SetClampsAction::ExecuteForProcess is not supported"; @@ -225,6 +236,29 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } +bool SetAttributeAction::ExecuteForUID(uid_t uid) const { + std::string path; + + if (!attribute_->GetPathForUID(uid, &path)) { + LOG(ERROR) << "Failed to find cgroup for uid " << uid; + return false; + } + + if (!WriteStringToFile(value_, path)) { + if (access(path.c_str(), F_OK) < 0) { + if (optional_) { + return true; + } else { + LOG(ERROR) << "No such cgroup attribute: " << path; + return false; + } + } + PLOG(ERROR) << "Failed to write '" << value_ << "' to " << path; + return false; + } + return true; +} + SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_[ProfileAction::RCT_TASK]); @@ -552,6 +586,16 @@ bool TaskProfile::ExecuteForTask(int tid) const { return true; } +bool TaskProfile::ExecuteForUID(uid_t uid) const { + for (const auto& element : elements_) { + if (!element->ExecuteForUID(uid)) { + LOG(VERBOSE) << "Applying profile action " << element->Name() << " failed"; + return false; + } + } + return true; +} + void TaskProfile::EnableResourceCaching(ProfileAction::ResourceCacheType cache_type) { if (res_cached_) { return; @@ -804,6 +848,24 @@ const IProfileAttribute* TaskProfiles::GetAttribute(std::string_view name) const return nullptr; } +template +bool TaskProfiles::SetUserProfiles(uid_t uid, std::span profiles, bool use_fd_cache) { + for (const auto& name : profiles) { + TaskProfile* profile = GetProfile(name); + if (profile != nullptr) { + if (use_fd_cache) { + profile->EnableResourceCaching(ProfileAction::RCT_PROCESS); + } + if (!profile->ExecuteForUID(uid)) { + PLOG(WARNING) << "Failed to apply " << name << " process profile"; + } + } else { + PLOG(WARNING) << "Failed to find " << name << "process profile"; + } + } + return true; +} + template bool TaskProfiles::SetProcessProfiles(uid_t uid, pid_t pid, std::span profiles, bool use_fd_cache) { @@ -857,3 +919,5 @@ template bool TaskProfiles::SetTaskProfiles(int tid, std::span profiles, bool use_fd_cache); +template bool TaskProfiles::SetUserProfiles(uid_t uid, std::span profiles, + bool use_fd_cache); diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 85b3f9162..a8ecb873d 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -36,6 +36,7 @@ class IProfileAttribute { virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; + virtual bool GetPathForUID(uid_t uid, std::string* path) const = 0; }; class ProfileAttribute : public IProfileAttribute { @@ -53,6 +54,7 @@ class ProfileAttribute : public IProfileAttribute { void Reset(const CgroupController& controller, const std::string& file_name) override; bool GetPathForTask(int tid, std::string* path) const override; + bool GetPathForUID(uid_t uid, std::string* path) const override; private: CgroupController controller_; @@ -72,6 +74,7 @@ class ProfileAction { // Default implementations will fail virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; }; virtual bool ExecuteForTask(int) const { return false; }; + virtual bool ExecuteForUID(uid_t) const { return false; }; virtual void EnableResourceCaching(ResourceCacheType) {} virtual void DropResourceCaching(ResourceCacheType) {} @@ -116,6 +119,7 @@ class SetAttributeAction : public ProfileAction { const char* Name() const override { return "SetAttribute"; } bool ExecuteForProcess(uid_t uid, pid_t pid) const override; bool ExecuteForTask(int tid) const override; + bool ExecuteForUID(uid_t uid) const override; private: const IProfileAttribute* attribute_; @@ -179,6 +183,7 @@ class TaskProfile { bool ExecuteForProcess(uid_t uid, pid_t pid) const; bool ExecuteForTask(int tid) const; + bool ExecuteForUID(uid_t uid) const; void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); @@ -216,6 +221,8 @@ class TaskProfiles { bool SetProcessProfiles(uid_t uid, pid_t pid, std::span profiles, bool use_fd_cache); template bool SetTaskProfiles(int tid, std::span profiles, bool use_fd_cache); + template + bool SetUserProfiles(uid_t uid, std::span profiles, bool use_fd_cache); private: TaskProfiles(); diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index 09ac44c6b..aa74f9d25 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -121,6 +121,10 @@ class ProfileAttributeMock : public IProfileAttribute { return true; }; + bool GetPathForUID(uid_t, std::string*) const override { + return false; + } + private: const std::string file_name_; }; From 43a172fb1d6fbf9b92f4de8ee12b89d886cef705 Mon Sep 17 00:00:00 2001 From: Rajesh Nyamagoud Date: Thu, 22 Sep 2022 20:25:52 +0000 Subject: [PATCH 09/45] Changes to adapt confirmationui AIDL spec. Replaced HIDL spec implementation with AIDL spec in confirmationui module. Ignore-AOSP-First: Dependent on internal change. Bug: b/205760172 Test: Run confirmation UI test using CTS Verifier, atest VtsHalConfirmationUITargetTest Change-Id: I49b9cb6d93fa35fd611003f7545d2ce4976eec7c --- trusty/confirmationui/Android.bp | 23 ++-- .../confirmationui/TrustyConfirmationUI.cpp | 124 +++++++++--------- trusty/confirmationui/TrustyConfirmationUI.h | 52 ++++---- ....hardware.confirmationui-service.trusty.rc | 5 + ...ardware.confirmationui-service.trusty.xml} | 5 +- ...dware.confirmationui@1.0-service.trusty.rc | 4 - .../include/TrustyConfirmationuiHal.h | 16 +-- trusty/confirmationui/service.cpp | 25 ++-- 8 files changed, 124 insertions(+), 130 deletions(-) create mode 100644 trusty/confirmationui/android.hardware.confirmationui-service.trusty.rc rename trusty/confirmationui/{android.hardware.confirmationui@1.0-service.trusty.xml => android.hardware.confirmationui-service.trusty.xml} (71%) delete mode 100644 trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.rc diff --git a/trusty/confirmationui/Android.bp b/trusty/confirmationui/Android.bp index 092241528..29ef3c098 100644 --- a/trusty/confirmationui/Android.bp +++ b/trusty/confirmationui/Android.bp @@ -24,21 +24,23 @@ package { } cc_binary { - name: "android.hardware.confirmationui@1.0-service.trusty", + name: "android.hardware.confirmationui-service.trusty", relative_install_path: "hw", vendor: true, shared_libs: [ - "android.hardware.confirmationui@1.0", + "android.hardware.confirmationui-V1-ndk", "android.hardware.confirmationui.not-so-secure-input", - "android.hardware.confirmationui@1.0-lib.trusty", + "android.hardware.confirmationui-lib.trusty", + "libbinder_ndk", + "libteeui_hal_support", "libbase", "libhidlbase", "libutils", ], - init_rc: ["android.hardware.confirmationui@1.0-service.trusty.rc"], + init_rc: ["android.hardware.confirmationui-service.trusty.rc"], - vintf_fragments: ["android.hardware.confirmationui@1.0-service.trusty.xml"], + vintf_fragments: ["android.hardware.confirmationui-service.trusty.xml"], srcs: [ "service.cpp", @@ -52,17 +54,20 @@ cc_binary { } cc_library { - name: "android.hardware.confirmationui@1.0-lib.trusty", + name: "android.hardware.confirmationui-lib.trusty", + defaults: [ + "keymint_use_latest_hal_aidl_ndk_shared", + ], vendor: true, shared_libs: [ - "android.hardware.confirmationui@1.0", - "android.hardware.keymaster@4.0", + "android.hardware.confirmationui-V1-ndk", "libbase", + "libcutils", "libdmabufheap", - "libhidlbase", "libteeui_hal_support", "libtrusty", "libutils", + "libbinder_ndk", ], export_include_dirs: ["include"], diff --git a/trusty/confirmationui/TrustyConfirmationUI.cpp b/trusty/confirmationui/TrustyConfirmationUI.cpp index c6625e0a1..f01a4e1e9 100644 --- a/trusty/confirmationui/TrustyConfirmationUI.cpp +++ b/trusty/confirmationui/TrustyConfirmationUI.cpp @@ -18,8 +18,6 @@ #include "TrustyConfirmationUI.h" #include -#include -#include #include #include #include @@ -42,12 +40,7 @@ #include #include -namespace android { -namespace hardware { -namespace confirmationui { -namespace V1_0 { -namespace implementation { - +namespace aidl::android::hardware::confirmationui { using namespace secure_input; using ::android::trusty::confirmationui::TrustyAppError; @@ -64,8 +57,6 @@ using ::teeui::ResultMsg; using ::secure_input::createSecureInput; -using ::android::hardware::keymaster::V4_0::HardwareAuthToken; - using ::std::tie; using TeeuiRc = ::teeui::ResponseCode; @@ -87,46 +78,47 @@ class Finalize { void release() { f_ = {}; } }; -ResponseCode convertRc(TeeuiRc trc) { +int convertRc(TeeuiRc trc) { static_assert( - uint32_t(TeeuiRc::OK) == uint32_t(ResponseCode::OK) && - uint32_t(TeeuiRc::Canceled) == uint32_t(ResponseCode::Canceled) && - uint32_t(TeeuiRc::Aborted) == uint32_t(ResponseCode::Aborted) && - uint32_t(TeeuiRc::OperationPending) == uint32_t(ResponseCode::OperationPending) && - uint32_t(TeeuiRc::Ignored) == uint32_t(ResponseCode::Ignored) && - uint32_t(TeeuiRc::SystemError) == uint32_t(ResponseCode::SystemError) && - uint32_t(TeeuiRc::Unimplemented) == uint32_t(ResponseCode::Unimplemented) && - uint32_t(TeeuiRc::Unexpected) == uint32_t(ResponseCode::Unexpected) && - uint32_t(TeeuiRc::UIError) == uint32_t(ResponseCode::UIError) && - uint32_t(TeeuiRc::UIErrorMissingGlyph) == uint32_t(ResponseCode::UIErrorMissingGlyph) && + uint32_t(TeeuiRc::OK) == uint32_t(IConfirmationUI::OK) && + uint32_t(TeeuiRc::Canceled) == uint32_t(IConfirmationUI::CANCELED) && + uint32_t(TeeuiRc::Aborted) == uint32_t(IConfirmationUI::ABORTED) && + uint32_t(TeeuiRc::OperationPending) == uint32_t(IConfirmationUI::OPERATION_PENDING) && + uint32_t(TeeuiRc::Ignored) == uint32_t(IConfirmationUI::IGNORED) && + uint32_t(TeeuiRc::SystemError) == uint32_t(IConfirmationUI::SYSTEM_ERROR) && + uint32_t(TeeuiRc::Unimplemented) == uint32_t(IConfirmationUI::UNIMPLEMENTED) && + uint32_t(TeeuiRc::Unexpected) == uint32_t(IConfirmationUI::UNEXPECTED) && + uint32_t(TeeuiRc::UIError) == uint32_t(IConfirmationUI::UI_ERROR) && + uint32_t(TeeuiRc::UIErrorMissingGlyph) == + uint32_t(IConfirmationUI::UI_ERROR_MISSING_GLYPH) && uint32_t(TeeuiRc::UIErrorMessageTooLong) == - uint32_t(ResponseCode::UIErrorMessageTooLong) && + uint32_t(IConfirmationUI::UI_ERROR_MESSAGE_TOO_LONG) && uint32_t(TeeuiRc::UIErrorMalformedUTF8Encoding) == - uint32_t(ResponseCode::UIErrorMalformedUTF8Encoding), + uint32_t(IConfirmationUI::UI_ERROR_MALFORMED_UTF8ENCODING), "teeui::ResponseCode and " "::android::hardware::confirmationui::V1_0::Responsecude are out of " "sync"); - return ResponseCode(trc); + return static_cast(trc); } teeui::UIOption convertUIOption(UIOption uio) { - static_assert(uint32_t(UIOption::AccessibilityInverted) == + static_assert(uint32_t(UIOption::ACCESSIBILITY_INVERTED) == uint32_t(teeui::UIOption::AccessibilityInverted) && - uint32_t(UIOption::AccessibilityMagnified) == + uint32_t(UIOption::ACCESSIBILITY_MAGNIFIED) == uint32_t(teeui::UIOption::AccessibilityMagnified), "teeui::UIOPtion and ::android::hardware::confirmationui::V1_0::UIOption " - "anre out of sync"); + "are out of sync"); return teeui::UIOption(uio); } -inline MsgString hidl2MsgString(const hidl_string& s) { +inline MsgString stdString2MsgString(const string& s) { return {s.c_str(), s.c_str() + s.size()}; } -template inline MsgVector hidl2MsgVector(const hidl_vec& v) { +template inline MsgVector stdVector2MsgVector(const vector& v) { return {v}; } -inline MsgVector hidl2MsgVector(const hidl_vec& v) { +inline MsgVector stdVector2MsgVector(const vector& v) { MsgVector result(v.size()); for (unsigned int i = 0; i < v.size(); ++i) { result[i] = convertUIOption(v[i]); @@ -137,7 +129,7 @@ inline MsgVector hidl2MsgVector(const hidl_vec& v) { } // namespace TrustyConfirmationUI::TrustyConfirmationUI() - : listener_state_(ListenerState::None), prompt_result_(ResponseCode::Ignored) {} + : listener_state_(ListenerState::None), prompt_result_(IConfirmationUI::IGNORED) {} TrustyConfirmationUI::~TrustyConfirmationUI() { ListenerState state = listener_state_; @@ -385,15 +377,16 @@ TrustyConfirmationUI::promptUserConfirmation_(const MsgString& promptText, // ############################## Start 4th Phase - cleanup ################################## } -// Methods from ::android::hardware::confirmationui::V1_0::IConfirmationUI +// Methods from ::aidl::android::hardware::confirmationui::IConfirmationUI // follow. -Return TrustyConfirmationUI::promptUserConfirmation( - const sp& resultCB, const hidl_string& promptText, - const hidl_vec& extraData, const hidl_string& locale, - const hidl_vec& uiOptions) { +::ndk::ScopedAStatus TrustyConfirmationUI::promptUserConfirmation( + const shared_ptr& resultCB, const vector& promptTextBytes, + const vector& extraData, const string& locale, const vector& uiOptions) { std::unique_lock stateLock(listener_state_lock_, std::defer_lock); + string promptText(promptTextBytes.begin(), promptTextBytes.end()); if (!stateLock.try_lock()) { - return ResponseCode::OperationPending; + return ndk::ScopedAStatus( + AStatus_fromServiceSpecificError(IConfirmationUI::OPERATION_PENDING)); } switch (listener_state_) { case ListenerState::None: @@ -401,23 +394,25 @@ Return TrustyConfirmationUI::promptUserConfirmation( case ListenerState::Starting: case ListenerState::SetupDone: case ListenerState::Interactive: - return ResponseCode::OperationPending; + return ndk::ScopedAStatus( + AStatus_fromServiceSpecificError(IConfirmationUI::OPERATION_PENDING)); case ListenerState::Terminating: callback_thread_.join(); listener_state_ = ListenerState::None; break; default: - return ResponseCode::Unexpected; + return ndk::ScopedAStatus(AStatus_fromServiceSpecificError(IConfirmationUI::UNEXPECTED)); } assert(listener_state_ == ListenerState::None); callback_thread_ = std::thread( - [this](sp resultCB, hidl_string promptText, - hidl_vec extraData, hidl_string locale, hidl_vec uiOptions) { - auto [trc, msg, token] = - promptUserConfirmation_(hidl2MsgString(promptText), hidl2MsgVector(extraData), - hidl2MsgString(locale), hidl2MsgVector(uiOptions)); + [this](const shared_ptr& resultCB, const string& promptText, + const vector& extraData, const string& locale, + const vector& uiOptions) { + auto [trc, msg, token] = promptUserConfirmation_( + stdString2MsgString(promptText), stdVector2MsgVector(extraData), + stdString2MsgString(locale), stdVector2MsgVector(uiOptions)); bool do_callback = (listener_state_ == ListenerState::Interactive || listener_state_ == ListenerState::SetupDone) && resultCB; @@ -426,7 +421,7 @@ Return TrustyConfirmationUI::promptUserConfirmation( if (do_callback) { auto error = resultCB->result(prompt_result_, msg, token); if (!error.isOk()) { - LOG(ERROR) << "Result callback failed " << error.description(); + LOG(ERROR) << "Result callback failed " << error.getDescription(); } } else { listener_state_condv_.notify_all(); @@ -442,14 +437,14 @@ Return TrustyConfirmationUI::promptUserConfirmation( if (listener_state_ == ListenerState::Terminating) { callback_thread_.join(); listener_state_ = ListenerState::None; - return prompt_result_; + return ndk::ScopedAStatus(AStatus_fromServiceSpecificError(prompt_result_)); } - return ResponseCode::OK; + return ndk::ScopedAStatus::ok(); } -Return +::ndk::ScopedAStatus TrustyConfirmationUI::deliverSecureInputEvent(const HardwareAuthToken& secureInputToken) { - ResponseCode rc = ResponseCode::Ignored; + int rc = IConfirmationUI::IGNORED; { /* * deliverSecureInputEvent is only used by the VTS test to mock human input. A correct @@ -467,13 +462,17 @@ TrustyConfirmationUI::deliverSecureInputEvent(const HardwareAuthToken& secureInp listener_state_condv_.wait(stateLock, [this] { return listener_state_ != ListenerState::SetupDone; }); - if (listener_state_ != ListenerState::Interactive) return ResponseCode::Ignored; + if (listener_state_ != ListenerState::Interactive) + return ndk::ScopedAStatus(AStatus_fromServiceSpecificError(IConfirmationUI::IGNORED)); auto sapp = app_.lock(); - if (!sapp) return ResponseCode::Ignored; + if (!sapp) + return ndk::ScopedAStatus(AStatus_fromServiceSpecificError(IConfirmationUI::IGNORED)); auto [error, response] = sapp->issueCmd( static_cast(secureInputToken.challenge)); - if (error != TrustyAppError::OK) return ResponseCode::SystemError; + if (error != TrustyAppError::OK) + return ndk::ScopedAStatus( + AStatus_fromServiceSpecificError(IConfirmationUI::SYSTEM_ERROR)); auto& [trc] = response; if (trc != TeeuiRc::Ignored) secureInputDelivered_ = true; rc = convertRc(trc); @@ -484,11 +483,14 @@ TrustyConfirmationUI::deliverSecureInputEvent(const HardwareAuthToken& secureInp // Canceled into OK. Canceled is only returned if the delivered event canceled // the operation, which means that the event was successfully delivered. Thus // we return OK. - if (rc == ResponseCode::Canceled) return ResponseCode::OK; - return rc; + if (rc == IConfirmationUI::CANCELED) return ndk::ScopedAStatus::ok(); + if (rc != IConfirmationUI::OK) { + return ndk::ScopedAStatus(AStatus_fromServiceSpecificError(rc)); + } + return ndk::ScopedAStatus::ok(); } -Return TrustyConfirmationUI::abort() { +::ndk::ScopedAStatus TrustyConfirmationUI::abort() { { std::unique_lock stateLock(listener_state_lock_); if (listener_state_ == ListenerState::SetupDone || @@ -499,15 +501,11 @@ Return TrustyConfirmationUI::abort() { } } listener_state_condv_.notify_all(); - return Void(); + return ndk::ScopedAStatus::ok(); } -android::sp createTrustyConfirmationUI() { - return new TrustyConfirmationUI(); +std::shared_ptr createTrustyConfirmationUI() { + return ndk::SharedRefBase::make(); } -} // namespace implementation -} // namespace V1_0 -} // namespace confirmationui -} // namespace hardware -} // namespace android +} // namespace aidl::android::hardware::confirmationui diff --git a/trusty/confirmationui/TrustyConfirmationUI.h b/trusty/confirmationui/TrustyConfirmationUI.h index 0bd703c9f..6e85704b6 100644 --- a/trusty/confirmationui/TrustyConfirmationUI.h +++ b/trusty/confirmationui/TrustyConfirmationUI.h @@ -17,9 +17,11 @@ #ifndef ANDROID_HARDWARE_CONFIRMATIONUI_V1_0_TRUSTY_CONFIRMATIONUI_H #define ANDROID_HARDWARE_CONFIRMATIONUI_V1_0_TRUSTY_CONFIRMATIONUI_H -#include -#include -#include +#include +#include +#include +#include +#include #include #include @@ -30,35 +32,29 @@ #include "TrustyApp.h" -namespace android { -namespace hardware { -namespace confirmationui { -namespace V1_0 { -namespace implementation { +namespace aidl::android::hardware::confirmationui { -using ::android::sp; -using ::android::hardware::hidl_array; -using ::android::hardware::hidl_string; -using ::android::hardware::hidl_vec; -using ::android::hardware::Return; -using ::android::hardware::Void; +using std::shared_ptr; +using std::string; +using std::vector; +using ::aidl::android::hardware::security::keymint::HardwareAuthToken; using ::android::trusty::confirmationui::TrustyApp; -class TrustyConfirmationUI : public IConfirmationUI { +class TrustyConfirmationUI : public BnConfirmationUI { public: TrustyConfirmationUI(); virtual ~TrustyConfirmationUI(); - // Methods from ::android::hardware::confirmationui::V1_0::IConfirmationUI + // Methods from ::aidl::android::hardware::confirmationui::IConfirmationUI // follow. - Return promptUserConfirmation(const sp& resultCB, - const hidl_string& promptText, - const hidl_vec& extraData, - const hidl_string& locale, - const hidl_vec& uiOptions) override; - Return deliverSecureInputEvent( - const ::android::hardware::keymaster::V4_0::HardwareAuthToken& secureInputToken) override; - Return abort() override; + ::ndk::ScopedAStatus + promptUserConfirmation(const shared_ptr& resultCB, + const vector& promptText, const vector& extraData, + const string& locale, const vector& uiOptions) override; + ::ndk::ScopedAStatus + deliverSecureInputEvent(const HardwareAuthToken& secureInputToken) override; + + ::ndk::ScopedAStatus abort() override; private: std::weak_ptr app_; @@ -85,7 +81,7 @@ class TrustyConfirmationUI : public IConfirmationUI { bool abort_called_; std::mutex listener_state_lock_; std::condition_variable listener_state_condv_; - ResponseCode prompt_result_; + int prompt_result_; bool secureInputDelivered_; std::tuple, teeui::MsgVector> @@ -95,10 +91,6 @@ class TrustyConfirmationUI : public IConfirmationUI { const teeui::MsgVector& uiOptions); }; -} // namespace implementation -} // namespace V1_0 -} // namespace confirmationui -} // namespace hardware -} // namespace android +} // namespace aidl::android::hardware::confirmationui #endif // ANDROID_HARDWARE_CONFIRMATIONUI_V1_0_TRUSTY_CONFIRMATIONUI_H diff --git a/trusty/confirmationui/android.hardware.confirmationui-service.trusty.rc b/trusty/confirmationui/android.hardware.confirmationui-service.trusty.rc new file mode 100644 index 000000000..b5c315986 --- /dev/null +++ b/trusty/confirmationui/android.hardware.confirmationui-service.trusty.rc @@ -0,0 +1,5 @@ +service vendor.confirmationui_default /vendor/bin/hw/android.hardware.confirmationui-service.trusty + interface aidl android.hardware.confirmationui.IConfirmationUI/default + class hal + user system + group drmrpc input system diff --git a/trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.xml b/trusty/confirmationui/android.hardware.confirmationui-service.trusty.xml similarity index 71% rename from trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.xml rename to trusty/confirmationui/android.hardware.confirmationui-service.trusty.xml index 9008b872e..afa2e8e66 100644 --- a/trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.xml +++ b/trusty/confirmationui/android.hardware.confirmationui-service.trusty.xml @@ -1,8 +1,7 @@ - + android.hardware.confirmationui - hwbinder - 1.0 + 1 IConfirmationUI default diff --git a/trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.rc b/trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.rc deleted file mode 100644 index 3ba6fc04b..000000000 --- a/trusty/confirmationui/android.hardware.confirmationui@1.0-service.trusty.rc +++ /dev/null @@ -1,4 +0,0 @@ -service confirmationui-1-0 /vendor/bin/hw/android.hardware.confirmationui@1.0-service.trusty - class hal - user system - group drmrpc input system diff --git a/trusty/confirmationui/include/TrustyConfirmationuiHal.h b/trusty/confirmationui/include/TrustyConfirmationuiHal.h index 2ab9389b1..8000ee2f0 100644 --- a/trusty/confirmationui/include/TrustyConfirmationuiHal.h +++ b/trusty/confirmationui/include/TrustyConfirmationuiHal.h @@ -16,18 +16,10 @@ #pragma once -#include +#include -namespace android { -namespace hardware { -namespace confirmationui { -namespace V1_0 { -namespace implementation { +namespace aidl::android::hardware::confirmationui { -android::sp createTrustyConfirmationUI(); +std::shared_ptr createTrustyConfirmationUI(); -} // namespace implementation -} // namespace V1_0 -} // namespace confirmationui -} // namespace hardware -} // namespace android +} // namespace aidl::android::hardware::confirmationui diff --git a/trusty/confirmationui/service.cpp b/trusty/confirmationui/service.cpp index dd7e84b44..b286c0a98 100644 --- a/trusty/confirmationui/service.cpp +++ b/trusty/confirmationui/service.cpp @@ -15,21 +15,28 @@ */ #include -#include +#include +#include #include -using android::sp; -using android::hardware::confirmationui::V1_0::implementation::createTrustyConfirmationUI; +using ::aidl::android::hardware::confirmationui::createTrustyConfirmationUI; +using ::aidl::android::hardware::confirmationui::IConfirmationUI; int main() { - ::android::hardware::configureRpcThreadpool(1, true /*willJoinThreadpool*/); - auto service = createTrustyConfirmationUI(); - auto status = service->registerAsService(); - if (status != android::OK) { - LOG(FATAL) << "Could not register service for ConfirmationUI 1.0 (" << status << ")"; + ABinderProcess_setThreadPoolMaxThreadCount(0); + + auto confirmationui = createTrustyConfirmationUI(); + + const auto instance = std::string(IConfirmationUI::descriptor) + "/default"; + binder_status_t status = + AServiceManager_addService(confirmationui->asBinder().get(), instance.c_str()); + + if (status != STATUS_OK) { + LOG(FATAL) << "Could not register service for " << instance.c_str() << "(" << status << ")"; return -1; } - ::android::hardware::joinRpcThreadpool(); + + ABinderProcess_joinThreadPool(); return -1; } From 23d35bbfafc784a7dfe686e7954a53cac60caf2b Mon Sep 17 00:00:00 2001 From: Wayne Ma Date: Wed, 28 Sep 2022 15:23:04 +0800 Subject: [PATCH 10/45] Make libstatspull_bindgen available to resolv apex. Test: m successed Change-Id: Ia367ab5a87794c82238291b27a783278f319e767 (cherry picked from commit 8044045a1ecf2afae948a9877341b1ea372f2a80) Merged-In: Ia367ab5a87794c82238291b27a783278f319e767 --- libstats/pull_rust/Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/libstats/pull_rust/Android.bp b/libstats/pull_rust/Android.bp index 4ffa98d14..cc9f5b4cf 100644 --- a/libstats/pull_rust/Android.bp +++ b/libstats/pull_rust/Android.bp @@ -47,6 +47,7 @@ rust_bindgen { min_sdk_version: "apex_inherit", apex_available: [ "//apex_available:platform", + "com.android.resolv", "com.android.virt", ] } From b5f75c4ecbcfeff6f4bc78b8d24e638b51339e51 Mon Sep 17 00:00:00 2001 From: Seth Moore Date: Wed, 2 Nov 2022 21:41:58 -0700 Subject: [PATCH 11/45] Add dependency on split out RKP HAL Bug: 254112961 Test: N/A Ignore-AOSP-First: need to fix internal-only dependencies first Change-Id: Ia13374cbc4f29964d91da3ce31d5fffa4d54d5ce --- trusty/keymaster/Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/trusty/keymaster/Android.bp b/trusty/keymaster/Android.bp index adc9fdffb..31f0a7224 100644 --- a/trusty/keymaster/Android.bp +++ b/trusty/keymaster/Android.bp @@ -109,6 +109,7 @@ cc_binary { "keymint_use_latest_hal_aidl_ndk_shared", ], shared_libs: [ + "android.hardware.security.rkp-V3-ndk", "android.hardware.security.secureclock-V1-ndk", "android.hardware.security.sharedsecret-V1-ndk", "lib_android_keymaster_keymint_utils", From 9d99701bead30ffc6a9849fb33024680082ca782 Mon Sep 17 00:00:00 2001 From: Seth Moore Date: Fri, 4 Nov 2022 16:40:46 +0000 Subject: [PATCH 12/45] Revert "Add dependency on split out RKP HAL" Revert "Add dependency on split out RKP HAL" Revert "Split rkp from keymint." Revert "Add dependency on newly-split RKP HAL" Revert "Add dependencies on newly-split RKP HAL" Revert "Add dependency on split out RKP HAL" Revert submission 20364235-split-rkp-aidl Reason for revert: Build break in android.hardware.identity-api Reverted Changes: Ib86454bbb:Update dependencies on HAL types moved from keymin... I501c967e2:Add dependencies on newly-split RKP HAL I08560f9af:Add dependency on split out RKP HAL I87133e385:Add dependency on split out RKP HAL Ia13374cbc:Add dependency on split out RKP HAL I72bc1774c:Add dependency on newly-split RKP HAL I71ac265e3:Add dependency on newly-split RKP HAL Ie0e17bb2c:Update the RKP aidl dependency I5d24f47ce:Update README and CHANGELOG for RKP I4b2498dd1:Split rkp from keymint. I266009d75:Add dependency on newly-split rkp HAL Change-Id: Ibac15817a4d2170e4c8c36956d7be1ad0f3cb8a8 --- trusty/keymaster/Android.bp | 1 - 1 file changed, 1 deletion(-) diff --git a/trusty/keymaster/Android.bp b/trusty/keymaster/Android.bp index 31f0a7224..adc9fdffb 100644 --- a/trusty/keymaster/Android.bp +++ b/trusty/keymaster/Android.bp @@ -109,7 +109,6 @@ cc_binary { "keymint_use_latest_hal_aidl_ndk_shared", ], shared_libs: [ - "android.hardware.security.rkp-V3-ndk", "android.hardware.security.secureclock-V1-ndk", "android.hardware.security.sharedsecret-V1-ndk", "lib_android_keymaster_keymint_utils", From 681abb61a2ad8b626019b8c117055856019e8a1d Mon Sep 17 00:00:00 2001 From: Seth Moore Date: Fri, 4 Nov 2022 17:39:05 +0000 Subject: [PATCH 13/45] Revert^2 "Add dependency on split out RKP HAL" 9d99701bead30ffc6a9849fb33024680082ca782 Change-Id: I9dcb9b94b0e22466cd42592f4921eec3e4fcb13d --- trusty/keymaster/Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/trusty/keymaster/Android.bp b/trusty/keymaster/Android.bp index adc9fdffb..31f0a7224 100644 --- a/trusty/keymaster/Android.bp +++ b/trusty/keymaster/Android.bp @@ -109,6 +109,7 @@ cc_binary { "keymint_use_latest_hal_aidl_ndk_shared", ], shared_libs: [ + "android.hardware.security.rkp-V3-ndk", "android.hardware.security.secureclock-V1-ndk", "android.hardware.security.sharedsecret-V1-ndk", "lib_android_keymaster_keymint_utils", From ecfbf9c6f48915b76a62c2668137a985e04d1436 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Tue, 6 Dec 2022 15:42:48 +0000 Subject: [PATCH 14/45] Add task profile "Dex2OatBackground". Bug: 261557677 Test: Presubmit Ignore-AOSP-First: Will cherry-pick as soon as the CL is merged. Change-Id: I33f4d1d2270da82cf90a772ef52b450bcecafec2 --- libprocessgroup/profiles/task_profiles.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index 15f95fcff..39c124e94 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -805,6 +805,10 @@ "Name": "Dex2OatBootComplete", "Profiles": [ "Dex2oatPerformance", "LowIoPriority", "TimerSlackHigh" ] }, + { + "Name": "Dex2OatBootBackground", + "Profiles": [ "Dex2OatBootComplete" ] + }, { "Name": "OtaProfiles", "Profiles": [ "ServiceCapacityLow", "LowIoPriority", "HighEnergySaving" ] From ab9b5df4b84d83426006d1e84b0c361ae8c9b18e Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Fri, 9 Dec 2022 22:30:57 +0000 Subject: [PATCH 15/45] Replace "apex_inherit" min_sdk_version Replace "apex_inherit" min_sdk_version to a conditional setting. If environment veriable KEEP_APEX_INHERIT is set, using "apex_inherit" as the min_sdk_version, otherwise set the number to "29". For more detail please refer https://docs.google.com/document/d/1R2vZw0cQa-haAMgFyQ682uSq9aGBNQrzMHKIsU17-XY/edit?usp=sharing&resourcekey=0-gUbs463r9LCKs7vdP_Xkmg Test: build APEX uses this library, and presubmit Bug: 254634795 Ignore-AOSP-First: Need to submit with other changes. Change-Id: Ie6984128e6b84ba73de3f4c08eca5560657c5ca2 --- libutils/Android.bp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libutils/Android.bp b/libutils/Android.bp index af4a3dc5d..d77efd6df 100644 --- a/libutils/Android.bp +++ b/libutils/Android.bp @@ -21,11 +21,13 @@ cc_library_headers { vendor_ramdisk_available: true, host_supported: true, native_bridge_supported: true, + defaults: [ + "apex-lowest-min-sdk-version", + ], apex_available: [ "//apex_available:platform", "//apex_available:anyapex", ], - min_sdk_version: "apex_inherit", header_libs: [ "libbase_headers", @@ -124,7 +126,10 @@ cc_defaults { cc_defaults { name: "libutils_impl_defaults", - defaults: ["libutils_defaults"], + defaults: [ + "libutils_defaults", + "apex-lowest-min-sdk-version", + ], native_bridge_supported: true, srcs: [ @@ -167,7 +172,6 @@ cc_defaults { "//apex_available:anyapex", "//apex_available:platform", ], - min_sdk_version: "apex_inherit", afdo: true, } From 473f03bfd9497abe614ea34d96c8b6305c31a1ff Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Fri, 9 Dec 2022 23:50:58 +0000 Subject: [PATCH 16/45] Rename "Dex2OatBootBackground" to "Dex2OatBackground". Bug: 261557677 Change-Id: I52e778d13cffcae4212acb05ef2bd62b827fbaf3 Test: Presubmit Ignore-AOSP-First: Will cherry-pick as soon as the CL is merged. --- libprocessgroup/profiles/task_profiles.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index 39c124e94..c48509788 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -806,7 +806,7 @@ "Profiles": [ "Dex2oatPerformance", "LowIoPriority", "TimerSlackHigh" ] }, { - "Name": "Dex2OatBootBackground", + "Name": "Dex2OatBackground", "Profiles": [ "Dex2OatBootComplete" ] }, { From 90879edeea0b2fd1e766428693d736163f39c511 Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Fri, 20 Jan 2023 13:52:35 -0800 Subject: [PATCH 17/45] Listen on property_service_for_system socket It is easy to dos the property_service socket, since it will wait for a complete data packet from one command before moving on to the next one. To prevent low privilege apps interfering with system and root apps, add a second property_service socket that only they can use Bug: 262237198 Test: Run POC in one shell, set properties as root and system in another Ignore-AOSP-First: Security fix Change-Id: I1d6fec833fc24352546bb90f770d3c4b675f5716 --- init/property_service.cpp | 53 +++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 87ffdb917..14e55b36a 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -57,12 +57,12 @@ #include #include #include +#include #include #include #include #include #include - #include "debug_ramdisk.h" #include "epoll.h" #include "init.h" @@ -111,12 +111,13 @@ constexpr auto API_LEVEL_CURRENT = 10000; static bool persistent_properties_loaded = false; -static int property_set_fd = -1; static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; +static std::thread property_service_for_system_thread; +static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -394,6 +395,7 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } + auto lock = std::lock_guard{set_property_lock}; prop_info* pi = (prop_info*)__system_property_find(name.c_str()); if (pi != nullptr) { // ro.* properties are actually "write-once". @@ -582,10 +584,10 @@ uint32_t HandlePropertySetNoSocket(const std::string& name, const std::string& v return *ret; } -static void handle_property_set_fd() { +static void handle_property_set_fd(int fd) { static constexpr uint32_t kDefaultSocketTimeout = 2000; /* ms */ - int s = accept4(property_set_fd, nullptr, nullptr, SOCK_CLOEXEC); + int s = accept4(fd, nullptr, nullptr, SOCK_CLOEXEC); if (s == -1) { return; } @@ -1422,19 +1424,21 @@ static void HandleInitSocket() { } } -static void PropertyServiceThread() { +static void PropertyServiceThread(int fd, bool listen_init) { Epoll epoll; if (auto result = epoll.Open(); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(property_set_fd, handle_property_set_fd); + if (auto result = epoll.RegisterHandler(fd, std::bind(handle_property_set_fd, fd)); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { - LOG(FATAL) << result.error(); + if (listen_init) { + if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { + LOG(FATAL) << result.error(); + } } while (true) { @@ -1485,6 +1489,23 @@ void PersistWriteThread::Write(std::string name, std::string value, SocketConnec cv_.notify_all(); } +void StartThread(const char* name, int mode, int gid, std::thread& t, bool listen_init) { + int fd = -1; + if (auto result = CreateSocket(name, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, + /*passcred=*/false, /*should_listen=*/false, mode, /*uid=*/0, + /*gid=*/gid, /*socketcon=*/{}); + result.ok()) { + fd = *result; + } else { + LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); + } + + listen(fd, 8); + + auto new_thread = std::thread(PropertyServiceThread, fd, listen_init); + t.swap(new_thread); +} + void StartPropertyService(int* epoll_socket) { InitPropertySet("ro.property_service.version", "2"); @@ -1496,19 +1517,9 @@ void StartPropertyService(int* epoll_socket) { init_socket = sockets[1]; StartSendingMessages(); - if (auto result = CreateSocket(PROP_SERVICE_NAME, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, - /*passcred=*/false, /*should_listen=*/false, 0666, /*uid=*/0, - /*gid=*/0, /*socketcon=*/{}); - result.ok()) { - property_set_fd = *result; - } else { - LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); - } - - listen(property_set_fd, 8); - - auto new_thread = std::thread{PropertyServiceThread}; - property_service_thread.swap(new_thread); + StartThread(PROP_SERVICE_FOR_SYSTEM_NAME, 0660, AID_SYSTEM, property_service_for_system_thread, + true); + StartThread(PROP_SERVICE_NAME, 0666, 0, property_service_thread, false); auto async_persist_writes = android::base::GetBoolProperty("ro.property_service.async_persist_writes", false); From f4846e2d08aca746622dbb5826e9f33cdf02ab6d Mon Sep 17 00:00:00 2001 From: AleX Pelosi Date: Fri, 17 Feb 2023 01:39:21 +0000 Subject: [PATCH 18/45] BatteryMonitor: batteryStateOfHealth should be a property Bug: 251427118 Test: m Ignore-AOSP-First: will cherry-pick to aosp later Change-Id: I21bcd160f51cf8818d0c3c8c54c615314808e586 Signed-off-by: AleX Pelosi --- healthd/BatteryMonitor.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp index b180a58fe..66e1e63ef 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -392,12 +392,13 @@ void BatteryMonitor::updateValues(void) { mHealthInfo->batteryFullChargeDesignCapacityUah = getIntField(mHealthdConfig->batteryFullChargeDesignCapacityUahPath); - if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) - mHealthInfo->batteryStateOfHealth = getIntField(mHealthdConfig->batteryStateOfHealthPath); - if (!mHealthdConfig->batteryHealthStatusPath.isEmpty()) mBatteryHealthStatus = getIntField(mHealthdConfig->batteryHealthStatusPath); + if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) + mHealthInfo->batteryHealthData->batteryStateOfHealth = + getIntField(mHealthdConfig->batteryStateOfHealthPath); + if (!mHealthdConfig->batteryManufacturingDatePath.isEmpty()) mHealthInfo->batteryHealthData->batteryManufacturingDateSeconds = getIntField(mHealthdConfig->batteryManufacturingDatePath); @@ -591,6 +592,10 @@ int BatteryMonitor::getBatteryHealthData(int id) { if (!mHealthdConfig->batteryFirstUsageDatePath.isEmpty()) return getIntField(mHealthdConfig->batteryFirstUsageDatePath); } + if (id == BATTERY_PROP_STATE_OF_HEALTH) { + if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) + return getIntField(mHealthdConfig->batteryStateOfHealthPath); + } return 0; } @@ -669,6 +674,11 @@ status_t BatteryMonitor::getProperty(int id, struct BatteryProperty *val) { ret = OK; break; + case BATTERY_PROP_STATE_OF_HEALTH: + val->valueInt64 = getBatteryHealthData(BATTERY_PROP_STATE_OF_HEALTH); + ret = OK; + break; + default: break; } From 6f7f7c8bcdd73df2c4b8d956442b797c593beaed Mon Sep 17 00:00:00 2001 From: Jack Wu Date: Sun, 19 Feb 2023 01:38:45 +0800 Subject: [PATCH 19/45] Revert "BatteryMonitor: batteryStateOfHealth should be a property" This reverts commit f4846e2d08aca746622dbb5826e9f33cdf02ab6d. Reason for revert: merge to aosp first Bug: 251427118 Test: m Ignore-AOSP-First: merge to aosp first Change-Id: I84bf3d2059303172161466333db7ad45da5d5270 Signed-off-by: Jack Wu --- healthd/BatteryMonitor.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp index 66e1e63ef..b180a58fe 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -392,13 +392,12 @@ void BatteryMonitor::updateValues(void) { mHealthInfo->batteryFullChargeDesignCapacityUah = getIntField(mHealthdConfig->batteryFullChargeDesignCapacityUahPath); + if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) + mHealthInfo->batteryStateOfHealth = getIntField(mHealthdConfig->batteryStateOfHealthPath); + if (!mHealthdConfig->batteryHealthStatusPath.isEmpty()) mBatteryHealthStatus = getIntField(mHealthdConfig->batteryHealthStatusPath); - if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) - mHealthInfo->batteryHealthData->batteryStateOfHealth = - getIntField(mHealthdConfig->batteryStateOfHealthPath); - if (!mHealthdConfig->batteryManufacturingDatePath.isEmpty()) mHealthInfo->batteryHealthData->batteryManufacturingDateSeconds = getIntField(mHealthdConfig->batteryManufacturingDatePath); @@ -592,10 +591,6 @@ int BatteryMonitor::getBatteryHealthData(int id) { if (!mHealthdConfig->batteryFirstUsageDatePath.isEmpty()) return getIntField(mHealthdConfig->batteryFirstUsageDatePath); } - if (id == BATTERY_PROP_STATE_OF_HEALTH) { - if (!mHealthdConfig->batteryStateOfHealthPath.isEmpty()) - return getIntField(mHealthdConfig->batteryStateOfHealthPath); - } return 0; } @@ -674,11 +669,6 @@ status_t BatteryMonitor::getProperty(int id, struct BatteryProperty *val) { ret = OK; break; - case BATTERY_PROP_STATE_OF_HEALTH: - val->valueInt64 = getBatteryHealthData(BATTERY_PROP_STATE_OF_HEALTH); - ret = OK; - break; - default: break; } From 65d8e53b606e4ae7387f421c5277a942f8e729d8 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 2 Mar 2023 14:12:49 -0800 Subject: [PATCH 20/45] libprocessgroup: fix boot time performance regression The way processes are accounted in DoKillProcessGroupOnce has been changed recently, which affects retries in KillProcessGroup. More specifically, initialPid was not counted before and would not cause a retry with 5ms sleep. Restore previous behavior to avoid boot time regressions. Bug: 271198843 Signed-off-by: Suren Baghdasaryan Change-Id: Ibc1bdd855898688a4a03806671e6ac31570aedf9 (cherry picked from commit on android-review.googlesource.com host: 4f7cc8c3455b2a28984e2682c1b8075cb57d67fe) Merged-In: Ibc1bdd855898688a4a03806671e6ac31570aedf9 --- libprocessgroup/processgroup.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 92a27efb7..71ba7ced2 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -377,6 +377,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, std::set pgids; pgids.emplace(initialPid); std::set pids; + int processes = 0; std::unique_ptr fd(nullptr, fclose); @@ -395,6 +396,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, pid_t pid; bool file_is_empty = true; while (fscanf(fd.get(), "%d\n", &pid) == 1 && pid >= 0) { + processes++; file_is_empty = false; if (pid == 0) { // Should never happen... but if it does, trying to kill this @@ -424,15 +426,12 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, } } - int processes = 0; // Kill all process groups. for (const auto pgid : pgids) { LOG(VERBOSE) << "Killing process group " << -pgid << " in uid " << uid << " as part of process cgroup " << initialPid; - if (kill(-pgid, signal) == 0) { - processes++; - } else if (errno != ESRCH) { + if (kill(-pgid, signal) == -1 && errno != ESRCH) { PLOG(WARNING) << "kill(" << -pgid << ", " << signal << ") failed"; } } @@ -442,9 +441,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, LOG(VERBOSE) << "Killing pid " << pid << " in uid " << uid << " as part of process cgroup " << initialPid; - if (kill(pid, signal) == 0) { - processes++; - } else if (errno != ESRCH) { + if (kill(pid, signal) == -1 && errno != ESRCH) { PLOG(WARNING) << "kill(" << pid << ", " << signal << ") failed"; } } From 606afc7b7451aba90e3634076d9b59a5ef08186b Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Fri, 24 Mar 2023 14:31:57 -0700 Subject: [PATCH 21/45] Fix deadlock caused by two-threaded property controls Two threaded property controls were introduced in ag/21063815 to prevent DOS for power controls. However, this causes deadlocks, so limit the second thread to just sys.powerctl messages. Bug: 273785601 Test: Boots, power messages work Ignore-AOSP-First: Security fix Change-Id: Ie27dc3b0cd9e2d28e94f2ad398c55ee27bc35835 --- init/property_service.cpp | 52 +++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 2d084db46..4242912ed 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -117,7 +117,6 @@ static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; static std::thread property_service_for_system_thread; -static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -395,32 +394,37 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - auto lock = std::lock_guard{set_property_lock}; - prop_info* pi = (prop_info*)__system_property_find(name.c_str()); - if (pi != nullptr) { - // ro.* properties are actually "write-once". - if (StartsWith(name, "ro.")) { - *error = "Read-only property was already set"; - return {PROP_ERROR_READ_ONLY_PROPERTY}; - } - - __system_property_update(pi, value.c_str(), valuelen); + if (name == "sys.powerctl") { + // No action here - NotifyPropertyChange will trigger the appropriate action, and since this + // can come to the second thread, we mustn't call out to the __system_property_* functions + // which support multiple readers but only one mutator. } else { - int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); - if (rc < 0) { - *error = "__system_property_add failed"; - return {PROP_ERROR_SET_FAILED}; - } - } + prop_info* pi = (prop_info*)__system_property_find(name.c_str()); + if (pi != nullptr) { + // ro.* properties are actually "write-once". + if (StartsWith(name, "ro.")) { + *error = "Read-only property was already set"; + return {PROP_ERROR_READ_ONLY_PROPERTY}; + } - // Don't write properties to disk until after we have read all default - // properties to prevent them from being overwritten by default values. - if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { - if (persist_write_thread) { - persist_write_thread->Write(name, value, std::move(*socket)); - return {}; + __system_property_update(pi, value.c_str(), valuelen); + } else { + int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); + if (rc < 0) { + *error = "__system_property_add failed"; + return {PROP_ERROR_SET_FAILED}; + } + } + + // Don't write properties to disk until after we have read all default + // properties to prevent them from being overwritten by default values. + if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { + if (persist_write_thread) { + persist_write_thread->Write(name, value, std::move(*socket)); + return {}; + } + WritePersistentProperty(name, value); } - WritePersistentProperty(name, value); } NotifyPropertyChange(name, value); From d3dc653d22f97820742c86b4bc4edb627a0dfd21 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Tue, 4 Apr 2023 18:41:13 +0000 Subject: [PATCH 22/45] libprocessgroup: Add sendSignalToProcessGroup Add a function which sends signals to all members of a process group, but does not wait for the processes to exit, or for the associated cgroup to be removed. Bug: 274646058 Ignore-AOSP-First: Dependency of ActivityManager change which developed on interal git_master Test: Force-stop of chrome with 15 tabs completes ~500ms faster Test: Full Play store update causes no ANR (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:d87b6018d25cbbd33b345dc58c634718bf5d0def) Merged-In: I37dbdecb3394101abbee8495e71f6912b3c031f5 Change-Id: I37dbdecb3394101abbee8495e71f6912b3c031f5 --- libprocessgroup/include/processgroup/processgroup.h | 5 +++++ libprocessgroup/processgroup.cpp | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 8fa9fd552..48bc0b7f3 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -76,6 +76,11 @@ int killProcessGroup(uid_t uid, int initialPid, int signal, int* max_processes = // that it only returns 0 in the case that the cgroup exists and it contains no processes. int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_processes = nullptr); +// Sends the provided signal to all members of a process group, but does not wait for processes to +// exit, or for the cgroup to be removed. Callers should also ensure that killProcessGroup is called +// later to ensure the cgroup is fully removed, otherwise system resources may leak. +int sendSignalToProcessGroup(uid_t uid, int initialPid, int signal); + int createProcessGroup(uid_t uid, int initialPid, bool memControl = false); // Set various properties of a process group. For these functions to work, the process group must diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index ae9914d64..a02159401 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -542,6 +542,15 @@ int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_process return KillProcessGroup(uid, initialPid, signal, 0 /*retries*/, max_processes); } +int sendSignalToProcessGroup(uid_t uid, int initialPid, int signal) { + std::string hierarchy_root_path; + if (CgroupsAvailable()) { + CgroupGetControllerPath(CGROUPV2_CONTROLLER_NAME, &hierarchy_root_path); + } + const char* cgroup = hierarchy_root_path.c_str(); + return DoKillProcessGroupOnce(cgroup, uid, initialPid, signal); +} + static int createProcessGroupInternal(uid_t uid, int initialPid, std::string cgroup, bool activate_controllers) { auto uid_path = ConvertUidToPath(cgroup.c_str(), uid); From 3a81dda861c85b15b1ea614b6c15b2274105d113 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 24 Apr 2023 18:14:53 -0700 Subject: [PATCH 23/45] Re-add code to skip gettings logs on logd crashes. Also add new unit tests to verify this behavior. Bug: 276934420 Test: New unit tests pass. Test: Ran new unit tests without pthread_setname_np call and verified Test: the tests fail. Test: Force crash logd and verify log messages are not gathered. Test: Force crash a logd thread and verify log messages are not gathered. Change-Id: If8effef68f629432923cdc89e57d28ef5b8b4ce2 Merged-In: If8effef68f629432923cdc89e57d28ef5b8b4ce2 (cherry picked from commit bda106416096d51d872bffacfd251e586f982004) --- debuggerd/debuggerd_test.cpp | 56 ++++++++++++++++++++++ debuggerd/libdebuggerd/tombstone_proto.cpp | 9 +++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index a00a2026f..39d7fff1c 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -2703,3 +2704,58 @@ TEST_F(CrasherTest, verify_build_id) { } ASSERT_TRUE(found_valid_elf) << "Did not find any elf files with valid BuildIDs to check."; } + +const char kLogMessage[] = "Should not see this log message."; + +// Verify that the logd process does not read the log. +TEST_F(CrasherTest, logd_skips_reading_logs) { + StartProcess([]() { + pthread_setname_np(pthread_self(), "logd"); + LOG(INFO) << kLogMessage; + abort(); + }); + + unique_fd output_fd; + StartIntercept(&output_fd); + FinishCrasher(); + AssertDeath(SIGABRT); + int intercept_result; + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + // logd should not contain our log message. + ASSERT_NOT_MATCH(result, kLogMessage); +} + +// Verify that the logd process does not read the log when the non-main +// thread crashes. +TEST_F(CrasherTest, logd_skips_reading_logs_not_main_thread) { + StartProcess([]() { + pthread_setname_np(pthread_self(), "logd"); + LOG(INFO) << kLogMessage; + + std::thread thread([]() { + pthread_setname_np(pthread_self(), "not_logd_thread"); + // Raise the signal on the side thread. + raise_debugger_signal(kDebuggerdTombstone); + }); + thread.join(); + _exit(0); + }); + + unique_fd output_fd; + StartIntercept(&output_fd, kDebuggerdTombstone); + FinishCrasher(); + AssertDeath(0); + + int intercept_result; + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_BACKTRACE_FRAME(result, "raise_debugger_signal"); + ASSERT_NOT_MATCH(result, kLogMessage); +} diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp index 9a565deb6..acd814efa 100644 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto.cpp @@ -690,7 +690,14 @@ void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::AndroidUnwinder* // Only dump logs on debuggable devices. if (android::base::GetBoolProperty("ro.debuggable", false)) { - dump_logcat(&result, main_thread.pid); + // Get the thread that corresponds to the main pid of the process. + const ThreadInfo& thread = threads.at(main_thread.pid); + + // Do not attempt to dump logs of the logd process because the gathering + // of logs can hang until a timeout occurs. + if (thread.thread_name != "logd") { + dump_logcat(&result, main_thread.pid); + } } dump_open_fds(&result, open_files); From 4a8c1461ff2135e29bb00bcd32989567108e776a Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Wed, 12 Apr 2023 01:24:23 +0000 Subject: [PATCH 24/45] libprocessgroup: implement task profile validity checks Provide profile validity check functions for cases when user wants to check whether a profile can be successfully applied before actually applying it. Add test cases to cover new APIs. Also add a wrapper function for framework code to call it. Bug: 277233783 Test: atest task_profiles_test Test: manually verify freezer with outdated cgroup configuration Signed-off-by: Suren Baghdasaryan Signed-off-by: Li Li Change-Id: Iefb321dead27adbe67721972f164efea213c06cb Merged-In: Iefb321dead27adbe67721972f164efea213c06cb --- .../include/processgroup/processgroup.h | 4 + libprocessgroup/processgroup.cpp | 10 ++ libprocessgroup/task_profiles.cpp | 121 ++++++++++++++++++ libprocessgroup/task_profiles.h | 20 ++- libprocessgroup/task_profiles_test.cpp | 50 ++++++++ 5 files changed, 202 insertions(+), 3 deletions(-) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 48bc0b7f3..dbaeb9397 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -96,6 +96,10 @@ void removeAllEmptyProcessGroups(void); // Returns false in case of error, true in case of success bool getAttributePathForTask(const std::string& attr_name, int tid, std::string* path); +// Check if a profile can be applied without failing. +// Returns true if it can be applied without failing, false otherwise +bool isProfileValidForProcess(const std::string& profile_name, int uid, int pid); + #endif // __ANDROID_VNDK__ __END_DECLS diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index a02159401..234b793d4 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -657,3 +657,13 @@ bool setProcessGroupLimit(uid_t, int pid, int64_t limit_in_bytes) { bool getAttributePathForTask(const std::string& attr_name, int tid, std::string* path) { return CgroupGetAttributePathForTask(attr_name, tid, path); } + +bool isProfileValidForProcess(const std::string& profile_name, int uid, int pid) { + const TaskProfile* tp = TaskProfiles::GetInstance().GetProfile(profile_name); + + if (tp == nullptr) { + return false; + } + + return tp->IsValidForProcess(uid, pid); +} \ No newline at end of file diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 17318289f..44dba2a16 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -259,6 +259,31 @@ bool SetAttributeAction::ExecuteForUID(uid_t uid) const { return true; } +bool SetAttributeAction::IsValidForProcess(uid_t, pid_t pid) const { + return IsValidForTask(pid); +} + +bool SetAttributeAction::IsValidForTask(int tid) const { + std::string path; + + if (!attribute_->GetPathForTask(tid, &path)) { + return false; + } + + if (!access(path.c_str(), W_OK)) { + // operation will succeed + return true; + } + + if (!access(path.c_str(), F_OK)) { + // file exists but not writable + return false; + } + + // file does not exist, ignore if optional + return optional_; +} + SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_[ProfileAction::RCT_TASK]); @@ -396,6 +421,39 @@ void SetCgroupAction::DropResourceCaching(ResourceCacheType cache_type) { FdCacheHelper::Drop(fd_[cache_type]); } +bool SetCgroupAction::IsValidForProcess(uid_t uid, pid_t pid) const { + std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_[ProfileAction::RCT_PROCESS])) { + return true; + } + + if (fd_[ProfileAction::RCT_PROCESS] == FdCacheHelper::FDS_INACCESSIBLE) { + return false; + } + + std::string procs_path = controller()->GetProcsFilePath(path_, uid, pid); + return access(procs_path.c_str(), W_OK) == 0; +} + +bool SetCgroupAction::IsValidForTask(int) const { + std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_[ProfileAction::RCT_TASK])) { + return true; + } + + if (fd_[ProfileAction::RCT_TASK] == FdCacheHelper::FDS_INACCESSIBLE) { + return false; + } + + if (fd_[ProfileAction::RCT_TASK] == FdCacheHelper::FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + return false; + } + + std::string tasks_path = controller()->GetTasksFilePath(path_); + return access(tasks_path.c_str(), W_OK) == 0; +} + WriteFileAction::WriteFileAction(const std::string& task_path, const std::string& proc_path, const std::string& value, bool logfailures) : task_path_(task_path), proc_path_(proc_path), value_(value), logfailures_(logfailures) { @@ -532,6 +590,37 @@ void WriteFileAction::DropResourceCaching(ResourceCacheType cache_type) { FdCacheHelper::Drop(fd_[cache_type]); } +bool WriteFileAction::IsValidForProcess(uid_t, pid_t) const { + std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_[ProfileAction::RCT_PROCESS])) { + return true; + } + + if (fd_[ProfileAction::RCT_PROCESS] == FdCacheHelper::FDS_INACCESSIBLE) { + return false; + } + + return access(proc_path_.empty() ? task_path_.c_str() : proc_path_.c_str(), W_OK) == 0; +} + +bool WriteFileAction::IsValidForTask(int) const { + std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_[ProfileAction::RCT_TASK])) { + return true; + } + + if (fd_[ProfileAction::RCT_TASK] == FdCacheHelper::FDS_INACCESSIBLE) { + return false; + } + + if (fd_[ProfileAction::RCT_TASK] == FdCacheHelper::FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + return false; + } + + return access(task_path_.c_str(), W_OK) == 0; +} + bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { for (const auto& profile : profiles_) { profile->ExecuteForProcess(uid, pid); @@ -558,6 +647,24 @@ void ApplyProfileAction::DropResourceCaching(ResourceCacheType cache_type) { } } +bool ApplyProfileAction::IsValidForProcess(uid_t uid, pid_t pid) const { + for (const auto& profile : profiles_) { + if (!profile->IsValidForProcess(uid, pid)) { + return false; + } + } + return true; +} + +bool ApplyProfileAction::IsValidForTask(int tid) const { + for (const auto& profile : profiles_) { + if (!profile->IsValidForTask(tid)) { + return false; + } + } + return true; +} + void TaskProfile::MoveTo(TaskProfile* profile) { profile->elements_ = std::move(elements_); profile->res_cached_ = res_cached_; @@ -620,6 +727,20 @@ void TaskProfile::DropResourceCaching(ProfileAction::ResourceCacheType cache_typ res_cached_ = false; } +bool TaskProfile::IsValidForProcess(uid_t uid, pid_t pid) const { + for (const auto& element : elements_) { + if (!element->IsValidForProcess(uid, pid)) return false; + } + return true; +} + +bool TaskProfile::IsValidForTask(int tid) const { + for (const auto& element : elements_) { + if (!element->IsValidForTask(tid)) return false; + } + return true; +} + void TaskProfiles::DropResourceCaching(ProfileAction::ResourceCacheType cache_type) const { for (auto& iter : profiles_) { iter.second->DropResourceCaching(cache_type); diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index a8ecb873d..a62c5b0a9 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -72,12 +72,14 @@ class ProfileAction { virtual const char* Name() const = 0; // Default implementations will fail - virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; }; - virtual bool ExecuteForTask(int) const { return false; }; - virtual bool ExecuteForUID(uid_t) const { return false; }; + virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; } + virtual bool ExecuteForTask(int) const { return false; } + virtual bool ExecuteForUID(uid_t) const { return false; } virtual void EnableResourceCaching(ResourceCacheType) {} virtual void DropResourceCaching(ResourceCacheType) {} + virtual bool IsValidForProcess(uid_t uid, pid_t pid) const { return false; } + virtual bool IsValidForTask(int tid) const { return false; } protected: enum CacheUseResult { SUCCESS, FAIL, UNUSED }; @@ -103,6 +105,8 @@ class SetTimerSlackAction : public ProfileAction { const char* Name() const override { return "SetTimerSlack"; } bool ExecuteForTask(int tid) const override; + bool IsValidForProcess(uid_t uid, pid_t pid) const override { return true; } + bool IsValidForTask(int tid) const override { return true; } private: unsigned long slack_; @@ -120,6 +124,8 @@ class SetAttributeAction : public ProfileAction { bool ExecuteForProcess(uid_t uid, pid_t pid) const override; bool ExecuteForTask(int tid) const override; bool ExecuteForUID(uid_t uid) const override; + bool IsValidForProcess(uid_t uid, pid_t pid) const override; + bool IsValidForTask(int tid) const override; private: const IProfileAttribute* attribute_; @@ -137,6 +143,8 @@ class SetCgroupAction : public ProfileAction { bool ExecuteForTask(int tid) const override; void EnableResourceCaching(ResourceCacheType cache_type) override; void DropResourceCaching(ResourceCacheType cache_type) override; + bool IsValidForProcess(uid_t uid, pid_t pid) const override; + bool IsValidForTask(int tid) const override; const CgroupController* controller() const { return &controller_; } @@ -161,6 +169,8 @@ class WriteFileAction : public ProfileAction { bool ExecuteForTask(int tid) const override; void EnableResourceCaching(ResourceCacheType cache_type) override; void DropResourceCaching(ResourceCacheType cache_type) override; + bool IsValidForProcess(uid_t uid, pid_t pid) const override; + bool IsValidForTask(int tid) const override; private: std::string task_path_, proc_path_, value_; @@ -186,6 +196,8 @@ class TaskProfile { bool ExecuteForUID(uid_t uid) const; void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); + bool IsValidForProcess(uid_t uid, pid_t pid) const; + bool IsValidForTask(int tid) const; private: const std::string name_; @@ -204,6 +216,8 @@ class ApplyProfileAction : public ProfileAction { bool ExecuteForTask(int tid) const override; void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type) override; void DropResourceCaching(ProfileAction::ResourceCacheType cache_type) override; + bool IsValidForProcess(uid_t uid, pid_t pid) const override; + bool IsValidForTask(int tid) const override; private: std::vector> profiles_; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index 6a5b48bf3..eadbe7697 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -175,6 +175,32 @@ TEST_P(SetAttributeFixture, SetAttribute) { } } +class TaskProfileFixture : public TestWithParam { + public: + ~TaskProfileFixture() = default; +}; + +TEST_P(TaskProfileFixture, TaskProfile) { + // Treehugger runs host tests inside a container without cgroupv2 support. + if (!IsCgroupV2MountedRw()) { + GTEST_SKIP(); + return; + } + const TestParam params = GetParam(); + ProfileAttributeMock pa(params.attr_name); + // Test simple profile with one action + std::shared_ptr tp = std::make_shared("test_profile"); + tp->Add(std::make_unique(&pa, params.attr_value, params.optional_attr)); + EXPECT_EQ(tp->IsValidForProcess(getuid(), getpid()), params.result); + EXPECT_EQ(tp->IsValidForTask(getpid()), params.result); + // Test aggregate profile + TaskProfile tp2("meta_profile"); + std::vector> profiles = {tp}; + tp2.Add(std::make_unique(profiles)); + EXPECT_EQ(tp2.IsValidForProcess(getuid(), getpid()), params.result); + EXPECT_EQ(tp2.IsValidForTask(getpid()), params.result); +} + // Test the four combinations of optional_attr {false, true} and cgroup attribute { does not exist, // exists }. INSTANTIATE_TEST_SUITE_P( @@ -215,4 +241,28 @@ INSTANTIATE_TEST_SUITE_P( .log_prefix = "Failed to write", .log_suffix = geteuid() == 0 ? "Invalid argument" : "Permission denied"})); +// Test TaskProfile IsValid calls. +INSTANTIATE_TEST_SUITE_P( + TaskProfileTestSuite, TaskProfileFixture, + Values( + // Test operating on non-existing cgroup attribute fails. + TestParam{.attr_name = "no-such-attribute", + .attr_value = ".", + .optional_attr = false, + .result = false}, + // Test operating on optional non-existing cgroup attribute succeeds. + TestParam{.attr_name = "no-such-attribute", + .attr_value = ".", + .optional_attr = true, + .result = true}, + // Test operating on existing cgroup attribute succeeds. + TestParam{.attr_name = "cgroup.procs", + .attr_value = ".", + .optional_attr = false, + .result = true}, + // Test operating on optional existing cgroup attribute succeeds. + TestParam{.attr_name = "cgroup.procs", + .attr_value = ".", + .optional_attr = true, + .result = true})); } // namespace From e80a6b6dd403ef0cabb35dfefa798a6e72d775d3 Mon Sep 17 00:00:00 2001 From: Vincent Donnefort Date: Fri, 28 Apr 2023 09:30:23 +0100 Subject: [PATCH 25/45] ramdisk_node_list: Add urandom node Bionic requires random numbers to init the shadow call stack. Those numbers are obtained via the syscall getrandom (non-blocking) and will fallback to /dev/urandom if the former fails. When loading pKVM modules, we are so early in the boot process that the only source of entropy for the linux RNG are the architecture random number generators... which might be available on some platforms. Without any source of entropy, the only way of generating a random number is to try to generate some, which is what the bionic fallback expects via urandom. As a consequence, add the urandom node to the initramfs. Bug: 274876849 Merged-In: I111e2db53fabd63d070b8e9ab9c52faebf484ab3 Change-Id: I34a0e3f7c72de7344512366d4a96183b445edc2e --- rootdir/ramdisk_node_list | 1 + 1 file changed, 1 insertion(+) diff --git a/rootdir/ramdisk_node_list b/rootdir/ramdisk_node_list index d3ab8a66e..4f45faaec 100644 --- a/rootdir/ramdisk_node_list +++ b/rootdir/ramdisk_node_list @@ -1,3 +1,4 @@ dir dev 0755 0 0 nod dev/null 0600 0 0 c 1 3 nod dev/console 0600 0 0 c 5 1 +nod dev/urandom 0600 0 0 c 1 9 From ac9fbbcb05254d00d5b4f5c8d3d2827c28370ef6 Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Tue, 2 May 2023 22:09:32 +0000 Subject: [PATCH 26/45] libsnapshot: Turn off vabc_legacy_tests on presubmit Temporarily turn off these tests until root cause is found. Bug: 279009697 Test: presubmit Signed-off-by: Akilesh Kailash (cherry picked from https://android-review.googlesource.com/q/commit:defe8381aac9d461f6de8690b5917771b5f6752f) Merged-In: I90f695fac318b71871ff60c1f74c90265437687d Change-Id: I90f695fac318b71871ff60c1f74c90265437687d --- fs_mgr/TEST_MAPPING | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/fs_mgr/TEST_MAPPING b/fs_mgr/TEST_MAPPING index db27cf000..d357e4571 100644 --- a/fs_mgr/TEST_MAPPING +++ b/fs_mgr/TEST_MAPPING @@ -24,9 +24,8 @@ { "name": "vab_legacy_tests" }, - { - "name": "vabc_legacy_tests" - }, + // TODO: b/279009697 + //{"name": "vabc_legacy_tests"}, { "name": "cow_api_test" } @@ -43,9 +42,8 @@ }, { "name": "vab_legacy_tests" - }, - { - "name": "vabc_legacy_tests" } + // TODO: b/279009697 + //{"name": "vabc_legacy_tests"} ] } From 778d7e80a6d9967a874944e3f02f0c04213bf31a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Thu, 27 Apr 2023 19:27:23 +0000 Subject: [PATCH 27/45] remove inprocess tethering Test: TreeHugger Bug: 279942846 (cherry picked from https://android-review.googlesource.com/q/commit:e37468b295851b97db07936e15f53af660607cb4) Merged-In: Ia3a5d289cceac96d310e04fbae3588789cc859ca Change-Id: Ia3a5d289cceac96d310e04fbae3588789cc859ca --- libcutils/fs_config.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libcutils/fs_config.cpp b/libcutils/fs_config.cpp index 79d79dd44..f90a1bc3c 100644 --- a/libcutils/fs_config.cpp +++ b/libcutils/fs_config.cpp @@ -84,14 +84,12 @@ static const struct fs_path_config android_dirs[] = { { 00777, AID_ROOT, AID_ROOT, 0, "sdcard" }, { 00751, AID_ROOT, AID_SDCARD_R, 0, "storage" }, { 00750, AID_ROOT, AID_SYSTEM, 0, "system/apex/com.android.tethering/bin/for-system" }, - { 00750, AID_ROOT, AID_SYSTEM, 0, "system/apex/com.android.tethering.inprocess/bin/for-system" }, { 00751, AID_ROOT, AID_SHELL, 0, "system/bin" }, { 00755, AID_ROOT, AID_ROOT, 0, "system/etc/ppp" }, { 00755, AID_ROOT, AID_SHELL, 0, "system/vendor" }, { 00750, AID_ROOT, AID_SHELL, 0, "system/xbin" }, { 00751, AID_ROOT, AID_SHELL, 0, "system/apex/*/bin" }, { 00750, AID_ROOT, AID_SYSTEM, 0, "system_ext/apex/com.android.tethering/bin/for-system" }, - { 00750, AID_ROOT, AID_SYSTEM, 0, "system_ext/apex/com.android.tethering.inprocess/bin/for-system" }, { 00751, AID_ROOT, AID_SHELL, 0, "system_ext/bin" }, { 00751, AID_ROOT, AID_SHELL, 0, "system_ext/apex/*/bin" }, { 00751, AID_ROOT, AID_SHELL, 0, "vendor/bin" }, @@ -199,9 +197,7 @@ static const struct fs_path_config android_files[] = { // the following files have enhanced capabilities and ARE included // in user builds. { 06755, AID_CLAT, AID_CLAT, 0, "system/apex/com.android.tethering/bin/for-system/clatd" }, - { 06755, AID_CLAT, AID_CLAT, 0, "system/apex/com.android.tethering.inprocess/bin/for-system/clatd" }, { 06755, AID_CLAT, AID_CLAT, 0, "system_ext/apex/com.android.tethering/bin/for-system/clatd" }, - { 06755, AID_CLAT, AID_CLAT, 0, "system_ext/apex/com.android.tethering.inprocess/bin/for-system/clatd" }, { 00700, AID_SYSTEM, AID_SHELL, CAP_MASK_LONG(CAP_BLOCK_SUSPEND), "system/bin/inputflinger" }, { 00750, AID_ROOT, AID_SHELL, CAP_MASK_LONG(CAP_SETUID) | From 7c9c8f52e75daf30ae0cc6130c321769a23e320d Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Thu, 11 May 2023 16:02:30 +0100 Subject: [PATCH 28/45] Run art_boot before odsign. It's necessary to have the right dalvik.vm.* flags in place when they are validated by odrefresh. Test: See the other CL in the topic. Bug: 281850017 Ignore-AOSP-First: Will cherry-pick to AOSP later Change-Id: Ib64790dde97faaa6b62ead2c1c8dd53c97f97f9c --- rootdir/init.rc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rootdir/init.rc b/rootdir/init.rc index 7326783c8..41c60a7f9 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -1024,6 +1024,11 @@ on post-fs-data exec_start derive_classpath load_exports /data/system/environ/classpath + # Start ART's oneshot boot service to propagate boot experiment flags to + # dalvik.vm.*. This needs to be done before odsign since odrefresh uses and + # validates those properties against the signed cache-info.xml. + exec_start art_boot + # Start the on-device signing daemon, and wait for it to finish, to ensure # ART artifacts are generated if needed. # Must start after 'derive_classpath' to have *CLASSPATH variables set. From 878406e7f1d6ecec51bca80f834011e0ba2f1126 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 12 May 2023 16:29:02 -0700 Subject: [PATCH 29/45] Revert "task_profiles.json: Convert tabs into spaces" This reverts commit 6ad747ac2d3d0508a9a9223c8d0a2acbad513c29. Bug: 261857030 Ignore-AOSP-First: this change is for the U branch only. Change-Id: I93447b71146e6e9297ef49026d90dc4c35b91244 Signed-off-by: Bart Van Assche --- libprocessgroup/profiles/task_profiles.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index e44d3bf72..4b8fc19db 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -462,7 +462,7 @@ { "Controller": "blkio", "Path": "background" - } + } }, { "Name": "SetAttribute", @@ -502,7 +502,7 @@ { "Controller": "blkio", "Path": "" - } + } }, { "Name": "SetAttribute", @@ -542,7 +542,7 @@ { "Controller": "blkio", "Path": "" - } + } }, { "Name": "SetAttribute", @@ -582,7 +582,7 @@ { "Controller": "blkio", "Path": "" - } + } }, { "Name": "SetAttribute", From f4a3d72ee8f35fb7bd8e750c1d7b3bd182bb3c3e Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 12 May 2023 16:29:39 -0700 Subject: [PATCH 30/45] Revert "Updating Attributes on task_profiles.json" This reverts commit 92153fb955cd2c59cf9eba07461ddaba1c3d7792. Bug: 261857030 Ignore-AOSP-First: this change is for the U branch only. Change-Id: I99417707f0d0b8c7dca3927b6ce9d52436621f4e Signed-off-by: Bart Van Assche --- libprocessgroup/profiles/task_profiles.json | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index 4b8fc19db..c48509788 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -80,20 +80,17 @@ { "Name": "BfqWeight", "Controller": "io", - "File": "blkio.bfq.weight", - "FileV2": "io.bfq.weight" + "File": "io.bfq.weight" }, { "Name": "CfqGroupIdle", "Controller": "io", - "File": "blkio.group_idle", - "FileV2": "io.group_idle" + "File": "io.group_idle" }, { "Name": "CfqWeight", "Controller": "io", - "File": "blkio.weight", - "FileV2": "io.weight" + "File": "io.weight" } ], From 9fe04000806f9f4d74771e6421e8d3c078fe1887 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 12 May 2023 16:29:43 -0700 Subject: [PATCH 31/45] Revert "libprocessgroup: Add I/O scheduler attributes to task_profiles.json" This reverts commit 9c0fcbb0f7107d455a004719f40435a2a2318fda. Bug: 261857030 Ignore-AOSP-First: this change is for the U branch only. Change-Id: Id4bd3494b33dd6bc0644406d599c9bfd597c7435 Signed-off-by: Bart Van Assche --- libprocessgroup/profiles/task_profiles.json | 123 -------------------- 1 file changed, 123 deletions(-) diff --git a/libprocessgroup/profiles/task_profiles.json b/libprocessgroup/profiles/task_profiles.json index c48509788..1fc66ba10 100644 --- a/libprocessgroup/profiles/task_profiles.json +++ b/libprocessgroup/profiles/task_profiles.json @@ -76,21 +76,6 @@ "Name": "FreezerState", "Controller": "freezer", "File": "cgroup.freeze" - }, - { - "Name": "BfqWeight", - "Controller": "io", - "File": "io.bfq.weight" - }, - { - "Name": "CfqGroupIdle", - "Controller": "io", - "File": "io.group_idle" - }, - { - "Name": "CfqWeight", - "Controller": "io", - "File": "io.weight" } ], @@ -459,33 +444,6 @@ { "Controller": "blkio", "Path": "background" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "BfqWeight", - "Value": "10", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqGroupIdle", - "Value": "0", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqWeight", - "Value": "200", - "Optional": "true" } } ] @@ -499,33 +457,6 @@ { "Controller": "blkio", "Path": "" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "BfqWeight", - "Value": "100", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqGroupIdle", - "Value": "0", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqWeight", - "Value": "1000", - "Optional": "true" } } ] @@ -539,33 +470,6 @@ { "Controller": "blkio", "Path": "" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "BfqWeight", - "Value": "100", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqGroupIdle", - "Value": "0", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqWeight", - "Value": "1000", - "Optional": "true" } } ] @@ -579,33 +483,6 @@ { "Controller": "blkio", "Path": "" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "BfqWeight", - "Value": "100", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqGroupIdle", - "Value": "0", - "Optional": "true" - } - }, - { - "Name": "SetAttribute", - "Params": - { - "Name": "CfqWeight", - "Value": "1000", - "Optional": "true" } } ] From 5be6ec751c88726ff809d9fa0b35a587660d36d4 Mon Sep 17 00:00:00 2001 From: Jack Wu Date: Thu, 13 Apr 2023 19:37:46 +0800 Subject: [PATCH 32/45] BatteryMonitor: support battery health NOT_AVAILABLE from health status Battery health is supported but there is not enough information to determine an accurate value This is a temporary state. Bug: 276400004 Test: m Ignore-AOSP-First: cherry-pick from aosp Change-Id: I0d422db20c51ef7e9dc4fa904729beda625c9fea Merged-In: I0d422db20c51ef7e9dc4fa904729beda625c9fea Signed-off-by: Jack Wu --- healthd/BatteryMonitor.cpp | 2 ++ healthd/include/healthd/BatteryMonitor.h | 1 + 2 files changed, 3 insertions(+) diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp index 66e1e63ef..f68d65a54 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -242,6 +242,8 @@ BatteryHealth getBatteryHealthStatus(int status) { value = BatteryHealth::DEAD; else if (status == BatteryMonitor::BH_FAILED) value = BatteryHealth::UNSPECIFIED_FAILURE; + else if (status == BatteryMonitor::BH_NOT_AVAILABLE) + value = BatteryHealth::NOT_AVAILABLE; else value = BatteryHealth::UNKNOWN; diff --git a/healthd/include/healthd/BatteryMonitor.h b/healthd/include/healthd/BatteryMonitor.h index 7b4f46c8a..a4c013b86 100644 --- a/healthd/include/healthd/BatteryMonitor.h +++ b/healthd/include/healthd/BatteryMonitor.h @@ -62,6 +62,7 @@ class BatteryMonitor { BH_MARGINAL, BH_NEEDS_REPLACEMENT, BH_FAILED, + BH_NOT_AVAILABLE, }; BatteryMonitor(); From 7003fba5f2864ad5138a42fb4bef36bea0cbc944 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Wed, 24 May 2023 20:01:10 +0000 Subject: [PATCH 33/45] Limit the number of log messages in a tombstone. Some testing environments can have a test that is sending many thousands of messages to the log. When this type of process crashes all of these log messages are captured and can cause OOM errors while creating the tombstone. Added a test to verify the log messages are truncated. Leaving this test disabled for now since it is inherently flaky due to having to assume that 500 messages are in the log. Added a test for a newline in a log message since it's somewhat related to this change. NOTE: The total number of messages is capped at 500, but if a message contains multiple newlines, the total messages will exceed 500. Counting messages this way seems to be in the spirit of the cap, that a process logging a large message with multiple newlines does not completely fill the tombstone log data. Bug: 269182937 Bug: 282661754 Test: All unit tests pass. Test: The disabled max_log_messages test passes. Change-Id: If18e62b29f899c2c4670101b402e37762bffbec6 Merged-In: If18e62b29f899c2c4670101b402e37762bffbec6 (cherry picked from commit 98d6242dc767d6f5fa15e4f63643391ae8e7c47f) --- debuggerd/debuggerd_test.cpp | 45 ++++++++++++++++++++++ debuggerd/libdebuggerd/tombstone_proto.cpp | 7 +++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 39d7fff1c..4cd619337 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -2759,3 +2759,48 @@ TEST_F(CrasherTest, logd_skips_reading_logs_not_main_thread) { ASSERT_BACKTRACE_FRAME(result, "raise_debugger_signal"); ASSERT_NOT_MATCH(result, kLogMessage); } + +// Disable this test since there is a high liklihood that this would +// be flaky since it requires 500 messages being in the log. +TEST_F(CrasherTest, DISABLED_max_log_messages) { + StartProcess([]() { + for (size_t i = 0; i < 600; i++) { + LOG(INFO) << "Message number " << i; + } + abort(); + }); + + unique_fd output_fd; + StartIntercept(&output_fd); + FinishCrasher(); + AssertDeath(SIGABRT); + int intercept_result; + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_NOT_MATCH(result, "Message number 99"); + ASSERT_MATCH(result, "Message number 100"); + ASSERT_MATCH(result, "Message number 599"); +} + +TEST_F(CrasherTest, log_with_newline) { + StartProcess([]() { + LOG(INFO) << "This line has a newline.\nThis is on the next line."; + abort(); + }); + + unique_fd output_fd; + StartIntercept(&output_fd); + FinishCrasher(); + AssertDeath(SIGABRT); + int intercept_result; + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_MATCH(result, ":\\s*This line has a newline."); + ASSERT_MATCH(result, ":\\s*This is on the next line."); +} diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp index acd814efa..7b2e0689e 100644 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto.cpp @@ -70,6 +70,9 @@ using android::base::StringPrintf; +// The maximum number of messages to save in the protobuf per file. +static constexpr size_t kMaxLogMessages = 500; + // Use the demangler from libc++. extern "C" char* __cxa_demangle(const char*, char*, size_t*, int* status); @@ -491,8 +494,8 @@ static void dump_mappings(Tombstone* tombstone, unwindstack::Maps* maps, } static void dump_log_file(Tombstone* tombstone, const char* logger, pid_t pid) { - logger_list* logger_list = - android_logger_list_open(android_name_to_log_id(logger), ANDROID_LOG_NONBLOCK, 0, pid); + logger_list* logger_list = android_logger_list_open(android_name_to_log_id(logger), + ANDROID_LOG_NONBLOCK, kMaxLogMessages, pid); LogBuffer buffer; From cf140fb761dc0437af9c2268d4869e2778c24900 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Wed, 24 May 2023 13:47:37 -0700 Subject: [PATCH 34/45] Check get_gwp_asan_callbacks before calling. When using the bootstrap linker, the get_gwp_asan_callbacks is not set. Therefore, check it is not nullptr before calling it during crash processing. Bug: 284098779 Test: Ran crasher64 using /system/bin/bootstrap/linker64 and verify Test: debuggerd code does not crash. Test: All unit tests pass. Change-Id: Ifc710fe4bef24661700444a1b69432bfc29d580f Merged-In: Ifc710fe4bef24661700444a1b69432bfc29d580f (cherry picked from commit 004a16739d4c746b3b5e082d986a30bb21476f40) --- debuggerd/handler/debuggerd_handler.cpp | 32 ++++++++++++++----------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index baa5bfb50..c6a535ad6 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -566,20 +566,23 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c process_info = g_callbacks.get_process_info(); } - // GWP-ASan catches use-after-free and heap-buffer-overflow by using PROT_NONE - // guard pages, which lead to SEGV. Normally, debuggerd prints a bug report - // and the process terminates, but in some cases, we actually want to print - // the bug report and let the signal handler return, and restart the process. - // In order to do that, we need to disable GWP-ASan's guard pages. The - // following callbacks handle this case. - gwp_asan_callbacks_t gwp_asan_callbacks = g_callbacks.get_gwp_asan_callbacks(); - if (signal_number == SIGSEGV && signal_has_si_addr(info) && - gwp_asan_callbacks.debuggerd_needs_gwp_asan_recovery && - gwp_asan_callbacks.debuggerd_gwp_asan_pre_crash_report && - gwp_asan_callbacks.debuggerd_gwp_asan_post_crash_report && - gwp_asan_callbacks.debuggerd_needs_gwp_asan_recovery(info->si_addr)) { - gwp_asan_callbacks.debuggerd_gwp_asan_pre_crash_report(info->si_addr); - process_info.recoverable_gwp_asan_crash = true; + gwp_asan_callbacks_t gwp_asan_callbacks = {}; + if (g_callbacks.get_gwp_asan_callbacks != nullptr) { + // GWP-ASan catches use-after-free and heap-buffer-overflow by using PROT_NONE + // guard pages, which lead to SEGV. Normally, debuggerd prints a bug report + // and the process terminates, but in some cases, we actually want to print + // the bug report and let the signal handler return, and restart the process. + // In order to do that, we need to disable GWP-ASan's guard pages. The + // following callbacks handle this case. + gwp_asan_callbacks = g_callbacks.get_gwp_asan_callbacks(); + if (signal_number == SIGSEGV && signal_has_si_addr(info) && + gwp_asan_callbacks.debuggerd_needs_gwp_asan_recovery && + gwp_asan_callbacks.debuggerd_gwp_asan_pre_crash_report && + gwp_asan_callbacks.debuggerd_gwp_asan_post_crash_report && + gwp_asan_callbacks.debuggerd_needs_gwp_asan_recovery(info->si_addr)) { + gwp_asan_callbacks.debuggerd_gwp_asan_pre_crash_report(info->si_addr); + process_info.recoverable_gwp_asan_crash = true; + } } // If sival_int is ~0, it means that the fallback handler has been called @@ -764,6 +767,7 @@ void debuggerd_init(debuggerd_callbacks_t* callbacks) { bool debuggerd_handle_signal(int signal_number, siginfo_t* info, void* context) { if (signal_number != SIGSEGV || !signal_has_si_addr(info)) return false; + if (g_callbacks.get_gwp_asan_callbacks == nullptr) return false; gwp_asan_callbacks_t gwp_asan_callbacks = g_callbacks.get_gwp_asan_callbacks(); if (gwp_asan_callbacks.debuggerd_needs_gwp_asan_recovery == nullptr || gwp_asan_callbacks.debuggerd_gwp_asan_pre_crash_report == nullptr || From e067e96da26111e446c5deadefd839a6451dd6d1 Mon Sep 17 00:00:00 2001 From: Will McVicker Date: Thu, 25 May 2023 16:43:31 -0700 Subject: [PATCH 35/45] toolbox/modprobe: Fix fallback path when mod_dirs is empty Due to GKI, the kernel UTS release string will not always (if ever) match the vendor's UTS release string that is used to create the initramfs file structure -- /lib/modules/. This causes module load failures when `-d DIR` is omitted. To fix this, we can include all of the versions under /lib/modules that match the kernel's major and minor version instead of directly using the value of uname(). In addition, we can also support modules being loaded directly from /lib/modules. Test: verify GKI kernel + initramfs with different UTS strings Test: verify GKI kernel + initramfs with modules directly in /lib/modules Fixes: 83207784251c ("toolbox/modprobe: Fallback to /lib/modules/ ") Bug: 282917063 Bug: 254835242 Merged-In: I5368f5cff139ba3165323a6a91066be38bfa0736 Change-Id: I5368f5cff139ba3165323a6a91066be38bfa0736 --- toolbox/modprobe.cpp | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/toolbox/modprobe.cpp b/toolbox/modprobe.cpp index 17f815697..17d4e319b 100644 --- a/toolbox/modprobe.cpp +++ b/toolbox/modprobe.cpp @@ -85,6 +85,26 @@ void MyLogger(android::base::LogId id, android::base::LogSeverity severity, cons } } +// Find directories in format of "/lib/modules/x.y.z-*". +static int KernelVersionNameFilter(const dirent* de) { + unsigned int major, minor; + static std::string kernel_version; + utsname uts; + + if (kernel_version.empty()) { + if ((uname(&uts) != 0) || (sscanf(uts.release, "%u.%u", &major, &minor) != 2)) { + LOG(ERROR) << "Could not parse the kernel version from uname"; + return 0; + } + kernel_version = android::base::StringPrintf("%u.%u", major, minor); + } + + if (android::base::StartsWith(de->d_name, kernel_version)) { + return 1; + } + return 0; +} + } // anonymous namespace extern "C" int modprobe_main(int argc, char** argv) { @@ -192,9 +212,22 @@ extern "C" int modprobe_main(int argc, char** argv) { } if (mod_dirs.empty()) { - utsname uts; - uname(&uts); - mod_dirs.emplace_back(android::base::StringPrintf("/lib/modules/%s", uts.release)); + static constexpr auto LIB_MODULES_PREFIX = "/lib/modules/"; + dirent** kernel_dirs = NULL; + + int n = scandir(LIB_MODULES_PREFIX, &kernel_dirs, KernelVersionNameFilter, NULL); + if (n == -1) { + PLOG(ERROR) << "Failed to scan dir " << LIB_MODULES_PREFIX; + return EXIT_FAILURE; + } else if (n > 0) { + while (n--) { + mod_dirs.emplace_back(LIB_MODULES_PREFIX + std::string(kernel_dirs[n]->d_name)); + } + } + free(kernel_dirs); + + // Allow modules to be directly inside /lib/modules + mod_dirs.emplace_back(LIB_MODULES_PREFIX); } LOG(DEBUG) << "mode is " << mode; @@ -212,11 +245,6 @@ extern "C" int modprobe_main(int argc, char** argv) { return EXIT_FAILURE; } } - if (mod_dirs.empty()) { - LOG(ERROR) << "No module configuration directories given."; - print_usage(); - return EXIT_FAILURE; - } if (parameter_count && modules.size() > 1) { LOG(ERROR) << "Only one module may be loaded when specifying module parameters."; print_usage(); From 4a7d98402c01c896a204e8b428f39e98dac598c6 Mon Sep 17 00:00:00 2001 From: Jack Wu Date: Fri, 19 May 2023 14:31:53 +0800 Subject: [PATCH 36/45] BatteryMonitor: support battery health INCONSISTENT from health status Report Battery health INCONSISTENT when there is a battery recalibration pending. Bug: 283182048 Test: m Ignore-AOSP-First: cherry-pick from aosp Change-Id: I8b944ddac7cc919fc95b1b71b015101642a62f96 Merged-In: I8b944ddac7cc919fc95b1b71b015101642a62f96 Signed-off-by: Jack Wu --- healthd/BatteryMonitor.cpp | 2 ++ healthd/include/healthd/BatteryMonitor.h | 1 + 2 files changed, 3 insertions(+) diff --git a/healthd/BatteryMonitor.cpp b/healthd/BatteryMonitor.cpp index f68d65a54..bd7955a7f 100644 --- a/healthd/BatteryMonitor.cpp +++ b/healthd/BatteryMonitor.cpp @@ -244,6 +244,8 @@ BatteryHealth getBatteryHealthStatus(int status) { value = BatteryHealth::UNSPECIFIED_FAILURE; else if (status == BatteryMonitor::BH_NOT_AVAILABLE) value = BatteryHealth::NOT_AVAILABLE; + else if (status == BatteryMonitor::BH_INCONSISTENT) + value = BatteryHealth::INCONSISTENT; else value = BatteryHealth::UNKNOWN; diff --git a/healthd/include/healthd/BatteryMonitor.h b/healthd/include/healthd/BatteryMonitor.h index a4c013b86..e9998ba7a 100644 --- a/healthd/include/healthd/BatteryMonitor.h +++ b/healthd/include/healthd/BatteryMonitor.h @@ -63,6 +63,7 @@ class BatteryMonitor { BH_NEEDS_REPLACEMENT, BH_FAILED, BH_NOT_AVAILABLE, + BH_INCONSISTENT, }; BatteryMonitor(); From 99b308c9f6ce20a8758e8b5295268fc69602d42a Mon Sep 17 00:00:00 2001 From: Vova Sharaienko Date: Tue, 30 May 2023 19:47:07 +0000 Subject: [PATCH 37/45] Increasing length of the datagram for Unix Domain Socket - address p99 StatsD socket loss issue Bug: 284508851 Test: atest statsd_test Test: atest statsd_benchmark Ignore-AOSP-First: mitigate data loss in Android U Change-Id: I4124ba8d4d78733eb666073f6d29dfe0c0552c0f Merged-In: I4124ba8d4d78733eb666073f6d29dfe0c0552c0f --- rootdir/init.rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rootdir/init.rc b/rootdir/init.rc index 41c60a7f9..1e6918d00 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -315,7 +315,7 @@ on init write /proc/sys/kernel/randomize_va_space 2 write /proc/sys/vm/mmap_min_addr 32768 write /proc/sys/net/ipv4/ping_group_range "0 2147483647" - write /proc/sys/net/unix/max_dgram_qlen 600 + write /proc/sys/net/unix/max_dgram_qlen 2400 # Assign reasonable ceiling values for socket rcv/snd buffers. # These should almost always be overridden by the target per the From e55e3dcd3f59008f1e3a770b5265be88b332f22c Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 6 Jun 2023 16:45:05 -0700 Subject: [PATCH 38/45] vts_fs_test: Relax filesystem constraints for fixed partitions. Adjust this test to align with VSR. First, skip the test if kernel is < 5.10 or < Android 13. Second, allow non-dynamic partitions to be vfat. Bug: 286038059 Test: vts_fs_test on CF (cherry picked from https://android-review.googlesource.com/q/commit:084c94e43eb2916c84c72de524c6e8c3d5ec2d4f) Merged-In: I5bd874f68296f7155d8c4366ebc13fe3d59c3ee6 Change-Id: I5bd874f68296f7155d8c4366ebc13fe3d59c3ee6 --- fs_mgr/tests/vts_fs_test.cpp | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/fs_mgr/tests/vts_fs_test.cpp b/fs_mgr/tests/vts_fs_test.cpp index bb2ceb9b5..4d771fa04 100644 --- a/fs_mgr/tests/vts_fs_test.cpp +++ b/fs_mgr/tests/vts_fs_test.cpp @@ -55,6 +55,21 @@ TEST(fs, ErofsSupported) { } TEST(fs, PartitionTypes) { + // Requirements only apply to Android 13+, 5.10+ devices. + int vsr_level = GetVsrLevel(); + if (vsr_level < __ANDROID_API_T__) { + GTEST_SKIP(); + } + + struct utsname uts; + ASSERT_EQ(uname(&uts), 0); + + unsigned int major, minor; + ASSERT_EQ(sscanf(uts.release, "%u.%u", &major, &minor), 2); + if (major < 5 || (major == 5 && minor < 10)) { + GTEST_SKIP(); + } + android::fs_mgr::Fstab fstab; ASSERT_TRUE(android::fs_mgr::ReadFstabFromFile("/proc/mounts", &fstab)); @@ -64,12 +79,7 @@ TEST(fs, PartitionTypes) { ASSERT_TRUE(android::base::Readlink("/dev/block/by-name/super", &super_bdev)); ASSERT_TRUE(android::base::Readlink("/dev/block/by-name/userdata", &userdata_bdev)); - int vsr_level = GetVsrLevel(); - - std::vector must_be_f2fs; - if (vsr_level >= __ANDROID_API_T__) { - must_be_f2fs.emplace_back("/data"); - } + std::vector must_be_f2fs = {"/data"}; if (vsr_level >= __ANDROID_API_U__) { must_be_f2fs.emplace_back("/metadata"); } @@ -98,17 +108,13 @@ TEST(fs, PartitionTypes) { continue; } - if (vsr_level < __ANDROID_API_T__) { - continue; - } - if (vsr_level == __ANDROID_API_T__ && parent_bdev != super_bdev) { - // Only check for dynamic partitions at this VSR level. - continue; - } - if (entry.flags & MS_RDONLY) { - std::vector allowed = {"erofs", "ext4", "f2fs"}; + if (parent_bdev != super_bdev) { + // Ignore non-AOSP partitions (eg anything outside of super). + continue; + } + std::vector allowed = {"erofs", "ext4", "f2fs"}; EXPECT_NE(std::find(allowed.begin(), allowed.end(), entry.fs_type), allowed.end()) << entry.mount_point; } else { From 4866d284f81dec3068c8f4f015d82c0db46c2b85 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 16 May 2023 15:53:57 -0700 Subject: [PATCH 39/45] ueventd: Fix a race condition in handling device-mapper events. We've had flake in libdm_test for a long time, with no clear cause. Lately however it has become particularly reproducible when running the UeventAfterLoadTable test in isolation, and thus we've identified the root cause. uevents for device-mapper are fired when the sysfs node is added, but at that time, the "dm" subnode has not yet been added. The root node and dm node are added very close together, so usually it works, but sometimes ueventd is too fast. Instead of relying on sysfs, query the uuid/name node directly from device-mapper. Bug: 286011429 Test: libdm_test (cherry picked from https://android-review.googlesource.com/q/commit:59abbfe64706a7ea0c4e932ae40bc8bde9746dce) Merged-In: I258de5de05d813c3cb7f129e82e56dbfe8bf3117 Change-Id: I85e240807e0ce5ade211ec65453ab06d4066992a --- fs_mgr/libdm/dm.cpp | 19 +++++++ fs_mgr/libdm/dm_test.cpp | 94 +++++++++++++++++++++++---------- fs_mgr/libdm/include/libdm/dm.h | 2 + init/devices.cpp | 18 +++---- 4 files changed, 94 insertions(+), 39 deletions(-) diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index deffae1ad..1e8c14fe2 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -243,6 +243,25 @@ bool DeviceMapper::GetDeviceUniquePath(const std::string& name, std::string* pat return true; } +bool DeviceMapper::GetDeviceNameAndUuid(dev_t dev, std::string* name, std::string* uuid) { + struct dm_ioctl io; + InitIo(&io, {}); + io.dev = dev; + + if (ioctl(fd_, DM_DEV_STATUS, &io) < 0) { + PLOG(ERROR) << "Failed to find device dev: " << major(dev) << ":" << minor(dev); + return false; + } + + if (name) { + *name = io.name; + } + if (uuid) { + *uuid = io.uuid; + } + return true; +} + std::optional DeviceMapper::GetDetailedInfo(const std::string& name) const { struct dm_ioctl io; InitIo(&io, name); diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index 788cf5174..c522eafae 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -42,16 +43,40 @@ using namespace std; using namespace std::chrono_literals; using namespace android::dm; -using unique_fd = android::base::unique_fd; +using android::base::make_scope_guard; +using android::base::unique_fd; -TEST(libdm, HasMinimumTargets) { +class DmTest : public ::testing::Test { + protected: + void SetUp() override { + const testing::TestInfo* const test_info = + testing::UnitTest::GetInstance()->current_test_info(); + test_name_ = test_info->name(); + test_full_name_ = test_info->test_suite_name() + "/"s + test_name_; + + LOG(INFO) << "Starting test: " << test_full_name_; + } + void TearDown() override { + LOG(INFO) << "Tearing down test: " << test_full_name_; + + auto& dm = DeviceMapper::Instance(); + ASSERT_TRUE(dm.DeleteDeviceIfExists(test_name_)); + + LOG(INFO) << "Teardown complete for test: " << test_full_name_; + } + + std::string test_name_; + std::string test_full_name_; +}; + +TEST_F(DmTest, HasMinimumTargets) { DmTargetTypeInfo info; DeviceMapper& dm = DeviceMapper::Instance(); ASSERT_TRUE(dm.GetTargetByName("linear", &info)); } -TEST(libdm, DmLinear) { +TEST_F(DmTest, DmLinear) { unique_fd tmp1(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp1, 0); unique_fd tmp2(CreateTempFile("file_2", 4096)); @@ -127,7 +152,7 @@ TEST(libdm, DmLinear) { ASSERT_TRUE(dev.Destroy()); } -TEST(libdm, DmSuspendResume) { +TEST_F(DmTest, DmSuspendResume) { unique_fd tmp1(CreateTempFile("file_suspend_resume", 512)); ASSERT_GE(tmp1, 0); @@ -156,7 +181,7 @@ TEST(libdm, DmSuspendResume) { ASSERT_EQ(dm.GetState(dev.name()), DmDeviceState::ACTIVE); } -TEST(libdm, DmVerityArgsAvb2) { +TEST_F(DmTest, DmVerityArgsAvb2) { std::string device = "/dev/block/platform/soc/1da4000.ufshc/by-name/vendor_a"; std::string algorithm = "sha1"; std::string digest = "4be7e823b8c40f7bd5c8ccd5123f0722c5baca21"; @@ -178,7 +203,7 @@ TEST(libdm, DmVerityArgsAvb2) { EXPECT_EQ(target.GetParameterString(), expected); } -TEST(libdm, DmSnapshotArgs) { +TEST_F(DmTest, DmSnapshotArgs) { DmTargetSnapshot target1(0, 512, "base", "cow", SnapshotStorageMode::Persistent, 8); if (DmTargetSnapshot::ReportsOverflow("snapshot")) { EXPECT_EQ(target1.GetParameterString(), "base cow PO 8"); @@ -200,7 +225,7 @@ TEST(libdm, DmSnapshotArgs) { EXPECT_EQ(target3.name(), "snapshot-merge"); } -TEST(libdm, DmSnapshotOriginArgs) { +TEST_F(DmTest, DmSnapshotOriginArgs) { DmTargetSnapshotOrigin target(0, 512, "base"); EXPECT_EQ(target.GetParameterString(), "base"); EXPECT_EQ(target.name(), "snapshot-origin"); @@ -330,7 +355,7 @@ bool CheckSnapshotAvailability() { return true; } -TEST(libdm, DmSnapshot) { +TEST_F(DmTest, DmSnapshot) { if (!CheckSnapshotAvailability()) { return; } @@ -374,7 +399,7 @@ TEST(libdm, DmSnapshot) { ASSERT_EQ(read, data); } -TEST(libdm, DmSnapshotOverflow) { +TEST_F(DmTest, DmSnapshotOverflow) { if (!CheckSnapshotAvailability()) { return; } @@ -421,7 +446,7 @@ TEST(libdm, DmSnapshotOverflow) { } } -TEST(libdm, ParseStatusText) { +TEST_F(DmTest, ParseStatusText) { DmTargetSnapshot::Status status; // Bad inputs @@ -448,7 +473,7 @@ TEST(libdm, ParseStatusText) { EXPECT_TRUE(DmTargetSnapshot::ParseStatusText("Overflow", &status)); } -TEST(libdm, DmSnapshotMergePercent) { +TEST_F(DmTest, DmSnapshotMergePercent) { DmTargetSnapshot::Status status; // Correct input @@ -502,7 +527,7 @@ TEST(libdm, DmSnapshotMergePercent) { EXPECT_LE(DmTargetSnapshot::MergePercent(status, 0), 0.0); } -TEST(libdm, CryptArgs) { +TEST_F(DmTest, CryptArgs) { DmTargetCrypt target1(0, 512, "sha1", "abcdefgh", 50, "/dev/loop0", 100); ASSERT_EQ(target1.name(), "crypt"); ASSERT_TRUE(target1.Valid()); @@ -518,7 +543,7 @@ TEST(libdm, CryptArgs) { "iv_large_sectors sector_size:64"); } -TEST(libdm, DefaultKeyArgs) { +TEST_F(DmTest, DefaultKeyArgs) { DmTargetDefaultKey target(0, 4096, "aes-xts-plain64", "abcdef0123456789", "/dev/loop0", 0); target.SetSetDun(); ASSERT_EQ(target.name(), "default-key"); @@ -529,7 +554,7 @@ TEST(libdm, DefaultKeyArgs) { "iv_large_sectors"); } -TEST(libdm, DefaultKeyLegacyArgs) { +TEST_F(DmTest, DefaultKeyLegacyArgs) { DmTargetDefaultKey target(0, 4096, "AES-256-XTS", "abcdef0123456789", "/dev/loop0", 0); target.SetUseLegacyOptionsFormat(); ASSERT_EQ(target.name(), "default-key"); @@ -537,7 +562,7 @@ TEST(libdm, DefaultKeyLegacyArgs) { ASSERT_EQ(target.GetParameterString(), "AES-256-XTS abcdef0123456789 /dev/loop0 0"); } -TEST(libdm, DeleteDeviceWithTimeout) { +TEST_F(DmTest, DeleteDeviceWithTimeout) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -561,7 +586,7 @@ TEST(libdm, DeleteDeviceWithTimeout) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, IsDmBlockDevice) { +TEST_F(DmTest, IsDmBlockDevice) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -580,7 +605,7 @@ TEST(libdm, IsDmBlockDevice) { ASSERT_FALSE(dm.IsDmBlockDevice(loop.device())); } -TEST(libdm, GetDmDeviceNameByPath) { +TEST_F(DmTest, GetDmDeviceNameByPath) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -601,7 +626,7 @@ TEST(libdm, GetDmDeviceNameByPath) { ASSERT_EQ("libdm-test-dm-linear", *name); } -TEST(libdm, GetParentBlockDeviceByPath) { +TEST_F(DmTest, GetParentBlockDeviceByPath) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -621,7 +646,7 @@ TEST(libdm, GetParentBlockDeviceByPath) { ASSERT_EQ(loop.device(), *sub_block_device); } -TEST(libdm, DeleteDeviceDeferredNoReferences) { +TEST_F(DmTest, DeleteDeviceDeferredNoReferences) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -647,7 +672,7 @@ TEST(libdm, DeleteDeviceDeferredNoReferences) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { +TEST_F(DmTest, DeleteDeviceDeferredWaitsForLastReference) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -682,7 +707,7 @@ TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, CreateEmptyDevice) { +TEST_F(DmTest, CreateEmptyDevice) { DeviceMapper& dm = DeviceMapper::Instance(); ASSERT_TRUE(dm.CreateEmptyDevice("empty-device")); auto guard = @@ -692,9 +717,7 @@ TEST(libdm, CreateEmptyDevice) { ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device")); } -TEST(libdm, UeventAfterLoadTable) { - static const char* kDeviceName = "libdm-test-uevent-load-table"; - +TEST_F(DmTest, UeventAfterLoadTable) { struct utsname u; ASSERT_EQ(uname(&u), 0); @@ -706,18 +729,31 @@ TEST(libdm, UeventAfterLoadTable) { } DeviceMapper& dm = DeviceMapper::Instance(); - ASSERT_TRUE(dm.CreateEmptyDevice(kDeviceName)); + ASSERT_TRUE(dm.CreateEmptyDevice(test_name_)); DmTable table; table.Emplace(0, 1); - ASSERT_TRUE(dm.LoadTable(kDeviceName, table)); + ASSERT_TRUE(dm.LoadTable(test_name_, table)); std::string ignore_path; - ASSERT_TRUE(dm.WaitForDevice(kDeviceName, 5s, &ignore_path)); + ASSERT_TRUE(dm.WaitForDevice(test_name_, 5s, &ignore_path)); - auto info = dm.GetDetailedInfo(kDeviceName); + auto info = dm.GetDetailedInfo(test_name_); ASSERT_TRUE(info.has_value()); ASSERT_TRUE(info->IsSuspended()); - ASSERT_TRUE(dm.DeleteDevice(kDeviceName)); + ASSERT_TRUE(dm.DeleteDevice(test_name_)); +} + +TEST_F(DmTest, GetNameAndUuid) { + auto& dm = DeviceMapper::Instance(); + ASSERT_TRUE(dm.CreatePlaceholderDevice(test_name_)); + + dev_t dev; + ASSERT_TRUE(dm.GetDeviceNumber(test_name_, &dev)); + + std::string name, uuid; + ASSERT_TRUE(dm.GetDeviceNameAndUuid(dev, &name, &uuid)); + ASSERT_EQ(name, test_name_); + ASSERT_FALSE(uuid.empty()); } diff --git a/fs_mgr/libdm/include/libdm/dm.h b/fs_mgr/libdm/include/libdm/dm.h index dbef8f902..3e7ecc645 100644 --- a/fs_mgr/libdm/include/libdm/dm.h +++ b/fs_mgr/libdm/include/libdm/dm.h @@ -298,6 +298,8 @@ class DeviceMapper final : public IDeviceMapper { // a placeholder table containing dm-error. bool CreatePlaceholderDevice(const std::string& name); + bool GetDeviceNameAndUuid(dev_t dev, std::string* name, std::string* uuid); + private: // Maximum possible device mapper targets registered in the kernel. // This is only used to read the list of targets from kernel so we allocate diff --git a/init/devices.cpp b/init/devices.cpp index 39442a0e8..d29ffd604 100644 --- a/init/devices.cpp +++ b/init/devices.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -112,17 +113,14 @@ static bool FindVbdDevicePrefix(const std::string& path, std::string* result) { // the supplied buffer with the dm module's instantiated name. // If it doesn't start with a virtual block device, or there is some // error, return false. -static bool FindDmDevice(const std::string& path, std::string* name, std::string* uuid) { - if (!StartsWith(path, "/devices/virtual/block/dm-")) return false; +static bool FindDmDevice(const Uevent& uevent, std::string* name, std::string* uuid) { + if (!StartsWith(uevent.path, "/devices/virtual/block/dm-")) return false; + if (uevent.action == "remove") return false; // Avoid error spam from ioctl - if (!ReadFileToString("/sys" + path + "/dm/name", name)) { - return false; - } - ReadFileToString("/sys" + path + "/dm/uuid", uuid); + dev_t dev = makedev(uevent.major, uevent.minor); - *name = android::base::Trim(*name); - *uuid = android::base::Trim(*uuid); - return true; + auto& dm = android::dm::DeviceMapper::Instance(); + return dm.GetDeviceNameAndUuid(dev, name, uuid); } Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid, @@ -392,7 +390,7 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev type = "pci"; } else if (FindVbdDevicePrefix(uevent.path, &device)) { type = "vbd"; - } else if (FindDmDevice(uevent.path, &partition, &uuid)) { + } else if (FindDmDevice(uevent, &partition, &uuid)) { std::vector symlinks = {"/dev/block/mapper/" + partition}; if (!uuid.empty()) { symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid); From ad6ca1451d034cb381d99f9b170a1024bb1d5014 Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Tue, 11 Jul 2023 09:05:11 -0700 Subject: [PATCH 40/45] Revert "Fix deadlock caused by two-threaded property controls" This reverts commit 606afc7b7451aba90e3634076d9b59a5ef08186b. These fixes for b/262208935 introduced a race condition. We believe the race is fixed by ag/23879563, but at this point in the release feel that reverting the fixes and refixing in main is the better solution Test: Builds, boots Bug: 283202477 Bug: 288991737 Ignore-AOSP-First: Reverting CL only in internal (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3196be61fcf0e0303cf819630876b9c812b9474e) Merged-In: I9ae6863b0ea5e064c59d9d34c03d33fa1da12fdc Change-Id: I9ae6863b0ea5e064c59d9d34c03d33fa1da12fdc --- init/property_service.cpp | 50 ++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 4242912ed..2d084db46 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -117,6 +117,7 @@ static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; static std::thread property_service_for_system_thread; +static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -394,37 +395,32 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - if (name == "sys.powerctl") { - // No action here - NotifyPropertyChange will trigger the appropriate action, and since this - // can come to the second thread, we mustn't call out to the __system_property_* functions - // which support multiple readers but only one mutator. + auto lock = std::lock_guard{set_property_lock}; + prop_info* pi = (prop_info*)__system_property_find(name.c_str()); + if (pi != nullptr) { + // ro.* properties are actually "write-once". + if (StartsWith(name, "ro.")) { + *error = "Read-only property was already set"; + return {PROP_ERROR_READ_ONLY_PROPERTY}; + } + + __system_property_update(pi, value.c_str(), valuelen); } else { - prop_info* pi = (prop_info*)__system_property_find(name.c_str()); - if (pi != nullptr) { - // ro.* properties are actually "write-once". - if (StartsWith(name, "ro.")) { - *error = "Read-only property was already set"; - return {PROP_ERROR_READ_ONLY_PROPERTY}; - } - - __system_property_update(pi, value.c_str(), valuelen); - } else { - int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); - if (rc < 0) { - *error = "__system_property_add failed"; - return {PROP_ERROR_SET_FAILED}; - } + int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); + if (rc < 0) { + *error = "__system_property_add failed"; + return {PROP_ERROR_SET_FAILED}; } + } - // Don't write properties to disk until after we have read all default - // properties to prevent them from being overwritten by default values. - if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { - if (persist_write_thread) { - persist_write_thread->Write(name, value, std::move(*socket)); - return {}; - } - WritePersistentProperty(name, value); + // Don't write properties to disk until after we have read all default + // properties to prevent them from being overwritten by default values. + if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { + if (persist_write_thread) { + persist_write_thread->Write(name, value, std::move(*socket)); + return {}; } + WritePersistentProperty(name, value); } NotifyPropertyChange(name, value); From de5f915991bb445ddadd4a3889874ee97dc7913d Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Tue, 11 Jul 2023 09:05:27 -0700 Subject: [PATCH 41/45] Revert "Listen on property_service_for_system socket" This reverts commit 90879edeea0b2fd1e766428693d736163f39c511. These fixes for b/262208935 introduced a race condition. We believe the race is fixed by ag/23879563, but at this point in the release feel that reverting the fixes and refixing in main is the better solution Test: Builds, boots Bug: 283202477 Bug: 288991737 Ignore-AOSP-First: Reverting CL only in internal (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:be392fc69318d376f18eed7c8ef5ded0d1dc8648) Merged-In: I28491e90847f6aa0c9767b27e1d99190637048b9 Change-Id: I28491e90847f6aa0c9767b27e1d99190637048b9 --- init/property_service.cpp | 53 ++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 2d084db46..8da69822c 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -57,12 +57,12 @@ #include #include #include -#include #include #include #include #include #include + #include "debug_ramdisk.h" #include "epoll.h" #include "init.h" @@ -111,13 +111,12 @@ constexpr auto API_LEVEL_CURRENT = 10000; static bool persistent_properties_loaded = false; +static int property_set_fd = -1; static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; -static std::thread property_service_for_system_thread; -static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -395,7 +394,6 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - auto lock = std::lock_guard{set_property_lock}; prop_info* pi = (prop_info*)__system_property_find(name.c_str()); if (pi != nullptr) { // ro.* properties are actually "write-once". @@ -581,10 +579,10 @@ uint32_t HandlePropertySetNoSocket(const std::string& name, const std::string& v return *ret; } -static void handle_property_set_fd(int fd) { +static void handle_property_set_fd() { static constexpr uint32_t kDefaultSocketTimeout = 2000; /* ms */ - int s = accept4(fd, nullptr, nullptr, SOCK_CLOEXEC); + int s = accept4(property_set_fd, nullptr, nullptr, SOCK_CLOEXEC); if (s == -1) { return; } @@ -1421,21 +1419,19 @@ static void HandleInitSocket() { } } -static void PropertyServiceThread(int fd, bool listen_init) { +static void PropertyServiceThread() { Epoll epoll; if (auto result = epoll.Open(); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(fd, std::bind(handle_property_set_fd, fd)); + if (auto result = epoll.RegisterHandler(property_set_fd, handle_property_set_fd); !result.ok()) { LOG(FATAL) << result.error(); } - if (listen_init) { - if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { - LOG(FATAL) << result.error(); - } + if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { + LOG(FATAL) << result.error(); } while (true) { @@ -1486,23 +1482,6 @@ void PersistWriteThread::Write(std::string name, std::string value, SocketConnec cv_.notify_all(); } -void StartThread(const char* name, int mode, int gid, std::thread& t, bool listen_init) { - int fd = -1; - if (auto result = CreateSocket(name, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, - /*passcred=*/false, /*should_listen=*/false, mode, /*uid=*/0, - /*gid=*/gid, /*socketcon=*/{}); - result.ok()) { - fd = *result; - } else { - LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); - } - - listen(fd, 8); - - auto new_thread = std::thread(PropertyServiceThread, fd, listen_init); - t.swap(new_thread); -} - void StartPropertyService(int* epoll_socket) { InitPropertySet("ro.property_service.version", "2"); @@ -1514,9 +1493,19 @@ void StartPropertyService(int* epoll_socket) { init_socket = sockets[1]; StartSendingMessages(); - StartThread(PROP_SERVICE_FOR_SYSTEM_NAME, 0660, AID_SYSTEM, property_service_for_system_thread, - true); - StartThread(PROP_SERVICE_NAME, 0666, 0, property_service_thread, false); + if (auto result = CreateSocket(PROP_SERVICE_NAME, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, + /*passcred=*/false, /*should_listen=*/false, 0666, /*uid=*/0, + /*gid=*/0, /*socketcon=*/{}); + result.ok()) { + property_set_fd = *result; + } else { + LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); + } + + listen(property_set_fd, 8); + + auto new_thread = std::thread{PropertyServiceThread}; + property_service_thread.swap(new_thread); auto async_persist_writes = android::base::GetBoolProperty("ro.property_service.async_persist_writes", false); From 3adb2a63bed9e97a5a179316b4f14a3196df81e3 Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Tue, 11 Jul 2023 09:05:11 -0700 Subject: [PATCH 42/45] Revert "Fix deadlock caused by two-threaded property controls" This reverts commit 606afc7b7451aba90e3634076d9b59a5ef08186b. These fixes for b/262208935 introduced a race condition. We believe the race is fixed by ag/23879563, but at this point in the release feel that reverting the fixes and refixing in main is the better solution Test: Builds, boots Bug: 283202477 Bug: 288991737 Ignore-AOSP-First: Reverting CL only in internal (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3196be61fcf0e0303cf819630876b9c812b9474e) Merged-In: I9ae6863b0ea5e064c59d9d34c03d33fa1da12fdc Change-Id: I9ae6863b0ea5e064c59d9d34c03d33fa1da12fdc --- init/property_service.cpp | 50 ++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 4242912ed..2d084db46 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -117,6 +117,7 @@ static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; static std::thread property_service_for_system_thread; +static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -394,37 +395,32 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - if (name == "sys.powerctl") { - // No action here - NotifyPropertyChange will trigger the appropriate action, and since this - // can come to the second thread, we mustn't call out to the __system_property_* functions - // which support multiple readers but only one mutator. + auto lock = std::lock_guard{set_property_lock}; + prop_info* pi = (prop_info*)__system_property_find(name.c_str()); + if (pi != nullptr) { + // ro.* properties are actually "write-once". + if (StartsWith(name, "ro.")) { + *error = "Read-only property was already set"; + return {PROP_ERROR_READ_ONLY_PROPERTY}; + } + + __system_property_update(pi, value.c_str(), valuelen); } else { - prop_info* pi = (prop_info*)__system_property_find(name.c_str()); - if (pi != nullptr) { - // ro.* properties are actually "write-once". - if (StartsWith(name, "ro.")) { - *error = "Read-only property was already set"; - return {PROP_ERROR_READ_ONLY_PROPERTY}; - } - - __system_property_update(pi, value.c_str(), valuelen); - } else { - int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); - if (rc < 0) { - *error = "__system_property_add failed"; - return {PROP_ERROR_SET_FAILED}; - } + int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen); + if (rc < 0) { + *error = "__system_property_add failed"; + return {PROP_ERROR_SET_FAILED}; } + } - // Don't write properties to disk until after we have read all default - // properties to prevent them from being overwritten by default values. - if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { - if (persist_write_thread) { - persist_write_thread->Write(name, value, std::move(*socket)); - return {}; - } - WritePersistentProperty(name, value); + // Don't write properties to disk until after we have read all default + // properties to prevent them from being overwritten by default values. + if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) { + if (persist_write_thread) { + persist_write_thread->Write(name, value, std::move(*socket)); + return {}; } + WritePersistentProperty(name, value); } NotifyPropertyChange(name, value); From 8543f7dcd4fa94c7aeba938ad78679915ea8b1cd Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Tue, 11 Jul 2023 09:05:27 -0700 Subject: [PATCH 43/45] Revert "Listen on property_service_for_system socket" This reverts commit 90879edeea0b2fd1e766428693d736163f39c511. These fixes for b/262208935 introduced a race condition. We believe the race is fixed by ag/23879563, but at this point in the release feel that reverting the fixes and refixing in main is the better solution Test: Builds, boots Bug: 283202477 Bug: 288991737 Ignore-AOSP-First: Reverting CL only in internal (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:be392fc69318d376f18eed7c8ef5ded0d1dc8648) Merged-In: I28491e90847f6aa0c9767b27e1d99190637048b9 Change-Id: I28491e90847f6aa0c9767b27e1d99190637048b9 --- init/property_service.cpp | 53 ++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index 2d084db46..8da69822c 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -57,12 +57,12 @@ #include #include #include -#include #include #include #include #include #include + #include "debug_ramdisk.h" #include "epoll.h" #include "init.h" @@ -111,13 +111,12 @@ constexpr auto API_LEVEL_CURRENT = 10000; static bool persistent_properties_loaded = false; +static int property_set_fd = -1; static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; static std::mutex accept_messages_lock; static std::thread property_service_thread; -static std::thread property_service_for_system_thread; -static std::mutex set_property_lock; static std::unique_ptr persist_write_thread; @@ -395,7 +394,6 @@ static std::optional PropertySet(const std::string& name, const std::s return {PROP_ERROR_INVALID_VALUE}; } - auto lock = std::lock_guard{set_property_lock}; prop_info* pi = (prop_info*)__system_property_find(name.c_str()); if (pi != nullptr) { // ro.* properties are actually "write-once". @@ -581,10 +579,10 @@ uint32_t HandlePropertySetNoSocket(const std::string& name, const std::string& v return *ret; } -static void handle_property_set_fd(int fd) { +static void handle_property_set_fd() { static constexpr uint32_t kDefaultSocketTimeout = 2000; /* ms */ - int s = accept4(fd, nullptr, nullptr, SOCK_CLOEXEC); + int s = accept4(property_set_fd, nullptr, nullptr, SOCK_CLOEXEC); if (s == -1) { return; } @@ -1421,21 +1419,19 @@ static void HandleInitSocket() { } } -static void PropertyServiceThread(int fd, bool listen_init) { +static void PropertyServiceThread() { Epoll epoll; if (auto result = epoll.Open(); !result.ok()) { LOG(FATAL) << result.error(); } - if (auto result = epoll.RegisterHandler(fd, std::bind(handle_property_set_fd, fd)); + if (auto result = epoll.RegisterHandler(property_set_fd, handle_property_set_fd); !result.ok()) { LOG(FATAL) << result.error(); } - if (listen_init) { - if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { - LOG(FATAL) << result.error(); - } + if (auto result = epoll.RegisterHandler(init_socket, HandleInitSocket); !result.ok()) { + LOG(FATAL) << result.error(); } while (true) { @@ -1486,23 +1482,6 @@ void PersistWriteThread::Write(std::string name, std::string value, SocketConnec cv_.notify_all(); } -void StartThread(const char* name, int mode, int gid, std::thread& t, bool listen_init) { - int fd = -1; - if (auto result = CreateSocket(name, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, - /*passcred=*/false, /*should_listen=*/false, mode, /*uid=*/0, - /*gid=*/gid, /*socketcon=*/{}); - result.ok()) { - fd = *result; - } else { - LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); - } - - listen(fd, 8); - - auto new_thread = std::thread(PropertyServiceThread, fd, listen_init); - t.swap(new_thread); -} - void StartPropertyService(int* epoll_socket) { InitPropertySet("ro.property_service.version", "2"); @@ -1514,9 +1493,19 @@ void StartPropertyService(int* epoll_socket) { init_socket = sockets[1]; StartSendingMessages(); - StartThread(PROP_SERVICE_FOR_SYSTEM_NAME, 0660, AID_SYSTEM, property_service_for_system_thread, - true); - StartThread(PROP_SERVICE_NAME, 0666, 0, property_service_thread, false); + if (auto result = CreateSocket(PROP_SERVICE_NAME, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, + /*passcred=*/false, /*should_listen=*/false, 0666, /*uid=*/0, + /*gid=*/0, /*socketcon=*/{}); + result.ok()) { + property_set_fd = *result; + } else { + LOG(FATAL) << "start_property_service socket creation failed: " << result.error(); + } + + listen(property_set_fd, 8); + + auto new_thread = std::thread{PropertyServiceThread}; + property_service_thread.swap(new_thread); auto async_persist_writes = android::base::GetBoolProperty("ro.property_service.async_persist_writes", false); From e10d8405a2ec05d5e81dcaa5cdd645a072100989 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 25 Jul 2023 14:50:18 -0700 Subject: [PATCH 44/45] libprocessgroup: fix reset of file_v2_name ProfileAttribute::Reset does not reset file_v2_name, fix that. Also provide ProfileAttribute::file_name() to consolidate the code. Bug: 292636609 Signed-off-by: Suren Baghdasaryan (cherry picked from commit 2ffbeaef3a926b0b5fd873136588deb3ec61ef96) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:72254810d9d63f8267d32d251456a78cd4891698) Merged-In: I5b33ca47b4fa5cabf582c8804bd13f72f6e58411 Change-Id: I5b33ca47b4fa5cabf582c8804bd13f72f6e58411 --- libprocessgroup/task_profiles.cpp | 22 +++++++++++++--------- libprocessgroup/task_profiles.h | 8 +++++--- libprocessgroup/task_profiles_test.cpp | 7 +++---- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 44dba2a16..59b135013 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -114,9 +114,16 @@ bool FdCacheHelper::IsAppDependentPath(const std::string& path) { IProfileAttribute::~IProfileAttribute() = default; -void ProfileAttribute::Reset(const CgroupController& controller, const std::string& file_name) { +const std::string& ProfileAttribute::file_name() const { + if (controller()->version() == 2 && !file_v2_name_.empty()) return file_v2_name_; + return file_name_; +} + +void ProfileAttribute::Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) { controller_ = controller; file_name_ = file_name; + file_v2_name_ = file_v2_name; } bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { @@ -129,12 +136,11 @@ bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { return true; } - const std::string& file_name = - controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; if (subgroup.empty()) { - *path = StringPrintf("%s/%s", controller()->path(), file_name.c_str()); + *path = StringPrintf("%s/%s", controller()->path(), file_name().c_str()); } else { - *path = StringPrintf("%s/%s/%s", controller()->path(), subgroup.c_str(), file_name.c_str()); + *path = StringPrintf("%s/%s/%s", controller()->path(), subgroup.c_str(), + file_name().c_str()); } return true; } @@ -144,9 +150,7 @@ bool ProfileAttribute::GetPathForUID(uid_t uid, std::string* path) const { return true; } - const std::string& file_name = - controller()->version() == 2 && !file_v2_name_.empty() ? file_v2_name_ : file_name_; - *path = StringPrintf("%s/uid_%d/%s", controller()->path(), uid, file_name.c_str()); + *path = StringPrintf("%s/uid_%u/%s", controller()->path(), uid, file_name().c_str()); return true; } @@ -816,7 +820,7 @@ bool TaskProfiles::Load(const CgroupMap& cg_map, const std::string& file_name) { attributes_[name] = std::make_unique(controller, file_attr, file_v2_attr); } else { - iter->second->Reset(controller, file_attr); + iter->second->Reset(controller, file_attr, file_v2_attr); } } else { LOG(WARNING) << "Controller " << controller_name << " is not found"; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index a62c5b0a9..ac8918e65 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -32,7 +32,8 @@ class IProfileAttribute { public: virtual ~IProfileAttribute() = 0; - virtual void Reset(const CgroupController& controller, const std::string& file_name) = 0; + virtual void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) = 0; virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; @@ -50,8 +51,9 @@ class ProfileAttribute : public IProfileAttribute { ~ProfileAttribute() = default; const CgroupController* controller() const override { return &controller_; } - const std::string& file_name() const override { return file_name_; } - void Reset(const CgroupController& controller, const std::string& file_name) override; + const std::string& file_name() const override; + void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) override; bool GetPathForTask(int tid, std::string* path) const override; bool GetPathForUID(uid_t uid, std::string* path) const override; diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index eadbe7697..da74bb012 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -102,7 +102,8 @@ class ProfileAttributeMock : public IProfileAttribute { public: ProfileAttributeMock(const std::string& file_name) : file_name_(file_name) {} ~ProfileAttributeMock() override = default; - void Reset(const CgroupController& controller, const std::string& file_name) override { + void Reset(const CgroupController& controller, const std::string& file_name, + const std::string& file_v2_name) override { CHECK(false); } const CgroupController* controller() const override { @@ -125,9 +126,7 @@ class ProfileAttributeMock : public IProfileAttribute { return true; }; - bool GetPathForUID(uid_t, std::string*) const override { - return false; - } + bool GetPathForUID(uid_t, std::string*) const override { return false; } private: const std::string file_name_; From cfd2e50e2db63e98e18418b1ec63eb9272a7425a Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Tue, 25 Jul 2023 15:45:45 -0700 Subject: [PATCH 45/45] libprocessgroup: optimize SetAttributeAction::ExecuteForProcess performance Current implementation of SetAttributeAction::ExecuteForProcess reuses SetAttributeAction::ExecuteForTask while not utilizing available uid/pid information. This results in a call to GetPathForTask() which is an expensive function due to it reading and parsing /proc/$pid/cgroups. This can be avoided if we utilize available uid/pid info and the fact that cgroup v2 attributes share the cgroup v2 hierarchy as process groups, which use a known path template. Bug: 292636609 Signed-off-by: Suren Baghdasaryan (cherry picked from commit 961c01ce23bb886583ca8cac1640806346c09a7f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:1e81ee13638c5ca03157db674009d4cf7c2a1263) Merged-In: I02e3046bd85d0dfebc68ab444f1796bb54cc69c7 Change-Id: I02e3046bd85d0dfebc68ab444f1796bb54cc69c7 --- libprocessgroup/task_profiles.cpp | 45 +++++++++++++++++++------- libprocessgroup/task_profiles.h | 4 +++ libprocessgroup/task_profiles_test.cpp | 3 ++ 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 59b135013..f51b07671 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -126,6 +126,16 @@ void ProfileAttribute::Reset(const CgroupController& controller, const std::stri file_v2_name_ = file_v2_name; } +bool ProfileAttribute::GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const { + if (controller()->version() == 2) { + // all cgroup v2 attributes use the same process group hierarchy + *path = StringPrintf("%s/uid_%u/pid_%d/%s", controller()->path(), uid, pid, + file_name().c_str()); + return true; + } + return GetPathForTask(pid, path); +} + bool ProfileAttribute::GetPathForTask(int tid, std::string* path) const { std::string subgroup; if (!controller()->GetTaskGroup(tid, &subgroup)) { @@ -209,18 +219,7 @@ bool SetTimerSlackAction::ExecuteForTask(int) const { #endif -bool SetAttributeAction::ExecuteForProcess(uid_t, pid_t pid) const { - return ExecuteForTask(pid); -} - -bool SetAttributeAction::ExecuteForTask(int tid) const { - std::string path; - - if (!attribute_->GetPathForTask(tid, &path)) { - LOG(ERROR) << "Failed to find cgroup for tid " << tid; - return false; - } - +bool SetAttributeAction::WriteValueToFile(const std::string& path) const { if (!WriteStringToFile(value_, path)) { if (access(path.c_str(), F_OK) < 0) { if (optional_) { @@ -240,6 +239,28 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } +bool SetAttributeAction::ExecuteForProcess(uid_t uid, pid_t pid) const { + std::string path; + + if (!attribute_->GetPathForProcess(uid, pid, &path)) { + LOG(ERROR) << "Failed to find cgroup for uid " << uid << " pid " << pid; + return false; + } + + return WriteValueToFile(path); +} + +bool SetAttributeAction::ExecuteForTask(int tid) const { + std::string path; + + if (!attribute_->GetPathForTask(tid, &path)) { + LOG(ERROR) << "Failed to find cgroup for tid " << tid; + return false; + } + + return WriteValueToFile(path); +} + bool SetAttributeAction::ExecuteForUID(uid_t uid) const { std::string path; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index ac8918e65..4663f64e2 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -36,6 +36,7 @@ class IProfileAttribute { const std::string& file_v2_name) = 0; virtual const CgroupController* controller() const = 0; virtual const std::string& file_name() const = 0; + virtual bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const = 0; virtual bool GetPathForTask(int tid, std::string* path) const = 0; virtual bool GetPathForUID(uid_t uid, std::string* path) const = 0; }; @@ -55,6 +56,7 @@ class ProfileAttribute : public IProfileAttribute { void Reset(const CgroupController& controller, const std::string& file_name, const std::string& file_v2_name) override; + bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const override; bool GetPathForTask(int tid, std::string* path) const override; bool GetPathForUID(uid_t uid, std::string* path) const override; @@ -133,6 +135,8 @@ class SetAttributeAction : public ProfileAction { const IProfileAttribute* attribute_; std::string value_; bool optional_; + + bool WriteValueToFile(const std::string& path) const; }; // Set cgroup profile element diff --git a/libprocessgroup/task_profiles_test.cpp b/libprocessgroup/task_profiles_test.cpp index da74bb012..99d819a7c 100644 --- a/libprocessgroup/task_profiles_test.cpp +++ b/libprocessgroup/task_profiles_test.cpp @@ -111,6 +111,9 @@ class ProfileAttributeMock : public IProfileAttribute { return {}; } const std::string& file_name() const override { return file_name_; } + bool GetPathForProcess(uid_t uid, pid_t pid, std::string* path) const override { + return GetPathForTask(pid, path); + } bool GetPathForTask(int tid, std::string* path) const override { #ifdef __ANDROID__ CHECK(CgroupGetControllerPath(CGROUPV2_CONTROLLER_NAME, path));