Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c94ce7b1 authored by Tom Cherry's avatar Tom Cherry
Browse files

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
parent 243ab9cc

init/devices.cpp

+29 −25
Original line number Original line Diff line number Diff line
@@ -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

// Given a path that may start with a platform device, find the parent platform device by finding a

// platform device prefix.  If it doesn't start with a platform device, return false

// parent directory with a 'subsystem' symlink that points to the platform bus.

bool PlatformDeviceList::Find(const std::string& path, std::string* out_path) const {

// If it doesn't start with a platform device, return false

    out_path->clear();

bool DeviceHandler::FindPlatformDevice(std::string path, std::string* platform_device_path) const {

    // platform_devices is searched backwards, since parents are added before their children,

    platform_device_path->clear();

    // and we want to match as deep of a child as we can.



    for (auto it = platform_devices_.crbegin(); it != platform_devices_.crend(); ++it) {

    // Uevents don't contain the mount point, so we need to add it here.

        auto platform_device_path_length = it->length();

    path.insert(0, sysfs_mount_point_);

        if (platform_device_path_length < path.length() &&



            path[platform_device_path_length] == '/' &&

    std::string directory = android::base::Dirname(path);

            android::base::StartsWith(path, it->c_str())) {



            *out_path = *it;

    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;

            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;

    return false;

}

}
@@ -258,7 +271,7 @@ out: 




std::vector<std::string> DeviceHandler::GetCharacterDeviceSymlinks(const Uevent& uevent) const {

std::vector<std::string> DeviceHandler::GetCharacterDeviceSymlinks(const Uevent& uevent) const {

    std::string parent_device;

    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

    // skip path to the parent driver

    std::string path = uevent.path.substr(parent_device.length());

    std::string path = uevent.path.substr(parent_device.length());
@@ -316,7 +329,7 @@ std::vector<std::string> DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev 
    std::string device;

    std::string device;

    std::string type;

    std::string type;





    if (platform_devices_.Find(uevent.path, &device)) {

    if (FindPlatformDevice(uevent.path, &device)) {

        // Skip /devices/platform or /devices/ if present

        // Skip /devices/platform or /devices/ if present

        static const std::string devices_platform_prefix = "/devices/platform/";

        static const std::string devices_platform_prefix = "/devices/platform/";

        static const std::string devices_prefix = "/devices/";

        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 {

void DeviceHandler::HandleBlockDeviceEvent(const Uevent& uevent) const {

    // if it's not a /dev device, nothing to do

    // if it's not a /dev device, nothing to do

    if (uevent.major < 0 || uevent.minor < 0) return;

    if (uevent.major < 0 || uevent.minor < 0) return;
@@ -458,8 +463,6 @@ void DeviceHandler::HandleDeviceEvent(const Uevent& uevent) { 




    if (uevent.subsystem == "block") {

    if (uevent.subsystem == "block") {

        HandleBlockDeviceEvent(uevent);

        HandleBlockDeviceEvent(uevent);

    } else if (uevent.subsystem == "platform") {

        HandlePlatformDeviceEvent(uevent);

    } else {

    } else {

        HandleGenericDeviceEvent(uevent);

        HandleGenericDeviceEvent(uevent);

    }

    }
@@ -472,7 +475,8 @@ DeviceHandler::DeviceHandler(std::vector<Permissions> dev_permissions, 
      sysfs_permissions_(std::move(sysfs_permissions)),

      sysfs_permissions_(std::move(sysfs_permissions)),

      subsystems_(std::move(subsystems)),

      subsystems_(std::move(subsystems)),

      sehandle_(selinux_android_file_context_handle()),

      sehandle_(selinux_android_file_context_handle()),

      skip_restorecon_(skip_restorecon) {}

      skip_restorecon_(skip_restorecon),

      sysfs_mount_point_("/sys") {}





DeviceHandler::DeviceHandler()

DeviceHandler::DeviceHandler()

    : DeviceHandler(std::vector<Permissions>{}, std::vector<SysfsPermissions>{},

    : DeviceHandler(std::vector<Permissions>{}, std::vector<SysfsPermissions>{},

init/devices.h

+6 −21
Original line number Original line Diff line number Diff line
@@ -93,20 +93,6 @@ class Subsystem { 
    DevnameSource devname_source_;

    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 {

class DeviceHandler {

  public:

  public:

    friend class DeviceHandlerTester;

    friend class DeviceHandlerTester;
@@ -119,16 +105,11 @@ class DeviceHandler { 




    void HandleDeviceEvent(const Uevent& uevent);

    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;

    std::vector<std::string> GetBlockDeviceSymlinks(const Uevent& uevent) const;

    void set_skip_restorecon(bool value) { skip_restorecon_ = value; }

    void set_skip_restorecon(bool value) { skip_restorecon_ = value; }





  private:

  private:

    bool FindPlatformDevice(std::string path, std::string* platform_device_path) const;

    std::tuple<mode_t, uid_t, gid_t> GetDevicePermissions(

    std::tuple<mode_t, uid_t, gid_t> GetDevicePermissions(

        const std::string& path, const std::vector<std::string>& links) const;

        const std::string& path, const std::vector<std::string>& links) const;

    void MakeDevice(const std::string& path, int block, int major, int minor,

    void MakeDevice(const std::string& path, int block, int major, int minor,
@@ -136,13 +117,17 @@ class DeviceHandler { 
    std::vector<std::string> GetCharacterDeviceSymlinks(const Uevent& uevent) const;

    std::vector<std::string> GetCharacterDeviceSymlinks(const Uevent& uevent) const;

    void HandleDevice(const std::string& action, const std::string& devpath, int block, int major,

    void HandleDevice(const std::string& action, const std::string& devpath, int block, int major,

                      int minor, const std::vector<std::string>& links) const;

                      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<Permissions> dev_permissions_;

    std::vector<SysfsPermissions> sysfs_permissions_;

    std::vector<SysfsPermissions> sysfs_permissions_;

    std::vector<Subsystem> subsystems_;

    std::vector<Subsystem> subsystems_;

    PlatformDeviceList platform_devices_;

    selabel_handle* sehandle_;

    selabel_handle* sehandle_;

    bool skip_restorecon_;

    bool skip_restorecon_;

    std::string sysfs_mount_point_;

};

};





// Exposed for testing

// Exposed for testing

init/devices_test.cpp

+16 −44
Original line number Original line Diff line number Diff line
@@ -16,33 +16,29 @@ 




#include "devices.h"

#include "devices.h"





#include <string>

#include <vector>



#include <android-base/scopeguard.h>

#include <android-base/scopeguard.h>

#include <android-base/test_utils.h>

#include <gtest/gtest.h>

#include <gtest/gtest.h>





#include "util.h"



using namespace std::string_literals;



class DeviceHandlerTester {

class DeviceHandlerTester {

  public:

  public:

    void AddPlatformDevice(const std::string& path) {

    void TestGetSymlinks(const std::string& platform_device, const Uevent& uevent,

        Uevent uevent = {

                         const std::vector<std::string> expected_links, bool block) {

            .action = "add", .subsystem = "platform", .path = path,

        TemporaryDir fake_sys_root;

        };

        device_handler_.sysfs_mount_point_ = fake_sys_root.path;

        device_handler_.HandlePlatformDeviceEvent(uevent);

    }





    void RemovePlatformDevice(const std::string& path) {

        std::string platform_device_dir = fake_sys_root.path + platform_device;

        Uevent uevent = {

        mkdir_recursive(platform_device_dir, 0777, nullptr);

            .action = "remove", .subsystem = "platform", .path = path,

        };

        device_handler_.HandlePlatformDeviceEvent(uevent);

    }





    void TestGetSymlinks(const std::string& platform_device_name, const Uevent& uevent,

        std::string platform_bus = fake_sys_root.path + "/bus/platform"s;

                         const std::vector<std::string> expected_links, bool block) {

        mkdir_recursive(platform_bus, 0777, nullptr);

        AddPlatformDevice(platform_device_name);

        symlink(platform_bus.c_str(), (platform_device_dir + "/subsystem").c_str());

        auto platform_device_remover = android::base::make_scope_guard(



            [this, &platform_device_name]() { RemovePlatformDevice(platform_device_name); });

        mkdir_recursive(android::base::Dirname(fake_sys_root.path + uevent.path), 0777, nullptr);





        std::vector<std::string> result;

        std::vector<std::string> result;

        if (block) {

        if (block) {
@@ -65,30 +61,6 @@ class DeviceHandlerTester { 
    DeviceHandler device_handler_;

    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) {

TEST(device_handler, get_character_device_symlinks_success) {

    const char* platform_device = "/devices/platform/some_device_name";

    const char* platform_device = "/devices/platform/some_device_name";

    Uevent uevent = {

    Uevent uevent = {

init/init_first_stage.cpp

+0 −6
Original line number Original line Diff line number Diff line
@@ -181,12 +181,6 @@ void FirstStageMount::InitRequiredDevices() { 
}

}





RegenerationAction FirstStageMount::UeventCallback(const Uevent& uevent) {

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.

    // Ignores everything that is not a block device.

    if (uevent.subsystem != "block") {

    if (uevent.subsystem != "block") {

        return RegenerationAction::kContinue;

        return RegenerationAction::kContinue;

init/ueventd.cpp

+1 −17
Original line number Original line Diff line number Diff line
@@ -128,15 +128,7 @@ class ColdBoot { 
void ColdBoot::UeventHandlerMain(unsigned int process_num, unsigned int total_processes) {

void ColdBoot::UeventHandlerMain(unsigned int process_num, unsigned int total_processes) {

    for (unsigned int i = process_num; i < uevent_queue_.size(); i += total_processes) {

    for (unsigned int i = process_num; i < uevent_queue_.size(); i += total_processes) {

        auto& uevent = uevent_queue_[i];

        auto& uevent = uevent_queue_[i];

        if (uevent.action == "add" || uevent.action == "change" || uevent.action == "online") {

        device_handler_.HandleDeviceEvent(uevent);

            device_handler_.FixupSysPermissions(uevent.path, uevent.subsystem);

        }



        if (uevent.subsystem == "block") {

            device_handler_.HandleBlockDeviceEvent(uevent);

        } else {

            device_handler_.HandleGenericDeviceEvent(uevent);

        }

    }

    }

    _exit(EXIT_SUCCESS);

    _exit(EXIT_SUCCESS);

}

}
@@ -145,14 +137,6 @@ void ColdBoot::RegenerateUevents() { 
    uevent_listener_.RegenerateUevents([this](const Uevent& uevent) {

    uevent_listener_.RegenerateUevents([this](const Uevent& uevent) {

        HandleFirmwareEvent(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));

        uevent_queue_.emplace_back(std::move(uevent));

        return RegenerationAction::kContinue;

        return RegenerationAction::kContinue;

    });

    });