From f3dab34d22e64cbe079bcd9e54906137149147b1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Jan 2020 14:53:36 +0100 Subject: [PATCH 1/5] =?UTF-8?q?mount-util:=20rename=20cleaned=20=E2=86=92?= =?UTF-8?q?=20simplified,=20because=20that's=20what=20we=20actually=20did?= =?UTF-8?q?=20here?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/shared/mount-util.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 95d7ea9691..cf500a126b 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -118,7 +118,7 @@ int bind_remount_recursive_with_mountinfo( FILE *proc_self_mountinfo) { _cleanup_set_free_free_ Set *done = NULL; - _cleanup_free_ char *cleaned = NULL; + _cleanup_free_ char *simplified = NULL; int r; assert(proc_self_mountinfo); @@ -134,11 +134,11 @@ int bind_remount_recursive_with_mountinfo( * If the "blacklist" parameter is specified it may contain a list of subtrees to exclude from the * remount operation. Note that we'll ignore the blacklist for the top-level path. */ - cleaned = strdup(prefix); - if (!cleaned) + simplified = strdup(prefix); + if (!simplified) return -ENOMEM; - path_simplify(cleaned, false); + path_simplify(simplified, false); done = set_new(&path_hash_ops); if (!done) @@ -177,26 +177,26 @@ int bind_remount_recursive_with_mountinfo( if (!path || !type) continue; - if (!path_startswith(path, cleaned)) + if (!path_startswith(path, simplified)) continue; /* Ignore this mount if it is blacklisted, but only if it isn't the top-level mount * we shall operate on. */ - if (!path_equal(path, cleaned)) { + if (!path_equal(path, simplified)) { bool blacklisted = false; char **i; STRV_FOREACH(i, blacklist) { - if (path_equal(*i, cleaned)) + if (path_equal(*i, simplified)) continue; - if (!path_startswith(*i, cleaned)) + if (!path_startswith(*i, simplified)) continue; if (path_startswith(path, *i)) { blacklisted = true; log_debug("Not remounting %s blacklisted by %s, called for %s", - path, *i, cleaned); + path, *i, simplified); break; } } @@ -211,7 +211,7 @@ int bind_remount_recursive_with_mountinfo( * already triggered, then we will find * another entry for this. */ if (streq(type, "autofs")) { - top_autofs = top_autofs || path_equal(path, cleaned); + top_autofs = top_autofs || path_equal(path, simplified); continue; } @@ -226,25 +226,25 @@ int bind_remount_recursive_with_mountinfo( * the root is either already done, or an autofs, we * are done */ if (set_isempty(todo) && - (top_autofs || set_contains(done, cleaned))) + (top_autofs || set_contains(done, simplified))) return 0; - if (!set_contains(done, cleaned) && - !set_contains(todo, cleaned)) { + if (!set_contains(done, simplified) && + !set_contains(todo, simplified)) { /* The prefix directory itself is not yet a mount, make it one. */ - if (mount(cleaned, cleaned, NULL, MS_BIND|MS_REC, NULL) < 0) + if (mount(simplified, simplified, NULL, MS_BIND|MS_REC, NULL) < 0) return -errno; orig_flags = 0; - (void) get_mount_flags(cleaned, &orig_flags, table); + (void) get_mount_flags(simplified, &orig_flags, table); orig_flags &= ~MS_RDONLY; - if (mount(NULL, cleaned, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) + if (mount(NULL, simplified, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) return -errno; log_debug("Made top-level directory %s a mount point.", prefix); - r = set_put_strdup(done, cleaned); + r = set_put_strdup(done, simplified); if (r < 0) return r; } From 4eaf0d9401ab26609b737fe25aa19dcc1e1aa314 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Jan 2020 14:55:15 +0100 Subject: [PATCH 2/5] mount-util: don't mask away MS_RDONLY twice We have the flags mask for that, and if callers really wanted us to mask this away, then they should pass the correct mask. --- src/shared/mount-util.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index cf500a126b..935691ce77 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -237,7 +237,6 @@ int bind_remount_recursive_with_mountinfo( orig_flags = 0; (void) get_mount_flags(simplified, &orig_flags, table); - orig_flags &= ~MS_RDONLY; if (mount(NULL, simplified, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) return -errno; @@ -279,7 +278,6 @@ int bind_remount_recursive_with_mountinfo( /* Try to reuse the original flag set */ orig_flags = 0; (void) get_mount_flags(x, &orig_flags, table); - orig_flags &= ~MS_RDONLY; if (mount(NULL, x, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) return -errno; From 08b1f5c7d1193dd51eaa9d3db43c65dd1fd71c2c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Jan 2020 14:59:41 +0100 Subject: [PATCH 3/5] mount-util: clean up get_mount_flags() This cleans up the function in multiple ways: - change order of parameters to follow our usualy system of putting return parameters last - rename return parameter "ret" as we usually do - don't initialize local variables we override immediately anyway - downgrade log messages to LOG_DEBUG (since we don't log about any other errors here above LOG_DEBUG, as this is mostly an "API"-style function) - handle that mnt_fs_get_vfs_options() may return NULL (according to docs) - manually map the ST_xyz to MS_xyz flags on statvfs(), because while they are mostly the same, they aren't entirely the same, MS_RELATIME and ST_RELATIME are defined differently (sad!) --- src/shared/mount-util.c | 48 ++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 935691ce77..09fa7d3943 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -76,35 +76,57 @@ int umount_recursive(const char *prefix, int flags) { return n; } -/* Get the mount flags for the mountpoint at "path" from "table" */ -static int get_mount_flags(const char *path, unsigned long *flags, struct libmnt_table *table) { - struct statvfs buf = {}; - struct libmnt_fs *fs = NULL; - const char *opts = NULL; +static int get_mount_flags( + struct libmnt_table *table, + const char *path, + unsigned long *ret) { + struct libmnt_fs *fs; + struct statvfs buf; + const char *opts; int r = 0; + /* Get the mount flags for the mountpoint at "path" from "table". We have a fallback using statvfs() + * in place (which provides us with mostly the same info), but it's just a fallback, since using it + * means triggering autofs or NFS mounts, which we'd rather avoid needlessly. */ + fs = mnt_table_find_target(table, path, MNT_ITER_FORWARD); if (!fs) { - log_warning("Could not find '%s' in mount table", path); + log_debug("Could not find '%s' in mount table, ignoring.", path); goto fallback; } opts = mnt_fs_get_vfs_options(fs); - r = mnt_optstr_get_flags(opts, flags, mnt_get_builtin_optmap(MNT_LINUX_MAP)); + if (!opts) { + *ret = 0; + return 0; + } + + r = mnt_optstr_get_flags(opts, ret, mnt_get_builtin_optmap(MNT_LINUX_MAP)); if (r != 0) { - log_warning_errno(r, "Could not get flags for '%s': %m", path); + log_debug_errno(r, "Could not get flags for '%s', ignoring: %m", path); goto fallback; } - /* relatime is default and trying to set it in an unprivileged container causes EPERM */ - *flags &= ~MS_RELATIME; + /* MS_RELATIME is default and trying to set it in an unprivileged container causes EPERM */ + *ret &= ~MS_RELATIME; return 0; fallback: if (statvfs(path, &buf) < 0) return -errno; - *flags = buf.f_flag; + /* The statvfs() flags and the mount flags mostly have the same values, but for some cases do + * not. Hence map the flags manually. (Strictly speaking, ST_RELATIME/MS_RELATIME is the most + * prominent one that doesn't match, but that's the one we mask away anyway, see above.) */ + + *ret = + FLAGS_SET(buf.f_flag, ST_RDONLY) * MS_RDONLY | + FLAGS_SET(buf.f_flag, ST_NODEV) * MS_NODEV | + FLAGS_SET(buf.f_flag, ST_NOEXEC) * MS_NOEXEC | + FLAGS_SET(buf.f_flag, ST_NOSUID) * MS_NOSUID | + FLAGS_SET(buf.f_flag, ST_NOATIME) * MS_NOATIME | + FLAGS_SET(buf.f_flag, ST_NODIRATIME) * MS_NODIRATIME; + return 0; } @@ -236,7 +258,7 @@ int bind_remount_recursive_with_mountinfo( return -errno; orig_flags = 0; - (void) get_mount_flags(simplified, &orig_flags, table); + (void) get_mount_flags(table, simplified, &orig_flags); if (mount(NULL, simplified, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) return -errno; @@ -277,7 +299,7 @@ int bind_remount_recursive_with_mountinfo( /* Try to reuse the original flag set */ orig_flags = 0; - (void) get_mount_flags(x, &orig_flags, table); + (void) get_mount_flags(table, x, &orig_flags); if (mount(NULL, x, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) return -errno; From 8403219fc13abebde55e24c19280b1b481c68dd1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Jan 2020 15:05:55 +0100 Subject: [PATCH 4/5] mount-util: line break overly long function prototypes --- src/shared/mount-util.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 09fa7d3943..c74f391b06 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -143,6 +143,7 @@ int bind_remount_recursive_with_mountinfo( _cleanup_free_ char *simplified = NULL; int r; + assert(prefix); assert(proc_self_mountinfo); /* Recursively remount a directory (and all its submounts) read-only or read-write. If the directory is already @@ -309,7 +310,12 @@ int bind_remount_recursive_with_mountinfo( } } -int bind_remount_recursive(const char *prefix, unsigned long new_flags, unsigned long flags_mask, char **blacklist) { +int bind_remount_recursive( + const char *prefix, + unsigned long new_flags, + unsigned long flags_mask, + char **blacklist) { + _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; int r; From 7cce68e1e042e84decae21cd5d67750235f3d539 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Jan 2020 15:06:06 +0100 Subject: [PATCH 5/5] core: make sure we use the correct mount flag when re-mounting bind mounts When in a userns environment we cannot take away per-mount point flags set on a mount point that was passed to us. Hence we need to be careful to always check the actual mount flags in place and manipulate only those flags of them that we actually want to change and not reset more as side-effect. We mostly got this right already in bind_remount_recursive_with_mountinfo(), but didn't in the simpler bind_remount_one_with_mountinfo(). Catch up. (The old code assumed that the MountEntry.flags field contained the right flag settings, but it actually doesn't for new mounts we just established as for those mount() establishes the initial flags for us, and we have to read them back to figure out which ones the kernel picked.) Fixes: #13622 --- src/core/namespace.c | 10 +--------- src/shared/mount-util.c | 32 ++++++++++++++++++++++++++++++++ src/shared/mount-util.h | 1 + 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index fee4c98096..d4702cdd96 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -1063,14 +1063,6 @@ static int apply_mount( return 0; } -/* Change per-mount flags on an existing mount */ -static int bind_remount_one(const char *path, unsigned long orig_flags, unsigned long new_flags, unsigned long flags_mask) { - if (mount(NULL, path, NULL, (orig_flags & ~flags_mask) | MS_REMOUNT | MS_BIND | new_flags, NULL) < 0) - return -errno; - - return 0; -} - static int make_read_only(const MountEntry *m, char **blacklist, FILE *proc_self_mountinfo) { unsigned long new_flags = 0, flags_mask = 0; bool submounts = false; @@ -1102,7 +1094,7 @@ static int make_read_only(const MountEntry *m, char **blacklist, FILE *proc_self if (submounts) r = bind_remount_recursive_with_mountinfo(mount_entry_path(m), new_flags, flags_mask, blacklist, proc_self_mountinfo); else - r = bind_remount_one(mount_entry_path(m), m->flags, new_flags, flags_mask); + r = bind_remount_one_with_mountinfo(mount_entry_path(m), new_flags, flags_mask, proc_self_mountinfo); /* Not that we only turn on the MS_RDONLY flag here, we never turn it off. Something that was marked * read-only already stays this way. This improves compatibility with container managers, where we diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index c74f391b06..32c5332822 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -326,6 +326,38 @@ int bind_remount_recursive( return bind_remount_recursive_with_mountinfo(prefix, new_flags, flags_mask, blacklist, proc_self_mountinfo); } +int bind_remount_one_with_mountinfo( + const char *path, + unsigned long new_flags, + unsigned long flags_mask, + FILE *proc_self_mountinfo) { + + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + unsigned long orig_flags = 0; + int r; + + assert(path); + assert(proc_self_mountinfo); + + rewind(proc_self_mountinfo); + + table = mnt_new_table(); + if (!table) + return -ENOMEM; + + r = mnt_table_parse_stream(table, proc_self_mountinfo, "/proc/self/mountinfo"); + if (r < 0) + return r; + + /* Try to reuse the original flag set */ + (void) get_mount_flags(table, path, &orig_flags); + + if (mount(NULL, path, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL) < 0) + return -errno; + + return 0; +} + int mount_move_root(const char *path) { assert(path); diff --git a/src/shared/mount-util.h b/src/shared/mount-util.h index 9a8d073631..06fddacf16 100644 --- a/src/shared/mount-util.h +++ b/src/shared/mount-util.h @@ -10,6 +10,7 @@ int repeat_unmount(const char *path, int flags); int umount_recursive(const char *target, int flags); int bind_remount_recursive(const char *prefix, unsigned long new_flags, unsigned long flags_mask, char **blacklist); int bind_remount_recursive_with_mountinfo(const char *prefix, unsigned long new_flags, unsigned long flags_mask, char **blacklist, FILE *proc_self_mountinfo); +int bind_remount_one_with_mountinfo(const char *path, unsigned long new_flags, unsigned long flags_mask, FILE *proc_self_mountinfo); int mount_move_root(const char *path);