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
diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp
index deffae1..1e8c14f 100644
--- a/fs_mgr/libdm/dm.cpp
+++ b/fs_mgr/libdm/dm.cpp
@@ -243,6 +243,25 @@
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 {
struct dm_ioctl io;
InitIo(&io, name);
diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp
index 788cf51..c522eaf 100644
--- a/fs_mgr/libdm/dm_test.cpp
+++ b/fs_mgr/libdm/dm_test.cpp
@@ -30,6 +30,7 @@
#include <thread>
#include <android-base/file.h>
+#include <android-base/logging.h>
#include <android-base/scopeguard.h>
#include <android-base/strings.h>
#include <android-base/unique_fd.h>
@@ -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 @@
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 @@
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 @@
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 @@
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 @@
return true;
}
-TEST(libdm, DmSnapshot) {
+TEST_F(DmTest, DmSnapshot) {
if (!CheckSnapshotAvailability()) {
return;
}
@@ -374,7 +399,7 @@
ASSERT_EQ(read, data);
}
-TEST(libdm, DmSnapshotOverflow) {
+TEST_F(DmTest, DmSnapshotOverflow) {
if (!CheckSnapshotAvailability()) {
return;
}
@@ -421,7 +446,7 @@
}
}
-TEST(libdm, ParseStatusText) {
+TEST_F(DmTest, ParseStatusText) {
DmTargetSnapshot::Status status;
// Bad inputs
@@ -448,7 +473,7 @@
EXPECT_TRUE(DmTargetSnapshot::ParseStatusText("Overflow", &status));
}
-TEST(libdm, DmSnapshotMergePercent) {
+TEST_F(DmTest, DmSnapshotMergePercent) {
DmTargetSnapshot::Status status;
// Correct input
@@ -502,7 +527,7 @@
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 @@
"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 @@
"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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
DeviceMapper& dm = DeviceMapper::Instance();
- ASSERT_TRUE(dm.CreateEmptyDevice(kDeviceName));
+ ASSERT_TRUE(dm.CreateEmptyDevice(test_name_));
DmTable table;
table.Emplace<DmTargetError>(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 dbef8f9..3e7ecc6 100644
--- a/fs_mgr/libdm/include/libdm/dm.h
+++ b/fs_mgr/libdm/include/libdm/dm.h
@@ -298,6 +298,8 @@
// 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 39442a0..d29ffd6 100644
--- a/init/devices.cpp
+++ b/init/devices.cpp
@@ -32,6 +32,7 @@
#include <android-base/logging.h>
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
+#include <libdm/dm.h>
#include <private/android_filesystem_config.h>
#include <selinux/android.h>
#include <selinux/selinux.h>
@@ -112,17 +113,14 @@
// 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 @@
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<std::string> symlinks = {"/dev/block/mapper/" + partition};
if (!uuid.empty()) {
symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid);