From fe8154175c411b0dba6bbc881cf9dfe501be74c5 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 23 Apr 2019 15:11:07 -0700 Subject: [PATCH] init: simplify async restorecon In the future, property service may run in its own thread or process, which means that PropertyChildReap() needs to be refactored to not run as part of the init signal handler. The new method spawns a new thread that handles the queue of paths that require restorecon. It then communicates back to property service via android::base::SetProperty(). Property service distinguishes the thread from other callers of SetProperty() by checking the pid in the credentials for the socket connection, thus avoiding dependencies on the rest of init. The new method also drops the genericness, since restorecon is the only function that we should ever need to run asynchronously Test: async restorecon works, including with queued requests Change-Id: I2ca00459969e77b1820776dac23d0a0d974e330b --- init/property_service.cpp | 125 +++++++++++++++----------------------- init/property_service.h | 2 - init/sigchld_handler.cpp | 5 +- 3 files changed, 50 insertions(+), 82 deletions(-) diff --git a/init/property_service.cpp b/init/property_service.cpp index bf3b3172d..81adbe4a3 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -41,7 +41,9 @@ #include #include +#include #include +#include #include #include @@ -83,6 +85,8 @@ using android::properties::PropertyInfoEntry; namespace android { namespace init { +static constexpr const char kRestoreconProperty[] = "selinux.restorecon_recursive"; + static bool persistent_properties_loaded = false; static int property_set_fd = -1; @@ -187,88 +191,51 @@ static uint32_t PropertySet(const std::string& name, const std::string& value, s return PROP_SUCCESS; } -typedef int (*PropertyAsyncFunc)(const std::string&, const std::string&); +class AsyncRestorecon { + public: + void TriggerRestorecon(const std::string& path) { + auto guard = std::lock_guard{mutex_}; + paths_.emplace(path); -struct PropertyChildInfo { - pid_t pid; - PropertyAsyncFunc func; - std::string name; - std::string value; + if (!thread_started_) { + thread_started_ = true; + std::thread{&AsyncRestorecon::ThreadFunction, this}.detach(); + } + } + + private: + void ThreadFunction() { + auto lock = std::unique_lock{mutex_}; + + while (!paths_.empty()) { + auto path = paths_.front(); + paths_.pop(); + + lock.unlock(); + if (selinux_android_restorecon(path.c_str(), SELINUX_ANDROID_RESTORECON_RECURSE) != 0) { + LOG(ERROR) << "Asynchronous restorecon of '" << path << "' failed'"; + } + android::base::SetProperty(kRestoreconProperty, path); + lock.lock(); + } + + thread_started_ = false; + } + + std::mutex mutex_; + std::queue paths_; + bool thread_started_ = false; }; -static std::queue property_children; - -static void PropertyChildLaunch() { - auto& info = property_children.front(); - pid_t pid = fork(); - if (pid < 0) { - LOG(ERROR) << "Failed to fork for property_set_async"; - while (!property_children.empty()) { - property_children.pop(); - } - return; - } - if (pid != 0) { - info.pid = pid; - } else { - if (info.func(info.name, info.value) != 0) { - LOG(ERROR) << "property_set_async(\"" << info.name << "\", \"" << info.value - << "\") failed"; - } - _exit(0); - } -} - -bool PropertyChildReap(pid_t pid) { - if (property_children.empty()) { - return false; - } - auto& info = property_children.front(); - if (info.pid != pid) { - return false; - } - std::string error; - if (PropertySet(info.name, info.value, &error) != PROP_SUCCESS) { - LOG(ERROR) << "Failed to set async property " << info.name << " to " << info.value << ": " - << error; - } - property_children.pop(); - if (!property_children.empty()) { - PropertyChildLaunch(); - } - return true; -} - -static uint32_t PropertySetAsync(const std::string& name, const std::string& value, - PropertyAsyncFunc func, std::string* error) { - if (value.empty()) { - return PropertySet(name, value, error); - } - - PropertyChildInfo info; - info.func = func; - info.name = name; - info.value = value; - property_children.push(info); - if (property_children.size() == 1) { - PropertyChildLaunch(); - } - return PROP_SUCCESS; -} - -static int RestoreconRecursiveAsync(const std::string& name, const std::string& value) { - return selinux_android_restorecon(value.c_str(), SELINUX_ANDROID_RESTORECON_RECURSE); -} - uint32_t InitPropertySet(const std::string& name, const std::string& value) { if (StartsWith(name, "ctl.")) { LOG(ERROR) << "InitPropertySet: Do not set ctl. properties from init; call the Service " "functions directly"; return PROP_ERROR_INVALID_NAME; } - if (name == "selinux.restorecon_recursive") { - LOG(ERROR) << "InitPropertySet: Do not set selinux.restorecon_recursive from init; use the " - "restorecon builtin directly"; + if (name == kRestoreconProperty) { + LOG(ERROR) << "InitPropertySet: Do not set '" << kRestoreconProperty + << "' from init; use the restorecon builtin directly"; return PROP_ERROR_INVALID_NAME; } @@ -506,8 +473,14 @@ uint32_t HandlePropertySet(const std::string& name, const std::string& value, << process_log_string; } - if (name == "selinux.restorecon_recursive") { - return PropertySetAsync(name, value, RestoreconRecursiveAsync, error); + // If a process other than init is writing a non-empty value, it means that process is + // requesting that init performs a restorecon operation on the path specified by 'value'. + // We use a thread to do this restorecon operation to prevent holding up init, as it may take + // a long time to complete. + if (name == kRestoreconProperty && cr.pid != 1 && !value.empty()) { + static AsyncRestorecon async_restorecon; + async_restorecon.TriggerRestorecon(value); + return PROP_SUCCESS; } return PropertySet(name, value, error); @@ -661,7 +634,7 @@ static void LoadProperties(char* data, const char* filter, const char* filename, } if (StartsWith(key, "ctl.") || key == "sys.powerctl"s || - key == "selinux.restorecon_recursive"s) { + std::string{key} == kRestoreconProperty) { LOG(ERROR) << "Ignoring disallowed property '" << key << "' with special meaning in prop file '" << filename << "'"; continue; diff --git a/init/property_service.h b/init/property_service.h index 207c03bb9..7f9f84422 100644 --- a/init/property_service.h +++ b/init/property_service.h @@ -32,8 +32,6 @@ extern uint32_t (*property_set)(const std::string& name, const std::string& valu uint32_t HandlePropertySet(const std::string& name, const std::string& value, const std::string& source_context, const ucred& cr, std::string* error); -extern bool PropertyChildReap(pid_t pid); - void property_init(); void property_load_boot_defaults(bool load_debug_prop); void load_persist_props(); diff --git a/init/sigchld_handler.cpp b/init/sigchld_handler.cpp index 0b0332429..987b2f98c 100644 --- a/init/sigchld_handler.cpp +++ b/init/sigchld_handler.cpp @@ -29,7 +29,6 @@ #include #include "init.h" -#include "property_service.h" #include "service.h" using android::base::StringPrintf; @@ -61,9 +60,7 @@ static bool ReapOneProcess() { std::string wait_string; Service* service = nullptr; - if (PropertyChildReap(pid)) { - name = "Async property child"; - } else if (SubcontextChildReap(pid)) { + if (SubcontextChildReap(pid)) { name = "Subcontext"; } else { service = ServiceList::GetInstance().FindService(pid, &Service::pid);