From 0188274148f1462bfdc1c95457a44e2458fffa5a Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 10 Mar 2020 09:08:02 -0700 Subject: [PATCH 1/2] Revert "init: handle property service callbacks asynchronously" This is apparently causing problems with reboot. This reverts commit d2dab830d3ec260ccd982d147d4eb0ef74071261. Bug: 150863651 Test: build Merged-In: Ib8a4835cdc8358a54c7acdebc5c95038963a0419 Change-Id: Ib8a4835cdc8358a54c7acdebc5c95038963a0419 --- init/Android.bp | 2 - init/action_manager.cpp | 22 +-- init/action_manager.h | 7 +- init/builtins.cpp | 15 -- init/init.cpp | 279 ++++++++++++++---------------------- init/init.h | 3 - init/init_test.cpp | 1 - init/lmkd_service.cpp | 3 +- init/mount_namespace.cpp | 11 -- init/property_service.cpp | 202 +++++++------------------- init/property_service.h | 2 - init/property_service.proto | 1 - init/reboot.cpp | 8 +- init/service.h | 14 +- init/service_list.h | 30 ++-- init/service_lock.cpp | 25 ---- init/service_lock.h | 40 ------ init/service_parser.cpp | 2 - init/sigchld_handler.cpp | 2 - 19 files changed, 191 insertions(+), 478 deletions(-) delete mode 100644 init/service_lock.cpp delete mode 100644 init/service_lock.h diff --git a/init/Android.bp b/init/Android.bp index 52628f372..3bb08db6c 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -28,7 +28,6 @@ init_common_sources = [ "rlimit_parser.cpp", "service.cpp", "service_list.cpp", - "service_lock.cpp", "service_parser.cpp", "service_utils.cpp", "subcontext.cpp", @@ -82,7 +81,6 @@ cc_defaults { "-Wextra", "-Wno-unused-parameter", "-Werror", - "-Wthread-safety", "-DALLOW_FIRST_STAGE_CONSOLE=0", "-DALLOW_LOCAL_PROP_OVERRIDE=0", "-DALLOW_PERMISSIVE_SELINUX=0", diff --git a/init/action_manager.cpp b/init/action_manager.cpp index b45f5cd18..ebca762ca 100644 --- a/init/action_manager.cpp +++ b/init/action_manager.cpp @@ -41,12 +41,10 @@ void ActionManager::AddAction(std::unique_ptr action) { } void ActionManager::QueueEventTrigger(const std::string& trigger) { - auto lock = std::lock_guard{event_queue_lock_}; event_queue_.emplace(trigger); } void ActionManager::QueuePropertyChange(const std::string& name, const std::string& value) { - auto lock = std::lock_guard{event_queue_lock_}; event_queue_.emplace(std::make_pair(name, value)); } @@ -55,7 +53,6 @@ void ActionManager::QueueAllPropertyActions() { } void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& name) { - auto lock = std::lock_guard{event_queue_lock_}; auto action = std::make_unique(true, nullptr, "", 0, name, std::map{}); action->AddCommand(std::move(func), {name}, 0); @@ -65,18 +62,15 @@ void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& } void ActionManager::ExecuteOneCommand() { - { - auto lock = std::lock_guard{event_queue_lock_}; - // Loop through the event queue until we have an action to execute - while (current_executing_actions_.empty() && !event_queue_.empty()) { - for (const auto& action : actions_) { - if (std::visit([&action](const auto& event) { return action->CheckEvent(event); }, - event_queue_.front())) { - current_executing_actions_.emplace(action.get()); - } + // Loop through the event queue until we have an action to execute + while (current_executing_actions_.empty() && !event_queue_.empty()) { + for (const auto& action : actions_) { + if (std::visit([&action](const auto& event) { return action->CheckEvent(event); }, + event_queue_.front())) { + current_executing_actions_.emplace(action.get()); } - event_queue_.pop(); } + event_queue_.pop(); } if (current_executing_actions_.empty()) { @@ -109,7 +103,6 @@ void ActionManager::ExecuteOneCommand() { } bool ActionManager::HasMoreCommands() const { - auto lock = std::lock_guard{event_queue_lock_}; return !current_executing_actions_.empty() || !event_queue_.empty(); } @@ -120,7 +113,6 @@ void ActionManager::DumpState() const { } void ActionManager::ClearQueue() { - auto lock = std::lock_guard{event_queue_lock_}; // We are shutting down so don't claim the oneshot builtin actions back current_executing_actions_ = {}; event_queue_ = {}; diff --git a/init/action_manager.h b/init/action_manager.h index b6f93d9b5..a2b95acad 100644 --- a/init/action_manager.h +++ b/init/action_manager.h @@ -16,12 +16,9 @@ #pragma once -#include #include #include -#include - #include "action.h" #include "builtins.h" @@ -51,9 +48,7 @@ class ActionManager { void operator=(ActionManager const&) = delete; std::vector> actions_; - std::queue> event_queue_ - GUARDED_BY(event_queue_lock_); - mutable std::mutex event_queue_lock_; + std::queue> event_queue_; std::queue current_executing_actions_; std::size_t current_command_; }; diff --git a/init/builtins.cpp b/init/builtins.cpp index dd5af72b1..200bfff7d 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -151,7 +151,6 @@ static Result reboot_into_recovery(const std::vector& options template static void ForEachServiceInClass(const std::string& classname, F function) { - auto lock = std::lock_guard{service_lock}; for (const auto& service : ServiceList::GetInstance()) { if (service->classnames().count(classname)) std::invoke(function, service); } @@ -163,7 +162,6 @@ static Result do_class_start(const BuiltinArguments& args) { return {}; // Starting a class does not start services which are explicitly disabled. // They must be started individually. - auto lock = std::lock_guard{service_lock}; for (const auto& service : ServiceList::GetInstance()) { if (service->classnames().count(args[1])) { if (auto result = service->StartIfNotDisabled(); !result.ok()) { @@ -186,7 +184,6 @@ static Result do_class_start_post_data(const BuiltinArguments& args) { // stopped either. return {}; } - auto lock = std::lock_guard{service_lock}; for (const auto& service : ServiceList::GetInstance()) { if (service->classnames().count(args[1])) { if (auto result = service->StartIfPostData(); !result.ok()) { @@ -237,7 +234,6 @@ static Result do_domainname(const BuiltinArguments& args) { } static Result do_enable(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "Could not find service"; @@ -249,7 +245,6 @@ static Result do_enable(const BuiltinArguments& args) { } static Result do_exec(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; auto service = Service::MakeTemporaryOneshotService(args.args); if (!service.ok()) { return Error() << "Could not create exec service: " << service.error(); @@ -263,7 +258,6 @@ static Result do_exec(const BuiltinArguments& args) { } static Result do_exec_background(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; auto service = Service::MakeTemporaryOneshotService(args.args); if (!service.ok()) { return Error() << "Could not create exec background service: " << service.error(); @@ -277,7 +271,6 @@ static Result do_exec_background(const BuiltinArguments& args) { } static Result do_exec_start(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* service = ServiceList::GetInstance().FindService(args[1]); if (!service) { return Error() << "Service not found"; @@ -347,7 +340,6 @@ static Result do_insmod(const BuiltinArguments& args) { } static Result do_interface_restart(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindInterface(args[1]); if (!svc) return Error() << "interface " << args[1] << " not found"; svc->Restart(); @@ -355,7 +347,6 @@ static Result do_interface_restart(const BuiltinArguments& args) { } static Result do_interface_start(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindInterface(args[1]); if (!svc) return Error() << "interface " << args[1] << " not found"; if (auto result = svc->Start(); !result.ok()) { @@ -365,7 +356,6 @@ static Result do_interface_start(const BuiltinArguments& args) { } static Result do_interface_stop(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindInterface(args[1]); if (!svc) return Error() << "interface " << args[1] << " not found"; svc->Stop(); @@ -750,7 +740,6 @@ static Result do_setrlimit(const BuiltinArguments& args) { } static Result do_start(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "service " << args[1] << " not found"; if (auto result = svc->Start(); !result.ok()) { @@ -760,7 +749,6 @@ static Result do_start(const BuiltinArguments& args) { } static Result do_stop(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "service " << args[1] << " not found"; svc->Stop(); @@ -768,7 +756,6 @@ static Result do_stop(const BuiltinArguments& args) { } static Result do_restart(const BuiltinArguments& args) { - auto lock = std::lock_guard{service_lock}; Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "service " << args[1] << " not found"; svc->Restart(); @@ -1124,7 +1111,6 @@ static Result ExecWithFunctionOnFailure(const std::vector& ar function(StringPrintf("Exec service failed, status %d", siginfo.si_status)); } }); - auto lock = std::lock_guard{service_lock}; if (auto result = (*service)->ExecStart(); !result.ok()) { function("ExecStart failed: " + result.error().message()); } @@ -1264,7 +1250,6 @@ static Result parse_apex_configs() { } success &= parser.ParseConfigFile(c); } - auto lock = std::lock_guard{service_lock}; ServiceList::GetInstance().MarkServicesUpdate(); if (success) { return {}; diff --git a/init/init.cpp b/init/init.cpp index b0f929c7b..a7518fc2f 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -33,9 +33,7 @@ #include #include #include -#include #include -#include #include #include @@ -97,148 +95,15 @@ static int property_triggers_enabled = 0; static int signal_fd = -1; static int property_fd = -1; +static std::unique_ptr waiting_for_prop(nullptr); +static std::string wait_prop_name; +static std::string wait_prop_value; +static std::string shutdown_command; +static bool do_shutdown = false; + static std::unique_ptr subcontext; -// Init epolls various FDs to wait for various inputs. It previously waited on property changes -// with a blocking socket that contained the information related to the change, however, it was easy -// to fill that socket and deadlock the system. Now we use locks to handle the property changes -// directly in the property thread, however we still must wake the epoll to inform init that there -// is a change to process, so we use this FD. It is non-blocking, since we do not care how many -// times WakeEpoll() is called, only that the epoll will wake. -static int wake_epoll_fd = -1; -static void InstallInitNotifier(Epoll* epoll) { - int sockets[2]; - if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0, sockets) != 0) { - PLOG(FATAL) << "Failed to socketpair() between property_service and init"; - } - int epoll_fd = sockets[0]; - wake_epoll_fd = sockets[1]; - - auto drain_socket = [epoll_fd] { - char buf[512]; - while (read(epoll_fd, buf, sizeof(buf)) > 0) { - } - }; - - if (auto result = epoll->RegisterHandler(epoll_fd, drain_socket); !result) { - LOG(FATAL) << result.error(); - } -} - -static void WakeEpoll() { - constexpr char value[] = "1"; - write(wake_epoll_fd, value, sizeof(value)); -} - -static class PropWaiterState { - public: - bool StartWaiting(const char* name, const char* value) { - auto lock = std::lock_guard{lock_}; - if (waiting_for_prop_) { - return false; - } - if (GetProperty(name, "") != value) { - // Current property value is not equal to expected value - wait_prop_name_ = name; - wait_prop_value_ = value; - waiting_for_prop_.reset(new Timer()); - } else { - LOG(INFO) << "start_waiting_for_property(\"" << name << "\", \"" << value - << "\"): already set"; - } - return true; - } - - void ResetWaitForProp() { - auto lock = std::lock_guard{lock_}; - ResetWaitForPropLocked(); - } - - void CheckAndResetWait(const std::string& name, const std::string& value) { - auto lock = std::lock_guard{lock_}; - // We always record how long init waited for ueventd to tell us cold boot finished. - // If we aren't waiting on this property, it means that ueventd finished before we even - // started to wait. - if (name == kColdBootDoneProp) { - auto time_waited = waiting_for_prop_ ? waiting_for_prop_->duration().count() : 0; - std::thread([time_waited] { - SetProperty("ro.boottime.init.cold_boot_wait", std::to_string(time_waited)); - }).detach(); - } - - if (waiting_for_prop_) { - if (wait_prop_name_ == name && wait_prop_value_ == value) { - LOG(INFO) << "Wait for property '" << wait_prop_name_ << "=" << wait_prop_value_ - << "' took " << *waiting_for_prop_; - ResetWaitForPropLocked(); - WakeEpoll(); - } - } - } - - // This is not thread safe because it releases the lock when it returns, so the waiting state - // may change. However, we only use this function to prevent running commands in the main - // thread loop when we are waiting, so we do not care about false positives; only false - // negatives. StartWaiting() and this function are always called from the same thread, so false - // negatives are not possible and therefore we're okay. - bool MightBeWaiting() { - auto lock = std::lock_guard{lock_}; - return static_cast(waiting_for_prop_); - } - - private: - void ResetWaitForPropLocked() { - wait_prop_name_.clear(); - wait_prop_value_.clear(); - waiting_for_prop_.reset(); - } - - std::mutex lock_; - std::unique_ptr waiting_for_prop_{nullptr}; - std::string wait_prop_name_; - std::string wait_prop_value_; - -} prop_waiter_state; - -bool start_waiting_for_property(const char* name, const char* value) { - return prop_waiter_state.StartWaiting(name, value); -} - -void ResetWaitForProp() { - prop_waiter_state.ResetWaitForProp(); -} - -static class ShutdownState { - public: - void TriggerShutdown(const std::string& command) { - // We can't call HandlePowerctlMessage() directly in this function, - // because it modifies the contents of the action queue, which can cause the action queue - // to get into a bad state if this function is called from a command being executed by the - // action queue. Instead we set this flag and ensure that shutdown happens before the next - // command is run in the main init loop. - auto lock = std::lock_guard{shutdown_command_lock_}; - shutdown_command_ = command; - do_shutdown_ = true; - WakeEpoll(); - } - - std::optional CheckShutdown() { - auto lock = std::lock_guard{shutdown_command_lock_}; - if (do_shutdown_ && !IsShuttingDown()) { - do_shutdown_ = false; - return shutdown_command_; - } - return {}; - } - - private: - std::mutex shutdown_command_lock_; - std::string shutdown_command_; - bool do_shutdown_ = false; -} shutdown_state; - void DumpState() { - auto lock = std::lock_guard{service_lock}; ServiceList::GetInstance().DumpState(); ActionManager::GetInstance().DumpState(); } @@ -291,7 +156,39 @@ static void LoadBootScripts(ActionManager& action_manager, ServiceList& service_ } } -void PropertyChanged(const std::string& name, const std::string& value) { +bool start_waiting_for_property(const char* name, const char* value) { + if (waiting_for_prop) { + return false; + } + if (GetProperty(name, "") != value) { + // Current property value is not equal to expected value + wait_prop_name = name; + wait_prop_value = value; + waiting_for_prop.reset(new Timer()); + } else { + LOG(INFO) << "start_waiting_for_property(\"" << name << "\", \"" << value + << "\"): already set"; + } + return true; +} + +void ResetWaitForProp() { + wait_prop_name.clear(); + wait_prop_value.clear(); + waiting_for_prop.reset(); +} + +static void TriggerShutdown(const std::string& command) { + // We can't call HandlePowerctlMessage() directly in this function, + // because it modifies the contents of the action queue, which can cause the action queue + // to get into a bad state if this function is called from a command being executed by the + // action queue. Instead we set this flag and ensure that shutdown happens before the next + // command is run in the main init loop. + shutdown_command = command; + do_shutdown = true; +} + +void property_changed(const std::string& name, const std::string& value) { // If the property is sys.powerctl, we bypass the event queue and immediately handle it. // This is to ensure that init will always and immediately shutdown/reboot, regardless of // if there are other pending events to process or if init is waiting on an exec service or @@ -299,20 +196,30 @@ void PropertyChanged(const std::string& name, const std::string& value) { // In non-thermal-shutdown case, 'shutdown' trigger will be fired to let device specific // commands to be executed. if (name == "sys.powerctl") { - trigger_shutdown(value); + TriggerShutdown(value); } - if (property_triggers_enabled) { - ActionManager::GetInstance().QueuePropertyChange(name, value); - WakeEpoll(); + if (property_triggers_enabled) ActionManager::GetInstance().QueuePropertyChange(name, value); + + // We always record how long init waited for ueventd to tell us cold boot finished. + // If we aren't waiting on this property, it means that ueventd finished before we even started + // to wait. + if (name == kColdBootDoneProp) { + auto time_waited = waiting_for_prop ? waiting_for_prop->duration().count() : 0; + SetProperty("ro.boottime.init.cold_boot_wait", std::to_string(time_waited)); } - prop_waiter_state.CheckAndResetWait(name, value); + if (waiting_for_prop) { + if (wait_prop_name == name && wait_prop_value == value) { + LOG(INFO) << "Wait for property '" << wait_prop_name << "=" << wait_prop_value + << "' took " << *waiting_for_prop; + ResetWaitForProp(); + } + } } static std::optional HandleProcessActions() { std::optional next_process_action_time; - auto lock = std::lock_guard{service_lock}; for (const auto& s : ServiceList::GetInstance()) { if ((s->flags() & SVC_RUNNING) && s->timeout_period()) { auto timeout_time = s->time_started() + *s->timeout_period(); @@ -341,7 +248,7 @@ static std::optional HandleProcessActions() { return next_process_action_time; } -static Result DoControlStart(Service* service) REQUIRES(service_lock) { +static Result DoControlStart(Service* service) { return service->Start(); } @@ -350,7 +257,7 @@ static Result DoControlStop(Service* service) { return {}; } -static Result DoControlRestart(Service* service) REQUIRES(service_lock) { +static Result DoControlRestart(Service* service) { service->Restart(); return {}; } @@ -384,7 +291,7 @@ static const std::map& get_control_message_ return control_message_functions; } -bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t from_pid) { +bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t pid) { const auto& map = get_control_message_map(); const auto it = map.find(msg); @@ -393,7 +300,7 @@ bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t return false; } - std::string cmdline_path = StringPrintf("proc/%d/cmdline", from_pid); + std::string cmdline_path = StringPrintf("proc/%d/cmdline", pid); std::string process_cmdline; if (ReadFileToString(cmdline_path, &process_cmdline)) { std::replace(process_cmdline.begin(), process_cmdline.end(), '\0', ' '); @@ -404,8 +311,6 @@ bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t const ControlMessageFunction& function = it->second; - auto lock = std::lock_guard{service_lock}; - Service* svc = nullptr; switch (function.target) { @@ -423,24 +328,23 @@ bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t if (svc == nullptr) { LOG(ERROR) << "Control message: Could not find '" << name << "' for ctl." << msg - << " from pid: " << from_pid << " (" << process_cmdline << ")"; + << " from pid: " << pid << " (" << process_cmdline << ")"; return false; } if (auto result = function.action(svc); !result.ok()) { LOG(ERROR) << "Control message: Could not ctl." << msg << " for '" << name - << "' from pid: " << from_pid << " (" << process_cmdline - << "): " << result.error(); + << "' from pid: " << pid << " (" << process_cmdline << "): " << result.error(); return false; } LOG(INFO) << "Control message: Processed ctl." << msg << " for '" << name - << "' from pid: " << from_pid << " (" << process_cmdline << ")"; + << "' from pid: " << pid << " (" << process_cmdline << ")"; return true; } static Result wait_for_coldboot_done_action(const BuiltinArguments& args) { - if (!prop_waiter_state.StartWaiting(kColdBootDoneProp, "true")) { + if (!start_waiting_for_property(kColdBootDoneProp, "true")) { LOG(FATAL) << "Could not wait for '" << kColdBootDoneProp << "'"; } @@ -588,7 +492,6 @@ void HandleKeychord(const std::vector& keycodes) { } auto found = false; - auto lock = std::lock_guard{service_lock}; for (const auto& service : ServiceList::GetInstance()) { auto svc = service.get(); if (svc->keycodes() == keycodes) { @@ -675,6 +578,44 @@ void SendStartSendingMessagesMessage() { } } +static void HandlePropertyFd() { + auto message = ReadMessage(property_fd); + if (!message.ok()) { + LOG(ERROR) << "Could not read message from property service: " << message.error(); + return; + } + + auto property_message = PropertyMessage{}; + if (!property_message.ParseFromString(*message)) { + LOG(ERROR) << "Could not parse message from property service"; + return; + } + + switch (property_message.msg_case()) { + case PropertyMessage::kControlMessage: { + auto& control_message = property_message.control_message(); + bool success = HandleControlMessage(control_message.msg(), control_message.name(), + control_message.pid()); + + uint32_t response = success ? PROP_SUCCESS : PROP_ERROR_HANDLE_CONTROL_MESSAGE; + if (control_message.has_fd()) { + int fd = control_message.fd(); + TEMP_FAILURE_RETRY(send(fd, &response, sizeof(response), 0)); + close(fd); + } + break; + } + case PropertyMessage::kChangedMessage: { + auto& changed_message = property_message.changed_message(); + property_changed(changed_message.name(), changed_message.value()); + break; + } + default: + LOG(ERROR) << "Unknown message type from property service: " + << property_message.msg_case(); + } +} + int SecondStageMain(int argc, char** argv) { if (REBOOT_BOOTLOADER_ON_PANIC) { InstallRebootSignalHandlers(); @@ -682,7 +623,7 @@ int SecondStageMain(int argc, char** argv) { boot_clock::time_point start_time = boot_clock::now(); - trigger_shutdown = [](const std::string& command) { shutdown_state.TriggerShutdown(command); }; + trigger_shutdown = TriggerShutdown; SetStdioToDevNull(argv); InitKernelLogging(argv); @@ -742,8 +683,11 @@ int SecondStageMain(int argc, char** argv) { } InstallSignalFdHandler(&epoll); - InstallInitNotifier(&epoll); + StartPropertyService(&property_fd); + if (auto result = epoll.RegisterHandler(property_fd, HandlePropertyFd); !result.ok()) { + LOG(FATAL) << "Could not register epoll handler for property fd: " << result.error(); + } // Make the time that init stages started available for bootstat to log. RecordStageBoottimes(start_time); @@ -796,7 +740,6 @@ int SecondStageMain(int argc, char** argv) { Keychords keychords; am.QueueBuiltinAction( [&epoll, &keychords](const BuiltinArguments& args) -> Result { - auto lock = std::lock_guard{service_lock}; for (const auto& svc : ServiceList::GetInstance()) { keychords.Register(svc->keycodes()); } @@ -827,12 +770,12 @@ int SecondStageMain(int argc, char** argv) { // By default, sleep until something happens. auto epoll_timeout = std::optional{}; - auto shutdown_command = shutdown_state.CheckShutdown(); - if (shutdown_command) { - HandlePowerctlMessage(*shutdown_command); + if (do_shutdown && !IsShuttingDown()) { + do_shutdown = false; + HandlePowerctlMessage(shutdown_command); } - if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { + if (!(waiting_for_prop || Service::is_exec_service_running())) { am.ExecuteOneCommand(); } if (!IsShuttingDown()) { @@ -846,7 +789,7 @@ int SecondStageMain(int argc, char** argv) { } } - if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { + if (!(waiting_for_prop || Service::is_exec_service_running())) { // If there's more work to do, wake up again immediately. if (am.HasMoreCommands()) epoll_timeout = 0ms; } diff --git a/init/init.h b/init/init.h index bcf24e73f..4bbca6f3c 100644 --- a/init/init.h +++ b/init/init.h @@ -41,9 +41,6 @@ void SendLoadPersistentPropertiesMessage(); void SendStopSendingMessagesMessage(); void SendStartSendingMessagesMessage(); -void PropertyChanged(const std::string& name, const std::string& value); -bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t from_pid); - int SecondStageMain(int argc, char** argv); } // namespace init diff --git a/init/init_test.cpp b/init/init_test.cpp index 3053bd848..caf3e0343 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -167,7 +167,6 @@ service A something ServiceList service_list; TestInitText(init_script, BuiltinFunctionMap(), {}, &service_list); - auto lock = std::lock_guard{service_lock}; ASSERT_EQ(1, std::distance(service_list.begin(), service_list.end())); auto service = service_list.begin()->get(); diff --git a/init/lmkd_service.cpp b/init/lmkd_service.cpp index a531d0aea..dd1ab4d61 100644 --- a/init/lmkd_service.cpp +++ b/init/lmkd_service.cpp @@ -79,8 +79,7 @@ static bool UnregisterProcess(pid_t pid) { } static void RegisterServices(pid_t exclude_pid) { - auto lock = std::lock_guard{service_lock}; - for (const auto& service : ServiceList::GetInstance()) { + for (const auto& service : ServiceList::GetInstance().services()) { auto svc = service.get(); if (svc->oom_score_adjust() != DEFAULT_OOM_SCORE_ADJUST) { // skip if process is excluded or not yet forked (pid==0) diff --git a/init/mount_namespace.cpp b/init/mount_namespace.cpp index aa368492d..0749fe3b8 100644 --- a/init/mount_namespace.cpp +++ b/init/mount_namespace.cpp @@ -29,7 +29,6 @@ #include #include -#include "property_service.h" #include "util.h" namespace android { @@ -291,14 +290,6 @@ bool SwitchToDefaultMountNamespace() { return true; } if (default_ns_id != GetMountNamespaceId()) { - // The property service thread and its descendent threads must be in the correct mount - // namespace to call Service::Start(), however setns() only operates on a single thread and - // fails when secondary threads attempt to join the same mount namespace. Therefore, we - // must join the property service thread and its descendents before the setns() call. Those - // threads are then started again after the setns() call, and they'll be in the proper - // namespace. - PausePropertyService(); - if (setns(default_ns_fd.get(), CLONE_NEWNS) == -1) { PLOG(ERROR) << "Failed to switch back to the default mount namespace."; return false; @@ -308,8 +299,6 @@ bool SwitchToDefaultMountNamespace() { LOG(ERROR) << result.error(); return false; } - - ResumePropertyService(); } LOG(INFO) << "Switched to default mount namespace"; diff --git a/init/property_service.cpp b/init/property_service.cpp index 319a241c7..730bf6de4 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -92,10 +92,8 @@ namespace init { static bool persistent_properties_loaded = false; static int property_set_fd = -1; -static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; -static std::thread property_service_thread; static PropertyInfoAreaFile property_info_area; @@ -149,6 +147,17 @@ static bool CheckMacPerms(const std::string& name, const char* target_context, return has_access; } +static void SendPropertyChanged(const std::string& name, const std::string& value) { + auto property_msg = PropertyMessage{}; + auto* changed_message = property_msg.mutable_changed_message(); + changed_message->set_name(name); + changed_message->set_value(value); + + if (auto result = SendMessage(init_socket, property_msg); !result.ok()) { + LOG(ERROR) << "Failed to send property changed message: " << result.error(); + } +} + static uint32_t PropertySet(const std::string& name, const std::string& value, std::string* error) { size_t valuelen = value.size(); @@ -187,137 +196,47 @@ static uint32_t PropertySet(const std::string& name, const std::string& value, s // If init hasn't started its main loop, then it won't be handling property changed messages // anyway, so there's no need to try to send them. if (accept_messages) { - PropertyChanged(name, value); + SendPropertyChanged(name, value); } return PROP_SUCCESS; } -template -class SingleThreadExecutor { +class AsyncRestorecon { public: - virtual ~SingleThreadExecutor() {} - - template - void Run(F&& item) { + void TriggerRestorecon(const std::string& path) { auto guard = std::lock_guard{mutex_}; - items_.emplace(std::forward(item)); + paths_.emplace(path); - if (thread_state_ == ThreadState::kRunning || thread_state_ == ThreadState::kStopped) { - return; - } - - if (thread_state_ == ThreadState::kPendingJoin) { - thread_.join(); - } - - StartThread(); - } - - void StopAndJoin() { - auto lock = std::unique_lock{mutex_}; - if (thread_state_ == ThreadState::kPendingJoin) { - thread_.join(); - } else if (thread_state_ == ThreadState::kRunning) { - thread_state_ = ThreadState::kStopped; - lock.unlock(); - thread_.join(); - lock.lock(); - } - - thread_state_ = ThreadState::kStopped; - } - - void Restart() { - auto guard = std::lock_guard{mutex_}; - if (items_.empty()) { - thread_state_ = ThreadState::kNotStarted; - } else { - StartThread(); - } - } - - void MaybeJoin() { - auto guard = std::lock_guard{mutex_}; - if (thread_state_ == ThreadState::kPendingJoin) { - thread_.join(); - thread_state_ = ThreadState::kNotStarted; + if (!thread_started_) { + thread_started_ = true; + std::thread{&AsyncRestorecon::ThreadFunction, this}.detach(); } } private: - virtual void Execute(T&& item) = 0; - - void StartThread() { - thread_state_ = ThreadState::kRunning; - auto thread = std::thread{&SingleThreadExecutor::ThreadFunction, this}; - std::swap(thread_, thread); - } - void ThreadFunction() { auto lock = std::unique_lock{mutex_}; - while (!items_.empty()) { - auto item = items_.front(); - items_.pop(); + while (!paths_.empty()) { + auto path = paths_.front(); + paths_.pop(); lock.unlock(); - Execute(std::move(item)); + 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(); } - if (thread_state_ != ThreadState::kStopped) { - thread_state_ = ThreadState::kPendingJoin; - } + thread_started_ = false; } std::mutex mutex_; - std::queue items_; - enum class ThreadState { - kNotStarted, // Initial state when starting the program or when restarting with no items to - // process. - kRunning, // The thread is running and is in a state that it will process new items if - // are run. - kPendingJoin, // The thread has run to completion and is pending join(). A new thread must - // be launched for new items to be processed. - kStopped, // This executor has stopped and will not process more items until Restart() is - // called. Currently pending items will be processed and the thread will be - // joined. - }; - ThreadState thread_state_ = ThreadState::kNotStarted; - std::thread thread_; + std::queue paths_; + bool thread_started_ = false; }; -class RestoreconThread : public SingleThreadExecutor { - virtual void Execute(std::string&& path) override { - if (selinux_android_restorecon(path.c_str(), SELINUX_ANDROID_RESTORECON_RECURSE) != 0) { - LOG(ERROR) << "Asynchronous restorecon of '" << path << "' failed'"; - } - android::base::SetProperty(kRestoreconProperty, path); - } -}; - -struct ControlMessageInfo { - std::string message; - std::string name; - pid_t pid; - int fd; -}; - -class ControlMessageThread : public SingleThreadExecutor { - virtual void Execute(ControlMessageInfo&& info) override { - bool success = HandleControlMessage(info.message, info.name, info.pid); - - uint32_t response = success ? PROP_SUCCESS : PROP_ERROR_HANDLE_CONTROL_MESSAGE; - if (info.fd != -1) { - TEMP_FAILURE_RETRY(send(info.fd, &response, sizeof(response), 0)); - close(info.fd); - } - } -}; - -static RestoreconThread restorecon_thread; -static ControlMessageThread control_message_thread; - class SocketConnection { public: SocketConnection(int socket, const ucred& cred) : socket_(socket), cred_(cred) {} @@ -459,17 +378,29 @@ static uint32_t SendControlMessage(const std::string& msg, const std::string& na return PROP_ERROR_HANDLE_CONTROL_MESSAGE; } - // We must release the fd before spawning the thread, otherwise there will be a race with the - // thread. If the thread calls close() before this function calls Release(), then fdsan will see - // the wrong tag and abort(). + auto property_msg = PropertyMessage{}; + auto* control_message = property_msg.mutable_control_message(); + control_message->set_msg(msg); + control_message->set_name(name); + control_message->set_pid(pid); + + // We must release the fd before sending it to init, otherwise there will be a race with init. + // If init calls close() before Release(), then fdsan will see the wrong tag and abort(). int fd = -1; if (socket != nullptr && SelinuxGetVendorAndroidVersion() > __ANDROID_API_Q__) { fd = socket->Release(); + control_message->set_fd(fd); } - // Handling a control message likely calls SetProperty, which we must synchronously handle, - // therefore we must fork a thread to handle it. - control_message_thread.Run({msg, name, pid, fd}); + if (auto result = SendMessage(init_socket, property_msg); !result.ok()) { + // We've already released the fd above, so if we fail to send the message to init, we need + // to manually free it here. + if (fd != -1) { + close(fd); + } + *error = "Failed to send control message: " + result.error().message(); + return PROP_ERROR_HANDLE_CONTROL_MESSAGE; + } return PROP_SUCCESS; } @@ -571,7 +502,8 @@ uint32_t HandlePropertySet(const std::string& name, const std::string& 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()) { - restorecon_thread.Run(value); + static AsyncRestorecon async_restorecon; + async_restorecon.TriggerRestorecon(value); return PROP_SUCCESS; } @@ -1152,8 +1084,6 @@ void PropertyInit() { PropertyLoadBootDefaults(); } -static bool pause_property_service = false; - static void HandleInitSocket() { auto message = ReadMessage(init_socket); if (!message.ok()) { @@ -1188,10 +1118,6 @@ static void HandleInitSocket() { accept_messages = true; break; } - case InitMessage::kPausePropertyService: { - pause_property_service = true; - break; - } default: LOG(ERROR) << "Unknown message type from init: " << init_message.msg_case(); } @@ -1212,7 +1138,7 @@ static void PropertyServiceThread() { LOG(FATAL) << result.error(); } - while (!pause_property_service) { + while (true) { auto pending_functions = epoll.Wait(std::nullopt); if (!pending_functions.ok()) { LOG(ERROR) << pending_functions.error(); @@ -1221,34 +1147,9 @@ static void PropertyServiceThread() { (*function)(); } } - control_message_thread.MaybeJoin(); - restorecon_thread.MaybeJoin(); } } -void SendStopPropertyServiceMessage() { - auto init_message = InitMessage{}; - init_message.set_pause_property_service(true); - if (auto result = SendMessage(from_init_socket, init_message); !result.ok()) { - LOG(ERROR) << "Failed to send stop property service message: " << result.error(); - } -} - -void PausePropertyService() { - control_message_thread.StopAndJoin(); - restorecon_thread.StopAndJoin(); - SendStopPropertyServiceMessage(); - property_service_thread.join(); -} - -void ResumePropertyService() { - pause_property_service = false; - auto new_thread = std::thread{PropertyServiceThread}; - property_service_thread.swap(new_thread); - restorecon_thread.Restart(); - control_message_thread.Restart(); -} - void StartPropertyService(int* epoll_socket) { InitPropertySet("ro.property_service.version", "2"); @@ -1256,7 +1157,7 @@ void StartPropertyService(int* epoll_socket) { if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sockets) != 0) { PLOG(FATAL) << "Failed to socketpair() between property_service and init"; } - *epoll_socket = from_init_socket = sockets[0]; + *epoll_socket = sockets[0]; init_socket = sockets[1]; accept_messages = true; @@ -1270,8 +1171,7 @@ void StartPropertyService(int* epoll_socket) { listen(property_set_fd, 8); - auto new_thread = std::thread{PropertyServiceThread}; - property_service_thread.swap(new_thread); + std::thread{PropertyServiceThread}.detach(); } } // namespace init diff --git a/init/property_service.h b/init/property_service.h index e92132632..506d116e1 100644 --- a/init/property_service.h +++ b/init/property_service.h @@ -31,8 +31,6 @@ bool CanReadProperty(const std::string& source_context, const std::string& name) void PropertyInit(); void StartPropertyService(int* epoll_socket); -void ResumePropertyService(); -void PausePropertyService(); } // namespace init } // namespace android diff --git a/init/property_service.proto b/init/property_service.proto index 36245b228..08268d9bb 100644 --- a/init/property_service.proto +++ b/init/property_service.proto @@ -41,6 +41,5 @@ message InitMessage { bool load_persistent_properties = 1; bool stop_sending_messages = 2; bool start_sending_messages = 3; - bool pause_property_service = 4; }; } diff --git a/init/reboot.cpp b/init/reboot.cpp index cad192de9..8d8bd8ee6 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -85,7 +85,7 @@ static bool shutting_down = false; static const std::set kDebuggingServices{"tombstoned", "logd", "adbd", "console"}; -static std::vector GetDebuggingServices(bool only_post_data) REQUIRES(service_lock) { +static std::vector GetDebuggingServices(bool only_post_data) { std::vector ret; ret.reserve(kDebuggingServices.size()); for (const auto& s : ServiceList::GetInstance()) { @@ -181,7 +181,7 @@ class MountEntry { }; // Turn off backlight while we are performing power down cleanup activities. -static void TurnOffBacklight() REQUIRES(service_lock) { +static void TurnOffBacklight() { Service* service = ServiceList::GetInstance().FindService("blank_screen"); if (service == nullptr) { LOG(WARNING) << "cannot find blank_screen in TurnOffBacklight"; @@ -589,7 +589,6 @@ static void DoReboot(unsigned int cmd, const std::string& reason, const std::str // Start reboot monitor thread sem_post(&reboot_semaphore); - auto lock = std::lock_guard{service_lock}; // watchdogd is a vendor specific component but should be alive to complete shutdown safely. const std::set to_starts{"watchdogd"}; std::vector stop_first; @@ -709,7 +708,6 @@ static void EnterShutdown() { // Skip wait for prop if it is in progress ResetWaitForProp(); // Clear EXEC flag if there is one pending - auto lock = std::lock_guard{service_lock}; for (const auto& s : ServiceList::GetInstance()) { s->UnSetExec(); } @@ -753,7 +751,6 @@ static Result DoUserspaceReboot() { return Error() << "Failed to set sys.init.userspace_reboot.in_progress property"; } EnterShutdown(); - auto lock = std::lock_guard{service_lock}; if (!SetProperty("sys.powerctl", "")) { return Error() << "Failed to reset sys.powerctl property"; } @@ -914,7 +911,6 @@ void HandlePowerctlMessage(const std::string& command) { run_fsck = true; } else if (cmd_params[1] == "thermal") { // Turn off sources of heat immediately. - auto lock = std::lock_guard{service_lock}; TurnOffBacklight(); // run_fsck is false to avoid delay cmd = ANDROID_RB_THERMOFF; diff --git a/init/service.h b/init/service.h index d2a446207..cf3f0c290 100644 --- a/init/service.h +++ b/init/service.h @@ -27,14 +27,12 @@ #include #include -#include #include #include "action.h" #include "capabilities.h" #include "keyword_map.h" #include "parser.h" -#include "service_lock.h" #include "service_utils.h" #include "subcontext.h" @@ -79,17 +77,17 @@ class Service { bool IsRunning() { return (flags_ & SVC_RUNNING) != 0; } bool IsEnabled() { return (flags_ & SVC_DISABLED) == 0; } - Result ExecStart() REQUIRES(service_lock); - Result Start() REQUIRES(service_lock); - Result StartIfNotDisabled() REQUIRES(service_lock); - Result StartIfPostData() REQUIRES(service_lock); - Result Enable() REQUIRES(service_lock); + Result ExecStart(); + Result Start(); + Result StartIfNotDisabled(); + Result StartIfPostData(); + Result Enable(); void Reset(); void ResetIfPostData(); void Stop(); void Terminate(); void Timeout(); - void Restart() REQUIRES(service_lock); + void Restart(); void Reap(const siginfo_t& siginfo); void DumpState() const; void SetShutdownCritical() { flags_ |= SVC_SHUTDOWN_CRITICAL; } diff --git a/init/service_list.h b/init/service_list.h index 280a2280f..3b9018bc0 100644 --- a/init/service_list.h +++ b/init/service_list.h @@ -17,13 +17,9 @@ #pragma once #include -#include #include -#include - #include "service.h" -#include "service_lock.h" namespace android { namespace init { @@ -36,16 +32,16 @@ class ServiceList { ServiceList(); size_t CheckAllCommands(); - void AddService(std::unique_ptr service) REQUIRES(service_lock); - void RemoveService(const Service& svc) REQUIRES(service_lock); + void AddService(std::unique_ptr service); + void RemoveService(const Service& svc); template - void RemoveServiceIf(UnaryPredicate predicate) REQUIRES(service_lock) { + void RemoveServiceIf(UnaryPredicate predicate) { services_.erase(std::remove_if(services_.begin(), services_.end(), predicate), services_.end()); } template - Service* FindService(T value, F function = &Service::name) const REQUIRES(service_lock) { + Service* FindService(T value, F function = &Service::name) const { auto svc = std::find_if(services_.begin(), services_.end(), [&function, &value](const std::unique_ptr& s) { return std::invoke(function, s) == value; @@ -56,7 +52,7 @@ class ServiceList { return nullptr; } - Service* FindInterface(const std::string& interface_name) REQUIRES(service_lock) { + Service* FindInterface(const std::string& interface_name) { for (const auto& svc : services_) { if (svc->interfaces().count(interface_name) > 0) { return svc.get(); @@ -66,20 +62,18 @@ class ServiceList { return nullptr; } - void DumpState() const REQUIRES(service_lock); + void DumpState() const; - auto begin() const REQUIRES(service_lock) { return services_.begin(); } - auto end() const REQUIRES(service_lock) { return services_.end(); } - const std::vector>& services() const REQUIRES(service_lock) { - return services_; - } - const std::vector services_in_shutdown_order() const REQUIRES(service_lock); + auto begin() const { return services_.begin(); } + auto end() const { return services_.end(); } + const std::vector>& services() const { return services_; } + const std::vector services_in_shutdown_order() const; void MarkPostData(); bool IsPostData(); - void MarkServicesUpdate() REQUIRES(service_lock); + void MarkServicesUpdate(); bool IsServicesUpdated() const { return services_update_finished_; } - void DelayService(const Service& service) REQUIRES(service_lock); + void DelayService(const Service& service); void ResetState() { post_data_ = false; diff --git a/init/service_lock.cpp b/init/service_lock.cpp deleted file mode 100644 index 404d4396e..000000000 --- a/init/service_lock.cpp +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "service_lock.h" - -namespace android { -namespace init { - -RecursiveMutex service_lock; - -} // namespace init -} // namespace android diff --git a/init/service_lock.h b/init/service_lock.h deleted file mode 100644 index 6b94271be..000000000 --- a/init/service_lock.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#pragma once - -#include - -#include - -namespace android { -namespace init { - -// This class exists to add thread annotations, since they're absent from std::recursive_mutex. - -class CAPABILITY("mutex") RecursiveMutex { - public: - void lock() ACQUIRE() { mutex_.lock(); } - void unlock() RELEASE() { mutex_.unlock(); } - - private: - std::recursive_mutex mutex_; -}; - -extern RecursiveMutex service_lock; - -} // namespace init -} // namespace android diff --git a/init/service_parser.cpp b/init/service_parser.cpp index 51f4c9786..560f693f9 100644 --- a/init/service_parser.cpp +++ b/init/service_parser.cpp @@ -168,7 +168,6 @@ Result ServiceParser::ParseInterface(std::vector&& args) { const std::string fullname = interface_name + "/" + instance_name; - auto lock = std::lock_guard{service_lock}; for (const auto& svc : *service_list_) { if (svc->interfaces().count(fullname) > 0) { return Error() << "Interface '" << fullname << "' redefined in " << service_->name() @@ -599,7 +598,6 @@ Result ServiceParser::EndSection() { } } - auto lock = std::lock_guard{service_lock}; Service* old_service = service_list_->FindService(service_->name()); if (old_service) { if (!service_->is_override()) { diff --git a/init/sigchld_handler.cpp b/init/sigchld_handler.cpp index 064d64d54..9b2c7d939 100644 --- a/init/sigchld_handler.cpp +++ b/init/sigchld_handler.cpp @@ -64,8 +64,6 @@ static pid_t ReapOneProcess() { std::string wait_string; Service* service = nullptr; - auto lock = std::lock_guard{service_lock}; - if (SubcontextChildReap(pid)) { name = "Subcontext"; } else { From 0c19d6c99f6c312bcab4df1dcf78d8635829f313 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Thu, 12 Mar 2020 14:29:25 -0700 Subject: [PATCH 2/2] init: handle property messages asynchronously #2 A previous change moved property_service into its own thread, since there was otherwise a deadlock whenever a process called by init would try to set a property. This new thread, however, would send a message via a blocking socket to init for each property that it received, since init may need to take action depending on which property it is. Unfortunately, this means that the deadlock is still possible, the only difference is the socket's buffer must be filled before init deadlocks. This change, therefore, adds the following: 1) A lock for instructing init to reboot 2) A lock for waiting on properties 3) A lock for queueing new properties A previous version of this change was reverted and added locks around all service operations and allowed the property thread to spawn services directly. This was complex due to the fact that this code was not designed to be multi-threaded. It was reverted due to apparent issues during reboot. This change keeps a queue of processes pending control messages, which it will then handle in the future. It is less flexible but safer. Bug: 146877356 Bug: 148236233 Bug: 150863651 Bug: 151251827 Test: multiple reboot tests, safely restarting hwservicemanager Merged-In: Ice773436e85d3bf636bb0a892f3f6002bdf996b6 Change-Id: Ice773436e85d3bf636bb0a892f3f6002bdf996b6 (cherry picked from commit 802864c7826ea79f8cc29c52801e08bcfc15db76) --- init/Android.bp | 1 + init/action_manager.cpp | 22 ++- init/action_manager.h | 7 +- init/init.cpp | 322 ++++++++++++++++++++++++-------------- init/init.h | 5 +- init/property_service.cpp | 63 +++----- init/property_service.h | 3 + init/reboot.cpp | 12 +- 8 files changed, 261 insertions(+), 174 deletions(-) diff --git a/init/Android.bp b/init/Android.bp index 3bb08db6c..72a7bfed4 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -81,6 +81,7 @@ cc_defaults { "-Wextra", "-Wno-unused-parameter", "-Werror", + "-Wthread-safety", "-DALLOW_FIRST_STAGE_CONSOLE=0", "-DALLOW_LOCAL_PROP_OVERRIDE=0", "-DALLOW_PERMISSIVE_SELINUX=0", diff --git a/init/action_manager.cpp b/init/action_manager.cpp index ebca762ca..b45f5cd18 100644 --- a/init/action_manager.cpp +++ b/init/action_manager.cpp @@ -41,10 +41,12 @@ void ActionManager::AddAction(std::unique_ptr action) { } void ActionManager::QueueEventTrigger(const std::string& trigger) { + auto lock = std::lock_guard{event_queue_lock_}; event_queue_.emplace(trigger); } void ActionManager::QueuePropertyChange(const std::string& name, const std::string& value) { + auto lock = std::lock_guard{event_queue_lock_}; event_queue_.emplace(std::make_pair(name, value)); } @@ -53,6 +55,7 @@ void ActionManager::QueueAllPropertyActions() { } void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& name) { + auto lock = std::lock_guard{event_queue_lock_}; auto action = std::make_unique(true, nullptr, "", 0, name, std::map{}); action->AddCommand(std::move(func), {name}, 0); @@ -62,15 +65,18 @@ void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& } void ActionManager::ExecuteOneCommand() { - // Loop through the event queue until we have an action to execute - while (current_executing_actions_.empty() && !event_queue_.empty()) { - for (const auto& action : actions_) { - if (std::visit([&action](const auto& event) { return action->CheckEvent(event); }, - event_queue_.front())) { - current_executing_actions_.emplace(action.get()); + { + auto lock = std::lock_guard{event_queue_lock_}; + // Loop through the event queue until we have an action to execute + while (current_executing_actions_.empty() && !event_queue_.empty()) { + for (const auto& action : actions_) { + if (std::visit([&action](const auto& event) { return action->CheckEvent(event); }, + event_queue_.front())) { + current_executing_actions_.emplace(action.get()); + } } + event_queue_.pop(); } - event_queue_.pop(); } if (current_executing_actions_.empty()) { @@ -103,6 +109,7 @@ void ActionManager::ExecuteOneCommand() { } bool ActionManager::HasMoreCommands() const { + auto lock = std::lock_guard{event_queue_lock_}; return !current_executing_actions_.empty() || !event_queue_.empty(); } @@ -113,6 +120,7 @@ void ActionManager::DumpState() const { } void ActionManager::ClearQueue() { + auto lock = std::lock_guard{event_queue_lock_}; // We are shutting down so don't claim the oneshot builtin actions back current_executing_actions_ = {}; event_queue_ = {}; diff --git a/init/action_manager.h b/init/action_manager.h index a2b95acad..b6f93d9b5 100644 --- a/init/action_manager.h +++ b/init/action_manager.h @@ -16,9 +16,12 @@ #pragma once +#include #include #include +#include + #include "action.h" #include "builtins.h" @@ -48,7 +51,9 @@ class ActionManager { void operator=(ActionManager const&) = delete; std::vector> actions_; - std::queue> event_queue_; + std::queue> event_queue_ + GUARDED_BY(event_queue_lock_); + mutable std::mutex event_queue_lock_; std::queue current_executing_actions_; std::size_t current_command_; }; diff --git a/init/init.cpp b/init/init.cpp index a7518fc2f..b29dfa3f6 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -33,7 +33,9 @@ #include #include #include +#include #include +#include #include #include @@ -95,14 +97,155 @@ static int property_triggers_enabled = 0; static int signal_fd = -1; static int property_fd = -1; -static std::unique_ptr waiting_for_prop(nullptr); -static std::string wait_prop_name; -static std::string wait_prop_value; -static std::string shutdown_command; -static bool do_shutdown = false; - static std::unique_ptr subcontext; +struct PendingControlMessage { + std::string message; + std::string name; + pid_t pid; + int fd; +}; +static std::mutex pending_control_messages_lock; +static std::queue pending_control_messages; + +// Init epolls various FDs to wait for various inputs. It previously waited on property changes +// with a blocking socket that contained the information related to the change, however, it was easy +// to fill that socket and deadlock the system. Now we use locks to handle the property changes +// directly in the property thread, however we still must wake the epoll to inform init that there +// is a change to process, so we use this FD. It is non-blocking, since we do not care how many +// times WakeEpoll() is called, only that the epoll will wake. +static int wake_epoll_fd = -1; +static void InstallInitNotifier(Epoll* epoll) { + int sockets[2]; + if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0, sockets) != 0) { + PLOG(FATAL) << "Failed to socketpair() between property_service and init"; + } + int epoll_fd = sockets[0]; + wake_epoll_fd = sockets[1]; + + auto drain_socket = [epoll_fd] { + char buf[512]; + while (read(epoll_fd, buf, sizeof(buf)) > 0) { + } + }; + + if (auto result = epoll->RegisterHandler(epoll_fd, drain_socket); !result.ok()) { + LOG(FATAL) << result.error(); + } +} + +static void WakeEpoll() { + constexpr char value[] = "1"; + write(wake_epoll_fd, value, sizeof(value)); +} + +static class PropWaiterState { + public: + bool StartWaiting(const char* name, const char* value) { + auto lock = std::lock_guard{lock_}; + if (waiting_for_prop_) { + return false; + } + if (GetProperty(name, "") != value) { + // Current property value is not equal to expected value + wait_prop_name_ = name; + wait_prop_value_ = value; + waiting_for_prop_.reset(new Timer()); + } else { + LOG(INFO) << "start_waiting_for_property(\"" << name << "\", \"" << value + << "\"): already set"; + } + return true; + } + + void ResetWaitForProp() { + auto lock = std::lock_guard{lock_}; + ResetWaitForPropLocked(); + } + + void CheckAndResetWait(const std::string& name, const std::string& value) { + auto lock = std::lock_guard{lock_}; + // We always record how long init waited for ueventd to tell us cold boot finished. + // If we aren't waiting on this property, it means that ueventd finished before we even + // started to wait. + if (name == kColdBootDoneProp) { + auto time_waited = waiting_for_prop_ ? waiting_for_prop_->duration().count() : 0; + std::thread([time_waited] { + SetProperty("ro.boottime.init.cold_boot_wait", std::to_string(time_waited)); + }).detach(); + } + + if (waiting_for_prop_) { + if (wait_prop_name_ == name && wait_prop_value_ == value) { + LOG(INFO) << "Wait for property '" << wait_prop_name_ << "=" << wait_prop_value_ + << "' took " << *waiting_for_prop_; + ResetWaitForPropLocked(); + WakeEpoll(); + } + } + } + + // This is not thread safe because it releases the lock when it returns, so the waiting state + // may change. However, we only use this function to prevent running commands in the main + // thread loop when we are waiting, so we do not care about false positives; only false + // negatives. StartWaiting() and this function are always called from the same thread, so false + // negatives are not possible and therefore we're okay. + bool MightBeWaiting() { + auto lock = std::lock_guard{lock_}; + return static_cast(waiting_for_prop_); + } + + private: + void ResetWaitForPropLocked() { + wait_prop_name_.clear(); + wait_prop_value_.clear(); + waiting_for_prop_.reset(); + } + + std::mutex lock_; + std::unique_ptr waiting_for_prop_{nullptr}; + std::string wait_prop_name_; + std::string wait_prop_value_; + +} prop_waiter_state; + +bool start_waiting_for_property(const char* name, const char* value) { + return prop_waiter_state.StartWaiting(name, value); +} + +void ResetWaitForProp() { + prop_waiter_state.ResetWaitForProp(); +} + +static class ShutdownState { + public: + void TriggerShutdown(const std::string& command) { + // We can't call HandlePowerctlMessage() directly in this function, + // because it modifies the contents of the action queue, which can cause the action queue + // to get into a bad state if this function is called from a command being executed by the + // action queue. Instead we set this flag and ensure that shutdown happens before the next + // command is run in the main init loop. + auto lock = std::lock_guard{shutdown_command_lock_}; + shutdown_command_ = command; + do_shutdown_ = true; + WakeEpoll(); + } + + std::optional CheckShutdown() { + auto lock = std::lock_guard{shutdown_command_lock_}; + if (do_shutdown_ && !IsShuttingDown()) { + do_shutdown_ = false; + return shutdown_command_; + } + return {}; + } + + private: + std::mutex shutdown_command_lock_; + std::string shutdown_command_; + bool do_shutdown_ = false; +} shutdown_state; + void DumpState() { ServiceList::GetInstance().DumpState(); ActionManager::GetInstance().DumpState(); @@ -156,39 +299,7 @@ static void LoadBootScripts(ActionManager& action_manager, ServiceList& service_ } } -bool start_waiting_for_property(const char* name, const char* value) { - if (waiting_for_prop) { - return false; - } - if (GetProperty(name, "") != value) { - // Current property value is not equal to expected value - wait_prop_name = name; - wait_prop_value = value; - waiting_for_prop.reset(new Timer()); - } else { - LOG(INFO) << "start_waiting_for_property(\"" << name << "\", \"" << value - << "\"): already set"; - } - return true; -} - -void ResetWaitForProp() { - wait_prop_name.clear(); - wait_prop_value.clear(); - waiting_for_prop.reset(); -} - -static void TriggerShutdown(const std::string& command) { - // We can't call HandlePowerctlMessage() directly in this function, - // because it modifies the contents of the action queue, which can cause the action queue - // to get into a bad state if this function is called from a command being executed by the - // action queue. Instead we set this flag and ensure that shutdown happens before the next - // command is run in the main init loop. - shutdown_command = command; - do_shutdown = true; -} - -void property_changed(const std::string& name, const std::string& value) { +void PropertyChanged(const std::string& name, const std::string& value) { // If the property is sys.powerctl, we bypass the event queue and immediately handle it. // This is to ensure that init will always and immediately shutdown/reboot, regardless of // if there are other pending events to process or if init is waiting on an exec service or @@ -196,26 +307,15 @@ void property_changed(const std::string& name, const std::string& value) { // In non-thermal-shutdown case, 'shutdown' trigger will be fired to let device specific // commands to be executed. if (name == "sys.powerctl") { - TriggerShutdown(value); + trigger_shutdown(value); } - if (property_triggers_enabled) ActionManager::GetInstance().QueuePropertyChange(name, value); - - // We always record how long init waited for ueventd to tell us cold boot finished. - // If we aren't waiting on this property, it means that ueventd finished before we even started - // to wait. - if (name == kColdBootDoneProp) { - auto time_waited = waiting_for_prop ? waiting_for_prop->duration().count() : 0; - SetProperty("ro.boottime.init.cold_boot_wait", std::to_string(time_waited)); + if (property_triggers_enabled) { + ActionManager::GetInstance().QueuePropertyChange(name, value); + WakeEpoll(); } - if (waiting_for_prop) { - if (wait_prop_name == name && wait_prop_value == value) { - LOG(INFO) << "Wait for property '" << wait_prop_name << "=" << wait_prop_value - << "' took " << *waiting_for_prop; - ResetWaitForProp(); - } - } + prop_waiter_state.CheckAndResetWait(name, value); } static std::optional HandleProcessActions() { @@ -343,8 +443,46 @@ bool HandleControlMessage(const std::string& msg, const std::string& name, pid_t return true; } +bool QueueControlMessage(const std::string& message, const std::string& name, pid_t pid, int fd) { + auto lock = std::lock_guard{pending_control_messages_lock}; + if (pending_control_messages.size() > 100) { + LOG(ERROR) << "Too many pending control messages, dropped '" << message << "' for '" << name + << "' from pid: " << pid; + return false; + } + pending_control_messages.push({message, name, pid, fd}); + WakeEpoll(); + return true; +} + +static void HandleControlMessages() { + auto lock = std::unique_lock{pending_control_messages_lock}; + // Init historically would only execute handle one property message, including control messages + // in each iteration of its main loop. We retain this behavior here to prevent starvation of + // other actions in the main loop. + if (!pending_control_messages.empty()) { + auto control_message = pending_control_messages.front(); + pending_control_messages.pop(); + lock.unlock(); + + bool success = HandleControlMessage(control_message.message, control_message.name, + control_message.pid); + + uint32_t response = success ? PROP_SUCCESS : PROP_ERROR_HANDLE_CONTROL_MESSAGE; + if (control_message.fd != -1) { + TEMP_FAILURE_RETRY(send(control_message.fd, &response, sizeof(response), 0)); + close(control_message.fd); + } + lock.lock(); + } + // If we still have items to process, make sure we wake back up to do so. + if (!pending_control_messages.empty()) { + WakeEpoll(); + } +} + static Result wait_for_coldboot_done_action(const BuiltinArguments& args) { - if (!start_waiting_for_property(kColdBootDoneProp, "true")) { + if (!prop_waiter_state.StartWaiting(kColdBootDoneProp, "true")) { LOG(FATAL) << "Could not wait for '" << kColdBootDoneProp << "'"; } @@ -562,60 +700,6 @@ void SendLoadPersistentPropertiesMessage() { } } -void SendStopSendingMessagesMessage() { - auto init_message = InitMessage{}; - init_message.set_stop_sending_messages(true); - if (auto result = SendMessage(property_fd, init_message); !result.ok()) { - LOG(ERROR) << "Failed to send 'stop sending messages' message: " << result.error(); - } -} - -void SendStartSendingMessagesMessage() { - auto init_message = InitMessage{}; - init_message.set_start_sending_messages(true); - if (auto result = SendMessage(property_fd, init_message); !result.ok()) { - LOG(ERROR) << "Failed to send 'start sending messages' message: " << result.error(); - } -} - -static void HandlePropertyFd() { - auto message = ReadMessage(property_fd); - if (!message.ok()) { - LOG(ERROR) << "Could not read message from property service: " << message.error(); - return; - } - - auto property_message = PropertyMessage{}; - if (!property_message.ParseFromString(*message)) { - LOG(ERROR) << "Could not parse message from property service"; - return; - } - - switch (property_message.msg_case()) { - case PropertyMessage::kControlMessage: { - auto& control_message = property_message.control_message(); - bool success = HandleControlMessage(control_message.msg(), control_message.name(), - control_message.pid()); - - uint32_t response = success ? PROP_SUCCESS : PROP_ERROR_HANDLE_CONTROL_MESSAGE; - if (control_message.has_fd()) { - int fd = control_message.fd(); - TEMP_FAILURE_RETRY(send(fd, &response, sizeof(response), 0)); - close(fd); - } - break; - } - case PropertyMessage::kChangedMessage: { - auto& changed_message = property_message.changed_message(); - property_changed(changed_message.name(), changed_message.value()); - break; - } - default: - LOG(ERROR) << "Unknown message type from property service: " - << property_message.msg_case(); - } -} - int SecondStageMain(int argc, char** argv) { if (REBOOT_BOOTLOADER_ON_PANIC) { InstallRebootSignalHandlers(); @@ -623,7 +707,7 @@ int SecondStageMain(int argc, char** argv) { boot_clock::time_point start_time = boot_clock::now(); - trigger_shutdown = TriggerShutdown; + trigger_shutdown = [](const std::string& command) { shutdown_state.TriggerShutdown(command); }; SetStdioToDevNull(argv); InitKernelLogging(argv); @@ -683,11 +767,8 @@ int SecondStageMain(int argc, char** argv) { } InstallSignalFdHandler(&epoll); - + InstallInitNotifier(&epoll); StartPropertyService(&property_fd); - if (auto result = epoll.RegisterHandler(property_fd, HandlePropertyFd); !result.ok()) { - LOG(FATAL) << "Could not register epoll handler for property fd: " << result.error(); - } // Make the time that init stages started available for bootstat to log. RecordStageBoottimes(start_time); @@ -770,12 +851,12 @@ int SecondStageMain(int argc, char** argv) { // By default, sleep until something happens. auto epoll_timeout = std::optional{}; - if (do_shutdown && !IsShuttingDown()) { - do_shutdown = false; - HandlePowerctlMessage(shutdown_command); + auto shutdown_command = shutdown_state.CheckShutdown(); + if (shutdown_command) { + HandlePowerctlMessage(*shutdown_command); } - if (!(waiting_for_prop || Service::is_exec_service_running())) { + if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { am.ExecuteOneCommand(); } if (!IsShuttingDown()) { @@ -789,7 +870,7 @@ int SecondStageMain(int argc, char** argv) { } } - if (!(waiting_for_prop || Service::is_exec_service_running())) { + if (!(prop_waiter_state.MightBeWaiting() || Service::is_exec_service_running())) { // If there's more work to do, wake up again immediately. if (am.HasMoreCommands()) epoll_timeout = 0ms; } @@ -806,6 +887,7 @@ int SecondStageMain(int argc, char** argv) { (*function)(); } } + HandleControlMessages(); } return 0; diff --git a/init/init.h b/init/init.h index 4bbca6f3c..27f64e297 100644 --- a/init/init.h +++ b/init/init.h @@ -38,8 +38,9 @@ void DumpState(); void ResetWaitForProp(); void SendLoadPersistentPropertiesMessage(); -void SendStopSendingMessagesMessage(); -void SendStartSendingMessagesMessage(); + +void PropertyChanged(const std::string& name, const std::string& value); +bool QueueControlMessage(const std::string& message, const std::string& name, pid_t pid, int fd); int SecondStageMain(int argc, char** argv); diff --git a/init/property_service.cpp b/init/property_service.cpp index 730bf6de4..820652249 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -92,8 +92,11 @@ namespace init { static bool persistent_properties_loaded = false; static int property_set_fd = -1; +static int from_init_socket = -1; static int init_socket = -1; static bool accept_messages = false; +static std::mutex accept_messages_lock; +static std::thread property_service_thread; static PropertyInfoAreaFile property_info_area; @@ -115,6 +118,16 @@ static int PropertyAuditCallback(void* data, security_class_t /*cls*/, char* buf return 0; } +void StartSendingMessages() { + auto lock = std::lock_guard{accept_messages_lock}; + accept_messages = true; +} + +void StopSendingMessages() { + auto lock = std::lock_guard{accept_messages_lock}; + accept_messages = true; +} + bool CanReadProperty(const std::string& source_context, const std::string& name) { const char* target_context = nullptr; property_info_area->GetPropertyInfo(name.c_str(), &target_context, nullptr); @@ -147,17 +160,6 @@ static bool CheckMacPerms(const std::string& name, const char* target_context, return has_access; } -static void SendPropertyChanged(const std::string& name, const std::string& value) { - auto property_msg = PropertyMessage{}; - auto* changed_message = property_msg.mutable_changed_message(); - changed_message->set_name(name); - changed_message->set_value(value); - - if (auto result = SendMessage(init_socket, property_msg); !result.ok()) { - LOG(ERROR) << "Failed to send property changed message: " << result.error(); - } -} - static uint32_t PropertySet(const std::string& name, const std::string& value, std::string* error) { size_t valuelen = value.size(); @@ -195,8 +197,9 @@ static uint32_t PropertySet(const std::string& name, const std::string& value, s } // If init hasn't started its main loop, then it won't be handling property changed messages // anyway, so there's no need to try to send them. + auto lock = std::lock_guard{accept_messages_lock}; if (accept_messages) { - SendPropertyChanged(name, value); + PropertyChanged(name, value); } return PROP_SUCCESS; } @@ -373,33 +376,24 @@ class SocketConnection { static uint32_t SendControlMessage(const std::string& msg, const std::string& name, pid_t pid, SocketConnection* socket, std::string* error) { + auto lock = std::lock_guard{accept_messages_lock}; if (!accept_messages) { *error = "Received control message after shutdown, ignoring"; return PROP_ERROR_HANDLE_CONTROL_MESSAGE; } - auto property_msg = PropertyMessage{}; - auto* control_message = property_msg.mutable_control_message(); - control_message->set_msg(msg); - control_message->set_name(name); - control_message->set_pid(pid); - // We must release the fd before sending it to init, otherwise there will be a race with init. // If init calls close() before Release(), then fdsan will see the wrong tag and abort(). int fd = -1; if (socket != nullptr && SelinuxGetVendorAndroidVersion() > __ANDROID_API_Q__) { fd = socket->Release(); - control_message->set_fd(fd); } - if (auto result = SendMessage(init_socket, property_msg); !result.ok()) { - // We've already released the fd above, so if we fail to send the message to init, we need - // to manually free it here. - if (fd != -1) { - close(fd); - } - *error = "Failed to send control message: " + result.error().message(); - return PROP_ERROR_HANDLE_CONTROL_MESSAGE; + bool queue_success = QueueControlMessage(msg, name, pid, fd); + if (!queue_success && fd != -1) { + uint32_t response = PROP_ERROR_HANDLE_CONTROL_MESSAGE; + TEMP_FAILURE_RETRY(send(fd, &response, sizeof(response), 0)); + close(fd); } return PROP_SUCCESS; @@ -1110,14 +1104,6 @@ static void HandleInitSocket() { persistent_properties_loaded = true; break; } - case InitMessage::kStopSendingMessages: { - accept_messages = false; - break; - } - case InitMessage::kStartSendingMessages: { - accept_messages = true; - break; - } default: LOG(ERROR) << "Unknown message type from init: " << init_message.msg_case(); } @@ -1157,9 +1143,9 @@ void StartPropertyService(int* epoll_socket) { if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sockets) != 0) { PLOG(FATAL) << "Failed to socketpair() between property_service and init"; } - *epoll_socket = sockets[0]; + *epoll_socket = from_init_socket = sockets[0]; init_socket = sockets[1]; - accept_messages = true; + StartSendingMessages(); if (auto result = CreateSocket(PROP_SERVICE_NAME, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, false, 0666, 0, 0, {}); @@ -1171,7 +1157,8 @@ void StartPropertyService(int* epoll_socket) { listen(property_set_fd, 8); - std::thread{PropertyServiceThread}.detach(); + auto new_thread = std::thread{PropertyServiceThread}; + property_service_thread.swap(new_thread); } } // namespace init diff --git a/init/property_service.h b/init/property_service.h index 506d116e1..2d49a36fa 100644 --- a/init/property_service.h +++ b/init/property_service.h @@ -32,5 +32,8 @@ bool CanReadProperty(const std::string& source_context, const std::string& name) void PropertyInit(); void StartPropertyService(int* epoll_socket); +void StartSendingMessages(); +void StopSendingMessages(); + } // namespace init } // namespace android diff --git a/init/reboot.cpp b/init/reboot.cpp index 8d8bd8ee6..f006df3a3 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -59,6 +59,7 @@ #include "builtin_arguments.h" #include "init.h" #include "mount_namespace.h" +#include "property_service.h" #include "reboot_utils.h" #include "service.h" #include "service_list.h" @@ -711,17 +712,12 @@ static void EnterShutdown() { for (const auto& s : ServiceList::GetInstance()) { s->UnSetExec(); } - // We no longer process messages about properties changing coming from property service, so we - // need to tell property service to stop sending us these messages, otherwise it'll fill the - // buffers and block indefinitely, causing future property sets, including those that init makes - // during shutdown in Service::NotifyStateChange() to also block indefinitely. - SendStopSendingMessagesMessage(); } static void LeaveShutdown() { LOG(INFO) << "Leaving shutdown mode"; shutting_down = false; - SendStartSendingMessagesMessage(); + StartSendingMessages(); } static Result UnmountAllApexes() { @@ -981,6 +977,10 @@ void HandlePowerctlMessage(const std::string& command) { return; } + // We do not want to process any messages (queue'ing triggers, shutdown messages, control + // messages, etc) from properties during reboot. + StopSendingMessages(); + if (userspace_reboot) { HandleUserspaceReboot(); return;