From 924858cd18589de74e7f4347d27c77ca6a33716d Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 26 Jun 2019 17:00:00 -0700 Subject: [PATCH] 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 --- fs_mgr/Android.bp | 1 + fs_mgr/fs_mgr_dm_linear.cpp | 12 +--- fs_mgr/libdm/Android.bp | 4 ++ fs_mgr/libdm/dm.cpp | 110 ++++++++++++++++++++++++++------ fs_mgr/libdm/dm_test.cpp | 32 ++-------- fs_mgr/libdm/include/libdm/dm.h | 53 +++++++++++---- fs_mgr/libfs_avb/Android.bp | 1 + fs_mgr/libfs_avb/avb_util.cpp | 18 ++---- init/Android.mk | 1 + init/devices.cpp | 66 +++++++++++++------ 10 files changed, 197 insertions(+), 101 deletions(-) diff --git a/fs_mgr/Android.bp b/fs_mgr/Android.bp index 3d3503cf2..65f0eff55 100644 --- a/fs_mgr/Android.bp +++ b/fs_mgr/Android.bp @@ -73,6 +73,7 @@ cc_library { whole_static_libs: [ "liblogwrap", "libdm", + "libext2_uuid", "libfstab", ], cppflags: [ diff --git a/fs_mgr/fs_mgr_dm_linear.cpp b/fs_mgr/fs_mgr_dm_linear.cpp index 1f21a7176..cd089dc84 100644 --- a/fs_mgr/fs_mgr_dm_linear.cpp +++ b/fs_mgr/fs_mgr_dm_linear.cpp @@ -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; } diff --git a/fs_mgr/libdm/Android.bp b/fs_mgr/libdm/Android.bp index 21255df5f..e429d9fa7 100644 --- a/fs_mgr/libdm/Android.bp +++ b/fs_mgr/libdm/Android.bp @@ -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", ], diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index d54b6ef63..d56a4b163 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -20,12 +20,20 @@ #include #include +#include +#include + +#include #include #include +#include +#include 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 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& 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(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; diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index c5881dd64..7a834e2a5 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -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("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("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; } diff --git a/fs_mgr/libdm/include/libdm/dm.h b/fs_mgr/libdm/include/libdm/dm.h index 08376c02f..9c0c2f30f 100644 --- a/fs_mgr/libdm/include/libdm/dm.h +++ b/fs_mgr/libdm/include/libdm/dm.h @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -73,10 +74,6 @@ class DeviceMapper final { // Returns 'true' on success, false otherwise. bool DeleteDevice(const std::string& name); - // Reads the device mapper table from the device with given anme and - // returns it in a DmTable object. - const std::unique_ptr table(const std::string& name) const; - // Returns the current state of the underlying device mapper device // with given name. // One of INVALID, SUSPENDED or ACTIVE. @@ -84,6 +81,33 @@ class DeviceMapper final { // Creates a device, loads the given table, and activates it. If the device // is not able to be activated, it is destroyed, and false is returned. + // After creation, |path| contains the result of calling + // GetDmDevicePathByName, and the path is guaranteed to exist. If after + // |timeout_ms| the path is not available, the device will be deleted and + // this function will return false. + // + // This variant must be used when depending on the device path. The + // following manual sequence should not be used: + // + // 1. CreateDevice(name, table) + // 2. GetDmDevicePathByName(name, &path) + // 3. fs_mgr::WaitForFile(path, ) + // + // This sequence has a race condition where, if another process deletes a + // device, CreateDevice may acquire the same path. When this happens, the + // WaitForFile() may early-return since ueventd has not yet processed all + // of the outstanding udev events. The caller may unexpectedly get an + // ENOENT on a system call using the affected path. + // + // If |timeout_ms| is 0ms, then this function will return true whether or + // not |path| is available. It is the caller's responsibility to ensure + // there are no races. + bool CreateDevice(const std::string& name, const DmTable& table, std::string* path, + const std::chrono::milliseconds& timeout_ms); + + // Create a device and activate the given table, without waiting to acquire + // a valid path. If the caller will use GetDmDevicePathByName(), it should + // use the timeout variant above. bool CreateDevice(const std::string& name, const DmTable& table); // Loads the device mapper table from parameter into the underlying device @@ -110,8 +134,21 @@ class DeviceMapper final { // Returns the path to the device mapper device node in '/dev' corresponding to // 'name'. If the device does not exist, false is returned, and the path // parameter is not set. + // + // This returns a path in the format "/dev/block/dm-N" that can be easily + // re-used with sysfs. + // + // WaitForFile() should not be used in conjunction with this call, since it + // could race with ueventd. bool GetDmDevicePathByName(const std::string& name, std::string* path); + // Returns a device's unique path as generated by ueventd. This will return + // true as long as the device has been created, even if ueventd has not + // processed it yet. + // + // The formatting of this path is /dev/block/mapper/by-uuid/. + bool GetDeviceUniquePath(const std::string& name, std::string* path); + // Returns the dev_t for the named device-mapper node. bool GetDeviceNumber(const std::string& name, dev_t* dev); @@ -158,18 +195,12 @@ class DeviceMapper final { // limit we are imposing here of 256. static constexpr uint32_t kMaxPossibleDmDevices = 256; + bool CreateDevice(const std::string& name, const std::string& uuid = {}); bool GetTable(const std::string& name, uint32_t flags, std::vector* table); - void InitIo(struct dm_ioctl* io, const std::string& name = std::string()) const; DeviceMapper(); - // Creates a device mapper device with given name. - // Return 'true' on success and 'false' on failure to - // create OR if a device mapper device with the same name already - // exists. - bool CreateDevice(const std::string& name); - int fd_; // Non-copyable & Non-movable DeviceMapper(const DeviceMapper&) = delete; diff --git a/fs_mgr/libfs_avb/Android.bp b/fs_mgr/libfs_avb/Android.bp index a3c76abbb..414a186bb 100644 --- a/fs_mgr/libfs_avb/Android.bp +++ b/fs_mgr/libfs_avb/Android.bp @@ -61,6 +61,7 @@ cc_defaults { "libavb", "libavb_host_sysdeps", "libdm", + "libext2_uuid", "libfs_avb", "libfstab", "libgtest_host", diff --git a/fs_mgr/libfs_avb/avb_util.cpp b/fs_mgr/libfs_avb/avb_util.cpp index d9650f33e..4505382cc 100644 --- a/fs_mgr/libfs_avb/avb_util.cpp +++ b/fs_mgr/libfs_avb/avb_util.cpp @@ -104,31 +104,23 @@ bool HashtreeDmVeritySetup(FstabEntry* fstab_entry, const FsAvbHashtreeDescripto } table.set_readonly(true); + std::chrono::milliseconds timeout = {}; + if (wait_for_verity_dev) timeout = 1s; + + std::string dev_path; const std::string mount_point(Basename(fstab_entry->mount_point)); const std::string device_name(GetVerityDeviceName(*fstab_entry)); android::dm::DeviceMapper& dm = android::dm::DeviceMapper::Instance(); - if (!dm.CreateDevice(device_name, table)) { + if (!dm.CreateDevice(device_name, table, &dev_path, timeout)) { LERROR << "Couldn't create verity device!"; return false; } - std::string dev_path; - if (!dm.GetDmDevicePathByName(device_name, &dev_path)) { - LERROR << "Couldn't get verity device path!"; - return false; - } - // Marks the underlying block device as read-only. SetBlockDeviceReadOnly(fstab_entry->blk_device); // Updates fstab_rec->blk_device to verity device name. fstab_entry->blk_device = dev_path; - - // Makes sure we've set everything up properly. - if (wait_for_verity_dev && !WaitForFile(dev_path, 1s)) { - return false; - } - return true; } diff --git a/init/Android.mk b/init/Android.mk index 901777293..006e1bfaa 100644 --- a/init/Android.mk +++ b/init/Android.mk @@ -113,6 +113,7 @@ LOCAL_STATIC_LIBRARIES := \ libunwindstack \ libbacktrace \ libmodprobe \ + libext2_uuid \ LOCAL_SANITIZE := signed-integer-overflow # First stage init is weird: it may start without stdout/stderr, and no /proc. diff --git a/init/devices.cpp b/init/devices.cpp index e8e6cd788..f6e453aa6 100644 --- a/init/devices.cpp +++ b/init/devices.cpp @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -111,24 +110,16 @@ 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 FindDmDevicePartition(const std::string& path, std::string* result) { - result->clear(); +static bool FindDmDevice(const std::string& path, std::string* name, std::string* uuid) { if (!StartsWith(path, "/devices/virtual/block/dm-")) return false; - if (getpid() == 1) return false; // first_stage_init has no sepolicy needs - static std::map cache; - // wait_for_file will not work, the content is also delayed ... - for (android::base::Timer t; t.duration() < 200ms; std::this_thread::sleep_for(10ms)) { - if (ReadFileToString("/sys" + path + "/dm/name", result) && !result->empty()) { - // Got it, set cache with result, when node arrives - cache[path] = *result = Trim(*result); - return true; - } + if (!ReadFileToString("/sys" + path + "/dm/name", name)) { + return false; } - auto it = cache.find(path); - if ((it == cache.end()) || (it->second.empty())) return false; - // Return cached results, when node goes away - *result = it->second; + ReadFileToString("/sys" + path + "/dm/uuid", uuid); + + *name = android::base::Trim(*name); + *uuid = android::base::Trim(*uuid); return true; } @@ -325,6 +316,7 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev std::string device; std::string type; std::string partition; + std::string uuid; if (FindPlatformDevice(uevent.path, &device)) { // Skip /devices/platform or /devices/ if present @@ -342,8 +334,12 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev type = "pci"; } else if (FindVbdDevicePrefix(uevent.path, &device)) { type = "vbd"; - } else if (FindDmDevicePartition(uevent.path, &partition)) { - return {"/dev/block/mapper/" + partition}; + } else if (FindDmDevice(uevent.path, &partition, &uuid)) { + std::vector symlinks = {"/dev/block/mapper/" + partition}; + if (!uuid.empty()) { + symlinks.emplace_back("/dev/block/mapper/by-uuid/" + uuid); + } + return symlinks; } else { return {}; } @@ -379,10 +375,41 @@ std::vector DeviceHandler::GetBlockDeviceSymlinks(const Uevent& uev return links; } +static void RemoveDeviceMapperLinks(const std::string& devpath) { + std::vector dirs = { + "/dev/block/mapper", + "/dev/block/mapper/by-uuid", + }; + for (const auto& dir : dirs) { + if (access(dir.c_str(), F_OK) != 0) continue; + + std::unique_ptr dh(opendir(dir.c_str()), closedir); + if (!dh) { + PLOG(ERROR) << "Failed to open directory " << dir; + continue; + } + + struct dirent* dp; + std::string link_path; + while ((dp = readdir(dh.get())) != nullptr) { + if (dp->d_type != DT_LNK) continue; + + auto path = dir + "/" + dp->d_name; + if (Readlink(path, &link_path) && link_path == devpath) { + unlink(path.c_str()); + } + } + } +} + void DeviceHandler::HandleDevice(const std::string& action, const std::string& devpath, bool block, int major, int minor, const std::vector& links) const { if (action == "add") { MakeDevice(devpath, block, major, minor, links); + } + + // We don't have full device-mapper information until a change event is fired. + if (action == "add" || (action == "change" && StartsWith(devpath, "/dev/block/dm-"))) { for (const auto& link : links) { if (!mkdir_recursive(Dirname(link), 0755)) { PLOG(ERROR) << "Failed to create directory " << Dirname(link); @@ -401,6 +428,9 @@ void DeviceHandler::HandleDevice(const std::string& action, const std::string& d } if (action == "remove") { + if (StartsWith(devpath, "/dev/block/dm-")) { + RemoveDeviceMapperLinks(devpath); + } for (const auto& link : links) { std::string link_path; if (Readlink(link, &link_path) && link_path == devpath) {