libbrillo: Fix the mount namespace creation
MountNamespace::Create() creates the mount namespace by first unsharing
a new mount namespace and then bind mounting the /proc/self/ns/mnt file
of that namespace to the provided path. However, mount namespaces do
not allow bind mounting /proc/[pid]/ns/mnt file if the process is in the
same mount namespace.
This CL fixes this bug in the existing code by moving the mount
operation to the parent process from the child process where a new
namespace is unshared.
BUG=chromium:1052197
TEST=FEATURES=test emerge-betty libbrillo
TEST=USE="user_session_isolation" emerge-betty libbrillo chromeos-login
TEST=cros deploy --board=betty localhost:9222 libbrillo chromeos-login
system boots and login success
PID=pgrep -o -f /opt/google/chrome
readlink /proc/${PID}/ns/mnt and readlink /proc/1/ns/mnt are different
Change-Id: I3856f9c8d160feb6fdff32e9abdbbfda0947d3f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2147356
Tested-by: Betul Soysal <betuls@google.com>
Commit-Queue: Betul Soysal <betuls@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Auto-Submit: Betul Soysal <betuls@google.com>
Cr-Mirrored-From: https://chromium.googlesource.com/chromiumos/platform2
Cr-Mirrored-Commit: 2ba7a03d1bf739e3884a22ed300aa2c8a1aadc25
diff --git a/brillo/namespaces/mock_platform.h b/brillo/namespaces/mock_platform.h
index 905f6b1..1b96b46 100644
--- a/brillo/namespaces/mock_platform.h
+++ b/brillo/namespaces/mock_platform.h
@@ -7,6 +7,8 @@
#include "brillo/namespaces/platform.h"
+#include <string>
+
#include <base/files/file_path.h>
#include <gmock/gmock.h>
@@ -20,6 +22,14 @@
MOCK_METHOD(bool, Unmount, (const base::FilePath&, bool, bool*), (override));
MOCK_METHOD(pid_t, Fork, (), (override));
MOCK_METHOD(pid_t, Waitpid, (pid_t, int*), (override));
+ MOCK_METHOD(int,
+ Mount,
+ (const std::string&,
+ const std::string&,
+ const std::string&,
+ uint64_t,
+ const void*),
+ (override));
};
} // namespace brillo
diff --git a/brillo/namespaces/mount_namespace.cc b/brillo/namespaces/mount_namespace.cc
index e193113..1944983 100644
--- a/brillo/namespaces/mount_namespace.cc
+++ b/brillo/namespaces/mount_namespace.cc
@@ -10,13 +10,13 @@
#include <sys/mount.h>
#include <sys/types.h>
-#include <base/files/file_path.h>
-#include <base/logging.h>
-#include <brillo/namespaces/platform.h>
+#include <string>
-namespace {
-constexpr char kProcNsPath[] = "/proc/self/ns/mnt";
-}
+#include <base/files/file_path.h>
+#include <base/files/file_util.h>
+#include <base/logging.h>
+#include <base/strings/stringprintf.h>
+#include <brillo/namespaces/platform.h>
namespace brillo {
MountNamespace::MountNamespace(const base::FilePath& ns_path,
@@ -34,31 +34,56 @@
<< " already exists.";
return false;
}
+ int fd_mounted[2];
+ int fd_unshared[2];
+ char byte = '\0';
+ if (pipe(fd_mounted) != 0) {
+ PLOG(ERROR) << "Cannot create mount signalling pipe";
+ return false;
+ }
+ if (pipe(fd_unshared) != 0) {
+ PLOG(ERROR) << "Cannot create unshare signalling pipe";
+ return false;
+ }
pid_t pid = platform_->Fork();
if (pid < 0) {
- PLOG(ERROR) << "Fork failed.";
+ PLOG(ERROR) << "Fork failed";
} else if (pid == 0) {
// Child.
- if (unshare(CLONE_NEWNS) == 0 &&
- mount(kProcNsPath, ns_path_.value().c_str(), nullptr, MS_BIND,
- nullptr) == 0) {
- exit(0);
+ close(fd_mounted[1]);
+ close(fd_unshared[0]);
+ if (unshare(CLONE_NEWNS) != 0) {
+ PLOG(ERROR) << "unshare(CLONE_NEWNS) failed";
+ exit(1);
}
- exit(1);
+ base::WriteFileDescriptor(fd_unshared[1], &byte, 1);
+ base::ReadFromFD(fd_mounted[0], &byte, 1);
+ exit(0);
} else {
// Parent.
+ close(fd_mounted[0]);
+ close(fd_unshared[1]);
+ std::string proc_ns_path = base::StringPrintf("/proc/%d/ns/mnt", pid);
+ bool mount_success = true;
+ base::ReadFromFD(fd_unshared[0], &byte, 1);
+ if (platform_->Mount(proc_ns_path, ns_path_.value(), "", MS_BIND) != 0) {
+ PLOG(ERROR) << "Mount(" << proc_ns_path << ", " << ns_path_.value()
+ << ", MS_BIND) failed";
+ mount_success = false;
+ }
+ base::WriteFileDescriptor(fd_mounted[1], &byte, 1);
+
int status;
if (platform_->Waitpid(pid, &status) < 0) {
- PLOG(ERROR) << "waitpid(" << pid << ") failed.";
+ PLOG(ERROR) << "waitpid(" << pid << ") failed";
return false;
}
-
if (!WIFEXITED(status)) {
LOG(ERROR) << "Child process did not exit normally.";
} else if (WEXITSTATUS(status) != 0) {
- LOG(ERROR) << "Child process failed to create namespace.";
+ LOG(ERROR) << "Child process failed.";
} else {
- exists_ = true;
+ exists_ = mount_success;
}
}
return exists_;
diff --git a/brillo/namespaces/mount_namespace_test.cc b/brillo/namespaces/mount_namespace_test.cc
index 8b9caf2..1bfa038 100644
--- a/brillo/namespaces/mount_namespace_test.cc
+++ b/brillo/namespaces/mount_namespace_test.cc
@@ -6,9 +6,10 @@
#include "brillo/namespaces/mount_namespace.h"
#include "brillo/namespaces/platform.h"
-#include <memory>
#include <unistd.h>
+#include <memory>
+
#include <base/files/file_path.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
@@ -40,6 +41,7 @@
std::unique_ptr<MountNamespace> ns =
std::make_unique<MountNamespace>(base::FilePath(), &platform_);
EXPECT_CALL(platform_, Fork()).WillOnce(Return(1));
+ EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0));
EXPECT_CALL(platform_, Waitpid(_, _))
.WillOnce(DoAll(SetArgPointee<1>(0x00000000), Return(0)));
EXPECT_TRUE(ns->Create());
@@ -50,14 +52,24 @@
std::unique_ptr<MountNamespace> ns =
std::make_unique<MountNamespace>(base::FilePath(), &platform_);
EXPECT_CALL(platform_, Fork()).WillOnce(Return(1));
+ EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0));
EXPECT_CALL(platform_, Waitpid(_, _)).WillOnce(Return(-1));
EXPECT_FALSE(ns->Create());
}
+TEST_F(MountNamespaceTest, CreateNamespaceFailedOnMount) {
+ std::unique_ptr<MountNamespace> ns =
+ std::make_unique<MountNamespace>(base::FilePath(), &platform_);
+ EXPECT_CALL(platform_, Fork()).WillOnce(Return(1));
+ EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(-1));
+ EXPECT_FALSE(ns->Create());
+}
+
TEST_F(MountNamespaceTest, CreateNamespaceFailedOnStatus) {
std::unique_ptr<MountNamespace> ns =
std::make_unique<MountNamespace>(base::FilePath(), &platform_);
EXPECT_CALL(platform_, Fork()).WillOnce(Return(1));
+ EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0));
EXPECT_CALL(platform_, Waitpid(_, _))
.WillOnce(DoAll(SetArgPointee<1>(0xFFFFFFFF), Return(0)));
EXPECT_FALSE(ns->Create());
@@ -67,6 +79,7 @@
std::unique_ptr<MountNamespace> ns =
std::make_unique<MountNamespace>(base::FilePath(), &platform_);
EXPECT_CALL(platform_, Fork()).WillOnce(Return(1));
+ EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0));
EXPECT_CALL(platform_, Waitpid(_, _))
.WillOnce(DoAll(SetArgPointee<1>(0x00000000), Return(0)));
EXPECT_TRUE(ns->Create());
diff --git a/brillo/namespaces/platform.cc b/brillo/namespaces/platform.cc
index 31884b5..5fe9140 100644
--- a/brillo/namespaces/platform.cc
+++ b/brillo/namespaces/platform.cc
@@ -11,7 +11,6 @@
#include <stdio.h>
#include <sys/mount.h>
#include <sys/stat.h>
-#include <sys/types.h>
#include <sys/vfs.h>
#include <sys/wait.h>
@@ -56,6 +55,15 @@
return true;
}
+int Platform::Mount(const std::string& source,
+ const std::string& target,
+ const std::string& fs_type,
+ uint64_t mount_flags,
+ const void* data) {
+ return mount(source.c_str(), target.c_str(), fs_type.c_str(), mount_flags,
+ data);
+}
+
pid_t Platform::Fork() {
return fork();
}
diff --git a/brillo/namespaces/platform.h b/brillo/namespaces/platform.h
index 5d3cbb6..6ef6a73 100644
--- a/brillo/namespaces/platform.h
+++ b/brillo/namespaces/platform.h
@@ -5,7 +5,10 @@
#ifndef LIBBRILLO_BRILLO_NAMESPACES_PLATFORM_H_
#define LIBBRILLO_BRILLO_NAMESPACES_PLATFORM_H_
+#include <sys/types.h>
+
#include <memory>
+#include <string>
#include <base/files/file_path.h>
#include <base/macros.h>
@@ -31,6 +34,20 @@
// was_busy (OUT) - Set to true on return if the mount point was busy
virtual bool Unmount(const base::FilePath& path, bool lazy, bool* was_busy);
+ // Calls the platform mount.
+ //
+ // Parameters
+ // source - The path to mount from
+ // target - The path to mount to
+ // fs_type - File system type of the mount
+ // mount_flags - Flags spesifying the type of the mount operation
+ // data - Mount options
+ virtual int Mount(const std::string& source,
+ const std::string& target,
+ const std::string& fs_type,
+ uint64_t mount_flags,
+ const void* = nullptr);
+
// Checks the file system type of the |path| and returns true if the
// filesystem type is nsfs.
//
