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
Merged-In: I21636355d8e434f30e0cba568598a6cf139e67f9
Change-Id: I21636355d8e434f30e0cba568598a6cf139e67f9
(cherry picked from commit c94ce7b130979317d9523ba0d175f0ad369fb1c4)
diff --git a/init/devices.cpp b/init/devices.cpp
index c52d8f8..2943fb7 100644
--- a/init/devices.cpp
+++ b/init/devices.cpp
@@ -147,21 +147,34 @@
}
}
-// 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 @@
std::vector<std::string> 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::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::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 @@
if (uevent.subsystem == "block") {
HandleBlockDeviceEvent(uevent);
- } else if (uevent.subsystem == "platform") {
- HandlePlatformDeviceEvent(uevent);
} else {
HandleGenericDeviceEvent(uevent);
}
@@ -472,7 +475,8 @@
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<Permissions>{}, std::vector<SysfsPermissions>{},
diff --git a/init/devices.h b/init/devices.h
index 09a0ce3..362c38c 100644
--- a/init/devices.h
+++ b/init/devices.h
@@ -93,20 +93,6 @@
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<std::string> platform_devices_;
-};
-
class DeviceHandler {
public:
friend class DeviceHandlerTester;
@@ -119,16 +105,11 @@
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<std::string> 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<mode_t, uid_t, gid_t> GetDevicePermissions(
const std::string& path, const std::vector<std::string>& links) const;
void MakeDevice(const std::string& path, int block, int major, int minor,
@@ -136,13 +117,17 @@
std::vector<std::string> GetCharacterDeviceSymlinks(const Uevent& uevent) const;
void HandleDevice(const std::string& action, const std::string& devpath, int block, int major,
int minor, const std::vector<std::string>& 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<Permissions> dev_permissions_;
std::vector<SysfsPermissions> sysfs_permissions_;
std::vector<Subsystem> 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 41b101b..e1e4e49 100644
--- a/init/devices_test.cpp
+++ b/init/devices_test.cpp
@@ -16,33 +16,29 @@
#include "devices.h"
-#include <string>
-#include <vector>
-
#include <android-base/scopeguard.h>
+#include <android-base/test_utils.h>
#include <gtest/gtest.h>
+#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<std::string> 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<std::string> result;
if (block) {
@@ -65,30 +61,6 @@
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 0f2b1f3..ebbe1cd 100644
--- a/init/init_first_stage.cpp
+++ b/init/init_first_stage.cpp
@@ -181,12 +181,6 @@
}
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 31e4106..8cf9326 100644
--- a/init/ueventd.cpp
+++ b/init/ueventd.cpp
@@ -128,15 +128,7 @@
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 @@
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;
});