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

Commit ebd35be8 authored by Treehugger Robot's avatar Treehugger Robot Committed by Android (Google) Code Review
Browse files

Merge changes Ic8cf669c,I3df2889a,Id2d6045f,I8c15394d into main

* changes:
  ueventd: update the comment on parallel handling
  ueventd: make DeviceHandler::HandleUevent thread-safe
  libmodprobe: Remove unused ResetModuleCount function
  libmodprobe: add thread safety annotations
parents c7a304c9 e438de70
Loading
Loading
Loading
Loading
+7 −1
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@
#include <chrono>
#include <filesystem>
#include <memory>
#include <mutex>
#include <string>
#include <string_view>
#include <thread>
@@ -362,6 +363,7 @@ void DeviceHandler::TrackDeviceUevent(const Uevent& uevent) {
    std::string device;
    if (!Realpath(path, &device)) return;

    std::lock_guard<std::mutex> lock(device_update_lock_);
    tracked_uevents_.emplace_back(uevent, device);
}

@@ -733,10 +735,14 @@ void DeviceHandler::HandleUevent(const Uevent& uevent) {
    }

    if (uevent.action == "bind") {
        std::lock_guard<std::mutex> lock(device_update_lock_);

        bound_drivers_[uevent.path] = uevent.driver;
        HandleBindInternal(uevent.driver, "add", uevent);
        return;
    } else if (uevent.action == "unbind") {
        std::lock_guard<std::mutex> lock(device_update_lock_);

        if (bound_drivers_.count(uevent.path) == 0) return;
        HandleBindInternal(bound_drivers_[uevent.path], "remove", uevent);

@@ -811,8 +817,8 @@ DeviceHandler::DeviceHandler(std::vector<Permissions> dev_permissions,
      sysfs_permissions_(std::move(sysfs_permissions)),
      drivers_(std::move(drivers)),
      subsystems_(std::move(subsystems)),
      boot_devices_(std::move(boot_devices)),
      boot_part_uuid_(boot_part_uuid),
      boot_devices_(std::move(boot_devices)),
      skip_restorecon_(skip_restorecon),
      sysfs_mount_point_("/sys") {
    // If both a boot partition UUID and a list of boot devices are
+16 −10
Original line number Diff line number Diff line
@@ -22,11 +22,13 @@

#include <algorithm>
#include <map>
#include <mutex>
#include <set>
#include <string>
#include <vector>

#include <android-base/file.h>
#include <android-base/thread_annotations.h>
#include <selinux/label.h>

#include "uevent.h"
@@ -135,7 +137,7 @@ class DeviceHandler : public UeventHandler {
    virtual ~DeviceHandler() = default;

    bool CheckUeventForBootPartUuid(const Uevent& uevent);
    void HandleUevent(const Uevent& uevent) override;
    void HandleUevent(const Uevent& uevent) override EXCLUDES(device_update_lock_);

    // `androidboot.partition_map` allows associating a partition name for a raw block device
    // through a comma separated and semicolon deliminated list. For example,
@@ -169,21 +171,25 @@ class DeviceHandler : public UeventHandler {
    void FixupSysPermissions(const std::string& upath, const std::string& subsystem) const;
    void HandleAshmemUevent(const Uevent& uevent);

    void TrackDeviceUevent(const Uevent& uevent);
    void HandleBindInternal(std::string driver_name, std::string action, const Uevent& uevent);
    void TrackDeviceUevent(const Uevent& uevent) EXCLUDES(device_update_lock_);
    void HandleBindInternal(std::string driver_name, std::string action, const Uevent& uevent)
            EXCLUSIVE_LOCKS_REQUIRED(device_update_lock_);

    std::vector<Permissions> dev_permissions_;
    std::vector<SysfsPermissions> sysfs_permissions_;
    std::vector<Subsystem> drivers_;
    std::vector<Subsystem> subsystems_;
    const std::vector<Permissions> dev_permissions_;
    const std::vector<SysfsPermissions> sysfs_permissions_;
    const std::vector<Subsystem> drivers_;
    const std::vector<Subsystem> subsystems_;
    const std::string boot_part_uuid_;

    // These non const members are modified only at initialization or test
    std::set<std::string> boot_devices_;
    std::string boot_part_uuid_;
    bool found_boot_part_uuid_;
    bool skip_restorecon_;
    std::string sysfs_mount_point_;

    std::vector<TrackedUevent> tracked_uevents_;
    std::map<std::string, std::string> bound_drivers_;
    std::mutex device_update_lock_;
    std::vector<TrackedUevent> tracked_uevents_ GUARDED_BY(device_update_lock_);
    std::map<std::string, std::string> bound_drivers_ GUARDED_BY(device_update_lock_);
};

// Exposed for testing
+11 −8
Original line number Diff line number Diff line
@@ -68,14 +68,17 @@
// time during cold boot.

// Handling of uevent messages has two unique properties:
// 1) It can be done in isolation; it doesn't need to read or write any status once it is started.
// 2) It uses setegid() and setfscreatecon() so either care (aka locking) must be taken to ensure
//    that no file system operations are done while the uevent process has an abnormal egid or
//    fscreatecon or this handling must happen in a separate process.
// Given the above two properties, it is best to fork() subprocesses to handle the uevents.  This
// reduces the overhead and complexity that would be required in a solution with threads and locks.
// In testing, a racy multithreaded solution has the same performance as the fork() solution, so
// there is no reason to deal with the complexity of the former.
// 1) Messages can be handled in isolation when they do not depend on another. A device event
//    depends on another when the device is the same, parent, or child of another device which
//    should be handled first by another event. e.g. An add event must be handled first before
//    removal. A device foo must be added before foo/bar.
// 2) The cold boot is unlikely to have events that depend on another in a critical manner.
// Therefore, ueventd handles uevent message in parallel by fork() subprocesses. We chose fork()
// instead of threads, since selabel_lookup_best_match function was not thread-safe. It's
// been fixed today, and we are testing the thread-safety of selabel_lookup_best_match and other
// necessary functions (setfscreatecon and setegid syscall wrapper of bionic) in ueventd_test.
// However, we have not moved to thread-based parallelization. We didn't observe any significant
// performance gain with threads compared to multi-processes, and multi-process is simpler.

// One other important caveat during the boot process is the handling of SELinux restorecon.
// Since many devices have child devices, calling selinux_android_restorecon() recursively for each
+1 −0
Original line number Diff line number Diff line
@@ -6,6 +6,7 @@ cc_library_static {
    name: "libmodprobe",
    cflags: [
        "-Werror",
        "-Wthread-safety",
    ],
    vendor_available: true,
    ramdisk_available: true,
+12 −9
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@

#pragma once

#include <atomic>
#include <functional>
#include <mutex>
#include <set>
@@ -31,24 +32,24 @@ class Modprobe {
    Modprobe(const std::vector<std::string>&, const std::string load_file = "modules.load",
             bool use_blocklist = true);

    bool LoadModulesParallel(int num_threads);
    bool LoadModulesParallel(int num_threads) EXCLUDES(module_loaded_lock_);
    bool LoadListedModules(bool strict = true);
    bool LoadWithAliases(const std::string& module_name, bool strict,
                         const std::string& parameters = "");
                         const std::string& parameters = "") EXCLUDES(module_loaded_lock_);
    bool Remove(const std::string& module_name);
    std::vector<std::string> ListModules(const std::string& pattern);
    bool GetAllDependencies(const std::string& module, std::vector<std::string>* pre_dependencies,
                            std::vector<std::string>* dependencies,
                            std::vector<std::string>* post_dependencies);
    void ResetModuleCount() { module_count_ = 0; }
    int GetModuleCount() { return module_count_; }
    bool IsBlocklisted(const std::string& module_name);

  private:
    std::string MakeCanonical(const std::string& module_path);
    bool InsmodWithDeps(const std::string& module_name, const std::string& parameters);
    bool Insmod(const std::string& path_name, const std::string& parameters);
    bool Rmmod(const std::string& module_name);
    bool Insmod(const std::string& path_name, const std::string& parameters)
            EXCLUDES(module_loaded_lock_);
    bool Rmmod(const std::string& module_name) EXCLUDES(module_loaded_lock_);
    std::vector<std::string> GetDependencies(const std::string& module);
    bool ModuleExists(const std::string& module_name);
    void AddOption(const std::string& module_name, const std::string& option_name,
@@ -65,6 +66,7 @@ class Modprobe {
    void ParseKernelCmdlineOptions();
    void ParseCfg(const std::string& cfg, std::function<bool(const std::vector<std::string>&)> f);

    // These non const fields are initialized by the constructor and never be modified.
    std::vector<std::pair<std::string, std::string>> module_aliases_;
    std::unordered_map<std::string, std::vector<std::string>> module_deps_;
    std::vector<std::pair<std::string, std::string>> module_pre_softdep_;
@@ -72,9 +74,10 @@ class Modprobe {
    std::vector<std::string> module_load_;
    std::unordered_map<std::string, std::string> module_options_;
    std::set<std::string> module_blocklist_;

    std::mutex module_loaded_lock_;
    std::unordered_set<std::string> module_loaded_;
    std::unordered_set<std::string> module_loaded_paths_;
    int module_count_ = 0;
    bool blocklist_enabled = false;
    std::unordered_set<std::string> module_loaded_ GUARDED_BY(module_loaded_lock_);
    std::unordered_set<std::string> module_loaded_paths_ GUARDED_BY(module_loaded_lock_);
    std::atomic_int module_count_ = 0;
    const bool blocklist_enabled = false;
};