This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new 8f78923 Automatically remounted read-only bind mounts.
8f78923 is described below
commit 8f7892393a0b73563b093b4db4ffe28823e18c41
Author: James Peach <[email protected]>
AuthorDate: Mon Oct 29 08:06:06 2018 -0700
Automatically remounted read-only bind mounts.
To make a bind mount read-only, you have to first make the bind mount,
then remount it with the read-only flag. This is a bit arcane, which is
why `mount(8)` does it automatically.
This change updates `fs::mount()` to do the read-only remount
automatically when it is making a read-only bind mount so that every
caller doesn't have to carry special code to make it work correctly. All
the callers that make an explicit remount are updated to simply pass
the `MS_READONLY` flag if necessary.
Review: https://reviews.apache.org/r/69149/
---
src/examples/test_csi_plugin.cpp | 16 +----
src/linux/fs.cpp | 77 ++++++++--------------
src/linux/fs.hpp | 4 ++
src/slave/containerizer/docker.cpp | 17 ++---
.../mesos/isolators/docker/volume/isolator.cpp | 10 +--
.../mesos/isolators/filesystem/linux.cpp | 17 ++---
.../containerizer/mesos/isolators/gpu/isolator.cpp | 5 --
.../mesos/isolators/network/cni/cni.cpp | 31 +--------
.../mesos/isolators/volume/host_path.cpp | 10 +--
.../containerizer/mesos/isolators/volume/image.cpp | 10 +--
.../mesos/isolators/volume/sandbox_path.cpp | 10 +--
.../mesos/provisioner/backends/bind.cpp | 16 +----
src/tests/containerizer/fs_tests.cpp | 21 ++++++
13 files changed, 74 insertions(+), 170 deletions(-)
diff --git a/src/examples/test_csi_plugin.cpp b/src/examples/test_csi_plugin.cpp
index 7fa325e..66f4ee0 100644
--- a/src/examples/test_csi_plugin.cpp
+++ b/src/examples/test_csi_plugin.cpp
@@ -777,7 +777,7 @@ Status TestCSIPlugin::NodePublishVolume(
request->staging_target_path(),
request->target_path(),
None(),
- MS_BIND,
+ MS_BIND | (request->readonly() ? MS_RDONLY : 0),
None());
if (mount.isError()) {
@@ -787,20 +787,6 @@ Status TestCSIPlugin::NodePublishVolume(
request->target_path() + "': " + mount.error());
}
- if (request->readonly()) {
- mount = fs::mount(
- None(),
- request->target_path(),
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT,
- None());
-
- return Status(
- grpc::INTERNAL,
- "Failed to mount '" + request->target_path() +
- "' as read only: " + mount.error());
- }
-
return Status::OK;
}
diff --git a/src/linux/fs.cpp b/src/linux/fs.cpp
index 9055ef4..3a58bf9 100644
--- a/src/linux/fs.cpp
+++ b/src/linux/fs.cpp
@@ -482,27 +482,43 @@ Try<MountTable> MountTable::read(const string& path)
}
-Try<Nothing> mount(const Option<string>& source,
+Try<Nothing> mount(const Option<string>& _source,
const string& target,
- const Option<string>& type,
+ const Option<string>& _type,
unsigned long flags,
const void* data)
{
+ const char * source = _source.isSome() ? _source->c_str() : nullptr;
+ const char * type = _type.isSome() ? _type->c_str() : nullptr;
+
// The prototype of function 'mount' on Linux is as follows:
// int mount(const char *source,
// const char *target,
// const char *filesystemtype,
// unsigned long mountflags,
// const void *data);
- if (::mount(
- (source.isSome() ? source->c_str() : nullptr),
- target.c_str(),
- (type.isSome() ? type->c_str() : nullptr),
- flags,
- data) < 0) {
+ if (::mount(source, target.c_str(), type, flags, data) < 0) {
return ErrnoError();
}
+ const unsigned READ_ONLY_FLAG = MS_BIND | MS_RDONLY;
+ const unsigned READ_ONLY_MASK = MS_BIND | MS_RDONLY | MS_REC;
+
+ // Bind mounts need to be remounted for the read-only flag to take
+ // effect. If this was a read-only bind mount, automatically remount
+ // so that every caller doesn't have to deal with this. We don't do
+ // anything if this was already a remount.
+ if ((flags & (READ_ONLY_FLAG | MS_REMOUNT)) == READ_ONLY_FLAG) {
+ if (::mount(
+ nullptr,
+ target.c_str(),
+ nullptr,
+ MS_REMOUNT | (flags & READ_ONLY_MASK),
+ nullptr) < 0) {
+ return ErrnoError("Read-only remount failed");
+ }
+ }
+
return Nothing();
}
@@ -707,70 +723,35 @@ Try<Nothing> mountSpecialFilesystems(const string& root)
"/proc/bus",
None(),
None(),
- MS_BIND
- },
- {
- None(),
- "/proc/bus",
- None(),
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT | MS_NOSUID | MS_NOEXEC | MS_NODEV
+ MS_BIND | MS_RDONLY | MS_NOSUID | MS_NOEXEC | MS_NODEV
},
{
"/proc/fs",
"/proc/fs",
None(),
None(),
- MS_BIND
- },
- {
- None(),
- "/proc/fs",
- None(),
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT | MS_NOSUID | MS_NOEXEC | MS_NODEV
+ MS_BIND | MS_RDONLY | MS_NOSUID | MS_NOEXEC | MS_NODEV
},
{
"/proc/irq",
"/proc/irq",
None(),
None(),
- MS_BIND
+ MS_BIND | MS_RDONLY | MS_NOSUID | MS_NOEXEC | MS_NODEV
},
{
- None(),
- "/proc/irq",
- None(),
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT | MS_NOSUID | MS_NOEXEC | MS_NODEV
- },
- {
- "/proc/sys",
"/proc/sys",
- None(),
- None(),
- MS_BIND
- },
- {
- None(),
"/proc/sys",
None(),
None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT | MS_NOSUID | MS_NOEXEC | MS_NODEV
+ MS_BIND | MS_RDONLY | MS_NOSUID | MS_NOEXEC | MS_NODEV
},
{
"/proc/sysrq-trigger",
"/proc/sysrq-trigger",
None(),
None(),
- MS_BIND
- },
- {
- None(),
- "/proc/sysrq-trigger",
- None(),
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT | MS_NOSUID | MS_NOEXEC | MS_NODEV
+ MS_BIND | MS_RDONLY | MS_NOSUID | MS_NOEXEC | MS_NODEV
},
{
"sysfs",
diff --git a/src/linux/fs.hpp b/src/linux/fs.hpp
index 502f85c..31969f6 100644
--- a/src/linux/fs.hpp
+++ b/src/linux/fs.hpp
@@ -348,6 +348,10 @@ struct MountTable {
// @param flags Mount flags.
// @param data Extra data interpreted by different file systems.
// @return Whether the mount operation succeeds.
+//
+// Note that if this is a read-only bind mount (both the MS_BIND
+// and MS_READONLY flags are set), the target will automatically
+// be remounted in read-only mode.
Try<Nothing> mount(const Option<std::string>& source,
const std::string& target,
const Option<std::string>& type,
diff --git a/src/slave/containerizer/docker.cpp
b/src/slave/containerizer/docker.cpp
index 192dc29..fc29e20 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -579,25 +579,16 @@ Try<Nothing>
DockerContainerizerProcess::updatePersistentVolumes(
<< "' for persistent volume " << resource
<< " of container " << containerId;
+ const unsigned flags =
+ MS_BIND | (resource.disk().volume().mode() == Volume::RO ? MS_RDONLY :
0);
+
// Bind mount the persistent volume to the container.
- Try<Nothing> mount = fs::mount(source, target, None(), MS_BIND, nullptr);
+ Try<Nothing> mount = fs::mount(source, target, None(), flags, nullptr);
if (mount.isError()) {
return Error(
"Failed to mount persistent volume from '" +
source + "' to '" + target + "': " + mount.error());
}
-
- // If the mount needs to be read-only, do a remount.
- if (resource.disk().volume().mode() == Volume::RO) {
- mount = fs::mount(
- None(), target, None(), MS_BIND | MS_RDONLY | MS_REMOUNT, nullptr);
-
- if (mount.isError()) {
- return Error(
- "Failed to remount persistent volume as read-only from '" +
- source + "' to '" + target + "': " + mount.error());
- }
- }
}
#else
if (!current.persistentVolumes().empty() ||
diff --git a/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
b/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
index 24c9fd6..d193883 100644
--- a/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
+++ b/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
@@ -558,14 +558,8 @@ Future<Option<ContainerLaunchInfo>>
DockerVolumeIsolatorProcess::_prepare(
ContainerMountInfo* mount = launchInfo.add_mounts();
mount->set_source(source);
mount->set_target(target);
- mount->set_flags(MS_BIND | MS_REC);
-
- // If the mount needs to be read-only, do a remount.
- if (volumeMode == Volume::RO) {
- mount = launchInfo.add_mounts();
- mount->set_target(target);
- mount->set_flags(MS_BIND | MS_RDONLY | MS_REMOUNT);
- }
+ mount->set_flags(
+ MS_BIND | MS_REC | (volumeMode == Volume::RO ? MS_RDONLY : 0));
}
return launchInfo;
diff --git a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
index a47899c..c7d753a 100644
--- a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
+++ b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
@@ -594,24 +594,15 @@ Future<Nothing> LinuxFilesystemIsolatorProcess::update(
<< "' for persistent volume " << resource
<< " of container " << containerId;
- Try<Nothing> mount = fs::mount(source, target, None(), MS_BIND, nullptr);
+ const unsigned mountFlags =
+ MS_BIND | (resource.disk().volume().mode() == Volume::RO ? MS_RDONLY :
0);
+
+ Try<Nothing> mount = fs::mount(source, target, None(), mountFlags,
nullptr);
if (mount.isError()) {
return Failure(
"Failed to mount persistent volume from '" +
source + "' to '" + target + "': " + mount.error());
}
-
- // If the mount needs to be read-only, do a remount.
- if (resource.disk().volume().mode() == Volume::RO) {
- mount = fs::mount(
- None(), target, None(), MS_BIND | MS_RDONLY | MS_REMOUNT, nullptr);
-
- if (mount.isError()) {
- return Failure(
- "Failed to remount persistent volume as read-only from '" +
- source + "' to '" + target + "': " + mount.error());
- }
- }
}
// Store the new resources;
diff --git a/src/slave/containerizer/mesos/isolators/gpu/isolator.cpp
b/src/slave/containerizer/mesos/isolators/gpu/isolator.cpp
index dbbf92f..56d8357 100644
--- a/src/slave/containerizer/mesos/isolators/gpu/isolator.cpp
+++ b/src/slave/containerizer/mesos/isolators/gpu/isolator.cpp
@@ -406,11 +406,6 @@ Future<Option<ContainerLaunchInfo>>
NvidiaGpuIsolatorProcess::_prepare(
mount->set_source(volume.HOST_PATH());
mount->set_target(target);
mount->set_flags(MS_RDONLY | MS_BIND | MS_REC);
-
- // NOTE: MS_REMOUNT is needed to make a bind mount read only.
- mount = launchInfo.add_mounts();
- mount->set_target(target);
- mount->set_flags(MS_RDONLY | MS_REMOUNT | MS_BIND | MS_REC);
}
return launchInfo;
diff --git a/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
b/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
index 64271df..f19edce 100644
--- a/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
+++ b/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
@@ -2089,7 +2089,8 @@ int NetworkCniIsolatorSetup::execute()
// when we do the MS_RDONLY remount. To save the bother of reading
// the mount table to find the flags to propagate, we just always
// use the most restrictive flags here.
- const int bindflags = MS_BIND | MS_NOEXEC | MS_NODEV | MS_NOSUID;
+ const unsigned bindflags = MS_BIND | MS_NOEXEC | MS_NODEV | MS_NOSUID |
+ (flags.bind_readonly ? MS_RDONLY : 0);
foreachpair (const string& file, const string& source, files) {
// Do the bind mount for network files in the host filesystem if
@@ -2144,20 +2145,6 @@ int NetworkCniIsolatorSetup::execute()
<< file << "': " << mount.error() << endl;
return EXIT_FAILURE;
}
-
- if (flags.bind_readonly) {
- mount = fs::mount(
- source,
- file,
- None(),
- MS_RDONLY | MS_REMOUNT | bindflags,
- nullptr);
- if (mount.isError()) {
- cerr << "Failed to remount bind mount as readonly from '" << source
- << "' to '" << file << "': " << mount.error() << endl;
- return EXIT_FAILURE;
- }
- }
}
// Do the bind mount in the container filesystem.
@@ -2208,20 +2195,6 @@ int NetworkCniIsolatorSetup::execute()
<< target << "': " << mount.error() << endl;
return EXIT_FAILURE;
}
-
- if (flags.bind_readonly) {
- mount = fs::mount(
- source,
- target,
- None(),
- MS_RDONLY | MS_REMOUNT | bindflags,
- nullptr);
- if (mount.isError()) {
- cerr << "Failed to remount bind mount as readonly from '" << source
- << "' to '" << target << "': " << mount.error() << endl;
- return EXIT_FAILURE;
- }
- }
}
}
diff --git a/src/slave/containerizer/mesos/isolators/volume/host_path.cpp
b/src/slave/containerizer/mesos/isolators/volume/host_path.cpp
index 2e03ef5..88ecf91 100644
--- a/src/slave/containerizer/mesos/isolators/volume/host_path.cpp
+++ b/src/slave/containerizer/mesos/isolators/volume/host_path.cpp
@@ -312,14 +312,8 @@ Future<Option<ContainerLaunchInfo>>
VolumeHostPathIsolatorProcess::prepare(
ContainerMountInfo* mount = launchInfo.add_mounts();
mount->set_source(hostPath.get());
mount->set_target(mountPoint);
- mount->set_flags(MS_BIND | MS_REC);
-
- // If the mount needs to be read-only, do a remount.
- if (volume.mode() == Volume::RO) {
- mount = launchInfo.add_mounts();
- mount->set_target(mountPoint);
- mount->set_flags(MS_BIND | MS_RDONLY | MS_REMOUNT);
- }
+ mount->set_flags(
+ MS_BIND | MS_REC | (volume.mode() == Volume::RO ? MS_RDONLY : 0));
}
}
diff --git a/src/slave/containerizer/mesos/isolators/volume/image.cpp
b/src/slave/containerizer/mesos/isolators/volume/image.cpp
index 53cbaef..bb3fc65 100644
--- a/src/slave/containerizer/mesos/isolators/volume/image.cpp
+++ b/src/slave/containerizer/mesos/isolators/volume/image.cpp
@@ -250,14 +250,8 @@ Future<Option<ContainerLaunchInfo>>
VolumeImageIsolatorProcess::_prepare(
ContainerMountInfo* mount = launchInfo.add_mounts();
mount->set_source(source);
mount->set_target(target);
- mount->set_flags(MS_BIND | MS_REC);
-
- // If the mount needs to be read-only, do a remount.
- if (volumeMode == Volume::RO) {
- mount = launchInfo.add_mounts();
- mount->set_target(target);
- mount->set_flags(MS_BIND | MS_RDONLY | MS_REMOUNT);
- }
+ mount->set_flags(
+ MS_BIND | MS_REC | (volumeMode == Volume::RO ? MS_RDONLY : 0));
}
return launchInfo;
diff --git a/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
b/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
index 21d9528..300b3d9 100644
--- a/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
+++ b/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
@@ -378,14 +378,8 @@ Future<Option<ContainerLaunchInfo>>
VolumeSandboxPathIsolatorProcess::prepare(
ContainerMountInfo* mount = launchInfo.add_mounts();
mount->set_source(source);
mount->set_target(target);
- mount->set_flags(MS_BIND | MS_REC);
-
- // If the mount needs to be read-only, do a remount.
- if (volume.mode() == Volume::RO) {
- mount = launchInfo.add_mounts();
- mount->set_target(target);
- mount->set_flags(MS_BIND | MS_RDONLY | MS_REMOUNT);
- }
+ mount->set_flags(
+ MS_BIND | MS_REC | (volume.mode() == Volume::RO ? MS_RDONLY : 0));
#endif // __linux__
} else {
LOG(INFO) << "Linking SANDBOX_PATH volume from "
diff --git a/src/slave/containerizer/mesos/provisioner/backends/bind.cpp
b/src/slave/containerizer/mesos/provisioner/backends/bind.cpp
index 7d564dc..4cc4c52 100644
--- a/src/slave/containerizer/mesos/provisioner/backends/bind.cpp
+++ b/src/slave/containerizer/mesos/provisioner/backends/bind.cpp
@@ -128,7 +128,7 @@ Future<Nothing> BindBackendProcess::provision(
layers.front(),
rootfs,
None(),
- MS_BIND,
+ MS_BIND | MS_RDONLY,
nullptr);
if (mount.isError()) {
@@ -137,20 +137,6 @@ Future<Nothing> BindBackendProcess::provision(
"' to '" + rootfs + "': " + mount.error());
}
- // And remount it read-only.
- mount = fs::mount(
- None(), // Ignored.
- rootfs,
- None(),
- MS_BIND | MS_RDONLY | MS_REMOUNT,
- nullptr);
-
- if (mount.isError()) {
- return Failure(
- "Failed to remount rootfs '" + rootfs + "' read-only: " +
- mount.error());
- }
-
// Mark the mount as shared+slave.
mount = fs::mount(
None(),
diff --git a/src/tests/containerizer/fs_tests.cpp
b/src/tests/containerizer/fs_tests.cpp
index 23cad35..36fda9c 100644
--- a/src/tests/containerizer/fs_tests.cpp
+++ b/src/tests/containerizer/fs_tests.cpp
@@ -241,6 +241,27 @@ TEST_F(FsTest, MountInfoTableReadSortedParentOfSelf)
}
+TEST_F(FsTest, ROOT_ReadOnlyMount)
+{
+ string directory = os::getcwd();
+
+ string ro = path::join(directory, "ro");
+ string rw = path::join(directory, "rw");
+
+ ASSERT_SOME(os::mkdir(ro));
+ ASSERT_SOME(os::mkdir(rw));
+
+ ASSERT_SOME(fs::mount(rw, ro, None(), MS_BIND | MS_RDONLY, None()));
+
+ EXPECT_ERROR(os::touch(path::join(ro, "touched")));
+ EXPECT_SOME(os::touch(path::join(rw, "touched")));
+
+ EXPECT_SOME(fs::unmount(ro));
+
+ EXPECT_SOME(os::touch(path::join(ro, "touched")));
+}
+
+
TEST_F(FsTest, ROOT_SharedMount)
{
string directory = os::getcwd();
