From 911109c4149a2fca140cdf471d15deb38ebdcdcb Mon Sep 17 00:00:00 2001 From: Suren Baghdasaryan Date: Thu, 13 Feb 2020 17:28:00 -0800 Subject: [PATCH] libprocessgroup: Prevent SetProcessProfiles from using cached fd Because we cache file descriptors associated with cgroup "tasks" file it should not be used with SetProcessProfiles API which operates on entire processes rather than tasks. Change SetProcessProfiles API to prevent cache fd usage, modify ExecuteForProcess to not attempt to use cached fd. Also fix unconditional calls to EnableResourceCaching from ExecuteForTask which should be called only when SetTaskProfiles is used with use_fd_cache set to true. Bug: 149524788 Change-Id: I880efaf8217a4dd7ccfbb4fb167b2295cefc057a Signed-off-by: Suren Baghdasaryan --- .../include/processgroup/processgroup.h | 3 +- libprocessgroup/processgroup.cpp | 5 ++- libprocessgroup/task_profiles.cpp | 35 +++++++------------ libprocessgroup/task_profiles.h | 5 +-- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index 0b38b6bce..4aa439a6f 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -30,8 +30,7 @@ bool CgroupGetAttributePath(const std::string& attr_name, std::string* path); bool CgroupGetAttributePathForTask(const std::string& attr_name, int tid, std::string* path); bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache = false); -bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, - bool use_fd_cache = false); +bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); #ifndef __ANDROID_VNDK__ diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index 6272664bb..d669ebe7f 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -115,9 +115,8 @@ void DropTaskProfilesResourceCaching() { TaskProfiles::GetInstance().DropResourceCaching(); } -bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles, - bool use_fd_cache) { - return TaskProfiles::GetInstance().SetProcessProfiles(uid, pid, profiles, use_fd_cache); +bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles) { + return TaskProfiles::GetInstance().SetProcessProfiles(uid, pid, profiles); } 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 72f01af67..4af4589da 100644 --- a/libprocessgroup/task_profiles.cpp +++ b/libprocessgroup/task_profiles.cpp @@ -201,22 +201,6 @@ bool SetCgroupAction::AddTidToCgroup(int tid, int fd) { } bool SetCgroupAction::ExecuteForProcess(uid_t uid, pid_t pid) const { - std::lock_guard lock(fd_mutex_); - if (IsFdValid()) { - // fd is cached, reuse it - if (!AddTidToCgroup(pid, fd_)) { - LOG(ERROR) << "Failed to add task into cgroup"; - return false; - } - return true; - } - - if (fd_ == FDS_INACCESSIBLE) { - // no permissions to access the file, ignore - return true; - } - - // this is app-dependent path and fd is 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) { @@ -270,7 +254,6 @@ bool SetCgroupAction::ExecuteForTask(int tid) const { bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { for (const auto& profile : profiles_) { - profile->EnableResourceCaching(); if (!profile->ExecuteForProcess(uid, pid)) { PLOG(WARNING) << "ExecuteForProcess failed for aggregate profile"; } @@ -280,7 +263,6 @@ bool ApplyProfileAction::ExecuteForProcess(uid_t uid, pid_t pid) const { bool ApplyProfileAction::ExecuteForTask(int tid) const { for (const auto& profile : profiles_) { - profile->EnableResourceCaching(); if (!profile->ExecuteForTask(tid)) { PLOG(WARNING) << "ExecuteForTask failed for aggregate profile"; } @@ -288,6 +270,18 @@ bool ApplyProfileAction::ExecuteForTask(int tid) const { return true; } +void ApplyProfileAction::EnableResourceCaching() { + for (const auto& profile : profiles_) { + profile->EnableResourceCaching(); + } +} + +void ApplyProfileAction::DropResourceCaching() { + for (const auto& profile : profiles_) { + profile->DropResourceCaching(); + } +} + void TaskProfile::MoveTo(TaskProfile* profile) { profile->elements_ = std::move(elements_); profile->res_cached_ = res_cached_; @@ -527,13 +521,10 @@ const ProfileAttribute* TaskProfiles::GetAttribute(const std::string& name) cons } bool TaskProfiles::SetProcessProfiles(uid_t uid, pid_t pid, - const std::vector& profiles, bool use_fd_cache) { + const std::vector& profiles) { for (const auto& name : profiles) { TaskProfile* profile = GetProfile(name); if (profile != nullptr) { - if (use_fd_cache) { - profile->EnableResourceCaching(); - } if (!profile->ExecuteForProcess(uid, pid)) { PLOG(WARNING) << "Failed to apply " << name << " process profile"; } diff --git a/libprocessgroup/task_profiles.h b/libprocessgroup/task_profiles.h index a64ca50ad..28bc00c10 100644 --- a/libprocessgroup/task_profiles.h +++ b/libprocessgroup/task_profiles.h @@ -163,6 +163,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(); private: std::vector> profiles_; @@ -176,8 +178,7 @@ 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, - bool use_fd_cache); + bool SetProcessProfiles(uid_t uid, pid_t pid, const std::vector& profiles); bool SetTaskProfiles(int tid, const std::vector& profiles, bool use_fd_cache); private: