From ae42d601d09fa32b5c33758432cbd306b60f58ee Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 2 Sep 2021 19:47:12 -0700 Subject: [PATCH 01/18] libprocessgroup: Prevent error spam when tests disable all cpus in a cpuset UserLifecycleTests test disables all Little cores in the course of the test, which causes attempts to add a process into /dev/cpuset/restricted cpuset cgroup to fail with ENOSPC error code, indicating that a process is joining a cpuset cgroup with no online cpus. Current libprocessgroup implementation will log an error on each such occurrence, which spams the logs and makes it hard to analyze test results. Because this situation does not happen in production environment (we do not offline cpus), we can prevent flooding the logs by identifying this case, logging an appropriate error one time and ignore all later similar errors. Bug: 158766131 Test: adb shell "echo 0 > /sys/devices/system/cpu/cpu[0-3]/online" Test: start some apps, observe libprocessgroup errors in the logcat Signed-off-by: Suren Baghdasaryan Change-Id: Ia91d8839d86787569c255481bde077be51c43d93 Merged-In: Ia91d8839d86787569c255481bde077be51c43d93 --- libprocessgroup/task_profiles.cpp | 37 ++++++++++++++++++++++--------- libprocessgroup/task_profiles.h | 2 +- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index cf74e65572c3..e935f99de947 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -194,22 +194,39 @@ void SetCgroupAction::DropResourceCaching() { fd_.reset(FDS_NOT_CACHED); } -bool SetCgroupAction::AddTidToCgroup(int tid, int fd) { +bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { if (tid <= 0) { return true; } std::string value = std::to_string(tid); - if (TEMP_FAILURE_RETRY(write(fd, value.c_str(), value.length())) < 0) { - // If the thread is in the process of exiting, don't flag an error - if (errno != ESRCH) { - PLOG(ERROR) << "AddTidToCgroup failed to write '" << value << "'; fd=" << fd; - return false; + if (TEMP_FAILURE_RETRY(write(fd, value.c_str(), value.length())) == value.length()) { + return true; + } + + // If the thread is in the process of exiting, don't flag an error + if (errno == ESRCH) { + return true; + } + + // ENOSPC is returned when cpuset cgroup that we are joining has no online cpus + if (errno == ENOSPC && !strcmp(controller_name, "cpuset")) { + // This is an abnormal case happening only in testing, so report it only once + static bool empty_cpuset_reported = false; + + if (empty_cpuset_reported) { + return true; } + + LOG(ERROR) << "Failed to add task '" << value + << "' into cpuset because all cpus in that cpuset are offline"; + empty_cpuset_reported = true; + } else { + PLOG(ERROR) << "AddTidToCgroup failed to write '" << value << "'; fd=" << fd; } - return true; + return false; } bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { @@ -219,7 +236,7 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { PLOG(WARNING) << "Failed to open " << procs_path; return false; } - if (!AddTidToCgroup(pid, tmp_fd)) { + if (!AddTidToCgroup(pid, tmp_fd, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } @@ -231,7 +248,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { std::lock_guard lock(fd_mutex_); if (IsFdValid()) { // fd is cached, reuse it - if (!AddTidToCgroup(tid, fd_)) { + if (!AddTidToCgroup(tid, fd_, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } @@ -256,7 +273,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { PLOG(WARNING) << "Failed to open " << tasks_path << ": " << strerror(errno); return false; } - if (!AddTidToCgroup(tid, tmp_fd)) { + if (!AddTidToCgroup(tid, tmp_fd, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 25a84b0c1b84..97c38f4f39c4 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -134,7 +134,7 @@ class SetCgroupAction : public ProfileAction { mutable std::mutex fd_mutex_; static bool IsAppDependentPath(const std::string& path); - static bool AddTidToCgroup(int tid, int fd); + static bool AddTidToCgroup(int tid, int fd, const char* controller_name); bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; } }; From e808841d72816c68da3d6d9f451aba71bf4188bc Mon Sep 17 00:00:00 2001 From: Rick Yiu Date: Sun, 21 Nov 2021 15:57:36 +0800 Subject: [PATCH 02/18] libprocessgroup: Use WriteStringToFd for WriteFileAction Using WriteStringToFile will hold kernfs_mutex which is a big lock, so use WriteStringToFd instead. Besides, also support fd cache for it. Bug: 206970384 Test: build pass Change-Id: Id79f9e1095f52079393c58edb9a4d526f4cc6b5e Merged-In: Id79f9e1095f52079393c58edb9a4d526f4cc6b5e --- libprocessgroup/task_profiles.cpp | 109 ++++++++++++++++++++---------- libprocessgroup/task_profiles.h | 53 ++++++++++----- 2 files changed, 109 insertions(+), 53 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index e935f99de947..3834f9194beb 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -144,30 +144,13 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } -bool SetCgroupAction::IsAppDependentPath(const std::string& path) { - return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; -} - -SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) - : controller_(c), path_(p) { - // file descriptors for app-dependent paths can't be cached - if (IsAppDependentPath(path_)) { - // file descriptor is not cached - fd_.reset(FDS_APP_DEPENDENT); - return; - } - - // file descriptor can be cached later on request - fd_.reset(FDS_NOT_CACHED); -} - -void SetCgroupAction::EnableResourceCaching() { +void CachedFdProfileAction::EnableResourceCaching() { std::lock_guard lock(fd_mutex_); if (fd_ != FDS_NOT_CACHED) { return; } - std::string tasks_path = controller_.GetTasksFilePath(path_); + std::string tasks_path = GetPath(); if (access(tasks_path.c_str(), W_OK) != 0) { // file is not accessible @@ -185,7 +168,7 @@ void SetCgroupAction::EnableResourceCaching() { fd_ = std::move(fd); } -void SetCgroupAction::DropResourceCaching() { +void CachedFdProfileAction::DropResourceCaching() { std::lock_guard lock(fd_mutex_); if (fd_ == FDS_NOT_CACHED) { return; @@ -194,6 +177,26 @@ void SetCgroupAction::DropResourceCaching() { fd_.reset(FDS_NOT_CACHED); } +bool CachedFdProfileAction::IsAppDependentPath(const std::string& path) { + return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; +} + +void CachedFdProfileAction::InitFd(const std::string& path) { + // file descriptors for app-dependent paths can't be cached + if (IsAppDependentPath(path)) { + // file descriptor is not cached + fd_.reset(FDS_APP_DEPENDENT); + return; + } + // file descriptor can be cached later on request + fd_.reset(FDS_NOT_CACHED); +} + +SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) + : controller_(c), path_(p) { + InitFd(controller_.GetTasksFilePath(path_)); +} + bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { if (tid <= 0) { return true; @@ -270,7 +273,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { std::string tasks_path = controller()->GetTasksFilePath(path_); unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC))); if (tmp_fd < 0) { - PLOG(WARNING) << "Failed to open " << tasks_path << ": " << strerror(errno); + PLOG(WARNING) << "Failed to open " << tasks_path; return false; } if (!AddTidToCgroup(tid, tmp_fd, controller()->name())) { @@ -281,37 +284,73 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { return true; } -bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { - std::string filepath(filepath_), value(value_); +WriteFileAction::WriteFileAction(const std::string& path, const std::string& value, + bool logfailures) + : path_(path), value_(value), logfailures_(logfailures) { + InitFd(path_); +} - filepath = StringReplace(filepath, "", std::to_string(uid), true); - filepath = StringReplace(filepath, "", std::to_string(pid), true); - value = StringReplace(value, "", std::to_string(uid), true); - value = StringReplace(value, "", std::to_string(pid), true); +bool WriteFileAction::WriteValueToFile(const std::string& value, const std::string& path, + bool logfailures) { + // Use WriteStringToFd instead of WriteStringToFile because the latter will open file with + // O_TRUNC which causes kernfs_mutex contention + unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_WRONLY | O_CLOEXEC))); + + if (tmp_fd < 0) { + if (logfailures) PLOG(WARNING) << "Failed to open " << path; + return false; + } - if (!WriteStringToFile(value, filepath)) { - if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << filepath; + if (!WriteStringToFd(value, tmp_fd)) { + if (logfailures) PLOG(ERROR) << "Failed to write '" << value << "' to " << path; return false; } return true; } +bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { + std::lock_guard lock(fd_mutex_); + std::string value(value_); + std::string path(path_); + + value = StringReplace(value, "", std::to_string(uid), true); + value = StringReplace(value, "", std::to_string(pid), true); + path = StringReplace(path, "", std::to_string(uid), true); + path = StringReplace(path, "", std::to_string(pid), true); + + return WriteValueToFile(value, path, logfailures_); +} + bool WriteFileAction::ExecuteForTask(int tid) const { - std::string filepath(filepath_), value(value_); + std::lock_guard lock(fd_mutex_); + std::string value(value_); int uid = getuid(); - filepath = StringReplace(filepath, "", std::to_string(uid), true); - filepath = StringReplace(filepath, "", std::to_string(tid), true); value = StringReplace(value, "", std::to_string(uid), true); value = StringReplace(value, "", std::to_string(tid), true); - if (!WriteStringToFile(value, filepath)) { - if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << filepath; + if (IsFdValid()) { + // fd is cached, reuse it + if (!WriteStringToFd(value, fd_)) { + if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << path_; + return false; + } + return true; + } + + if (fd_ == FDS_INACCESSIBLE) { + // no permissions to access the file, ignore + return true; + } + + if (fd_ == FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + PLOG(ERROR) << "Application profile can't be applied to a thread"; return false; } - return true; + return WriteValueToFile(value, path_, logfailures_); } bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 97c38f4f39c4..278892dd55f6 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -108,50 +108,67 @@ class SetAttributeAction : public ProfileAction { std::string value_; }; -// Set cgroup profile element -class SetCgroupAction : public ProfileAction { +// Abstract profile element for cached fd +class CachedFdProfileAction : public ProfileAction { public: - SetCgroupAction(const CgroupController& c, const std::string& p); - - virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; - virtual bool ExecuteForTask(int tid) const; virtual void EnableResourceCaching(); virtual void DropResourceCaching(); - const CgroupController* controller() const { return &controller_; } - std::string path() const { return path_; } - - private: + protected: enum FdState { FDS_INACCESSIBLE = -1, FDS_APP_DEPENDENT = -2, FDS_NOT_CACHED = -3, }; - CgroupController controller_; - std::string path_; android::base::unique_fd fd_; mutable std::mutex fd_mutex_; static bool IsAppDependentPath(const std::string& path); - static bool AddTidToCgroup(int tid, int fd, const char* controller_name); + void InitFd(const std::string& path); bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; } + + virtual const std::string GetPath() const = 0; +}; + +// Set cgroup profile element +class SetCgroupAction : public CachedFdProfileAction { + public: + SetCgroupAction(const CgroupController& c, const std::string& p); + + virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; + virtual bool ExecuteForTask(int tid) const; + + const CgroupController* controller() const { return &controller_; } + + protected: + const std::string GetPath() const override { return controller_.GetTasksFilePath(path_); } + + private: + CgroupController controller_; + std::string path_; + + static bool AddTidToCgroup(int tid, int fd, const char* controller_name); }; // Write to file action -class WriteFileAction : public ProfileAction { +class WriteFileAction : public CachedFdProfileAction { public: - WriteFileAction(const std::string& filepath, const std::string& value, - bool logfailures) noexcept - : filepath_(filepath), value_(value), logfailures_(logfailures) {} + WriteFileAction(const std::string& path, const std::string& value, bool logfailures); virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; + protected: + const std::string GetPath() const override { return path_; } + private: - std::string filepath_, value_; + std::string path_, value_; bool logfailures_; + + static bool WriteValueToFile(const std::string& value, const std::string& path, + bool logfailures); }; class TaskProfile { From fdff9a34714b22f0bc14059ddd0b35d401be0f05 Mon Sep 17 00:00:00 2001 From: Randall Huang Date: Wed, 12 Jan 2022 10:03:30 +0800 Subject: [PATCH 03/18] [DO NOT MERGE] Retry to unmount /data If we fail to umount /data, device won't boot up at all. Bug: 208161227 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: I92d34a253039eb677d4df9fad8a0821fbc684f57 Merged-In: I92d34a253039eb677d4df9fad8a0821fbc684f57 --- fs_mgr/fs_mgr.cpp | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index 21df8af8ca13..9acc67802ba2 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -170,6 +170,22 @@ static bool should_force_check(int fs_stat) { FS_STAT_SET_RESERVED_BLOCKS_FAILED | FS_STAT_ENABLE_ENCRYPTION_FAILED); } +static bool umount_retry(const std::string& mount_point) { + int retry_count = 5; + bool umounted = false; + + while (retry_count-- > 0) { + umounted = umount(mount_point.c_str()) == 0; + if (umounted) { + LINFO << __FUNCTION__ << "(): unmount(" << mount_point << ") succeeded"; + break; + } + PERROR << __FUNCTION__ << "(): umount(" << mount_point << ") failed"; + if (retry_count) sleep(1); + } + return umounted; +} + static void check_fs(const std::string& blk_device, const std::string& fs_type, const std::string& target, int* fs_stat) { int status; @@ -209,25 +225,12 @@ static void check_fs(const std::string& blk_device, const std::string& fs_type, tmpmnt_opts.c_str()); PINFO << __FUNCTION__ << "(): mount(" << blk_device << "," << target << "," << fs_type << ")=" << ret; - if (!ret) { - bool umounted = false; - int retry_count = 5; - while (retry_count-- > 0) { - umounted = umount(target.c_str()) == 0; - if (umounted) { - LINFO << __FUNCTION__ << "(): unmount(" << target << ") succeeded"; - break; - } - PERROR << __FUNCTION__ << "(): umount(" << target << ") failed"; - if (retry_count) sleep(1); - } - if (!umounted) { - // boot may fail but continue and leave it to later stage for now. - PERROR << __FUNCTION__ << "(): umount(" << target << ") timed out"; - *fs_stat |= FS_STAT_RO_UNMOUNT_FAILED; - } - } else { + if (ret) { *fs_stat |= FS_STAT_RO_MOUNT_FAILED; + } else if (!umount_retry(target)) { + // boot may fail but continue and leave it to later stage for now. + PERROR << __FUNCTION__ << "(): umount(" << target << ") timed out"; + *fs_stat |= FS_STAT_RO_UNMOUNT_FAILED; } } @@ -1029,12 +1032,11 @@ static int handle_encryptable(const FstabEntry& entry) { return FS_MGR_MNTALL_DEV_NOT_ENCRYPTED; } } else if (should_use_metadata_encryption(entry)) { - if (umount(entry.mount_point.c_str()) == 0) { + if (umount_retry(entry.mount_point)) { return FS_MGR_MNTALL_DEV_NEEDS_METADATA_ENCRYPTION; - } else { - PERROR << "Could not umount " << entry.mount_point << " - fail since can't encrypt"; - return FS_MGR_MNTALL_FAIL; } + PERROR << "Could not umount " << entry.mount_point << " - fail since can't encrypt"; + return FS_MGR_MNTALL_FAIL; } else if (entry.fs_mgr_flags.file_encryption || entry.fs_mgr_flags.force_fde_or_fbe) { LINFO << entry.mount_point << " is file encrypted"; return FS_MGR_MNTALL_DEV_FILE_ENCRYPTED; From 80a8feec380d36a51fd13338342521409af0b3f4 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Wed, 29 Dec 2021 14:44:22 -0800 Subject: [PATCH 04/18] [DO NOT MERGE] Don't try to mount if the disk has no FS magic This avoids: [ 22.932529][ T1] init: [libfs_mgr]Invalid f2fs superblock on '/dev/block/platform/14700000.ufs/by-name/metadata' [ 22.934609][ T1] F2FS-fs (sda8): Magic Mismatch, valid(0xf2f52010) - read(0x0) [ 22.935061][ T1] F2FS-fs (sda8): Can't find valid F2FS filesystem in 1th superblock [ 22.937306][ T1] F2FS-fs (sda8): Magic Mismatch, valid(0xf2f52010) - read(0x0) [ 22.943700][ T1] F2FS-fs (sda8): Can't find valid F2FS filesystem in 2th superblock [ 22.951937][ T1] init: [libfs_mgr]__mount(source=/dev/block/platform/14700000.ufs/by-name/metadata,target=/metadata,type=f2fs)=-1: Invalid argument Bug: 210589189 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: I27989b25769eae83eb06ac86146f27baf288b7e1 Merged-In: I27989b25769eae83eb06ac86146f27baf288b7e1 --- fs_mgr/fs_mgr.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index 9acc67802ba2..eea5983ae923 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -1882,9 +1882,13 @@ int fs_mgr_do_mount_one(const FstabEntry& entry, const std::string& alt_mount_po auto& mount_point = alt_mount_point.empty() ? entry.mount_point : alt_mount_point; // Run fsck if needed - prepare_fs_for_mount(entry.blk_device, entry, mount_point); + int ret = prepare_fs_for_mount(entry.blk_device, entry, mount_point); + // Wiped case doesn't require to try __mount below. + if (ret & FS_STAT_INVALID_MAGIC) { + return FS_MGR_DOMNT_FAILED; + } - int ret = __mount(entry.blk_device, mount_point, entry); + ret = __mount(entry.blk_device, mount_point, entry); if (ret) { ret = (errno == EBUSY) ? FS_MGR_DOMNT_BUSY : FS_MGR_DOMNT_FAILED; } From 9efc42e4f28197ee59803b1248eb033091beda6d Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Thu, 30 Dec 2021 16:48:44 -0800 Subject: [PATCH 05/18] [DO NOT MERGE] Don't use FSCK_LOG_FILE used for ext4 only Bug: 210589189 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: Iaf7a61c6c9c6cd557f953b1a665e7a1640d357ae Merged-In: Iaf7a61c6c9c6cd557f953b1a665e7a1640d357ae --- fs_mgr/fs_mgr.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs_mgr/fs_mgr.cpp b/fs_mgr/fs_mgr.cpp index eea5983ae923..8896ec33c573 100644 --- a/fs_mgr/fs_mgr.cpp +++ b/fs_mgr/fs_mgr.cpp @@ -271,12 +271,12 @@ static void check_fs(const std::string& blk_device, const std::string& fs_type, LINFO << "Running " << F2FS_FSCK_BIN << " -f -c 10000 --debug-cache " << realpath(blk_device); ret = logwrap_fork_execvp(ARRAY_SIZE(f2fs_fsck_forced_argv), f2fs_fsck_forced_argv, - &status, false, LOG_KLOG | LOG_FILE, false, FSCK_LOG_FILE); + &status, false, LOG_KLOG | LOG_FILE, false, nullptr); } else { LINFO << "Running " << F2FS_FSCK_BIN << " -a -c 10000 --debug-cache " << realpath(blk_device); ret = logwrap_fork_execvp(ARRAY_SIZE(f2fs_fsck_argv), f2fs_fsck_argv, &status, false, - LOG_KLOG | LOG_FILE, false, FSCK_LOG_FILE); + LOG_KLOG | LOG_FILE, false, nullptr); } if (ret < 0) { /* No need to check for error in fork, we can't really handle it now */ From 312ad8309f84e33486ff171877f0889785df2dbd Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Thu, 6 Jan 2022 17:28:41 -0800 Subject: [PATCH 06/18] [DO NOT MERGE] Run check_fs only /data is mounted After switching to /system from ramdisk, first_stage_init has no required libraries for check_fs. [ 20.838811][ T1] init: [libfs_mgr]Created logical partition scratch on device /dev/block/dm-6 [ 20.972704][ T1] init: [libfs_mgr]Running /system/bin/fsck.f2fs -a -c 10000 --debug-cache /dev/block/dm-6 [ 20.977879][ T345] logwrapper: executing /system/bin/fsck.f2fs failed: No such file or directory [ 20.978470][ T1] fsck.f2fs: executing /system/bin/fsck.f2fs failed: No such file or directory [ 20.981137][ T1] fsck.f2fs: fsck.f2fs terminated by exit(255) [ 21.002958][ T1] init: [libfs_mgr]__mount(source=/dev/block/dm-6,target=/mnt/scratch,type=f2fs)=0: Success [ 21.017748][ T1] init: [libfs_mgr]umount(/mnt/scratch) [ 21.021028][ T1] init: [libfs_mgr]Running /system/bin/fsck.f2fs -a -c 10000 --debug-cache /dev/block/dm-6 [ 21.028759][ T347] logwrapper: executing /system/bin/fsck.f2fs failed: No such file or directory [ 21.028793][ T1] fsck.f2fs: executing /system/bin/fsck.f2fs failed: No such file or directory [ 21.049100][ T1] fsck.f2fs: fsck.f2fs terminated by exit(255) [ 21.068101][ T1] init: [libfs_mgr]__mount(source=/dev/block/dm-6,target=/mnt/scratch,type=f2fs)=0: Success Bug: 210589189 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: Ie41cba4e7553860fdb48996d9b58a34093f0b723 Merged-In: Ie41cba4e7553860fdb48996d9b58a34093f0b723 --- fs_mgr/fs_mgr_overlayfs.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs_mgr/fs_mgr_overlayfs.cpp b/fs_mgr/fs_mgr_overlayfs.cpp index 4d32bda80c7e..cd113f68453c 100644 --- a/fs_mgr/fs_mgr_overlayfs.cpp +++ b/fs_mgr/fs_mgr_overlayfs.cpp @@ -868,7 +868,10 @@ bool fs_mgr_overlayfs_mount_scratch(const std::string& device_path, const std::s entry.flags &= ~MS_RDONLY; fs_mgr_set_blk_ro(device_path, false); } - entry.fs_mgr_flags.check = true; + // check_fs requires apex runtime library + if (fs_mgr_overlayfs_already_mounted("/data", false)) { + entry.fs_mgr_flags.check = true; + } auto save_errno = errno; if (mounted) mounted = fs_mgr_do_mount_one(entry) == 0; if (!mounted) { From 11bea35f2ce6c74ec9af1adb920d861ff4f10fad Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Wed, 29 Dec 2021 13:17:15 -0800 Subject: [PATCH 07/18] [DO NOT MERGE] Mount /mnt/scracth with -o sync,nodiscard "-o discard" issues UNMAP commands to loopback resulting in punch_hole on the pinned file in /data. That will break the pinned block map. Bug: 210589189 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: Ia927c43fc75164ce5929173f5740737eac4de484 Merged-In: Ia927c43fc75164ce5929173f5740737eac4de484 --- fs_mgr/fs_mgr_overlayfs.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs_mgr/fs_mgr_overlayfs.cpp b/fs_mgr/fs_mgr_overlayfs.cpp index cd113f68453c..cb2f249bbdd5 100644 --- a/fs_mgr/fs_mgr_overlayfs.cpp +++ b/fs_mgr/fs_mgr_overlayfs.cpp @@ -866,6 +866,8 @@ bool fs_mgr_overlayfs_mount_scratch(const std::string& device_path, const std::s errno = save_errno; } entry.flags &= ~MS_RDONLY; + entry.flags |= MS_SYNCHRONOUS; + entry.fs_options = "nodiscard"; fs_mgr_set_blk_ro(device_path, false); } // check_fs requires apex runtime library From 8d3065db3cc3e5eda439b760fc826c73948fcf67 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Wed, 29 Dec 2021 15:11:00 -0800 Subject: [PATCH 08/18] [DO NOT MERGE] Allow to run fsck.f2fs in first_stage_ramdisk [ 23.065933][ T1] init: [libfs_mgr]Running /system/bin/fsck.f2fs -a -c 10000 --debug-cache /dev/block/sda8 [ 23.067470][ T1] logwrapper: Cannot log to file /dev/fscklogs/log [ 23.067829][ T1] logwrapper: [ 23.068997][ T332] logwrapper: executing /system/bin/fsck.f2fs failed: Permission denied [ 23.069759][ T1] fsck.f2fs: executing /system/bin/fsck.f2fs failed: Permission denied [ 23.071659][ T332] logwrapper: [ 23.083283][ T1] fsck.f2fs: fsck.f2fs terminated by exit(255) [ 23.083283][ T1] [ 23.111166][ T1] F2FS-fs (sda8): Found nat_bits in checkpoint [ 23.121242][ T1] F2FS-fs (sda8): Mounted with checkpoint version = 6a65cb64 [ 23.121831][ T1] init: [libfs_mgr]__mount(source=/dev/block/platform/14700000.ufs/by-name/metadata,target=/metadata,type=f2fs)=0: Success Bug: 210589189 Bug: 214203920 Signed-off-by: Jaegeuk Kim Change-Id: I720e2aa4b1ab845af96610cd0d9c6e3c9b4cae03 Merged-In: Change-Id: I720e2aa4b1ab845af96610cd0d9c6e3c9b4cae03 --- libcutils/fs_config.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libcutils/fs_config.cpp b/libcutils/fs_config.cpp index e9497a806f90..a6835fc70c14 100644 --- a/libcutils/fs_config.cpp +++ b/libcutils/fs_config.cpp @@ -211,6 +211,7 @@ static const struct fs_path_config android_files[] = { { 00755, AID_ROOT, AID_ROOT, 0, "first_stage_ramdisk/system/bin/resize2fs" }, { 00755, AID_ROOT, AID_ROOT, 0, "first_stage_ramdisk/system/bin/snapuserd" }, { 00755, AID_ROOT, AID_ROOT, 0, "first_stage_ramdisk/system/bin/tune2fs" }, + { 00755, AID_ROOT, AID_ROOT, 0, "first_stage_ramdisk/system/bin/fsck.f2fs" }, // generic defaults { 00755, AID_ROOT, AID_ROOT, 0, "bin/*" }, { 00640, AID_ROOT, AID_SHELL, 0, "fstab.*" }, From f79cfb532be3382b6b73009a80082f2fc8daca94 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 20 Jan 2022 10:58:43 -0800 Subject: [PATCH 09/18] libprocessgroup: Move fd caching code into FdCacheHelper Refactor file descriptor caching code and move it into FdCacheHelper because later on when we introduce fd caching for SetProcessProfiles the children of CachedFdProfileAction become different enough that sharing the same parent becomes a hindrance. Bug: 215557553 Signed-off-by: Suren Baghdasaryan Change-Id: If3812a0090c81a29e25f0888b0511cfaf48edea3 Merged-In: If3812a0090c81a29e25f0888b0511cfaf48edea3 --- libprocessgroup/task_profiles.cpp | 148 ++++++++++++++++++------------ libprocessgroup/task_profiles.h | 42 ++------- 2 files changed, 100 insertions(+), 90 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index 3834f9194beb..a61700cec13b 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -51,6 +51,67 @@ static constexpr const char* TASK_PROFILE_DB_VENDOR_FILE = "/vendor/etc/task_pro static constexpr const char* TEMPLATE_TASK_PROFILE_API_FILE = "/etc/task_profiles/task_profiles_%u.json"; +class FdCacheHelper { + public: + enum FdState { + FDS_INACCESSIBLE = -1, + FDS_APP_DEPENDENT = -2, + FDS_NOT_CACHED = -3, + }; + + static void Cache(const std::string& path, android::base::unique_fd& fd); + static void Drop(android::base::unique_fd& fd); + static void Init(const std::string& path, android::base::unique_fd& fd); + static bool IsCached(const android::base::unique_fd& fd) { return fd > FDS_INACCESSIBLE; } + + private: + static bool IsAppDependentPath(const std::string& path); +}; + +void FdCacheHelper::Init(const std::string& path, android::base::unique_fd& fd) { + // file descriptors for app-dependent paths can't be cached + if (IsAppDependentPath(path)) { + // file descriptor is not cached + fd.reset(FDS_APP_DEPENDENT); + return; + } + // file descriptor can be cached later on request + fd.reset(FDS_NOT_CACHED); +} + +void FdCacheHelper::Cache(const std::string& path, android::base::unique_fd& fd) { + if (fd != FDS_NOT_CACHED) { + return; + } + + if (access(path.c_str(), W_OK) != 0) { + // file is not accessible + fd.reset(FDS_INACCESSIBLE); + return; + } + + unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_WRONLY | O_CLOEXEC))); + if (tmp_fd < 0) { + PLOG(ERROR) << "Failed to cache fd '" << path << "'"; + fd.reset(FDS_INACCESSIBLE); + return; + } + + fd = std::move(tmp_fd); +} + +void FdCacheHelper::Drop(android::base::unique_fd& fd) { + if (fd == FDS_NOT_CACHED) { + return; + } + + fd.reset(FDS_NOT_CACHED); +} + +bool FdCacheHelper::IsAppDependentPath(const std::string& path) { + return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; +} + void ProfileAttribute::Reset(const CgroupController& controller, const std::string& file_name) { controller_ = controller; file_name_ = file_name; @@ -144,57 +205,9 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } -void CachedFdProfileAction::EnableResourceCaching() { - std::lock_guard lock(fd_mutex_); - if (fd_ != FDS_NOT_CACHED) { - return; - } - - std::string tasks_path = GetPath(); - - if (access(tasks_path.c_str(), W_OK) != 0) { - // file is not accessible - fd_.reset(FDS_INACCESSIBLE); - return; - } - - unique_fd fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC))); - if (fd < 0) { - PLOG(ERROR) << "Failed to cache fd '" << tasks_path << "'"; - fd_.reset(FDS_INACCESSIBLE); - return; - } - - fd_ = std::move(fd); -} - -void CachedFdProfileAction::DropResourceCaching() { - std::lock_guard lock(fd_mutex_); - if (fd_ == FDS_NOT_CACHED) { - return; - } - - fd_.reset(FDS_NOT_CACHED); -} - -bool CachedFdProfileAction::IsAppDependentPath(const std::string& path) { - return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; -} - -void CachedFdProfileAction::InitFd(const std::string& path) { - // file descriptors for app-dependent paths can't be cached - if (IsAppDependentPath(path)) { - // file descriptor is not cached - fd_.reset(FDS_APP_DEPENDENT); - return; - } - // file descriptor can be cached later on request - fd_.reset(FDS_NOT_CACHED); -} - SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { - InitFd(controller_.GetTasksFilePath(path_)); + FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_); } bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { @@ -249,7 +262,7 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { bool SetCgroupAction::ExecuteForTask(int tid) const { std::lock_guard lock(fd_mutex_); - if (IsFdValid()) { + if (FdCacheHelper::IsCached(fd_)) { // fd is cached, reuse it if (!AddTidToCgroup(tid, fd_, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; @@ -258,12 +271,12 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { return true; } - if (fd_ == FDS_INACCESSIBLE) { + if (fd_ == FdCacheHelper::FDS_INACCESSIBLE) { // no permissions to access the file, ignore return true; } - if (fd_ == FDS_APP_DEPENDENT) { + if (fd_ == FdCacheHelper::FDS_APP_DEPENDENT) { // application-dependent path can't be used with tid PLOG(ERROR) << "Application profile can't be applied to a thread"; return false; @@ -284,10 +297,20 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { return true; } +void SetCgroupAction::EnableResourceCaching() { + std::lock_guard lock(fd_mutex_); + FdCacheHelper::Cache(controller_.GetTasksFilePath(path_), fd_); +} + +void SetCgroupAction::DropResourceCaching() { + std::lock_guard lock(fd_mutex_); + FdCacheHelper::Drop(fd_); +} + WriteFileAction::WriteFileAction(const std::string& path, const std::string& value, bool logfailures) : path_(path), value_(value), logfailures_(logfailures) { - InitFd(path_); + FdCacheHelper::Init(path_, fd_); } bool WriteFileAction::WriteValueToFile(const std::string& value, const std::string& path, @@ -330,7 +353,7 @@ bool WriteFileAction::ExecuteForTask(int tid) const { value = StringReplace(value, "", std::to_string(uid), true); value = StringReplace(value, "", std::to_string(tid), true); - if (IsFdValid()) { + if (FdCacheHelper::IsCached(fd_)) { // fd is cached, reuse it if (!WriteStringToFd(value, fd_)) { if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << path_; @@ -339,12 +362,12 @@ bool WriteFileAction::ExecuteForTask(int tid) const { return true; } - if (fd_ == FDS_INACCESSIBLE) { + if (fd_ == FdCacheHelper::FDS_INACCESSIBLE) { // no permissions to access the file, ignore return true; } - if (fd_ == FDS_APP_DEPENDENT) { + if (fd_ == FdCacheHelper::FDS_APP_DEPENDENT) { // application-dependent path can't be used with tid PLOG(ERROR) << "Application profile can't be applied to a thread"; return false; @@ -353,6 +376,16 @@ bool WriteFileAction::ExecuteForTask(int tid) const { return WriteValueToFile(value, path_, logfailures_); } +void WriteFileAction::EnableResourceCaching() { + std::lock_guard lock(fd_mutex_); + FdCacheHelper::Cache(path_, fd_); +} + +void WriteFileAction::DropResourceCaching() { + std::lock_guard lock(fd_mutex_); + FdCacheHelper::Drop(fd_); +} + bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { for (const auto& profile : profiles_) { if (!profile->ExecuteForProcess(uid, pid)) { @@ -457,8 +490,7 @@ TaskProfiles::TaskProfiles() { android::base::StringPrintf(TEMPLATE_TASK_PROFILE_API_FILE, api_level); if (!access(api_profiles_path.c_str(), F_OK) || errno != ENOENT) { if (!Load(CgroupMap::GetInstance(), api_profiles_path)) { - LOG(ERROR) << "Loading " << api_profiles_path << " for [" << getpid() - << "] failed"; + LOG(ERROR) << "Loading " << api_profiles_path << " for [" << getpid() << "] failed"; } } } diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 278892dd55f6..11f20a2825fb 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -108,64 +108,42 @@ class SetAttributeAction : public ProfileAction { std::string value_; }; -// Abstract profile element for cached fd -class CachedFdProfileAction : public ProfileAction { - public: - virtual void EnableResourceCaching(); - virtual void DropResourceCaching(); - - protected: - enum FdState { - FDS_INACCESSIBLE = -1, - FDS_APP_DEPENDENT = -2, - FDS_NOT_CACHED = -3, - }; - - android::base::unique_fd fd_; - mutable std::mutex fd_mutex_; - - static bool IsAppDependentPath(const std::string& path); - - void InitFd(const std::string& path); - bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; } - - virtual const std::string GetPath() const = 0; -}; - // Set cgroup profile element -class SetCgroupAction : public CachedFdProfileAction { +class SetCgroupAction : public ProfileAction { public: SetCgroupAction(const CgroupController& c, const std::string& p); virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; + virtual void EnableResourceCaching(); + virtual void DropResourceCaching(); const CgroupController* controller() const { return &controller_; } - protected: - const std::string GetPath() const override { return controller_.GetTasksFilePath(path_); } - private: CgroupController controller_; std::string path_; + android::base::unique_fd fd_; + mutable std::mutex fd_mutex_; static bool AddTidToCgroup(int tid, int fd, const char* controller_name); }; // Write to file action -class WriteFileAction : public CachedFdProfileAction { +class WriteFileAction : public ProfileAction { public: WriteFileAction(const std::string& path, const std::string& value, bool logfailures); virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; - - protected: - const std::string GetPath() const override { return path_; } + virtual void EnableResourceCaching(); + virtual void DropResourceCaching(); private: std::string path_, value_; bool logfailures_; + android::base::unique_fd fd_; + mutable std::mutex fd_mutex_; static bool WriteValueToFile(const std::string& value, const std::string& path, bool logfailures); From 0b6ba30a7948d9f09b41c8a60807294258c1630f Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 20 Jan 2022 15:41:28 -0800 Subject: [PATCH 10/18] libprocessgroup: Add fd caching support for SetProcessProfiles Process profiles operating on paths that do not depend on pid or uid of the process can cache the fd of the file they are operating on. Add support for fd caching similar to how SetTaskProfiles caches the fd of the file it needs to write to. Bug: 215557553 Signed-off-by: Suren Baghdasaryan Change-Id: Ie73ebcbbf1919d90409f40c1f6b08743f4edf97c Merged-In: Ie73ebcbbf1919d90409f40c1f6b08743f4edf97c --- .../include/processgroup/processgroup.h | 2 + libprocessgroup/processgroup.cpp | 9 +- libprocessgroup/task_profiles.cpp | 176 +++++++++++------- libprocessgroup/task_profiles.h | 34 ++-- 4 files changed, 143 insertions(+), 78 deletions(-) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index fa2642d863ab..50416fa80000 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -34,6 +34,8 @@ bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& pr #ifndef __ANDROID_VNDK__ +bool SetProcessProfilesCached(uid_t uid, pid_t pid, const std::vector& profiles); + static constexpr const char* CGROUPS_RC_PATH = "/dev/cgroup_info/cgroup.rc"; bool UsePerAppMemcg(); diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index c824376e5d25..2ce3fa0c7434 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -112,11 +112,16 @@ static bool isMemoryCgroupSupported() { } void DropTaskProfilesResourceCaching() { - TaskProfiles::GetInstance().DropResourceCaching(); + TaskProfiles::GetInstance().DropResourceCaching(ProfileAction::RCT_TASK); + TaskProfiles::GetInstance().DropResourceCaching(ProfileAction::RCT_PROCESS); } bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles) { - return TaskProfiles::GetInstance().SetProcessProfiles(uid, pid, profiles); + return TaskProfiles::GetInstance().SetProcessProfiles(uid, pid, profiles, false); +} + +bool SetProcessProfilesCached(uid_t uid, pid_t pid, const std::vector& profiles) { + return TaskProfiles::GetInstance().SetProcessProfiles(uid, pid, profiles, true); } bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache) { diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index a61700cec13b..74ba7f61ac8c 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -207,7 +207,9 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) : controller_(c), path_(p) { - FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_); + FdCacheHelper::Init(controller_.GetTasksFilePath(path_), fd_[ProfileAction::RCT_TASK]); + // uid and pid don't matter because IsAppDependentPath ensures the path doesn't use them + FdCacheHelper::Init(controller_.GetProcsFilePath(path_, 0, 0), fd_[ProfileAction::RCT_PROCESS]); } bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { @@ -245,7 +247,40 @@ bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_nam return false; } +ProfileAction::CacheUseResult SetCgroupAction::UseCachedFd(ResourceCacheType cache_type, + int id) const { + std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_[cache_type])) { + // fd is cached, reuse it + if (!AddTidToCgroup(id, fd_[cache_type], controller()->name())) { + LOG(ERROR) << "Failed to add task into cgroup"; + return ProfileAction::FAIL; + } + return ProfileAction::SUCCESS; + } + + if (fd_[cache_type] == FdCacheHelper::FDS_INACCESSIBLE) { + // no permissions to access the file, ignore + return ProfileAction::SUCCESS; + } + + if (cache_type == ResourceCacheType::RCT_TASK && + fd_[cache_type] == FdCacheHelper::FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + PLOG(ERROR) << "Application profile can't be applied to a thread"; + return ProfileAction::FAIL; + } + + return ProfileAction::UNUSED; +} + bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { + CacheUseResult result = UseCachedFd(ProfileAction::RCT_PROCESS, pid); + if (result != ProfileAction::UNUSED) { + return result == ProfileAction::SUCCESS; + } + + // fd was not cached or cached fd can't be used std::string procs_path = controller()->GetProcsFilePath(path_, uid, pid); unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(procs_path.c_str(), O_WRONLY | O_CLOEXEC))); if (tmp_fd < 0) { @@ -261,28 +296,12 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { } bool SetCgroupAction::ExecuteForTask(int tid) const { - std::lock_guard lock(fd_mutex_); - if (FdCacheHelper::IsCached(fd_)) { - // fd is cached, reuse it - if (!AddTidToCgroup(tid, fd_, controller()->name())) { - LOG(ERROR) << "Failed to add task into cgroup"; - return false; - } - return true; - } - - if (fd_ == FdCacheHelper::FDS_INACCESSIBLE) { - // no permissions to access the file, ignore - return true; - } - - if (fd_ == FdCacheHelper::FDS_APP_DEPENDENT) { - // application-dependent path can't be used with tid - PLOG(ERROR) << "Application profile can't be applied to a thread"; - return false; + CacheUseResult result = UseCachedFd(ProfileAction::RCT_TASK, tid); + if (result != ProfileAction::UNUSED) { + return result == ProfileAction::SUCCESS; } - // fd was not cached because cached fd can't be used + // fd was not cached or cached fd can't be used std::string tasks_path = controller()->GetTasksFilePath(path_); unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC))); if (tmp_fd < 0) { @@ -297,14 +316,30 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { return true; } -void SetCgroupAction::EnableResourceCaching() { +void SetCgroupAction::EnableResourceCaching(ResourceCacheType cache_type) { std::lock_guard lock(fd_mutex_); - FdCacheHelper::Cache(controller_.GetTasksFilePath(path_), fd_); + // Return early to prevent unnecessary calls to controller_.Get{Tasks|Procs}FilePath() which + // include regex evaluations + if (fd_[cache_type] != FdCacheHelper::FDS_NOT_CACHED) { + return; + } + switch (cache_type) { + case (ProfileAction::RCT_TASK): + FdCacheHelper::Cache(controller_.GetTasksFilePath(path_), fd_[cache_type]); + break; + case (ProfileAction::RCT_PROCESS): + // uid and pid don't matter because IsAppDependentPath ensures the path doesn't use them + FdCacheHelper::Cache(controller_.GetProcsFilePath(path_, 0, 0), fd_[cache_type]); + break; + default: + LOG(ERROR) << "Invalid cache type is specified!"; + break; + } } -void SetCgroupAction::DropResourceCaching() { +void SetCgroupAction::DropResourceCaching(ResourceCacheType cache_type) { std::lock_guard lock(fd_mutex_); - FdCacheHelper::Drop(fd_); + FdCacheHelper::Drop(fd_[cache_type]); } WriteFileAction::WriteFileAction(const std::string& path, const std::string& value, @@ -332,13 +367,43 @@ bool WriteFileAction::WriteValueToFile(const std::string& value, const std::stri return true; } -bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { +ProfileAction::CacheUseResult WriteFileAction::UseCachedFd(ResourceCacheType cache_type, + const std::string& value) const { std::lock_guard lock(fd_mutex_); + if (FdCacheHelper::IsCached(fd_)) { + // fd is cached, reuse it + if (!WriteStringToFd(value, fd_)) { + if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << path_; + return ProfileAction::FAIL; + } + return ProfileAction::SUCCESS; + } + + if (fd_ == FdCacheHelper::FDS_INACCESSIBLE) { + // no permissions to access the file, ignore + return ProfileAction::SUCCESS; + } + + if (cache_type == ResourceCacheType::RCT_TASK && fd_ == FdCacheHelper::FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + PLOG(ERROR) << "Application profile can't be applied to a thread"; + return ProfileAction::FAIL; + } + return ProfileAction::UNUSED; +} + +bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { std::string value(value_); - std::string path(path_); value = StringReplace(value, "", std::to_string(uid), true); value = StringReplace(value, "", std::to_string(pid), true); + + CacheUseResult result = UseCachedFd(ProfileAction::RCT_PROCESS, value); + if (result != ProfileAction::UNUSED) { + return result == ProfileAction::SUCCESS; + } + + std::string path(path_); path = StringReplace(path, "", std::to_string(uid), true); path = StringReplace(path, "", std::to_string(pid), true); @@ -346,51 +411,33 @@ bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { } bool WriteFileAction::ExecuteForTask(int tid) const { - std::lock_guard lock(fd_mutex_); std::string value(value_); int uid = getuid(); value = StringReplace(value, "", std::to_string(uid), true); value = StringReplace(value, "", std::to_string(tid), true); - if (FdCacheHelper::IsCached(fd_)) { - // fd is cached, reuse it - if (!WriteStringToFd(value, fd_)) { - if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << path_; - return false; - } - return true; - } - - if (fd_ == FdCacheHelper::FDS_INACCESSIBLE) { - // no permissions to access the file, ignore - return true; - } - - if (fd_ == FdCacheHelper::FDS_APP_DEPENDENT) { - // application-dependent path can't be used with tid - PLOG(ERROR) << "Application profile can't be applied to a thread"; - return false; + CacheUseResult result = UseCachedFd(ProfileAction::RCT_TASK, value); + if (result != ProfileAction::UNUSED) { + return result == ProfileAction::SUCCESS; } return WriteValueToFile(value, path_, logfailures_); } -void WriteFileAction::EnableResourceCaching() { +void WriteFileAction::EnableResourceCaching(ResourceCacheType) { std::lock_guard lock(fd_mutex_); FdCacheHelper::Cache(path_, fd_); } -void WriteFileAction::DropResourceCaching() { +void WriteFileAction::DropResourceCaching(ResourceCacheType) { std::lock_guard lock(fd_mutex_); FdCacheHelper::Drop(fd_); } bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { for (const auto& profile : profiles_) { - if (!profile->ExecuteForProcess(uid, pid)) { - PLOG(WARNING) << "ExecuteForProcess failed for aggregate profile"; - } + profile->ExecuteForProcess(uid, pid); } return true; } @@ -402,15 +449,15 @@ bool ApplyProfileAction::ExecuteForTask(int tid) const { return true; } -void ApplyProfileAction::EnableResourceCaching() { +void ApplyProfileAction::EnableResourceCaching(ResourceCacheType cache_type) { for (const auto& profile : profiles_) { - profile->EnableResourceCaching(); + profile->EnableResourceCaching(cache_type); } } -void ApplyProfileAction::DropResourceCaching() { +void ApplyProfileAction::DropResourceCaching(ResourceCacheType cache_type) { for (const auto& profile : profiles_) { - profile->DropResourceCaching(); + profile->DropResourceCaching(cache_type); } } @@ -440,33 +487,33 @@ bool TaskProfile::ExecuteForTask(int tid) const { return true; } -void TaskProfile::EnableResourceCaching() { +void TaskProfile::EnableResourceCaching(ProfileAction::ResourceCacheType cache_type) { if (res_cached_) { return; } for (auto& element : elements_) { - element->EnableResourceCaching(); + element->EnableResourceCaching(cache_type); } res_cached_ = true; } -void TaskProfile::DropResourceCaching() { +void TaskProfile::DropResourceCaching(ProfileAction::ResourceCacheType cache_type) { if (!res_cached_) { return; } for (auto& element : elements_) { - element->DropResourceCaching(); + element->DropResourceCaching(cache_type); } res_cached_ = false; } -void TaskProfiles::DropResourceCaching() const { +void TaskProfiles::DropResourceCaching(ProfileAction::ResourceCacheType cache_type) const { for (auto& iter : profiles_) { - iter.second->DropResourceCaching(); + iter.second->DropResourceCaching(cache_type); } } @@ -683,10 +730,13 @@ const ProfileAttribute* TaskProfiles::GetAttribute(const std::string& name) cons } bool TaskProfiles::SetProcessProfiles(uid_t uid, pid_t pid, - const std::vector& profiles) { + 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->ExecuteForProcess(uid, pid)) { PLOG(WARNING) << "Failed to apply " << name << " process profile"; } @@ -703,7 +753,7 @@ bool TaskProfiles::SetTaskProfiles(int tid, const std::vector& prof TaskProfile* profile = GetProfile(name); if (profile != nullptr) { if (use_fd_cache) { - profile->EnableResourceCaching(); + profile->EnableResourceCaching(ProfileAction::RCT_TASK); } if (!profile->ExecuteForTask(tid)) { PLOG(WARNING) << "Failed to apply " << name << " task profile"; diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 11f20a2825fb..1aaa196ba5c0 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -45,14 +45,19 @@ class ProfileAttribute { // Abstract profile element class ProfileAction { public: + enum ResourceCacheType { RCT_TASK = 0, RCT_PROCESS, RCT_COUNT }; + virtual ~ProfileAction() {} // Default implementations will fail virtual bool ExecuteForProcess(uid_t, pid_t) const { return false; }; virtual bool ExecuteForTask(int) const { return false; }; - virtual void EnableResourceCaching() {} - virtual void DropResourceCaching() {} + virtual void EnableResourceCaching(ResourceCacheType) {} + virtual void DropResourceCaching(ResourceCacheType) {} + + protected: + enum CacheUseResult { SUCCESS, FAIL, UNUSED }; }; // Profile actions @@ -115,18 +120,19 @@ class SetCgroupAction : public ProfileAction { virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; - virtual void EnableResourceCaching(); - virtual void DropResourceCaching(); + virtual void EnableResourceCaching(ResourceCacheType cache_type); + virtual void DropResourceCaching(ResourceCacheType cache_type); const CgroupController* controller() const { return &controller_; } private: CgroupController controller_; std::string path_; - android::base::unique_fd fd_; + android::base::unique_fd fd_[ProfileAction::RCT_COUNT]; mutable std::mutex fd_mutex_; static bool AddTidToCgroup(int tid, int fd, const char* controller_name); + CacheUseResult UseCachedFd(ResourceCacheType cache_type, int id) const; }; // Write to file action @@ -136,8 +142,8 @@ class WriteFileAction : public ProfileAction { virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; - virtual void EnableResourceCaching(); - virtual void DropResourceCaching(); + virtual void EnableResourceCaching(ResourceCacheType cache_type); + virtual void DropResourceCaching(ResourceCacheType cache_type); private: std::string path_, value_; @@ -147,6 +153,7 @@ class WriteFileAction : public ProfileAction { static bool WriteValueToFile(const std::string& value, const std::string& path, bool logfailures); + CacheUseResult UseCachedFd(ResourceCacheType cache_type, const std::string& value) const; }; class TaskProfile { @@ -158,8 +165,8 @@ class TaskProfile { bool ExecuteForProcess(uid_t uid, pid_t pid) const; bool ExecuteForTask(int tid) const; - void EnableResourceCaching(); - void DropResourceCaching(); + void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); + void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); private: bool res_cached_; @@ -174,8 +181,8 @@ class ApplyProfileAction : public ProfileAction { virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; - virtual void EnableResourceCaching(); - virtual void DropResourceCaching(); + virtual void EnableResourceCaching(ProfileAction::ResourceCacheType cache_type); + virtual void DropResourceCaching(ProfileAction::ResourceCacheType cache_type); private: std::vector> profiles_; @@ -188,8 +195,9 @@ class TaskProfiles { TaskProfile* GetProfile(const std::string& name) const; const ProfileAttribute* GetAttribute(const std::string& name) const; - void DropResourceCaching() const; - bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); + void DropResourceCaching(ProfileAction::ResourceCacheType cache_type) const; + 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); private: From c5b8f80b9b52303f825cc7426ffee209c794bda4 Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 20 Jan 2022 17:55:27 -0800 Subject: [PATCH 11/18] init.rc: Set permissions to cgroup.procs files Set permissions to cgroup.procs files in cgroup hierarchies similar to permissions for tasks files so that SetProcessProfiles can access them. Bug: 215557553 Signed-off-by: Suren Baghdasaryan Change-Id: Id0c82288392146c8d536d273790a0252580c4203 Merged-In: Id0c82288392146c8d536d273790a0252580c4203 --- rootdir/init.rc | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/rootdir/init.rc b/rootdir/init.rc index 5116c0fead79..8a385595e7a1 100644 --- a/rootdir/init.rc +++ b/rootdir/init.rc @@ -142,11 +142,21 @@ on init chown system system /dev/stune/background/tasks chown system system /dev/stune/top-app/tasks chown system system /dev/stune/rt/tasks + chown system system /dev/stune/cgroup.procs + chown system system /dev/stune/foreground/cgroup.procs + chown system system /dev/stune/background/cgroup.procs + chown system system /dev/stune/top-app/cgroup.procs + chown system system /dev/stune/rt/cgroup.procs chmod 0664 /dev/stune/tasks chmod 0664 /dev/stune/foreground/tasks chmod 0664 /dev/stune/background/tasks chmod 0664 /dev/stune/top-app/tasks chmod 0664 /dev/stune/rt/tasks + chmod 0664 /dev/stune/cgroup.procs + chmod 0664 /dev/stune/foreground/cgroup.procs + chmod 0664 /dev/stune/background/cgroup.procs + chmod 0664 /dev/stune/top-app/cgroup.procs + chmod 0664 /dev/stune/rt/cgroup.procs # cpuctl hierarchy for devices using utilclamp mkdir /dev/cpuctl/foreground @@ -172,6 +182,14 @@ on init chown system system /dev/cpuctl/system/tasks chown system system /dev/cpuctl/system-background/tasks chown system system /dev/cpuctl/dex2oat/tasks + chown system system /dev/cpuctl/cgroup.procs + chown system system /dev/cpuctl/foreground/cgroup.procs + chown system system /dev/cpuctl/background/cgroup.procs + chown system system /dev/cpuctl/top-app/cgroup.procs + chown system system /dev/cpuctl/rt/cgroup.procs + chown system system /dev/cpuctl/system/cgroup.procs + chown system system /dev/cpuctl/system-background/cgroup.procs + chown system system /dev/cpuctl/dex2oat/cgroup.procs chmod 0664 /dev/cpuctl/tasks chmod 0664 /dev/cpuctl/foreground/tasks chmod 0664 /dev/cpuctl/background/tasks @@ -180,12 +198,22 @@ on init chmod 0664 /dev/cpuctl/system/tasks chmod 0664 /dev/cpuctl/system-background/tasks chmod 0664 /dev/cpuctl/dex2oat/tasks + chmod 0664 /dev/cpuctl/cgroup.procs + chmod 0664 /dev/cpuctl/foreground/cgroup.procs + chmod 0664 /dev/cpuctl/background/cgroup.procs + chmod 0664 /dev/cpuctl/top-app/cgroup.procs + chmod 0664 /dev/cpuctl/rt/cgroup.procs + chmod 0664 /dev/cpuctl/system/cgroup.procs + chmod 0664 /dev/cpuctl/system-background/cgroup.procs + chmod 0664 /dev/cpuctl/dex2oat/cgroup.procs # Create a cpu group for NNAPI HAL processes mkdir /dev/cpuctl/nnapi-hal chown system system /dev/cpuctl/nnapi-hal chown system system /dev/cpuctl/nnapi-hal/tasks + chown system system /dev/cpuctl/nnapi-hal/cgroup.procs chmod 0664 /dev/cpuctl/nnapi-hal/tasks + chmod 0664 /dev/cpuctl/nnapi-hal/cgroup.procs write /dev/cpuctl/nnapi-hal/cpu.uclamp.min 1 write /dev/cpuctl/nnapi-hal/cpu.uclamp.latency_sensitive 1 @@ -193,19 +221,25 @@ on init mkdir /dev/cpuctl/camera-daemon chown system system /dev/cpuctl/camera-daemon chown system system /dev/cpuctl/camera-daemon/tasks + chown system system /dev/cpuctl/camera-daemon/cgroup.procs chmod 0664 /dev/cpuctl/camera-daemon/tasks + chmod 0664 /dev/cpuctl/camera-daemon/cgroup.procs # Create an stune group for camera-specific processes mkdir /dev/stune/camera-daemon chown system system /dev/stune/camera-daemon chown system system /dev/stune/camera-daemon/tasks + chown system system /dev/stune/camera-daemon/cgroup.procs chmod 0664 /dev/stune/camera-daemon/tasks + chmod 0664 /dev/stune/camera-daemon/cgroup.procs # Create an stune group for NNAPI HAL processes mkdir /dev/stune/nnapi-hal chown system system /dev/stune/nnapi-hal chown system system /dev/stune/nnapi-hal/tasks + chown system system /dev/stune/nnapi-hal/cgroup.procs chmod 0664 /dev/stune/nnapi-hal/tasks + chmod 0664 /dev/stune/nnapi-hal/cgroup.procs write /dev/stune/nnapi-hal/schedtune.boost 1 write /dev/stune/nnapi-hal/schedtune.prefer_idle 1 @@ -217,8 +251,12 @@ on init chown system system /dev/blkio/background chown system system /dev/blkio/tasks chown system system /dev/blkio/background/tasks + chown system system /dev/blkio/cgroup.procs + chown system system /dev/blkio/background/cgroup.procs chmod 0664 /dev/blkio/tasks chmod 0664 /dev/blkio/background/tasks + chmod 0664 /dev/blkio/cgroup.procs + chmod 0664 /dev/blkio/background/cgroup.procs write /dev/blkio/blkio.weight 1000 write /dev/blkio/background/blkio.weight 200 write /dev/blkio/background/blkio.bfq.weight 10 @@ -367,6 +405,13 @@ on init chown system system /dev/cpuset/top-app/tasks chown system system /dev/cpuset/restricted/tasks chown system system /dev/cpuset/camera-daemon/tasks + chown system system /dev/cpuset/cgroup.procs + chown system system /dev/cpuset/foreground/cgroup.procs + chown system system /dev/cpuset/background/cgroup.procs + chown system system /dev/cpuset/system-background/cgroup.procs + chown system system /dev/cpuset/top-app/cgroup.procs + chown system system /dev/cpuset/restricted/cgroup.procs + chown system system /dev/cpuset/camera-daemon/cgroup.procs # set system-background to 0775 so SurfaceFlinger can touch it chmod 0775 /dev/cpuset/system-background @@ -378,6 +423,13 @@ on init chmod 0664 /dev/cpuset/restricted/tasks chmod 0664 /dev/cpuset/tasks chmod 0664 /dev/cpuset/camera-daemon/tasks + chmod 0664 /dev/cpuset/foreground/cgroup.procs + chmod 0664 /dev/cpuset/background/cgroup.procs + chmod 0664 /dev/cpuset/system-background/cgroup.procs + chmod 0664 /dev/cpuset/top-app/cgroup.procs + chmod 0664 /dev/cpuset/restricted/cgroup.procs + chmod 0664 /dev/cpuset/cgroup.procs + chmod 0664 /dev/cpuset/camera-daemon/cgroup.procs # make the PSI monitor accessible to others chown system system /proc/pressure/memory From 65f3d09445699910947fd73b35979121b2bf07fb Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Tue, 25 Jan 2022 07:05:31 +0000 Subject: [PATCH 12/18] init: Wait for snapuserd before starting second stage This is a race between init process and bionic libc initialization of snapuserd. init->fork() ----------------> SecondStageMain() -> PropertyInit() | | v execveat ---> __libc_init_common() -> __system_properties_init() (snapuserd) When init process calls PropertyInit(), /dev/__properties__ directory is created. When bionic libc of snapuserd daemon invokes __system_properties_init _after_ init process PropertyInit() function is invoked, libc will try to initialize the property by reading /system/etc/selinux/plat_property_contexts. Since any reads on /system has to be served by snapuserd, this specific read from libc cannot be serviced leading to deadlock. Reproduce the race by inducing a sleep of 1500ms just before execveat() so that init process calls PropertyInit() before bionic libc initialization. This leads to deadlock immediately and with additional kernel instrumentation with debug logs confirms the failure: ====================================================== init: Relaunched snapuserd with pid: 428 ext4_file_open: SNAPUSERD: path /system/etc/selinux/plat_property_contexts - Pid: 428 comm 8 ext4_file_read_iter: SNAPUSERD for path: /system/etc/selinux/plat_property_contexts pid: 428 comm 8 [ 25.418043][ T428] ext4_file_read_iter+0x3dc/0x3e0 [ 25.423000][ T428] vfs_read+0x2e0/0x354 [ 25.426986][ T428] ksys_read+0x7c/0xec [ 25.430894][ T428] __arm64_sys_read+0x20/0x30 [ 25.435419][ T428] el0_svc_common.llvm.17612735770287389485+0xd0/0x1e0 [ 25.442095][ T428] do_el0_svc+0x28/0xa0 [ 25.446100][ T428] el0_svc+0x14/0x24 [ 25.449825][ T428] el0_sync_handler+0x88/0xec [ 25.454343][ T428] el0_sync+0x1c0/0x200 ===================================================== Fix: Before starting init second stage, we will wait for snapuserd daemon to be up and running. We do a simple probe by reading system partition. This read will eventually be serviced by daemon confirming that daemon is up and running. Furthermore, we are still in the kernel domain and sepolicy has not been enforced yet. Thus, access to these device mapper block devices are ok even though we may see audit logs. Note that daemon will re-initialize the __system_property_init() as part of WaitForSocket() call. This is subtle but important; since bionic libc initialized had failed silently, it is important that this re-initialization is done. Bug: 207298357 Test: Induce the failure by explicitly delaying the call of execveat(). With fix, no issues observed. Tested incremental OTA on pixel ~15 times. Ignore-AOSP-First: cherry-pick from AOSP Signed-off-by: Akilesh Kailash Change-Id: I86c2de977de052bfe9dcdc002dcbd9026601d0f3 --- init/snapuserd_transition.cpp | 58 +++++++++++++++++++++++++++++++++++ init/snapuserd_transition.h | 1 + 2 files changed, 59 insertions(+) diff --git a/init/snapuserd_transition.cpp b/init/snapuserd_transition.cpp index 40467b7d37bc..7fd3f65ca621 100644 --- a/init/snapuserd_transition.cpp +++ b/init/snapuserd_transition.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -227,6 +228,56 @@ void SnapuserdSelinuxHelper::FinishTransition() { } } +/* + * Before starting init second stage, we will wait + * for snapuserd daemon to be up and running; bionic libc + * may read /system/etc/selinux/plat_property_contexts file + * before invoking main() function. This will happen if + * init initializes property during second stage. Any access + * to /system without snapuserd daemon will lead to a deadlock. + * + * Thus, we do a simple probe by reading system partition. This + * read will eventually be serviced by daemon confirming that + * daemon is up and running. Furthermore, we are still in the kernel + * domain and sepolicy has not been enforced yet. Thus, access + * to these device mapper block devices are ok even though + * we may see audit logs. + */ +bool SnapuserdSelinuxHelper::TestSnapuserdIsReady() { + std::string dev = "/dev/block/mapper/system"s + fs_mgr_get_slot_suffix(); + android::base::unique_fd fd(open(dev.c_str(), O_RDONLY | O_DIRECT)); + if (fd < 0) { + PLOG(ERROR) << "open " << dev << " failed"; + return false; + } + + void* addr; + ssize_t page_size = getpagesize(); + if (posix_memalign(&addr, page_size, page_size) < 0) { + PLOG(ERROR) << "posix_memalign with page size " << page_size; + return false; + } + + std::unique_ptr buffer(addr, ::free); + + int iter = 0; + while (iter < 10) { + ssize_t n = TEMP_FAILURE_RETRY(pread(fd.get(), buffer.get(), page_size, 0)); + if (n < 0) { + // Wait for sometime before retry + std::this_thread::sleep_for(100ms); + } else if (n == page_size) { + return true; + } else { + LOG(ERROR) << "pread returned: " << n << " from: " << dev << " expected: " << page_size; + } + + iter += 1; + } + + return false; +} + void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { auto fd = GetRamdiskSnapuserdFd(); if (!fd) { @@ -248,6 +299,13 @@ void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { setenv(kSnapuserdFirstStagePidVar, std::to_string(pid).c_str(), 1); LOG(INFO) << "Relaunched snapuserd with pid: " << pid; + + if (!TestSnapuserdIsReady()) { + PLOG(FATAL) << "snapuserd daemon failed to launch"; + } else { + LOG(INFO) << "snapuserd daemon is up and running"; + } + return; } diff --git a/init/snapuserd_transition.h b/init/snapuserd_transition.h index a5ab652b7c2a..757af13776e4 100644 --- a/init/snapuserd_transition.h +++ b/init/snapuserd_transition.h @@ -51,6 +51,7 @@ class SnapuserdSelinuxHelper final { private: void RelaunchFirstStageSnapuserd(); void ExecSnapuserd(); + bool TestSnapuserdIsReady(); std::unique_ptr sm_; BlockDevInitializer block_dev_init_; From 2727e16517c243874739c26fb7cf2a8aa5d9871f Mon Sep 17 00:00:00 2001 From: Stephen Crane Date: Mon, 13 Dec 2021 16:41:17 -0800 Subject: [PATCH 13/18] storageproxyd: Use alternate data path if in DSU state Adds a check for a DSU mode boot in storageproxyd. Changes path handling so that storageproxyd will not allow opening a file in the root data path in DSU mode. Instead, storageproxyd creates an "alternate/" directory in the data directory and the TA must use this directory to store its backing file. Re-landing reverted change: Iad68872dc6915f64eaf26cd3c92c04d9071ef169 Test: Boot into DSU and inspect logs for "Cannot open root data file" Test: Test that TD writes in DSU mode don't corrupt host image storage when using a compatible storage TA that supports alternate data mode. Bug: 203719297 Merged-In: I1d07e7c3d15dc1beba2d340181d1b11a7988f869 Change-Id: I1d07e7c3d15dc1beba2d340181d1b11a7988f869 --- trusty/storage/proxy/Android.bp | 5 ++- trusty/storage/proxy/checkpoint_handling.cpp | 15 +++++++ trusty/storage/proxy/checkpoint_handling.h | 2 + trusty/storage/proxy/proxy.c | 7 ++- trusty/storage/proxy/storage.c | 46 +++++++++++++++++++- 5 files changed, 71 insertions(+), 4 deletions(-) diff --git a/trusty/storage/proxy/Android.bp b/trusty/storage/proxy/Android.bp index 38d868508cc2..94f26d8a669d 100644 --- a/trusty/storage/proxy/Android.bp +++ b/trusty/storage/proxy/Android.bp @@ -35,7 +35,10 @@ cc_binary { "liblog", "libhardware_legacy", ], - header_libs: ["libcutils_headers"], + header_libs: [ + "libcutils_headers", + "libgsi_headers", + ], static_libs: [ "libfstab", diff --git a/trusty/storage/proxy/checkpoint_handling.cpp b/trusty/storage/proxy/checkpoint_handling.cpp index 6c2fd363e2b7..3305d8da27b4 100644 --- a/trusty/storage/proxy/checkpoint_handling.cpp +++ b/trusty/storage/proxy/checkpoint_handling.cpp @@ -18,9 +18,12 @@ #include "log.h" #include +#include #include #include +#include + namespace { bool checkpointingDoneForever = false; @@ -75,3 +78,15 @@ int is_data_checkpoint_active(bool* active) { return 0; } + +/** + * is_gsi_running() - Check if a GSI image is running via DSU. + * + * This function is equivalent to android::gsi::IsGsiRunning(), but this API is + * not yet vendor-accessible although the underlying metadata file is. + * + */ +bool is_gsi_running() { + /* TODO(b/210501710): Expose GSI image running state to vendor storageproxyd */ + return !access(android::gsi::kGsiBootedIndicatorFile, F_OK); +} diff --git a/trusty/storage/proxy/checkpoint_handling.h b/trusty/storage/proxy/checkpoint_handling.h index f1bf27c8d442..dfe294778cde 100644 --- a/trusty/storage/proxy/checkpoint_handling.h +++ b/trusty/storage/proxy/checkpoint_handling.h @@ -32,6 +32,8 @@ extern "C" { */ int is_data_checkpoint_active(bool* active); +bool is_gsi_running(); + #ifdef __cplusplus } #endif diff --git a/trusty/storage/proxy/proxy.c b/trusty/storage/proxy/proxy.c index c690a287681f..262003427a5c 100644 --- a/trusty/storage/proxy/proxy.c +++ b/trusty/storage/proxy/proxy.c @@ -104,8 +104,11 @@ static int drop_privs(void) { return -1; } - /* no-execute for user, no access for group and other */ - umask(S_IXUSR | S_IRWXG | S_IRWXO); + /* + * No access for group and other. We need execute access for user to create + * an accessible directory. + */ + umask(S_IRWXG | S_IRWXO); return 0; } diff --git a/trusty/storage/proxy/storage.c b/trusty/storage/proxy/storage.c index 2fde30f844a4..d74a708070c8 100644 --- a/trusty/storage/proxy/storage.c +++ b/trusty/storage/proxy/storage.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -24,13 +25,16 @@ #include #include -#include "log.h" +#include "checkpoint_handling.h" #include "ipc.h" +#include "log.h" #include "storage.h" #define FD_TBL_SIZE 64 #define MAX_READ_SIZE 4096 +#define ALTERNATE_DATA_DIR "alternate/" + enum sync_state { SS_UNUSED = -1, SS_CLEAN = 0, @@ -44,6 +48,8 @@ static enum sync_state fs_state; static enum sync_state dir_state; static enum sync_state fd_state[FD_TBL_SIZE]; +static bool alternate_mode; + static struct { struct storage_file_read_resp hdr; uint8_t data[MAX_READ_SIZE]; @@ -216,6 +222,7 @@ int storage_file_open(struct storage_msg *msg, const void *r, size_t req_len) { char *path = NULL; + char* parent_path; const struct storage_file_open_req *req = r; struct storage_file_open_resp resp = {0}; @@ -234,6 +241,24 @@ int storage_file_open(struct storage_msg *msg, goto err_response; } + /* + * TODO(b/210501710): Expose GSI image running state to vendor + * storageproxyd. We want to control data file paths in vendor_init, but we + * don't have access to the necessary property there yet. When we have + * access to that property we can set the root data path read-only and only + * allow creation of files in alternate/. Checking paths here temporarily + * until that is fixed. + * + * We are just checking for "/" instead of "alternate/" because we still + * want to still allow access to "persist/" in alternate mode (for now, this + * may change in the future). + */ + if (alternate_mode && !strchr(req->name, '/')) { + ALOGE("%s: Cannot open root data file \"%s\" in alternate mode\n", __func__, req->name); + msg->result = STORAGE_ERR_ACCESS; + goto err_response; + } + int rc = asprintf(&path, "%s/%s", ssdir_name, req->name); if (rc < 0) { ALOGE("%s: asprintf failed\n", __func__); @@ -246,7 +271,23 @@ int storage_file_open(struct storage_msg *msg, if (req->flags & STORAGE_FILE_OPEN_TRUNCATE) open_flags |= O_TRUNC; + parent_path = dirname(path); if (req->flags & STORAGE_FILE_OPEN_CREATE) { + /* + * Create the alternate parent dir if needed & allowed. + * + * TODO(b/210501710): Expose GSI image running state to vendor + * storageproxyd. This directory should be created by vendor_init, once + * it has access to the necessary bit of information. + */ + if (strstr(req->name, ALTERNATE_DATA_DIR) == req->name) { + rc = mkdir(parent_path, S_IRWXU); + if (rc && errno != EEXIST) { + ALOGE("%s: Could not create parent directory \"%s\": %s\n", __func__, parent_path, + strerror(errno)); + } + } + /* open or create */ if (req->flags & STORAGE_FILE_OPEN_CREATE_EXCLUSIVE) { /* create exclusive */ @@ -467,6 +508,9 @@ int storage_file_set_size(struct storage_msg *msg, int storage_init(const char *dirname) { + /* If there is an active DSU image, use the alternate fs mode. */ + alternate_mode = is_gsi_running(); + fs_state = SS_CLEAN; dir_state = SS_CLEAN; for (uint i = 0; i < FD_TBL_SIZE; i++) { From ea98b60b86d7863ee121e4f8ab5fb1540289bf0e Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 2 Sep 2021 19:47:12 -0700 Subject: [PATCH 14/18] libprocessgroup: Prevent error spam when tests disable all cpus in a cpuset UserLifecycleTests test disables all Little cores in the course of the test, which causes attempts to add a process into /dev/cpuset/restricted cpuset cgroup to fail with ENOSPC error code, indicating that a process is joining a cpuset cgroup with no online cpus. Current libprocessgroup implementation will log an error on each such occurrence, which spams the logs and makes it hard to analyze test results. Because this situation does not happen in production environment (we do not offline cpus), we can prevent flooding the logs by identifying this case, logging an appropriate error one time and ignore all later similar errors. Bug: 158766131 Test: adb shell "echo 0 > /sys/devices/system/cpu/cpu[0-3]/online" Test: start some apps, observe libprocessgroup errors in the logcat Signed-off-by: Suren Baghdasaryan (cherry picked from commit ae42d601d09fa32b5c33758432cbd306b60f58ee) (cherry picked from commit 48e692cecd21059c9e94b9a399b96056f3b66289) Merged-In: Ia91d8839d86787569c255481bde077be51c43d93 Change-Id: Ia91d8839d86787569c255481bde077be51c43d93 --- libprocessgroup/task_profiles.cpp | 37 ++++++++++++++++++++++--------- libprocessgroup/task_profiles.h | 2 +- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index cf74e65572c3..e935f99de947 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -194,22 +194,39 @@ void SetCgroupAction::DropResourceCaching() { fd_.reset(FDS_NOT_CACHED); } -bool SetCgroupAction::AddTidToCgroup(int tid, int fd) { +bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { if (tid <= 0) { return true; } std::string value = std::to_string(tid); - if (TEMP_FAILURE_RETRY(write(fd, value.c_str(), value.length())) < 0) { - // If the thread is in the process of exiting, don't flag an error - if (errno != ESRCH) { - PLOG(ERROR) << "AddTidToCgroup failed to write '" << value << "'; fd=" << fd; - return false; + if (TEMP_FAILURE_RETRY(write(fd, value.c_str(), value.length())) == value.length()) { + return true; + } + + // If the thread is in the process of exiting, don't flag an error + if (errno == ESRCH) { + return true; + } + + // ENOSPC is returned when cpuset cgroup that we are joining has no online cpus + if (errno == ENOSPC && !strcmp(controller_name, "cpuset")) { + // This is an abnormal case happening only in testing, so report it only once + static bool empty_cpuset_reported = false; + + if (empty_cpuset_reported) { + return true; } + + LOG(ERROR) << "Failed to add task '" << value + << "' into cpuset because all cpus in that cpuset are offline"; + empty_cpuset_reported = true; + } else { + PLOG(ERROR) << "AddTidToCgroup failed to write '" << value << "'; fd=" << fd; } - return true; + return false; } bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { @@ -219,7 +236,7 @@ bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { PLOG(WARNING) << "Failed to open " << procs_path; return false; } - if (!AddTidToCgroup(pid, tmp_fd)) { + if (!AddTidToCgroup(pid, tmp_fd, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } @@ -231,7 +248,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { std::lock_guard lock(fd_mutex_); if (IsFdValid()) { // fd is cached, reuse it - if (!AddTidToCgroup(tid, fd_)) { + if (!AddTidToCgroup(tid, fd_, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } @@ -256,7 +273,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { PLOG(WARNING) << "Failed to open " << tasks_path << ": " << strerror(errno); return false; } - if (!AddTidToCgroup(tid, tmp_fd)) { + if (!AddTidToCgroup(tid, tmp_fd, controller()->name())) { LOG(ERROR) << "Failed to add task into cgroup"; return false; } diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 25a84b0c1b84..97c38f4f39c4 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -134,7 +134,7 @@ class SetCgroupAction : public ProfileAction { mutable std::mutex fd_mutex_; static bool IsAppDependentPath(const std::string& path); - static bool AddTidToCgroup(int tid, int fd); + static bool AddTidToCgroup(int tid, int fd, const char* controller_name); bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; } }; From 1458350370563adb9352ae06442974ac88b7e209 Mon Sep 17 00:00:00 2001 From: Rick Yiu Date: Sun, 21 Nov 2021 15:57:36 +0800 Subject: [PATCH 15/18] libprocessgroup: Use WriteStringToFd for WriteFileAction Using WriteStringToFile will hold kernfs_mutex which is a big lock, so use WriteStringToFd instead. Besides, also support fd cache for it. Bug: 206970384 Test: build pass (cherry picked from commit e808841d72816c68da3d6d9f451aba71bf4188bc) (cherry picked from commit b8d7ac60bdeca738346b61a1b84be34f0072af2f) Merged-In: Id79f9e1095f52079393c58edb9a4d526f4cc6b5e Change-Id: Id79f9e1095f52079393c58edb9a4d526f4cc6b5e --- libprocessgroup/task_profiles.cpp | 109 ++++++++++++++++++++---------- libprocessgroup/task_profiles.h | 53 ++++++++++----- 2 files changed, 109 insertions(+), 53 deletions(-) diff --git a/libprocessgroup/task_profiles.cpp b/libprocessgroup/task_profiles.cpp index e935f99de947..3834f9194beb 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -144,30 +144,13 @@ bool SetAttributeAction::ExecuteForTask(int tid) const { return true; } -bool SetCgroupAction::IsAppDependentPath(const std::string& path) { - return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; -} - -SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) - : controller_(c), path_(p) { - // file descriptors for app-dependent paths can't be cached - if (IsAppDependentPath(path_)) { - // file descriptor is not cached - fd_.reset(FDS_APP_DEPENDENT); - return; - } - - // file descriptor can be cached later on request - fd_.reset(FDS_NOT_CACHED); -} - -void SetCgroupAction::EnableResourceCaching() { +void CachedFdProfileAction::EnableResourceCaching() { std::lock_guard lock(fd_mutex_); if (fd_ != FDS_NOT_CACHED) { return; } - std::string tasks_path = controller_.GetTasksFilePath(path_); + std::string tasks_path = GetPath(); if (access(tasks_path.c_str(), W_OK) != 0) { // file is not accessible @@ -185,7 +168,7 @@ void SetCgroupAction::EnableResourceCaching() { fd_ = std::move(fd); } -void SetCgroupAction::DropResourceCaching() { +void CachedFdProfileAction::DropResourceCaching() { std::lock_guard lock(fd_mutex_); if (fd_ == FDS_NOT_CACHED) { return; @@ -194,6 +177,26 @@ void SetCgroupAction::DropResourceCaching() { fd_.reset(FDS_NOT_CACHED); } +bool CachedFdProfileAction::IsAppDependentPath(const std::string& path) { + return path.find("", 0) != std::string::npos || path.find("", 0) != std::string::npos; +} + +void CachedFdProfileAction::InitFd(const std::string& path) { + // file descriptors for app-dependent paths can't be cached + if (IsAppDependentPath(path)) { + // file descriptor is not cached + fd_.reset(FDS_APP_DEPENDENT); + return; + } + // file descriptor can be cached later on request + fd_.reset(FDS_NOT_CACHED); +} + +SetCgroupAction::SetCgroupAction(const CgroupController& c, const std::string& p) + : controller_(c), path_(p) { + InitFd(controller_.GetTasksFilePath(path_)); +} + bool SetCgroupAction::AddTidToCgroup(int tid, int fd, const char* controller_name) { if (tid <= 0) { return true; @@ -270,7 +273,7 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { std::string tasks_path = controller()->GetTasksFilePath(path_); unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(tasks_path.c_str(), O_WRONLY | O_CLOEXEC))); if (tmp_fd < 0) { - PLOG(WARNING) << "Failed to open " << tasks_path << ": " << strerror(errno); + PLOG(WARNING) << "Failed to open " << tasks_path; return false; } if (!AddTidToCgroup(tid, tmp_fd, controller()->name())) { @@ -281,37 +284,73 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { return true; } -bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { - std::string filepath(filepath_), value(value_); +WriteFileAction::WriteFileAction(const std::string& path, const std::string& value, + bool logfailures) + : path_(path), value_(value), logfailures_(logfailures) { + InitFd(path_); +} - filepath = StringReplace(filepath, "", std::to_string(uid), true); - filepath = StringReplace(filepath, "", std::to_string(pid), true); - value = StringReplace(value, "", std::to_string(uid), true); - value = StringReplace(value, "", std::to_string(pid), true); +bool WriteFileAction::WriteValueToFile(const std::string& value, const std::string& path, + bool logfailures) { + // Use WriteStringToFd instead of WriteStringToFile because the latter will open file with + // O_TRUNC which causes kernfs_mutex contention + unique_fd tmp_fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_WRONLY | O_CLOEXEC))); + + if (tmp_fd < 0) { + if (logfailures) PLOG(WARNING) << "Failed to open " << path; + return false; + } - if (!WriteStringToFile(value, filepath)) { - if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << filepath; + if (!WriteStringToFd(value, tmp_fd)) { + if (logfailures) PLOG(ERROR) << "Failed to write '" << value << "' to " << path; return false; } return true; } +bool WriteFileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { + std::lock_guard lock(fd_mutex_); + std::string value(value_); + std::string path(path_); + + value = StringReplace(value, "", std::to_string(uid), true); + value = StringReplace(value, "", std::to_string(pid), true); + path = StringReplace(path, "", std::to_string(uid), true); + path = StringReplace(path, "", std::to_string(pid), true); + + return WriteValueToFile(value, path, logfailures_); +} + bool WriteFileAction::ExecuteForTask(int tid) const { - std::string filepath(filepath_), value(value_); + std::lock_guard lock(fd_mutex_); + std::string value(value_); int uid = getuid(); - filepath = StringReplace(filepath, "", std::to_string(uid), true); - filepath = StringReplace(filepath, "", std::to_string(tid), true); value = StringReplace(value, "", std::to_string(uid), true); value = StringReplace(value, "", std::to_string(tid), true); - if (!WriteStringToFile(value, filepath)) { - if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << filepath; + if (IsFdValid()) { + // fd is cached, reuse it + if (!WriteStringToFd(value, fd_)) { + if (logfailures_) PLOG(ERROR) << "Failed to write '" << value << "' to " << path_; + return false; + } + return true; + } + + if (fd_ == FDS_INACCESSIBLE) { + // no permissions to access the file, ignore + return true; + } + + if (fd_ == FDS_APP_DEPENDENT) { + // application-dependent path can't be used with tid + PLOG(ERROR) << "Application profile can't be applied to a thread"; return false; } - return true; + return WriteValueToFile(value, path_, logfailures_); } bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index 97c38f4f39c4..278892dd55f6 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -108,50 +108,67 @@ class SetAttributeAction : public ProfileAction { std::string value_; }; -// Set cgroup profile element -class SetCgroupAction : public ProfileAction { +// Abstract profile element for cached fd +class CachedFdProfileAction : public ProfileAction { public: - SetCgroupAction(const CgroupController& c, const std::string& p); - - virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; - virtual bool ExecuteForTask(int tid) const; virtual void EnableResourceCaching(); virtual void DropResourceCaching(); - const CgroupController* controller() const { return &controller_; } - std::string path() const { return path_; } - - private: + protected: enum FdState { FDS_INACCESSIBLE = -1, FDS_APP_DEPENDENT = -2, FDS_NOT_CACHED = -3, }; - CgroupController controller_; - std::string path_; android::base::unique_fd fd_; mutable std::mutex fd_mutex_; static bool IsAppDependentPath(const std::string& path); - static bool AddTidToCgroup(int tid, int fd, const char* controller_name); + void InitFd(const std::string& path); bool IsFdValid() const { return fd_ > FDS_INACCESSIBLE; } + + virtual const std::string GetPath() const = 0; +}; + +// Set cgroup profile element +class SetCgroupAction : public CachedFdProfileAction { + public: + SetCgroupAction(const CgroupController& c, const std::string& p); + + virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; + virtual bool ExecuteForTask(int tid) const; + + const CgroupController* controller() const { return &controller_; } + + protected: + const std::string GetPath() const override { return controller_.GetTasksFilePath(path_); } + + private: + CgroupController controller_; + std::string path_; + + static bool AddTidToCgroup(int tid, int fd, const char* controller_name); }; // Write to file action -class WriteFileAction : public ProfileAction { +class WriteFileAction : public CachedFdProfileAction { public: - WriteFileAction(const std::string& filepath, const std::string& value, - bool logfailures) noexcept - : filepath_(filepath), value_(value), logfailures_(logfailures) {} + WriteFileAction(const std::string& path, const std::string& value, bool logfailures); virtual bool ExecuteForProcess(uid_t uid, pid_t pid) const; virtual bool ExecuteForTask(int tid) const; + protected: + const std::string GetPath() const override { return path_; } + private: - std::string filepath_, value_; + std::string path_, value_; bool logfailures_; + + static bool WriteValueToFile(const std::string& value, const std::string& path, + bool logfailures); }; class TaskProfile { From 36a78b8e7575ecac72a6c43083403cfbf763ee02 Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Tue, 25 Jan 2022 07:05:31 +0000 Subject: [PATCH 16/18] init: Wait for snapuserd before starting second stage This is a race between init process and bionic libc initialization of snapuserd. init->fork() ----------------> SecondStageMain() -> PropertyInit() | | v execveat ---> __libc_init_common() -> __system_properties_init() (snapuserd) When init process calls PropertyInit(), /dev/__properties__ directory is created. When bionic libc of snapuserd daemon invokes __system_properties_init _after_ init process PropertyInit() function is invoked, libc will try to initialize the property by reading /system/etc/selinux/plat_property_contexts. Since any reads on /system has to be served by snapuserd, this specific read from libc cannot be serviced leading to deadlock. Reproduce the race by inducing a sleep of 1500ms just before execveat() so that init process calls PropertyInit() before bionic libc initialization. This leads to deadlock immediately and with additional kernel instrumentation with debug logs confirms the failure: ====================================================== init: Relaunched snapuserd with pid: 428 ext4_file_open: SNAPUSERD: path /system/etc/selinux/plat_property_contexts - Pid: 428 comm 8 ext4_file_read_iter: SNAPUSERD for path: /system/etc/selinux/plat_property_contexts pid: 428 comm 8 [ 25.418043][ T428] ext4_file_read_iter+0x3dc/0x3e0 [ 25.423000][ T428] vfs_read+0x2e0/0x354 [ 25.426986][ T428] ksys_read+0x7c/0xec [ 25.430894][ T428] __arm64_sys_read+0x20/0x30 [ 25.435419][ T428] el0_svc_common.llvm.17612735770287389485+0xd0/0x1e0 [ 25.442095][ T428] do_el0_svc+0x28/0xa0 [ 25.446100][ T428] el0_svc+0x14/0x24 [ 25.449825][ T428] el0_sync_handler+0x88/0xec [ 25.454343][ T428] el0_sync+0x1c0/0x200 ===================================================== Fix: Before starting init second stage, we will wait for snapuserd daemon to be up and running. We do a simple probe by reading system partition. This read will eventually be serviced by daemon confirming that daemon is up and running. Furthermore, we are still in the kernel domain and sepolicy has not been enforced yet. Thus, access to these device mapper block devices are ok even though we may see audit logs. Note that daemon will re-initialize the __system_property_init() as part of WaitForSocket() call. This is subtle but important; since bionic libc initialized had failed silently, it is important that this re-initialization is done. Bug: 207298357 Test: Induce the failure by explicitly delaying the call of execveat(). With fix, no issues observed. Tested incremental OTA on pixel ~15 times. Ignore-AOSP-First: cherry-pick from AOSP Signed-off-by: Akilesh Kailash (cherry picked from commit 65f3d09445699910947fd73b35979121b2bf07fb) (cherry picked from commit 7043054dfee9e29cd9a77903d7d63ed2a07e1707) Merged-In: I86c2de977de052bfe9dcdc002dcbd9026601d0f3 Change-Id: I86c2de977de052bfe9dcdc002dcbd9026601d0f3 --- init/snapuserd_transition.cpp | 58 +++++++++++++++++++++++++++++++++++ init/snapuserd_transition.h | 1 + 2 files changed, 59 insertions(+) diff --git a/init/snapuserd_transition.cpp b/init/snapuserd_transition.cpp index 40467b7d37bc..7fd3f65ca621 100644 --- a/init/snapuserd_transition.cpp +++ b/init/snapuserd_transition.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -227,6 +228,56 @@ void SnapuserdSelinuxHelper::FinishTransition() { } } +/* + * Before starting init second stage, we will wait + * for snapuserd daemon to be up and running; bionic libc + * may read /system/etc/selinux/plat_property_contexts file + * before invoking main() function. This will happen if + * init initializes property during second stage. Any access + * to /system without snapuserd daemon will lead to a deadlock. + * + * Thus, we do a simple probe by reading system partition. This + * read will eventually be serviced by daemon confirming that + * daemon is up and running. Furthermore, we are still in the kernel + * domain and sepolicy has not been enforced yet. Thus, access + * to these device mapper block devices are ok even though + * we may see audit logs. + */ +bool SnapuserdSelinuxHelper::TestSnapuserdIsReady() { + std::string dev = "/dev/block/mapper/system"s + fs_mgr_get_slot_suffix(); + android::base::unique_fd fd(open(dev.c_str(), O_RDONLY | O_DIRECT)); + if (fd < 0) { + PLOG(ERROR) << "open " << dev << " failed"; + return false; + } + + void* addr; + ssize_t page_size = getpagesize(); + if (posix_memalign(&addr, page_size, page_size) < 0) { + PLOG(ERROR) << "posix_memalign with page size " << page_size; + return false; + } + + std::unique_ptr buffer(addr, ::free); + + int iter = 0; + while (iter < 10) { + ssize_t n = TEMP_FAILURE_RETRY(pread(fd.get(), buffer.get(), page_size, 0)); + if (n < 0) { + // Wait for sometime before retry + std::this_thread::sleep_for(100ms); + } else if (n == page_size) { + return true; + } else { + LOG(ERROR) << "pread returned: " << n << " from: " << dev << " expected: " << page_size; + } + + iter += 1; + } + + return false; +} + void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { auto fd = GetRamdiskSnapuserdFd(); if (!fd) { @@ -248,6 +299,13 @@ void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { setenv(kSnapuserdFirstStagePidVar, std::to_string(pid).c_str(), 1); LOG(INFO) << "Relaunched snapuserd with pid: " << pid; + + if (!TestSnapuserdIsReady()) { + PLOG(FATAL) << "snapuserd daemon failed to launch"; + } else { + LOG(INFO) << "snapuserd daemon is up and running"; + } + return; } diff --git a/init/snapuserd_transition.h b/init/snapuserd_transition.h index a5ab652b7c2a..757af13776e4 100644 --- a/init/snapuserd_transition.h +++ b/init/snapuserd_transition.h @@ -51,6 +51,7 @@ class SnapuserdSelinuxHelper final { private: void RelaunchFirstStageSnapuserd(); void ExecSnapuserd(); + bool TestSnapuserdIsReady(); std::unique_ptr sm_; BlockDevInitializer block_dev_init_; From c3483e4c8a302e7852e0a334ffa90089337520ec Mon Sep 17 00:00:00 2001 From: Shaju Mathew Date: Tue, 5 Apr 2022 14:55:10 +0000 Subject: [PATCH 17/18] Backport of Win-specific suppression of potentially rogue construct that can engage in directory traversal on the host. Bug:209438553 Ignore-AOSP-First: Resolution for potential security exploit. Test: Cursory test with adb. Change-Id: Id47c567ad92ae4d9d7325a7a8589825a2ff4232b Merged-In: Ie1f82db2fb14e1bdd183bf8d3d93d5e9f974be5d --- adb/client/file_sync_client.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/adb/client/file_sync_client.cpp b/adb/client/file_sync_client.cpp index e686973db4ea..3374812d51ef 100644 --- a/adb/client/file_sync_client.cpp +++ b/adb/client/file_sync_client.cpp @@ -477,6 +477,17 @@ class SyncConnection { if (!ReadFdExactly(fd, buf, len)) return false; buf[len] = 0; + // Address the unlikely scenario wherein a + // compromised device/service might be able to + // traverse across directories on the host. Let's + // shut that door! + if (strchr(buf, '/') +#if defined(_WIN32) + || strchr(buf, '\\') +#endif + ) { + return false; + } callback(dent.mode, dent.size, dent.mtime, buf); } } From 505eeb7699173d3a80b47c346bfdb052b023edcd Mon Sep 17 00:00:00 2001 From: Shaju Mathew Date: Tue, 5 Apr 2022 14:55:10 +0000 Subject: [PATCH 18/18] Backport of Win-specific suppression of potentially rogue construct that can engage in directory traversal on the host. Bug:209438553 Ignore-AOSP-First: Resolution for potential security exploit. Test: Cursory test with adb. Change-Id: Id47c567ad92ae4d9d7325a7a8589825a2ff4232b Merged-In: Ie1f82db2fb14e1bdd183bf8d3d93d5e9f974be5d --- adb/client/file_sync_client.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/adb/client/file_sync_client.cpp b/adb/client/file_sync_client.cpp index e686973db4ea..3374812d51ef 100644 --- a/adb/client/file_sync_client.cpp +++ b/adb/client/file_sync_client.cpp @@ -477,6 +477,17 @@ class SyncConnection { if (!ReadFdExactly(fd, buf, len)) return false; buf[len] = 0; + // Address the unlikely scenario wherein a + // compromised device/service might be able to + // traverse across directories on the host. Let's + // shut that door! + if (strchr(buf, '/') +#if defined(_WIN32) + || strchr(buf, '\\') +#endif + ) { + return false; + } callback(dent.mode, dent.size, dent.mtime, buf); } }