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

Commit 924858cd authored by David Anderson's avatar David Anderson
Browse files

libdm: Improve the reliability of dm device paths. 

This fixes a race condition where WaitForFile() after
GetDmDevicePathByName appears to succeed, but a subsequent operation on
the path fails. This can happen when CreateDevice() is called
immediately after a call to DeleteDevice (from any process), and the
path is re-used, enqueuing udev events to remove and re-add the block
device.

The fix for this is to introduce a new variant of CreateDevice() that
has a timeout parameter. When the timeout is positive, CreateDevice()
will wait for a /dev/block/mapper/by-uuid symlink to be created, which
signals that ueventd has finished processing the operation.

ueventd will now create these by-uuid symlinks for device-mapper nodes.
Unfortunately, the uuid is only available during "change" events, so we
have to special case device-mapper symlink creation. And since the uuid
is not available during "remove" events, we simply find matching links
to remove them.

This ensures that callers of CreateDevice() can use the device path
knowing that no asynchronous removals are pending. Code that uses the
old CreateDevice+WaitForFile pattern will be transitioned to the new
method.

Note that it is safe to ignore the timeout, or to use the "unsafe"
CreateDevice, if the caller ensures the path by other means. For example
first-stage init has no device removal, and regenerates uevents until
it has acquired all the paths it needs.

Finally, since libdm now inspects sysfs unconditionally, libdm consumers
need r_dir_file perms for sysfs_dm in their sepolicy. Additionally
linking to libdm now requires linking to libext2_uuid.

Bug: 135771280
Test: libdm_test
      device flashes, boots
Change-Id: If5a7383ea38f32a7fbbcf24842dce6a668050a70
parent 4280165a

fs_mgr/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -73,6 +73,7 @@ cc_library { 
    whole_static_libs: [

        "liblogwrap",

        "libdm",

        "libext2_uuid",

        "libfstab",

    ],

    cppflags: [

fs_mgr/fs_mgr_dm_linear.cpp

+1 −11
Original line number Diff line number Diff line
@@ -122,19 +122,9 @@ static bool CreateLogicalPartition(const LpMetadata& metadata, const LpMetadataP 
        table.set_readonly(false);

    }

    std::string name = GetPartitionName(partition);

    if (!dm.CreateDevice(name, table)) {

    if (!dm.CreateDevice(name, table, path, timeout_ms)) {

        return false;

    }

    if (!dm.GetDmDevicePathByName(name, path)) {

        return false;

    }

    if (timeout_ms > std::chrono::milliseconds::zero()) {

        if (!WaitForFile(*path, timeout_ms)) {

            DestroyLogicalPartition(name, {});

            LERROR << "Timed out waiting for device path: " << *path;

            return false;

        }

    }

    LINFO << "Created logical partition " << name << " on device " << *path;

    return true;

}

fs_mgr/libdm/Android.bp

+4 −0
Original line number Diff line number Diff line
@@ -29,6 +29,9 @@ cc_library_static { 
        "loop_control.cpp",

    ],



    static_libs: [

        "libext2_uuid",

    ],

    header_libs: [

        "libbase_headers",

        "liblog_headers",
@@ -46,6 +49,7 @@ cc_test { 
    static_libs: [

        "libdm",

        "libbase",

        "libext2_uuid",

        "libfs_mgr",

        "liblog",

    ],

fs_mgr/libdm/dm.cpp

+90 −20
Original line number Diff line number Diff line
@@ -20,12 +20,20 @@ 
#include <sys/sysmacros.h>

#include <sys/types.h>



#include <functional>

#include <thread>



#include <android-base/file.h>

#include <android-base/logging.h>

#include <android-base/macros.h>

#include <android-base/strings.h>

#include <uuid/uuid.h>



namespace android {

namespace dm {



using namespace std::literals;



DeviceMapper::DeviceMapper() : fd_(-1) {

    fd_ = TEMP_FAILURE_RETRY(open("/dev/device-mapper", O_RDWR | O_CLOEXEC));

    if (fd_ < 0) {
@@ -37,13 +45,13 @@ DeviceMapper& DeviceMapper::Instance() { 
    static DeviceMapper instance;

    return instance;

}



// Creates a new device mapper device

bool DeviceMapper::CreateDevice(const std::string& name) {

bool DeviceMapper::CreateDevice(const std::string& name, const std::string& uuid) {

    if (name.empty()) {

        LOG(ERROR) << "Unnamed device mapper device creation is not supported";

        return false;

    }



    if (name.size() >= DM_NAME_LEN) {

        LOG(ERROR) << "[" << name << "] is too long to be device mapper name";

        return false;
@@ -51,6 +59,9 @@ bool DeviceMapper::CreateDevice(const std::string& name) { 


    struct dm_ioctl io;

    InitIo(&io, name);

    if (!uuid.empty()) {

        snprintf(io.uuid, sizeof(io.uuid), "%s", uuid.c_str());

    }



    if (ioctl(fd_, DM_DEV_CREATE, &io)) {

        PLOG(ERROR) << "DM_DEV_CREATE failed for [" << name << "]";
@@ -67,16 +78,6 @@ bool DeviceMapper::CreateDevice(const std::string& name) { 
}



bool DeviceMapper::DeleteDevice(const std::string& name) {

    if (name.empty()) {

        LOG(ERROR) << "Unnamed device mapper device creation is not supported";

        return false;

    }



    if (name.size() >= DM_NAME_LEN) {

        LOG(ERROR) << "[" << name << "] is too long to be device mapper name";

        return false;

    }



    struct dm_ioctl io;

    InitIo(&io, name);
@@ -93,9 +94,81 @@ bool DeviceMapper::DeleteDevice(const std::string& name) { 
    return true;

}



const std::unique_ptr<DmTable> DeviceMapper::table(const std::string& /* name */) const {

    // TODO(b/110035986): Return the table, as read from the kernel instead

    return nullptr;

bool WaitForCondition(const std::function<bool()>& condition,

                      const std::chrono::milliseconds& timeout_ms) {

    auto start_time = std::chrono::steady_clock::now();

    while (true) {

        if (condition()) return true;



        std::this_thread::sleep_for(20ms);



        auto now = std::chrono::steady_clock::now();

        auto time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - start_time);

        if (time_elapsed > timeout_ms) return false;

    }

}



static std::string GenerateUuid() {

    uuid_t uuid_bytes;

    uuid_generate(uuid_bytes);



    char uuid_chars[37] = {};

    uuid_unparse_lower(uuid_bytes, uuid_chars);



    return std::string{uuid_chars};

}



bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table, std::string* path,

                                const std::chrono::milliseconds& timeout_ms) {

    std::string uuid = GenerateUuid();

    if (!CreateDevice(name, uuid)) {

        return false;

    }



    // We use the unique path for testing whether the device is ready. After

    // that, it's safe to use the dm-N path which is compatible with callers

    // that expect it to be formatted as such.

    std::string unique_path;

    if (!LoadTableAndActivate(name, table) || !GetDeviceUniquePath(name, &unique_path) ||

        !GetDmDevicePathByName(name, path)) {

        DeleteDevice(name);

        return false;

    }



    if (timeout_ms <= std::chrono::milliseconds::zero()) {

        return true;

    }



    auto condition = [&]() -> bool {

        // If the file exists but returns EPERM or something, we consider the

        // condition met.

        if (access(unique_path.c_str(), F_OK) != 0) {

            if (errno == ENOENT) return false;

        }

        return true;

    };

    if (!WaitForCondition(condition, timeout_ms)) {

        LOG(ERROR) << "Timed out waiting for device path: " << unique_path;

        DeleteDevice(name);

        return false;

    }

    return true;

}



bool DeviceMapper::GetDeviceUniquePath(const std::string& name, std::string* path) {

    struct dm_ioctl io;

    InitIo(&io, name);

    if (ioctl(fd_, DM_DEV_STATUS, &io) < 0) {

        PLOG(ERROR) << "Failed to get device path: " << name;

        return false;

    }



    if (io.uuid[0] == '\0') {

        LOG(ERROR) << "Device does not have a unique path: " << name;

        return false;

    }

    *path = "/dev/block/mapper/by-uuid/"s + io.uuid;

    return true;

}



DmDeviceState DeviceMapper::GetState(const std::string& name) const {
@@ -111,11 +184,8 @@ DmDeviceState DeviceMapper::GetState(const std::string& name) const { 
}



bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table) {

    if (!CreateDevice(name)) {

        return false;

    }

    if (!LoadTableAndActivate(name, table)) {

        DeleteDevice(name);

    std::string ignore_path;

    if (!CreateDevice(name, table, &ignore_path, 0ms)) {

        return false;

    }

    return true;

fs_mgr/libdm/dm_test.cpp

+4 −28
Original line number Diff line number Diff line
@@ -52,10 +52,10 @@ class TempDevice { 
  public:

    TempDevice(const std::string& name, const DmTable& table)

        : dm_(DeviceMapper::Instance()), name_(name), valid_(false) {

        valid_ = dm_.CreateDevice(name, table);

        valid_ = dm_.CreateDevice(name, table, &path_, 5s);

    }

    TempDevice(TempDevice&& other) noexcept

        : dm_(other.dm_), name_(other.name_), valid_(other.valid_) {

        : dm_(other.dm_), name_(other.name_), path_(other.path_), valid_(other.valid_) {

        other.valid_ = false;

    }

    ~TempDevice() {
@@ -70,29 +70,7 @@ class TempDevice { 
        valid_ = false;

        return dm_.DeleteDevice(name_);

    }

    bool WaitForUdev() const {

        auto start_time = std::chrono::steady_clock::now();

        while (true) {

            if (!access(path().c_str(), F_OK)) {

                return true;

            }

            if (errno != ENOENT) {

                return false;

            }

            std::this_thread::sleep_for(50ms);

            std::chrono::duration elapsed = std::chrono::steady_clock::now() - start_time;

            if (elapsed >= 5s) {

                return false;

            }

        }

    }

    std::string path() const {

        std::string device_path;

        if (!dm_.GetDmDevicePathByName(name_, &device_path)) {

            return "";

        }

        return device_path;

    }

    std::string path() const { return path_; }

    const std::string& name() const { return name_; }

    bool valid() const { return valid_; }
@@ -109,6 +87,7 @@ class TempDevice { 
  private:

    DeviceMapper& dm_;

    std::string name_;

    std::string path_;

    bool valid_;

};
@@ -139,7 +118,6 @@ TEST(libdm, DmLinear) { 
    TempDevice dev("libdm-test-dm-linear", table);

    ASSERT_TRUE(dev.valid());

    ASSERT_FALSE(dev.path().empty());

    ASSERT_TRUE(dev.WaitForUdev());



    auto& dm = DeviceMapper::Instance();
@@ -290,7 +268,6 @@ void SnapshotTestHarness::SetupImpl() { 
    origin_dev_ = std::make_unique<TempDevice>("libdm-test-dm-snapshot-origin", origin_table);

    ASSERT_TRUE(origin_dev_->valid());

    ASSERT_FALSE(origin_dev_->path().empty());

    ASSERT_TRUE(origin_dev_->WaitForUdev());



    // chunk size = 4K blocks.

    DmTable snap_table;
@@ -302,7 +279,6 @@ void SnapshotTestHarness::SetupImpl() { 
    snapshot_dev_ = std::make_unique<TempDevice>("libdm-test-dm-snapshot", snap_table);

    ASSERT_TRUE(snapshot_dev_->valid());

    ASSERT_FALSE(snapshot_dev_->path().empty());

    ASSERT_TRUE(snapshot_dev_->WaitForUdev());



    setup_ok_ = true;

}