Merge "libdm: Fix race conditions in LoopControl::Attach." am: 974dadf904

am: 4605a96076

Change-Id: Ia75415390bd61d4625bf7e7547c660b2b1e23f78

fs_mgr/libdm/Android.bp

@@ -27,6 +27,7 @@ cc_library_static {
         "dm_target.cpp",
         "dm.cpp",
         "loop_control.cpp",
+        "utility.cpp",
     ],
 
     static_libs: [

fs_mgr/libdm/dm.cpp

@@ -29,6 +29,8 @@
 #include <android-base/strings.h>
 #include <uuid/uuid.h>
 
+#include "utility.h"
+
 namespace android {
 namespace dm {
 
@@ -94,20 +96,6 @@ bool DeviceMapper::DeleteDevice(const std::string& name) {
     return true;
 }
 
-bool WaitForCondition(const std::function<bool()>& 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<std::chrono::milliseconds>(now - start_time);
-        if (time_elapsed > timeout_ms) return false;
-    }
-}
-
 static std::string GenerateUuid() {
     uuid_t uuid_bytes;
     uuid_generate(uuid_bytes);
@@ -138,16 +126,7 @@ bool DeviceMapper::CreateDevice(const std::string& name, const DmTable& table, s
     if (timeout_ms <= std::chrono::milliseconds::zero()) {
         return true;
     }
-
+    if (!WaitForFile(unique_path, timeout_ms)) {
-    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;

fs_mgr/libdm/dm_test.cpp

@@ -103,9 +103,9 @@ TEST(libdm, DmLinear) {
     ASSERT_TRUE(android::base::WriteFully(tmp1, message1, sizeof(message1)));
     ASSERT_TRUE(android::base::WriteFully(tmp2, message2, sizeof(message2)));
 
-    LoopDevice loop_a(tmp1);
+    LoopDevice loop_a(tmp1, 10s);
     ASSERT_TRUE(loop_a.valid());
-    LoopDevice loop_b(tmp2);
+    LoopDevice loop_b(tmp2, 10s);
     ASSERT_TRUE(loop_b.valid());
 
     // Define a 2-sector device, with each sector mapping to the first sector
@@ -255,9 +255,9 @@ void SnapshotTestHarness::SetupImpl() {
     cow_fd_ = CreateTempFile("cow_device", kCowDeviceSize);
     ASSERT_GE(cow_fd_, 0);
 
-    base_loop_ = std::make_unique<LoopDevice>(base_fd_);
+    base_loop_ = std::make_unique<LoopDevice>(base_fd_, 10s);
     ASSERT_TRUE(base_loop_->valid());
-    cow_loop_ = std::make_unique<LoopDevice>(cow_fd_);
+    cow_loop_ = std::make_unique<LoopDevice>(cow_fd_, 10s);
     ASSERT_TRUE(cow_loop_->valid());
 
     DmTable origin_table;

fs_mgr/libdm/include/libdm/loop_control.h

@@ -17,6 +17,7 @@
 #ifndef _LIBDM_LOOP_CONTROL_H_
 #define _LIBDM_LOOP_CONTROL_H_
 
+#include <chrono>
 #include <string>
 
 #include <android-base/unique_fd.h>
@@ -29,8 +30,15 @@ class LoopControl final {
     LoopControl();
 
     // Attaches the file specified by 'file_fd' to the loop device specified
-    // by 'loopdev'
+    // by 'loopdev'. It is possible that in between allocating and attaching
-    bool Attach(int file_fd, std::string* loopdev) const;
+    // a loop device, another process attaches to the chosen loop device. If
+    // this happens, Attach() will retry for up to |timeout_ms|. The timeout
+    // should not be zero.
+    //
+    // The caller does not have to call WaitForFile(); it is implicitly called.
+    // The given |timeout_ms| covers both potential sources of timeout.
+    bool Attach(int file_fd, const std::chrono::milliseconds& timeout_ms,
+                std::string* loopdev) const;
 
     // Detach the loop device given by 'loopdev' from the attached backing file.
     bool Detach(const std::string& loopdev) const;
@@ -56,13 +64,13 @@ class LoopDevice {
   public:
     // Create a loop device for the given file descriptor. It is closed when
     // LoopDevice is destroyed only if auto_close is true.
-    LoopDevice(int fd, bool auto_close = false);
+    LoopDevice(int fd, const std::chrono::milliseconds& timeout_ms, bool auto_close = false);
     // Create a loop device for the given file path. It will be opened for
     // reading and writing and closed when the loop device is detached.
-    explicit LoopDevice(const std::string& path);
+    LoopDevice(const std::string& path, const std::chrono::milliseconds& timeout_ms);
     ~LoopDevice();
 
-    bool valid() const { return fd_ != -1 && !device_.empty(); }
+    bool valid() const { return valid_; }
     const std::string& device() const { return device_; }
 
     LoopDevice(const LoopDevice&) = delete;
@@ -71,12 +79,13 @@ class LoopDevice {
     LoopDevice(LoopDevice&&) = default;
 
   private:
-    void Init();
+    void Init(const std::chrono::milliseconds& timeout_ms);
 
     android::base::unique_fd fd_;
     bool owns_fd_;
     std::string device_;
     LoopControl control_;
+    bool valid_ = false;
 };
 
 }  // namespace dm

fs_mgr/libdm/loop_control.cpp

@@ -27,6 +27,8 @@
 #include <android-base/stringprintf.h>
 #include <android-base/unique_fd.h>
 
+#include "utility.h"
+
 namespace android {
 namespace dm {
 
@@ -37,21 +39,40 @@ LoopControl::LoopControl() : control_fd_(-1) {
     }
 }
 
-bool LoopControl::Attach(int file_fd, std::string* loopdev) const {
+bool LoopControl::Attach(int file_fd, const std::chrono::milliseconds& timeout_ms,
-    if (!FindFreeLoopDevice(loopdev)) {
+                         std::string* loopdev) const {
-        LOG(ERROR) << "Failed to attach, no free loop devices";
+    auto start_time = std::chrono::steady_clock::now();
-        return false;
+    auto condition = [&]() -> WaitResult {
-    }
+        if (!FindFreeLoopDevice(loopdev)) {
+            LOG(ERROR) << "Failed to attach, no free loop devices";
+            return WaitResult::Fail;
+        }
 
-    android::base::unique_fd loop_fd(TEMP_FAILURE_RETRY(open(loopdev->c_str(), O_RDWR | O_CLOEXEC)));
+        auto now = std::chrono::steady_clock::now();
-    if (loop_fd < 0) {
+        auto time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - start_time);
-        PLOG(ERROR) << "Failed to open: " << *loopdev;
+        if (!WaitForFile(*loopdev, timeout_ms - time_elapsed)) {
-        return false;
+            LOG(ERROR) << "Timed out waiting for path: " << *loopdev;
-    }
+            return WaitResult::Fail;
+        }
 
-    int rc = ioctl(loop_fd, LOOP_SET_FD, file_fd);
+        android::base::unique_fd loop_fd(
-    if (rc < 0) {
+                TEMP_FAILURE_RETRY(open(loopdev->c_str(), O_RDWR | O_CLOEXEC)));
-        PLOG(ERROR) << "Failed LOOP_SET_FD";
+        if (loop_fd < 0) {
+            PLOG(ERROR) << "Failed to open: " << *loopdev;
+            return WaitResult::Fail;
+        }
+
+        if (int rc = ioctl(loop_fd, LOOP_SET_FD, file_fd); rc == 0) {
+            return WaitResult::Done;
+        }
+        if (errno != EBUSY) {
+            PLOG(ERROR) << "Failed LOOP_SET_FD";
+            return WaitResult::Fail;
+        }
+        return WaitResult::Wait;
+    };
+    if (!WaitForCondition(condition, timeout_ms)) {
+        LOG(ERROR) << "Timed out trying to acquire a loop device";
         return false;
     }
     return true;
@@ -112,17 +133,19 @@ bool LoopControl::EnableDirectIo(int fd) {
     return true;
 }
 
-LoopDevice::LoopDevice(int fd, bool auto_close) : fd_(fd), owns_fd_(auto_close) {
+LoopDevice::LoopDevice(int fd, const std::chrono::milliseconds& timeout_ms, bool auto_close)
-    Init();
+    : fd_(fd), owns_fd_(auto_close) {
+    Init(timeout_ms);
 }
 
-LoopDevice::LoopDevice(const std::string& path) : fd_(-1), owns_fd_(true) {
+LoopDevice::LoopDevice(const std::string& path, const std::chrono::milliseconds& timeout_ms)
+    : fd_(-1), owns_fd_(true) {
     fd_.reset(open(path.c_str(), O_RDWR | O_CLOEXEC));
     if (fd_ < -1) {
         PLOG(ERROR) << "open failed for " << path;
         return;
     }
-    Init();
+    Init(timeout_ms);
 }
 
 LoopDevice::~LoopDevice() {
@@ -134,8 +157,8 @@ LoopDevice::~LoopDevice() {
     }
 }
 
-void LoopDevice::Init() {
+void LoopDevice::Init(const std::chrono::milliseconds& timeout_ms) {
-    control_.Attach(fd_, &device_);
+    valid_ = control_.Attach(fd_, timeout_ms, &device_);
 }
 
 }  // namespace dm

fs_mgr/libdm/loop_control_test.cpp

@@ -53,7 +53,7 @@ TEST(libdm, LoopControl) {
     unique_fd fd = TempFile();
     ASSERT_GE(fd, 0);
 
-    LoopDevice loop(fd);
+    LoopDevice loop(fd, 10s);
     ASSERT_TRUE(loop.valid());
 
     char buffer[6];

fs_mgr/libdm/utility.cpp

@@ -0,0 +1,56 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "utility.h"
+
+#include <errno.h>
+#include <unistd.h>
+
+#include <thread>
+
+using namespace std::literals;
+
+namespace android {
+namespace dm {
+
+bool WaitForCondition(const std::function<WaitResult()>& condition,
+                      const std::chrono::milliseconds& timeout_ms) {
+    auto start_time = std::chrono::steady_clock::now();
+    while (true) {
+        auto result = condition();
+        if (result == WaitResult::Done) return true;
+        if (result == WaitResult::Fail) return false;
+
+        std::this_thread::sleep_for(20ms);
+
+        auto now = std::chrono::steady_clock::now();
+        auto time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - start_time);
+        if (time_elapsed > timeout_ms) return false;
+    }
+}
+
+bool WaitForFile(const std::string& path, const std::chrono::milliseconds& timeout_ms) {
+    auto condition = [&]() -> WaitResult {
+        // If the file exists but returns EPERM or something, we consider the
+        // condition met.
+        if (access(path.c_str(), F_OK) != 0) {
+            if (errno == ENOENT) return WaitResult::Wait;
+        }
+        return WaitResult::Done;
+    };
+    return WaitForCondition(condition, timeout_ms);
+}
+
+}  // namespace dm
+}  // namespace android

fs_mgr/libdm/utility.h

@@ -0,0 +1,30 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#pragma once
+
+#include <chrono>
+#include <functional>
+
+namespace android {
+namespace dm {
+
+enum class WaitResult { Wait, Done, Fail };
+
+bool WaitForFile(const std::string& path, const std::chrono::milliseconds& timeout_ms);
+bool WaitForCondition(const std::function<WaitResult()>& condition,
+                      const std::chrono::milliseconds& timeout_ms);
+
+}  // namespace dm
+}  // namespace android