[incfs] Correctly load incfs mounted on bound roots

MountRegistry didn't expect an incfs mount to live on a root
that's a bind mount itself. This CL updates the code to keep
track of multiple root records in /proc/self/mountinfo and
select the primary one, instead of ignoring all but the first
encountered

Bug: 180631486
Test: atest libincfs-test
Change-Id: I460e3102a16ebb354950b2d1af94aeb883f961ec

3 files changed, 105 insertions, 65 deletions

diff --git a/incfs/MountRegistry.cpp b/incfs/MountRegistry.cpp
index c4752f3..bffa3cc 100644
--- a/incfs/MountRegistry.cpp
+++ b/incfs/MountRegistry.cpp
@@ -18,17 +18,17 @@
 
 #include "MountRegistry.h"
 
-#include "incfs.h"
-#include "path.h"
-#include "split.h"
-
 #include <android-base/logging.h>
+#include <poll.h>
+#include <stdlib.h>
 
 #include <charconv>
+#include <set>
 #include <unordered_map>
 
-#include <poll.h>
-#include <stdlib.h>
+#include "incfs.h"
+#include "path.h"
+#include "split.h"
 
 using namespace std::literals;
 
@@ -69,7 +69,7 @@ std::vector<std::pair<std::string_view, std::string_view>> MountRegistry::Mounts
     std::vector<std::pair<std::string_view, std::string_view>> result;
     result.reserve(mBase->binds.size());
     for (auto it : mBase->binds) {
-        result.emplace_back(it->second.first, it->first);
+        result.emplace_back(it->second.subdir, it->first);
     }
     return result;
 }
@@ -88,12 +88,12 @@ std::pair<int, MountRegistry::BindMap::const_iterator> MountRegistry::Mounts::ro
         std::string_view path) const {
     auto it = rootByBindPoint.lower_bound(path);
     if (it != rootByBindPoint.end() && it->first == path) {
-        return {it->second.second, it};
+        return {it->second.rootIndex, it};
     }
     if (it != rootByBindPoint.begin()) {
         --it;
         if (path::startsWith(path, it->first) && path.size() > it->first.size()) {
-            const auto index = it->second.second;
+            const auto index = it->second.rootIndex;
             if (index >= int(roots.size()) || roots[index].empty()) {
                 LOG(ERROR) << "[incfs] Root for path '" << path << "' #" << index
                            << " is not valid";
@@ -121,7 +121,7 @@ std::pair<std::string_view, std::string> MountRegistry::Mounts::rootAndSubpathFo
         return {};
     }
 
-    const auto& bindSubdir = bindIt->second.first;
+    const auto& bindSubdir = bindIt->second.subdir;
     const auto pastBindSubdir = path::relativize(bindIt->first, normalPath);
     const auto& root = roots[index];
     return {root.path, path::join(bindSubdir, pastBindSubdir)};
@@ -130,7 +130,7 @@ std::pair<std::string_view, std::string> MountRegistry::Mounts::rootAndSubpathFo
 void MountRegistry::Mounts::addRoot(std::string_view root, std::string_view backingDir) {
     const auto index = roots.size();
     auto absolute = path::normalize(root);
-    auto it = rootByBindPoint.insert_or_assign(absolute, std::pair{std::string(), index}).first;
+    auto it = rootByBindPoint.insert_or_assign(absolute, Bind{std::string(), int(index)}).first;
     roots.push_back({std::move(absolute), path::normalize(backingDir), {it}});
 }
 
@@ -141,13 +141,17 @@ void MountRegistry::Mounts::removeRoot(std::string_view root) {
         LOG(WARNING) << "[incfs] Trying to remove non-existent root '" << root << '\'';
         return;
     }
-    const auto index = it->second.second;
+    const auto index = it->second.rootIndex;
     if (index >= int(roots.size())) {
         LOG(ERROR) << "[incfs] Root '" << root << "' has index " << index
                    << " out of bounds (total roots count is " << roots.size();
         return;
     }
 
+    for (auto bindIt : roots[index].binds) {
+        rootByBindPoint.erase(bindIt);
+    }
+
     if (index + 1 == int(roots.size())) {
         roots.pop_back();
         // Run a small GC job here as we may be able to remove some obsolete
@@ -158,37 +162,6 @@ void MountRegistry::Mounts::removeRoot(std::string_view root) {
     } else {
         roots[index].clear();
     }
-    rootByBindPoint.erase(it);
-}
-
-void MountRegistry::Mounts::moveBind(std::string_view src, std::string_view dest) {
-    auto srcAbsolute = path::normalize(src);
-    auto destAbsolute = path::normalize(dest);
-    if (srcAbsolute == destAbsolute) {
-        return;
-    }
-
-    auto [root, rootIt] = rootIndex(srcAbsolute);
-    if (root < 0) {
-        LOG(ERROR) << "[incfs] No root found for bind move from " << src << " to " << dest;
-        return;
-    }
-
-    if (roots[root].path == srcAbsolute) {
-        // moving the whole root
-        roots[root].path = destAbsolute;
-    }
-
-    // const_cast<> here is safe as we're erasing that element on the next line.
-    const auto newRootIt = rootByBindPoint
-                                   .insert_or_assign(std::move(destAbsolute),
-                                                     std::pair{std::move(const_cast<std::string&>(
-                                                                       rootIt->second.first)),
-                                                               root})
-                                   .first;
-    rootByBindPoint.erase(rootIt);
-    const auto bindIt = std::find(roots[root].binds.begin(), roots[root].binds.end(), rootIt);
-    *bindIt = newRootIt;
 }
 
 void MountRegistry::Mounts::addBind(std::string_view what, std::string_view where) {
@@ -201,11 +174,10 @@ void MountRegistry::Mounts::addBind(std::string_view what, std::string_view wher
 
     const auto& currentBind = rootIt->first;
     auto whatSubpath = path::relativize(currentBind, whatAbsolute);
-    const auto& subdir = rootIt->second.first;
+    const auto& subdir = rootIt->second.subdir;
     auto realSubdir = path::join(subdir, whatSubpath);
     auto it = rootByBindPoint
-                      .insert_or_assign(path::normalize(where),
-                                        std::pair{std::move(realSubdir), root})
+                      .insert_or_assign(path::normalize(where), Bind{std::move(realSubdir), root})
                       .first;
     roots[root].binds.push_back(it);
 }
@@ -319,8 +291,8 @@ static bool forEachLine(base::borrowed_fd fd, Callback&& cb) {
 
 bool MountRegistry::Mounts::loadFrom(base::borrowed_fd fd, std::string_view filesystem) {
     struct MountInfo {
-        std::string root;
         std::string backing;
+        std::set<std::string, std::less<>> roots;
         std::vector<std::pair<std::string, std::string>> bindPoints;
     };
     std::unordered_map<std::string, MountInfo> mountsByGroup(16);
@@ -347,15 +319,16 @@ bool MountRegistry::Mounts::loadFrom(base::borrowed_fd fd, std::string_view file
         mountPoint = path::normalize(mountPoint);
         auto& mount = mountsByGroup[std::string(groupId)];
         if (subdir == "/"sv) {
-            if (mount.root.empty()) {
-                mount.root.assign(mountPoint);
-                mount.backing.assign(items.rbegin()[1]);
-                fixProcPath(mount.backing);
-            } else {
-                LOG(WARNING) << "[incfs] incfs root '" << mount.root
-                             << "' mounted in multiple places, ignoring later mount '" << mountPoint
-                             << '\'';
+            mount.roots.emplace(mountPoint);
+            const auto backingDir = items.rbegin()[1];
+            if (!mount.backing.empty() && mount.backing != backingDir) {
+                LOG(WARNING) << "[incfs] root '" << *mount.roots.begin()
+                             << "' mounted in multiple places with different backing dirs, '"
+                             << mount.backing << "' vs new '" << backingDir
+                             << "'; updating to the new one";
             }
+            mount.backing.assign(backingDir);
+            fixProcPath(mount.backing);
             subdir = ""sv;
         }
         mount.bindPoints.emplace_back(std::string(subdir), std::move(mountPoint));
@@ -366,7 +339,7 @@ bool MountRegistry::Mounts::loadFrom(base::borrowed_fd fd, std::string_view file
     }
 
     rootByBindPoint.clear();
-    // preserve the allocated capacity, but clean existing data
+    // preserve the allocated capacity, but clear existing data
     roots.resize(mountsByGroup.size());
     for (auto& root : roots) {
         root.binds.clear();
@@ -378,13 +351,14 @@ bool MountRegistry::Mounts::loadFrom(base::borrowed_fd fd, std::string_view file
         auto& binds = root.binds;
         binds.reserve(mount.bindPoints.size());
         for (auto& [subdir, bind] : mount.bindPoints) {
-            auto it =
-                    rootByBindPoint
-                            .insert_or_assign(std::move(bind), std::pair(std::move(subdir), index))
-                            .first;
+            auto it = rootByBindPoint
+                              .insert_or_assign(std::move(bind), Bind{std::move(subdir), index})
+                              .first;
             binds.push_back(it);
         }
-        root.path = std::move(mount.root);
+        // a trick here: given that as of now we either have exactly one root, or the preferred one
+        // is always at the front, let's pick that one here.
+        root.path = std::move(mount.roots.extract(mount.roots.begin()).value());
         root.backing = std::move(mount.backing);
         ++index;
     }
@@ -396,7 +370,7 @@ bool MountRegistry::Mounts::loadFrom(base::borrowed_fd fd, std::string_view file
             LOG(INFO) << "[incfs]  '" << root << '\'';
             LOG(INFO) << "[incfs]    backing: '" << backing << '\'';
             for (auto&& bind : binds) {
-                LOG(INFO) << "[incfs]      bind : '" << bind->second.first << "'->'" << bind->first
+                LOG(INFO) << "[incfs]      bind : '" << bind->second.subdir << "'->'" << bind->first
                           << '\'';
             }
         }
diff --git a/incfs/include/MountRegistry.h b/incfs/include/MountRegistry.h
index f8cfe4d..395919b 100644
--- a/incfs/include/MountRegistry.h
+++ b/incfs/include/MountRegistry.h
@@ -35,8 +35,12 @@ namespace android::incfs {
 
 class MountRegistry final {
 public:
+    struct Bind {
+        std::string subdir;
+        int rootIndex;
+    };
     // std::less<> enables heterogeneous lookups, e.g. by a string_view
-    using BindMap = std::map<std::string, std::pair<std::string, int>, std::less<>>;
+    using BindMap = std::map<std::string, Bind, std::less<>>;
 
     class Mounts final {
         struct Root {
@@ -88,7 +92,6 @@ public:
         void addRoot(std::string_view root, std::string_view backingDir);
         void removeRoot(std::string_view root);
         void addBind(std::string_view what, std::string_view where);
-        void moveBind(std::string_view src, std::string_view dest);
         void removeBind(std::string_view what);
 
     private:
diff --git a/incfs/tests/MountRegistry_test.cpp b/incfs/tests/MountRegistry_test.cpp
index 2b82eb0..3e12dcf 100644
--- a/incfs/tests/MountRegistry_test.cpp
+++ b/incfs/tests/MountRegistry_test.cpp
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include "MountRegistry.h"
+
 #include <android-base/file.h>
 #include <android-base/logging.h>
 #include <android-base/unique_fd.h>
@@ -21,10 +23,11 @@
 #include <sys/select.h>
 #include <unistd.h>
 
+#include <iterator>
 #include <optional>
 #include <thread>
 
-#include "MountRegistry.h"
+#include "incfs.h"
 #include "path.h"
 
 using namespace android::incfs;
@@ -78,3 +81,63 @@ TEST_F(MountRegistryTest, MultiBind) {
     ASSERT_EQ(std::pair("/root"sv, "2/3/blah"s), r().rootAndSubpathFor("/bind2/blah"));
     ASSERT_EQ(std::pair("/root"sv, "2/3/blah"s), r().rootAndSubpathFor("/other/bind/blah"));
 }
+
+TEST_F(MountRegistryTest, MultiRoot) {
+    r().addRoot("/root", "/backing");
+    r().addBind("/root", "/bind");
+    ASSERT_STREQ("/root", r().rootFor("/root").data());
+    ASSERT_STREQ("/root", r().rootFor("/bind").data());
+    ASSERT_STREQ("/root", r().rootFor("/bind/2").data());
+}
+
+TEST_F(MountRegistryTest, MultiRootLoad) {
+    constexpr char mountsFile[] =
+            R"(4605 34 0:154 / /mnt/installer/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime shared:45 master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4561 35 0:154 / /mnt/androidwritable/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime shared:44 master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4560 99 0:154 / /storage/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4650 30 0:44 /MyFiles /mnt/pass_through/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,relatime shared:31 - 9p media rw,sync,dirsync,access=client,trans=virtio
+3181 79 0:146 / /data/incremental/MT_data_app_vmdl703/mount rw,nosuid,nodev,noatime shared:46 - incremental-fs /data/incremental/MT_data_app_vmdl703/backing_store rw,seclabel,read_timeout_ms=10000,readahead=0
+3182 77 0:146 / /var/run/mount/data/mount/data/incremental/MT_data_app_vmdl703/mount rw,nosuid,nodev,noatime shared:46 - incremental-fs /data/incremental/MT_data_app_vmdl703/backing_store rw,seclabel,read_timeout_ms=10000,readahead=0
+)";
+
+    TemporaryFile f;
+    ASSERT_TRUE(android::base::WriteFully(f.fd, mountsFile, std::size(mountsFile) - 1));
+    ASSERT_EQ(0, lseek(f.fd, 0, SEEK_SET)); // rewind
+
+    MountRegistry::Mounts m;
+    ASSERT_TRUE(m.loadFrom(f.fd, INCFS_NAME));
+
+    EXPECT_EQ(size_t(1), m.size());
+    EXPECT_STREQ("/data/incremental/MT_data_app_vmdl703/mount",
+                 m.rootFor("/data/incremental/MT_data_app_vmdl703/mount/123/2").data());
+    EXPECT_STREQ("/data/incremental/MT_data_app_vmdl703/mount",
+                 m.rootFor("/var/run/mount/data/mount/data/incremental/MT_data_app_vmdl703/mount/"
+                           "some/thing")
+                         .data());
+}
+
+TEST_F(MountRegistryTest, MultiRootLoadReversed) {
+    constexpr char mountsFile[] =
+            R"(4605 34 0:154 / /mnt/installer/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime shared:45 master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4561 35 0:154 / /mnt/androidwritable/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime shared:44 master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4560 99 0:154 / /storage/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,noatime master:43 - fuse /dev/fuse rw,lazytime,user_id=0,group_id=0,allow_other
+4650 30 0:44 /MyFiles /mnt/pass_through/0/0000000000000000000000000000CAFEF00D2019 rw,nosuid,nodev,noexec,relatime shared:31 - 9p media rw,sync,dirsync,access=client,trans=virtio
+3182 77 0:146 / /var/run/mount/data/mount/data/incremental/MT_data_app_vmdl703/mount rw,nosuid,nodev,noatime shared:46 - incremental-fs /data/incremental/MT_data_app_vmdl703/backing_store rw,seclabel,read_timeout_ms=10000,readahead=0
+3181 79 0:146 / /data/incremental/MT_data_app_vmdl703/mount rw,nosuid,nodev,noatime shared:46 - incremental-fs /data/incremental/MT_data_app_vmdl703/backing_store rw,seclabel,read_timeout_ms=10000,readahead=0
+)";
+
+    TemporaryFile f;
+    ASSERT_TRUE(android::base::WriteFully(f.fd, mountsFile, std::size(mountsFile) - 1));
+    ASSERT_EQ(0, lseek(f.fd, 0, SEEK_SET)); // rewind
+
+    MountRegistry::Mounts m;
+    ASSERT_TRUE(m.loadFrom(f.fd, INCFS_NAME));
+
+    EXPECT_EQ(size_t(1), m.size());
+    EXPECT_STREQ("/data/incremental/MT_data_app_vmdl703/mount",
+                 m.rootFor("/data/incremental/MT_data_app_vmdl703/mount/123/2").data());
+    EXPECT_STREQ("/data/incremental/MT_data_app_vmdl703/mount",
+                 m.rootFor("/var/run/mount/data/mount/data/incremental/MT_data_app_vmdl703/mount/"
+                           "some/thing")
+                         .data());
+}
\ No newline at end of file