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
This commit is contained in:
David Anderson
parent
90bbf8253f
commit
59abbfe647
4 changed files with 94 additions and 39 deletions
|
|
@ -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::Info> DeviceMapper::GetDetailedInfo(const std::string& name) const {
|
||||
struct dm_ioctl io;
|
||||
InitIo(&io, name);
|
||||
|
|
|
|||
|
|
@ -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 @@ 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<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());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 @@ 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<std::string> 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<std::string> symlinks = {"/dev/block/mapper/" + partition};
|
||||
if (!uuid.empty()) {
|
||||
symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid);
|
||||
|
|
|
|||
Loading…
Reference in a new issue