From 0188274148f1462bfdc1c95457a44e2458fffa5a Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 10 Mar 2020 09:08:02 -0700 Subject: [PATCH] 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 {