From 59abbfe64706a7ea0c4e932ae40bc8bde9746dce Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 16 May 2023 15:53:57 -0700 Subject: [PATCH] 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: 270183812 Test: libdm_test Change-Id: I258de5de05d813c3cb7f129e82e56dbfe8bf3117 --- fs_mgr/libdm/dm.cpp | 19 +++++++ fs_mgr/libdm/dm_test.cpp | 94 +++++++++++++++++++++++---------- fs_mgr/libdm/include/libdm/dm.h | 2 + init/devices.cpp | 18 +++---- 4 files changed, 94 insertions(+), 39 deletions(-) diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index deffae1ad..1e8c14fe2 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -243,6 +243,25 @@ bool DeviceMapper::GetDeviceUniquePath(const std::string& name, std::string* pat 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::GetDetailedInfo(const std::string& name) const { struct dm_ioctl io; InitIo(&io, name); diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index 788cf5174..c522eafae 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -42,16 +43,40 @@ using namespace std; using namespace std::chrono_literals; 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; DeviceMapper& dm = DeviceMapper::Instance(); ASSERT_TRUE(dm.GetTargetByName("linear", &info)); } -TEST(libdm, DmLinear) { +TEST_F(DmTest, DmLinear) { unique_fd tmp1(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp1, 0); unique_fd tmp2(CreateTempFile("file_2", 4096)); @@ -127,7 +152,7 @@ TEST(libdm, DmLinear) { ASSERT_TRUE(dev.Destroy()); } -TEST(libdm, DmSuspendResume) { +TEST_F(DmTest, DmSuspendResume) { unique_fd tmp1(CreateTempFile("file_suspend_resume", 512)); ASSERT_GE(tmp1, 0); @@ -156,7 +181,7 @@ TEST(libdm, DmSuspendResume) { 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 algorithm = "sha1"; std::string digest = "4be7e823b8c40f7bd5c8ccd5123f0722c5baca21"; @@ -178,7 +203,7 @@ TEST(libdm, DmVerityArgsAvb2) { EXPECT_EQ(target.GetParameterString(), expected); } -TEST(libdm, DmSnapshotArgs) { +TEST_F(DmTest, DmSnapshotArgs) { DmTargetSnapshot target1(0, 512, "base", "cow", SnapshotStorageMode::Persistent, 8); if (DmTargetSnapshot::ReportsOverflow("snapshot")) { EXPECT_EQ(target1.GetParameterString(), "base cow PO 8"); @@ -200,7 +225,7 @@ TEST(libdm, DmSnapshotArgs) { EXPECT_EQ(target3.name(), "snapshot-merge"); } -TEST(libdm, DmSnapshotOriginArgs) { +TEST_F(DmTest, DmSnapshotOriginArgs) { DmTargetSnapshotOrigin target(0, 512, "base"); EXPECT_EQ(target.GetParameterString(), "base"); EXPECT_EQ(target.name(), "snapshot-origin"); @@ -330,7 +355,7 @@ bool CheckSnapshotAvailability() { return true; } -TEST(libdm, DmSnapshot) { +TEST_F(DmTest, DmSnapshot) { if (!CheckSnapshotAvailability()) { return; } @@ -374,7 +399,7 @@ TEST(libdm, DmSnapshot) { ASSERT_EQ(read, data); } -TEST(libdm, DmSnapshotOverflow) { +TEST_F(DmTest, DmSnapshotOverflow) { if (!CheckSnapshotAvailability()) { return; } @@ -421,7 +446,7 @@ TEST(libdm, DmSnapshotOverflow) { } } -TEST(libdm, ParseStatusText) { +TEST_F(DmTest, ParseStatusText) { DmTargetSnapshot::Status status; // Bad inputs @@ -448,7 +473,7 @@ TEST(libdm, ParseStatusText) { EXPECT_TRUE(DmTargetSnapshot::ParseStatusText("Overflow", &status)); } -TEST(libdm, DmSnapshotMergePercent) { +TEST_F(DmTest, DmSnapshotMergePercent) { DmTargetSnapshot::Status status; // Correct input @@ -502,7 +527,7 @@ TEST(libdm, DmSnapshotMergePercent) { 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); ASSERT_EQ(target1.name(), "crypt"); ASSERT_TRUE(target1.Valid()); @@ -518,7 +543,7 @@ TEST(libdm, CryptArgs) { "iv_large_sectors sector_size:64"); } -TEST(libdm, DefaultKeyArgs) { +TEST_F(DmTest, DefaultKeyArgs) { DmTargetDefaultKey target(0, 4096, "aes-xts-plain64", "abcdef0123456789", "/dev/loop0", 0); target.SetSetDun(); ASSERT_EQ(target.name(), "default-key"); @@ -529,7 +554,7 @@ TEST(libdm, DefaultKeyArgs) { "iv_large_sectors"); } -TEST(libdm, DefaultKeyLegacyArgs) { +TEST_F(DmTest, DefaultKeyLegacyArgs) { DmTargetDefaultKey target(0, 4096, "AES-256-XTS", "abcdef0123456789", "/dev/loop0", 0); target.SetUseLegacyOptionsFormat(); ASSERT_EQ(target.name(), "default-key"); @@ -537,7 +562,7 @@ TEST(libdm, DefaultKeyLegacyArgs) { 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)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -561,7 +586,7 @@ TEST(libdm, DeleteDeviceWithTimeout) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, IsDmBlockDevice) { +TEST_F(DmTest, IsDmBlockDevice) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -580,7 +605,7 @@ TEST(libdm, IsDmBlockDevice) { ASSERT_FALSE(dm.IsDmBlockDevice(loop.device())); } -TEST(libdm, GetDmDeviceNameByPath) { +TEST_F(DmTest, GetDmDeviceNameByPath) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -601,7 +626,7 @@ TEST(libdm, GetDmDeviceNameByPath) { ASSERT_EQ("libdm-test-dm-linear", *name); } -TEST(libdm, GetParentBlockDeviceByPath) { +TEST_F(DmTest, GetParentBlockDeviceByPath) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -621,7 +646,7 @@ TEST(libdm, GetParentBlockDeviceByPath) { ASSERT_EQ(loop.device(), *sub_block_device); } -TEST(libdm, DeleteDeviceDeferredNoReferences) { +TEST_F(DmTest, DeleteDeviceDeferredNoReferences) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -647,7 +672,7 @@ TEST(libdm, DeleteDeviceDeferredNoReferences) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { +TEST_F(DmTest, DeleteDeviceDeferredWaitsForLastReference) { unique_fd tmp(CreateTempFile("file_1", 4096)); ASSERT_GE(tmp, 0); LoopDevice loop(tmp, 10s); @@ -682,7 +707,7 @@ TEST(libdm, DeleteDeviceDeferredWaitsForLastReference) { ASSERT_EQ(ENOENT, errno); } -TEST(libdm, CreateEmptyDevice) { +TEST_F(DmTest, CreateEmptyDevice) { DeviceMapper& dm = DeviceMapper::Instance(); ASSERT_TRUE(dm.CreateEmptyDevice("empty-device")); auto guard = @@ -692,9 +717,7 @@ TEST(libdm, CreateEmptyDevice) { ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device")); } -TEST(libdm, UeventAfterLoadTable) { - static const char* kDeviceName = "libdm-test-uevent-load-table"; - +TEST_F(DmTest, UeventAfterLoadTable) { struct utsname u; ASSERT_EQ(uname(&u), 0); @@ -706,18 +729,31 @@ TEST(libdm, UeventAfterLoadTable) { } DeviceMapper& dm = DeviceMapper::Instance(); - ASSERT_TRUE(dm.CreateEmptyDevice(kDeviceName)); + ASSERT_TRUE(dm.CreateEmptyDevice(test_name_)); DmTable table; table.Emplace(0, 1); - ASSERT_TRUE(dm.LoadTable(kDeviceName, table)); + ASSERT_TRUE(dm.LoadTable(test_name_, table)); 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->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()); } diff --git a/fs_mgr/libdm/include/libdm/dm.h b/fs_mgr/libdm/include/libdm/dm.h index dbef8f902..3e7ecc645 100644 --- a/fs_mgr/libdm/include/libdm/dm.h +++ b/fs_mgr/libdm/include/libdm/dm.h @@ -298,6 +298,8 @@ class DeviceMapper final : public IDeviceMapper { // a placeholder table containing dm-error. bool CreatePlaceholderDevice(const std::string& name); + bool GetDeviceNameAndUuid(dev_t dev, std::string* name, std::string* uuid); + private: // Maximum possible device mapper targets registered in the kernel. // This is only used to read the list of targets from kernel so we allocate diff --git a/init/devices.cpp b/init/devices.cpp index 39442a0e8..d29ffd604 100644 --- a/init/devices.cpp +++ b/init/devices.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -112,17 +113,14 @@ static bool FindVbdDevicePrefix(const std::string& path, std::string* result) { // the supplied buffer with the dm module's instantiated name. // If it doesn't start with a virtual block device, or there is some // error, return false. -static bool FindDmDevice(const std::string& path, std::string* name, std::string* uuid) { - if (!StartsWith(path, "/devices/virtual/block/dm-")) return false; +static bool FindDmDevice(const Uevent& uevent, std::string* name, std::string* uuid) { + 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)) { - return false; - } - ReadFileToString("/sys" + path + "/dm/uuid", uuid); + dev_t dev = makedev(uevent.major, uevent.minor); - *name = android::base::Trim(*name); - *uuid = android::base::Trim(*uuid); - return true; + auto& dm = android::dm::DeviceMapper::Instance(); + return dm.GetDeviceNameAndUuid(dev, name, uuid); } Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid, @@ -392,7 +390,7 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev type = "pci"; } else if (FindVbdDevicePrefix(uevent.path, &device)) { type = "vbd"; - } else if (FindDmDevice(uevent.path, &partition, &uuid)) { + } else if (FindDmDevice(uevent, &partition, &uuid)) { std::vector symlinks = {"/dev/block/mapper/" + partition}; if (!uuid.empty()) { symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid);