From c94ce7b130979317d9523ba0d175f0ad369fb1c4 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Fri, 16 Jun 2017 16:48:47 -0700 Subject: [PATCH] ueventd: remove PlatformDeviceList In order to create symlinks for USB and block devices, the path for their parent platform device must be known. Previously, ueventd would save each platform device that it encounters to a list and query this list when creating the symlinks. That, however, is racy because the uevent socket does not differentiate uevents from RegenerateUevents() and uevents sent by the kernel when probing a device first the first time. The below scenario is the faulty case: 1) Kernel probes parent platform device for a block device 2) ueventd calls RegenerateUevents() and starts processing uevents 3) Kernel probes block device and sends its uevents 4) ueventd picks up the block device uevent during its uevent processing, without yet regenerating the platform device uevent, causing improper symlinks to be created. This change stops storing the platform devices in a list, and instead traverses up the directory structure for each USB or block device until it reaches a platform device, defined as one whose subsystem is the platform bus. This fixes the race and simplifies the ueventd code. Bug: 62436493 Bug: 62681642 Test: Boot bullhead Test: Boot sailfish Test: Init unit tests Test: Boot hikey + hotplug/unplug sdcard Change-Id: I21636355d8e434f30e0cba568598a6cf139e67f9 --- init/devices.cpp | 54 ++++++++++++++++++---------------- init/devices.h | 27 ++++------------- init/devices_test.cpp | 62 +++++++++++---------------------------- init/init_first_stage.cpp | 6 ---- init/ueventd.cpp | 18 +----------- 5 files changed, 53 insertions(+), 114 deletions(-) diff --git a/init/devices.cpp b/init/devices.cpp index c52d8f860..2943fb732 100644 --- a/init/devices.cpp +++ b/init/devices.cpp @@ -147,21 +147,34 @@ void SysfsPermissions::SetPermissions(const std::string& path) const { } } -// Given a path that may start with a platform device, find the length of the -// platform device prefix. If it doesn't start with a platform device, return false -bool PlatformDeviceList::Find(const std::string& path, std::string* out_path) const { - out_path->clear(); - // platform_devices is searched backwards, since parents are added before their children, - // and we want to match as deep of a child as we can. - for (auto it = platform_devices_.crbegin(); it != platform_devices_.crend(); ++it) { - auto platform_device_path_length = it->length(); - if (platform_device_path_length < path.length() && - path[platform_device_path_length] == '/' && - android::base::StartsWith(path, it->c_str())) { - *out_path = *it; +// Given a path that may start with a platform device, find the parent platform device by finding a +// parent directory with a 'subsystem' symlink that points to the platform bus. +// If it doesn't start with a platform device, return false +bool DeviceHandler::FindPlatformDevice(std::string path, std::string* platform_device_path) const { + platform_device_path->clear(); + + // Uevents don't contain the mount point, so we need to add it here. + path.insert(0, sysfs_mount_point_); + + std::string directory = android::base::Dirname(path); + + while (directory != "/" && directory != ".") { + std::string subsystem_link_path; + if (android::base::Realpath(directory + "/subsystem", &subsystem_link_path) && + subsystem_link_path == sysfs_mount_point_ + "/bus/platform") { + // We need to remove the mount point that we added above before returning. + directory.erase(0, sysfs_mount_point_.size()); + *platform_device_path = directory; return true; } + + auto last_slash = path.rfind('/'); + if (last_slash == std::string::npos) return false; + + path.erase(last_slash); + directory = android::base::Dirname(path); } + return false; } @@ -258,7 +271,7 @@ out: std::vector DeviceHandler::GetCharacterDeviceSymlinks(const Uevent& uevent) const { std::string parent_device; - if (!platform_devices_.Find(uevent.path, &parent_device)) return {}; + if (!FindPlatformDevice(uevent.path, &parent_device)) return {}; // skip path to the parent driver std::string path = uevent.path.substr(parent_device.length()); @@ -316,7 +329,7 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev std::string device; std::string type; - if (platform_devices_.Find(uevent.path, &device)) { + if (FindPlatformDevice(uevent.path, &device)) { // Skip /devices/platform or /devices/ if present static const std::string devices_platform_prefix = "/devices/platform/"; static const std::string devices_prefix = "/devices/"; @@ -388,14 +401,6 @@ void DeviceHandler::HandleDevice(const std::string& action, const std::string& d } } -void DeviceHandler::HandlePlatformDeviceEvent(const Uevent& uevent) { - if (uevent.action == "add") { - platform_devices_.Add(uevent.path); - } else if (uevent.action == "remove") { - platform_devices_.Remove(uevent.path); - } -} - void DeviceHandler::HandleBlockDeviceEvent(const Uevent& uevent) const { // if it's not a /dev device, nothing to do if (uevent.major < 0 || uevent.minor < 0) return; @@ -458,8 +463,6 @@ void DeviceHandler::HandleDeviceEvent(const Uevent& uevent) { if (uevent.subsystem == "block") { HandleBlockDeviceEvent(uevent); - } else if (uevent.subsystem == "platform") { - HandlePlatformDeviceEvent(uevent); } else { HandleGenericDeviceEvent(uevent); } @@ -472,7 +475,8 @@ DeviceHandler::DeviceHandler(std::vector dev_permissions, sysfs_permissions_(std::move(sysfs_permissions)), subsystems_(std::move(subsystems)), sehandle_(selinux_android_file_context_handle()), - skip_restorecon_(skip_restorecon) {} + skip_restorecon_(skip_restorecon), + sysfs_mount_point_("/sys") {} DeviceHandler::DeviceHandler() : DeviceHandler(std::vector{}, std::vector{}, diff --git a/init/devices.h b/init/devices.h index 09a0ce3ee..362c38cb8 100644 --- a/init/devices.h +++ b/init/devices.h @@ -93,20 +93,6 @@ class Subsystem { DevnameSource devname_source_; }; -class PlatformDeviceList { - public: - void Add(const std::string& path) { platform_devices_.emplace_back(path); } - void Remove(const std::string& path) { - auto it = std::find(platform_devices_.begin(), platform_devices_.end(), path); - if (it != platform_devices_.end()) platform_devices_.erase(it); - } - bool Find(const std::string& path, std::string* out_path) const; - auto size() const { return platform_devices_.size(); } - - private: - std::vector platform_devices_; -}; - class DeviceHandler { public: friend class DeviceHandlerTester; @@ -119,16 +105,11 @@ class DeviceHandler { void HandleDeviceEvent(const Uevent& uevent); - void FixupSysPermissions(const std::string& upath, const std::string& subsystem) const; - - void HandlePlatformDeviceEvent(const Uevent& uevent); - void HandleBlockDeviceEvent(const Uevent& uevent) const; - void HandleGenericDeviceEvent(const Uevent& uevent) const; - std::vector GetBlockDeviceSymlinks(const Uevent& uevent) const; void set_skip_restorecon(bool value) { skip_restorecon_ = value; } private: + bool FindPlatformDevice(std::string path, std::string* platform_device_path) const; std::tuple GetDevicePermissions( const std::string& path, const std::vector& links) const; void MakeDevice(const std::string& path, int block, int major, int minor, @@ -136,13 +117,17 @@ class DeviceHandler { std::vector GetCharacterDeviceSymlinks(const Uevent& uevent) const; void HandleDevice(const std::string& action, const std::string& devpath, int block, int major, int minor, const std::vector& links) const; + void FixupSysPermissions(const std::string& upath, const std::string& subsystem) const; + + void HandleBlockDeviceEvent(const Uevent& uevent) const; + void HandleGenericDeviceEvent(const Uevent& uevent) const; std::vector dev_permissions_; std::vector sysfs_permissions_; std::vector subsystems_; - PlatformDeviceList platform_devices_; selabel_handle* sehandle_; bool skip_restorecon_; + std::string sysfs_mount_point_; }; // Exposed for testing diff --git a/init/devices_test.cpp b/init/devices_test.cpp index 41b101b22..e1e4e49f6 100644 --- a/init/devices_test.cpp +++ b/init/devices_test.cpp @@ -16,33 +16,29 @@ #include "devices.h" -#include -#include - #include +#include #include +#include "util.h" + +using namespace std::string_literals; + class DeviceHandlerTester { public: - void AddPlatformDevice(const std::string& path) { - Uevent uevent = { - .action = "add", .subsystem = "platform", .path = path, - }; - device_handler_.HandlePlatformDeviceEvent(uevent); - } - - void RemovePlatformDevice(const std::string& path) { - Uevent uevent = { - .action = "remove", .subsystem = "platform", .path = path, - }; - device_handler_.HandlePlatformDeviceEvent(uevent); - } - - void TestGetSymlinks(const std::string& platform_device_name, const Uevent& uevent, + void TestGetSymlinks(const std::string& platform_device, const Uevent& uevent, const std::vector expected_links, bool block) { - AddPlatformDevice(platform_device_name); - auto platform_device_remover = android::base::make_scope_guard( - [this, &platform_device_name]() { RemovePlatformDevice(platform_device_name); }); + TemporaryDir fake_sys_root; + device_handler_.sysfs_mount_point_ = fake_sys_root.path; + + std::string platform_device_dir = fake_sys_root.path + platform_device; + mkdir_recursive(platform_device_dir, 0777, nullptr); + + std::string platform_bus = fake_sys_root.path + "/bus/platform"s; + mkdir_recursive(platform_bus, 0777, nullptr); + symlink(platform_bus.c_str(), (platform_device_dir + "/subsystem").c_str()); + + mkdir_recursive(android::base::Dirname(fake_sys_root.path + uevent.path), 0777, nullptr); std::vector result; if (block) { @@ -65,30 +61,6 @@ class DeviceHandlerTester { DeviceHandler device_handler_; }; -TEST(device_handler, PlatformDeviceList) { - PlatformDeviceList platform_device_list; - - platform_device_list.Add("/devices/platform/some_device_name"); - platform_device_list.Add("/devices/platform/some_device_name/longer"); - platform_device_list.Add("/devices/platform/other_device_name"); - EXPECT_EQ(3U, platform_device_list.size()); - - std::string out_path; - EXPECT_FALSE(platform_device_list.Find("/devices/platform/not_found", &out_path)); - EXPECT_EQ("", out_path); - - EXPECT_FALSE(platform_device_list.Find("/devices/platform/some_device_name_with_same_prefix", - &out_path)); - - EXPECT_TRUE(platform_device_list.Find("/devices/platform/some_device_name/longer/longer_child", - &out_path)); - EXPECT_EQ("/devices/platform/some_device_name/longer", out_path); - - EXPECT_TRUE( - platform_device_list.Find("/devices/platform/some_device_name/other_child", &out_path)); - EXPECT_EQ("/devices/platform/some_device_name", out_path); -} - TEST(device_handler, get_character_device_symlinks_success) { const char* platform_device = "/devices/platform/some_device_name"; Uevent uevent = { diff --git a/init/init_first_stage.cpp b/init/init_first_stage.cpp index 0f2b1f3d4..ebbe1cd18 100644 --- a/init/init_first_stage.cpp +++ b/init/init_first_stage.cpp @@ -181,12 +181,6 @@ void FirstStageMount::InitRequiredDevices() { } RegenerationAction FirstStageMount::UeventCallback(const Uevent& uevent) { - // We need platform devices to create symlinks. - if (uevent.subsystem == "platform") { - device_handler_.HandleDeviceEvent(uevent); - return RegenerationAction::kContinue; - } - // Ignores everything that is not a block device. if (uevent.subsystem != "block") { return RegenerationAction::kContinue; diff --git a/init/ueventd.cpp b/init/ueventd.cpp index 31e41065b..8cf932675 100644 --- a/init/ueventd.cpp +++ b/init/ueventd.cpp @@ -128,15 +128,7 @@ class ColdBoot { void ColdBoot::UeventHandlerMain(unsigned int process_num, unsigned int total_processes) { for (unsigned int i = process_num; i < uevent_queue_.size(); i += total_processes) { auto& uevent = uevent_queue_[i]; - if (uevent.action == "add" || uevent.action == "change" || uevent.action == "online") { - device_handler_.FixupSysPermissions(uevent.path, uevent.subsystem); - } - - if (uevent.subsystem == "block") { - device_handler_.HandleBlockDeviceEvent(uevent); - } else { - device_handler_.HandleGenericDeviceEvent(uevent); - } + device_handler_.HandleDeviceEvent(uevent); } _exit(EXIT_SUCCESS); } @@ -145,14 +137,6 @@ void ColdBoot::RegenerateUevents() { uevent_listener_.RegenerateUevents([this](const Uevent& uevent) { HandleFirmwareEvent(uevent); - // This is the one mutable part of DeviceHandler, in which platform devices are - // added to a vector for later reference. Since there is no communication after - // fork()'ing subprocess handlers, all platform devices must be in the vector before - // we fork, and therefore they must be handled in this loop. - if (uevent.subsystem == "platform") { - device_handler_.HandlePlatformDeviceEvent(uevent); - } - uevent_queue_.emplace_back(std::move(uevent)); return RegenerationAction::kContinue; });