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

Commit 4866d284 authored by David Anderson's avatar David Anderson
Browse files

ueventd: Fix a race condition in handling device-mapper events. 

We've had flake in libdm_test for a long time, with no clear cause.
Lately however it has become particularly reproducible when running
the UeventAfterLoadTable test in isolation, and thus we've identified
the root cause.

uevents for device-mapper are fired when the sysfs node is added, but at
that time, the "dm" subnode has not yet been added. The root node and dm
node are added very close together, so usually it works, but sometimes
ueventd is too fast.

Instead of relying on sysfs, query the uuid/name node directly from
device-mapper.

Bug: 286011429
Test: libdm_test
(cherry picked from https://android-review.googlesource.com/q/commit:59abbfe64706a7ea0c4e932ae40bc8bde9746dce)
Merged-In: I258de5de05d813c3cb7f129e82e56dbfe8bf3117

Change-Id: I85e240807e0ce5ade211ec65453ab06d4066992a
parent 34d2ae6a

fs_mgr/libdm/dm.cpp

+19 −0
Original line number Original line Diff line number Diff line
@@ -243,6 +243,25 @@ bool DeviceMapper::GetDeviceUniquePath(const std::string& name, std::string* pat 
    return true;

    return true;

}

}





bool DeviceMapper::GetDeviceNameAndUuid(dev_t dev, std::string* name, std::string* uuid) {

    struct dm_ioctl io;

    InitIo(&io, {});

    io.dev = dev;



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

        PLOG(ERROR) << "Failed to find device dev: " << major(dev) << ":" << minor(dev);

        return false;

    }



    if (name) {

        *name = io.name;

    }

    if (uuid) {

        *uuid = io.uuid;

    }

    return true;

}



std::optional<DeviceMapper::Info> DeviceMapper::GetDetailedInfo(const std::string& name) const {

std::optional<DeviceMapper::Info> DeviceMapper::GetDetailedInfo(const std::string& name) const {

    struct dm_ioctl io;

    struct dm_ioctl io;

    InitIo(&io, name);

    InitIo(&io, name);

fs_mgr/libdm/dm_test.cpp

+65 −29
Original line number Original line Diff line number Diff line
@@ -30,6 +30,7 @@ 
#include <thread>

#include <thread>





#include <android-base/file.h>

#include <android-base/file.h>

#include <android-base/logging.h>

#include <android-base/scopeguard.h>

#include <android-base/scopeguard.h>

#include <android-base/strings.h>

#include <android-base/strings.h>

#include <android-base/unique_fd.h>

#include <android-base/unique_fd.h>
@@ -42,16 +43,40 @@ 
using namespace std;

using namespace std;

using namespace std::chrono_literals;

using namespace std::chrono_literals;

using namespace android::dm;

using namespace android::dm;

using unique_fd = android::base::unique_fd;

using android::base::make_scope_guard;

using android::base::unique_fd;





TEST(libdm, HasMinimumTargets) {

class DmTest : public ::testing::Test {

  protected:

    void SetUp() override {

        const testing::TestInfo* const test_info =

                testing::UnitTest::GetInstance()->current_test_info();

        test_name_ = test_info->name();

        test_full_name_ = test_info->test_suite_name() + "/"s + test_name_;



        LOG(INFO) << "Starting test: " << test_full_name_;

    }

    void TearDown() override {

        LOG(INFO) << "Tearing down test: " << test_full_name_;



        auto& dm = DeviceMapper::Instance();

        ASSERT_TRUE(dm.DeleteDeviceIfExists(test_name_));



        LOG(INFO) << "Teardown complete for test: " << test_full_name_;

    }



    std::string test_name_;

    std::string test_full_name_;

};



TEST_F(DmTest, HasMinimumTargets) {

    DmTargetTypeInfo info;

    DmTargetTypeInfo info;





    DeviceMapper& dm = DeviceMapper::Instance();

    DeviceMapper& dm = DeviceMapper::Instance();

    ASSERT_TRUE(dm.GetTargetByName("linear", &info));

    ASSERT_TRUE(dm.GetTargetByName("linear", &info));

}

}





TEST(libdm, DmLinear) {

TEST_F(DmTest, DmLinear) {

    unique_fd tmp1(CreateTempFile("file_1", 4096));

    unique_fd tmp1(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp1, 0);

    ASSERT_GE(tmp1, 0);

    unique_fd tmp2(CreateTempFile("file_2", 4096));

    unique_fd tmp2(CreateTempFile("file_2", 4096));
@@ -127,7 +152,7 @@ TEST(libdm, DmLinear) { 
    ASSERT_TRUE(dev.Destroy());

    ASSERT_TRUE(dev.Destroy());

}

}





TEST(libdm, DmSuspendResume) {

TEST_F(DmTest, DmSuspendResume) {

    unique_fd tmp1(CreateTempFile("file_suspend_resume", 512));

    unique_fd tmp1(CreateTempFile("file_suspend_resume", 512));

    ASSERT_GE(tmp1, 0);

    ASSERT_GE(tmp1, 0);
@@ -156,7 +181,7 @@ TEST(libdm, DmSuspendResume) { 
    ASSERT_EQ(dm.GetState(dev.name()), DmDeviceState::ACTIVE);

    ASSERT_EQ(dm.GetState(dev.name()), DmDeviceState::ACTIVE);

}

}





TEST(libdm, DmVerityArgsAvb2) {

TEST_F(DmTest, DmVerityArgsAvb2) {

    std::string device = "/dev/block/platform/soc/1da4000.ufshc/by-name/vendor_a";

    std::string device = "/dev/block/platform/soc/1da4000.ufshc/by-name/vendor_a";

    std::string algorithm = "sha1";

    std::string algorithm = "sha1";

    std::string digest = "4be7e823b8c40f7bd5c8ccd5123f0722c5baca21";

    std::string digest = "4be7e823b8c40f7bd5c8ccd5123f0722c5baca21";
@@ -178,7 +203,7 @@ TEST(libdm, DmVerityArgsAvb2) { 
    EXPECT_EQ(target.GetParameterString(), expected);

    EXPECT_EQ(target.GetParameterString(), expected);

}

}





TEST(libdm, DmSnapshotArgs) {

TEST_F(DmTest, DmSnapshotArgs) {

    DmTargetSnapshot target1(0, 512, "base", "cow", SnapshotStorageMode::Persistent, 8);

    DmTargetSnapshot target1(0, 512, "base", "cow", SnapshotStorageMode::Persistent, 8);

    if (DmTargetSnapshot::ReportsOverflow("snapshot")) {

    if (DmTargetSnapshot::ReportsOverflow("snapshot")) {

        EXPECT_EQ(target1.GetParameterString(), "base cow PO 8");

        EXPECT_EQ(target1.GetParameterString(), "base cow PO 8");
@@ -200,7 +225,7 @@ TEST(libdm, DmSnapshotArgs) { 
    EXPECT_EQ(target3.name(), "snapshot-merge");

    EXPECT_EQ(target3.name(), "snapshot-merge");

}

}





TEST(libdm, DmSnapshotOriginArgs) {

TEST_F(DmTest, DmSnapshotOriginArgs) {

    DmTargetSnapshotOrigin target(0, 512, "base");

    DmTargetSnapshotOrigin target(0, 512, "base");

    EXPECT_EQ(target.GetParameterString(), "base");

    EXPECT_EQ(target.GetParameterString(), "base");

    EXPECT_EQ(target.name(), "snapshot-origin");

    EXPECT_EQ(target.name(), "snapshot-origin");
@@ -330,7 +355,7 @@ bool CheckSnapshotAvailability() { 
    return true;

    return true;

}

}





TEST(libdm, DmSnapshot) {

TEST_F(DmTest, DmSnapshot) {

    if (!CheckSnapshotAvailability()) {

    if (!CheckSnapshotAvailability()) {

        return;

        return;

    }

    }
@@ -374,7 +399,7 @@ TEST(libdm, DmSnapshot) { 
    ASSERT_EQ(read, data);

    ASSERT_EQ(read, data);

}

}





TEST(libdm, DmSnapshotOverflow) {

TEST_F(DmTest, DmSnapshotOverflow) {

    if (!CheckSnapshotAvailability()) {

    if (!CheckSnapshotAvailability()) {

        return;

        return;

    }

    }
@@ -421,7 +446,7 @@ TEST(libdm, DmSnapshotOverflow) { 
    }

    }

}

}





TEST(libdm, ParseStatusText) {

TEST_F(DmTest, ParseStatusText) {

    DmTargetSnapshot::Status status;

    DmTargetSnapshot::Status status;





    // Bad inputs

    // Bad inputs
@@ -448,7 +473,7 @@ TEST(libdm, ParseStatusText) { 
    EXPECT_TRUE(DmTargetSnapshot::ParseStatusText("Overflow", &status));

    EXPECT_TRUE(DmTargetSnapshot::ParseStatusText("Overflow", &status));

}

}





TEST(libdm, DmSnapshotMergePercent) {

TEST_F(DmTest, DmSnapshotMergePercent) {

    DmTargetSnapshot::Status status;

    DmTargetSnapshot::Status status;





    // Correct input

    // Correct input
@@ -502,7 +527,7 @@ TEST(libdm, DmSnapshotMergePercent) { 
    EXPECT_LE(DmTargetSnapshot::MergePercent(status, 0), 0.0);

    EXPECT_LE(DmTargetSnapshot::MergePercent(status, 0), 0.0);

}

}





TEST(libdm, CryptArgs) {

TEST_F(DmTest, CryptArgs) {

    DmTargetCrypt target1(0, 512, "sha1", "abcdefgh", 50, "/dev/loop0", 100);

    DmTargetCrypt target1(0, 512, "sha1", "abcdefgh", 50, "/dev/loop0", 100);

    ASSERT_EQ(target1.name(), "crypt");

    ASSERT_EQ(target1.name(), "crypt");

    ASSERT_TRUE(target1.Valid());

    ASSERT_TRUE(target1.Valid());
@@ -518,7 +543,7 @@ TEST(libdm, CryptArgs) { 
              "iv_large_sectors sector_size:64");

              "iv_large_sectors sector_size:64");

}

}





TEST(libdm, DefaultKeyArgs) {

TEST_F(DmTest, DefaultKeyArgs) {

    DmTargetDefaultKey target(0, 4096, "aes-xts-plain64", "abcdef0123456789", "/dev/loop0", 0);

    DmTargetDefaultKey target(0, 4096, "aes-xts-plain64", "abcdef0123456789", "/dev/loop0", 0);

    target.SetSetDun();

    target.SetSetDun();

    ASSERT_EQ(target.name(), "default-key");

    ASSERT_EQ(target.name(), "default-key");
@@ -529,7 +554,7 @@ TEST(libdm, DefaultKeyArgs) { 
              "iv_large_sectors");

              "iv_large_sectors");

}

}





TEST(libdm, DefaultKeyLegacyArgs) {

TEST_F(DmTest, DefaultKeyLegacyArgs) {

    DmTargetDefaultKey target(0, 4096, "AES-256-XTS", "abcdef0123456789", "/dev/loop0", 0);

    DmTargetDefaultKey target(0, 4096, "AES-256-XTS", "abcdef0123456789", "/dev/loop0", 0);

    target.SetUseLegacyOptionsFormat();

    target.SetUseLegacyOptionsFormat();

    ASSERT_EQ(target.name(), "default-key");

    ASSERT_EQ(target.name(), "default-key");
@@ -537,7 +562,7 @@ TEST(libdm, DefaultKeyLegacyArgs) { 
    ASSERT_EQ(target.GetParameterString(), "AES-256-XTS abcdef0123456789 /dev/loop0 0");

    ASSERT_EQ(target.GetParameterString(), "AES-256-XTS abcdef0123456789 /dev/loop0 0");

}

}





TEST(libdm, DeleteDeviceWithTimeout) {

TEST_F(DmTest, DeleteDeviceWithTimeout) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -561,7 +586,7 @@ TEST(libdm, DeleteDeviceWithTimeout) { 
    ASSERT_EQ(ENOENT, errno);

    ASSERT_EQ(ENOENT, errno);

}

}





TEST(libdm, IsDmBlockDevice) {

TEST_F(DmTest, IsDmBlockDevice) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -580,7 +605,7 @@ TEST(libdm, IsDmBlockDevice) { 
    ASSERT_FALSE(dm.IsDmBlockDevice(loop.device()));

    ASSERT_FALSE(dm.IsDmBlockDevice(loop.device()));

}

}





TEST(libdm, GetDmDeviceNameByPath) {

TEST_F(DmTest, GetDmDeviceNameByPath) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -601,7 +626,7 @@ TEST(libdm, GetDmDeviceNameByPath) { 
    ASSERT_EQ("libdm-test-dm-linear", *name);

    ASSERT_EQ("libdm-test-dm-linear", *name);

}

}





TEST(libdm, GetParentBlockDeviceByPath) {

TEST_F(DmTest, GetParentBlockDeviceByPath) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -621,7 +646,7 @@ TEST(libdm, GetParentBlockDeviceByPath) { 
    ASSERT_EQ(loop.device(), *sub_block_device);

    ASSERT_EQ(loop.device(), *sub_block_device);

}

}





TEST(libdm, DeleteDeviceDeferredNoReferences) {

TEST_F(DmTest, DeleteDeviceDeferredNoReferences) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -647,7 +672,7 @@ TEST(libdm, DeleteDeviceDeferredNoReferences) { 
    ASSERT_EQ(ENOENT, errno);

    ASSERT_EQ(ENOENT, errno);

}

}





TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) {

TEST_F(DmTest, DeleteDeviceDeferredWaitsForLastReference) {

    unique_fd tmp(CreateTempFile("file_1", 4096));

    unique_fd tmp(CreateTempFile("file_1", 4096));

    ASSERT_GE(tmp, 0);

    ASSERT_GE(tmp, 0);

    LoopDevice loop(tmp, 10s);

    LoopDevice loop(tmp, 10s);
@@ -682,7 +707,7 @@ TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { 
    ASSERT_EQ(ENOENT, errno);

    ASSERT_EQ(ENOENT, errno);

}

}





TEST(libdm, CreateEmptyDevice) {

TEST_F(DmTest, CreateEmptyDevice) {

    DeviceMapper& dm = DeviceMapper::Instance();

    DeviceMapper& dm = DeviceMapper::Instance();

    ASSERT_TRUE(dm.CreateEmptyDevice("empty-device"));

    ASSERT_TRUE(dm.CreateEmptyDevice("empty-device"));

    auto guard =

    auto guard =
@@ -692,9 +717,7 @@ TEST(libdm, CreateEmptyDevice) { 
    ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device"));

    ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device"));

}

}





TEST(libdm, UeventAfterLoadTable) {

TEST_F(DmTest, UeventAfterLoadTable) {

    static const char* kDeviceName = "libdm-test-uevent-load-table";



    struct utsname u;

    struct utsname u;

    ASSERT_EQ(uname(&u), 0);

    ASSERT_EQ(uname(&u), 0);
@@ -706,18 +729,31 @@ TEST(libdm, UeventAfterLoadTable) { 
    }

    }





    DeviceMapper& dm = DeviceMapper::Instance();

    DeviceMapper& dm = DeviceMapper::Instance();

    ASSERT_TRUE(dm.CreateEmptyDevice(kDeviceName));

    ASSERT_TRUE(dm.CreateEmptyDevice(test_name_));





    DmTable table;

    DmTable table;

    table.Emplace<DmTargetError>(0, 1);

    table.Emplace<DmTargetError>(0, 1);

    ASSERT_TRUE(dm.LoadTable(kDeviceName, table));

    ASSERT_TRUE(dm.LoadTable(test_name_, table));





    std::string ignore_path;

    std::string ignore_path;

    ASSERT_TRUE(dm.WaitForDevice(kDeviceName, 5s, &ignore_path));

    ASSERT_TRUE(dm.WaitForDevice(test_name_, 5s, &ignore_path));





    auto info = dm.GetDetailedInfo(kDeviceName);

    auto info = dm.GetDetailedInfo(test_name_);

    ASSERT_TRUE(info.has_value());

    ASSERT_TRUE(info.has_value());

    ASSERT_TRUE(info->IsSuspended());

    ASSERT_TRUE(info->IsSuspended());





    ASSERT_TRUE(dm.DeleteDevice(kDeviceName));

    ASSERT_TRUE(dm.DeleteDevice(test_name_));

}



TEST_F(DmTest, GetNameAndUuid) {

    auto& dm = DeviceMapper::Instance();

    ASSERT_TRUE(dm.CreatePlaceholderDevice(test_name_));



    dev_t dev;

    ASSERT_TRUE(dm.GetDeviceNumber(test_name_, &dev));



    std::string name, uuid;

    ASSERT_TRUE(dm.GetDeviceNameAndUuid(dev, &name, &uuid));

    ASSERT_EQ(name, test_name_);

    ASSERT_FALSE(uuid.empty());

}
 
}

fs_mgr/libdm/include/libdm/dm.h

+2 −0
Original line number Original line Diff line number Diff line
@@ -298,6 +298,8 @@ class DeviceMapper final : public IDeviceMapper { 
    // a placeholder table containing dm-error.

    // a placeholder table containing dm-error.

    bool CreatePlaceholderDevice(const std::string& name);

    bool CreatePlaceholderDevice(const std::string& name);





    bool GetDeviceNameAndUuid(dev_t dev, std::string* name, std::string* uuid);



  private:

  private:

    // Maximum possible device mapper targets registered in the kernel.

    // Maximum possible device mapper targets registered in the kernel.

    // This is only used to read the list of targets from kernel so we allocate

    // This is only used to read the list of targets from kernel so we allocate

init/devices.cpp

+8 −10
Original line number Original line Diff line number Diff line
@@ -32,6 +32,7 @@ 
#include <android-base/logging.h>

#include <android-base/logging.h>

#include <android-base/stringprintf.h>

#include <android-base/stringprintf.h>

#include <android-base/strings.h>

#include <android-base/strings.h>

#include <libdm/dm.h>

#include <private/android_filesystem_config.h>

#include <private/android_filesystem_config.h>

#include <selinux/android.h>

#include <selinux/android.h>

#include <selinux/selinux.h>

#include <selinux/selinux.h>
@@ -112,17 +113,14 @@ static bool FindVbdDevicePrefix(const std::string& path, std::string* result) { 
// the supplied buffer with the dm module's instantiated name.

// the supplied buffer with the dm module's instantiated name.

// If it doesn't start with a virtual block device, or there is some

// If it doesn't start with a virtual block device, or there is some

// error, return false.

// error, return false.

static bool FindDmDevice(const std::string& path, std::string* name, std::string* uuid) {

static bool FindDmDevice(const Uevent& uevent, std::string* name, std::string* uuid) {

    if (!StartsWith(path, "/devices/virtual/block/dm-")) return false;

    if (!StartsWith(uevent.path, "/devices/virtual/block/dm-")) return false;

    if (uevent.action == "remove") return false;  // Avoid error spam from ioctl





    if (!ReadFileToString("/sys" + path + "/dm/name", name)) {

    dev_t dev = makedev(uevent.major, uevent.minor);

        return false;

    }

    ReadFileToString("/sys" + path + "/dm/uuid", uuid);





    *name = android::base::Trim(*name);

    auto& dm = android::dm::DeviceMapper::Instance();

    *uuid = android::base::Trim(*uuid);

    return dm.GetDeviceNameAndUuid(dev, name, uuid);

    return true;

}

}





Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid,

Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid,
@@ -392,7 +390,7 @@ std::vector<std::string> DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev 
        type = "pci";

        type = "pci";

    } else if (FindVbdDevicePrefix(uevent.path, &device)) {

    } else if (FindVbdDevicePrefix(uevent.path, &device)) {

        type = "vbd";

        type = "vbd";

    } else if (FindDmDevice(uevent.path, &partition, &uuid)) {

    } else if (FindDmDevice(uevent, &partition, &uuid)) {

        std::vector<std::string> symlinks = {"/dev/block/mapper/" + partition};

        std::vector<std::string> symlinks = {"/dev/block/mapper/" + partition};

        if (!uuid.empty()) {

        if (!uuid.empty()) {

            symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid);

            symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid);