From 981318d2b5537352c0d989bb70c64171c82e1adf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 10:49:23 +0100 Subject: [PATCH 01/14] test-namespace: drop spurious double new line --- src/test/test-namespace.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/test-namespace.c b/src/test/test-namespace.c index bf4b87e8a6..8ec3859a9b 100644 --- a/src/test/test-namespace.c +++ b/src/test/test-namespace.c @@ -147,7 +147,6 @@ static void test_protect_kernel_logs(void) { return; } - pid = fork(); assert_se(pid >= 0); From b898474fecda9e39aa06512ef7ba358c9685d84e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Mar 2021 22:26:06 +0100 Subject: [PATCH 02/14] mountpoint-util: a symlink is never a mount point The various flavours of stat() basically tell us for free if something is a symlink. If it is, then it's definitely not a mount point. Use that. All other inode types can be mount point, just symlinks cannot. --- src/basic/mountpoint-util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index c8f6675eb8..6d47419194 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -196,13 +196,15 @@ int fd_is_mount_point(int fd, const char *filename, int flags) { if (statx(fd, filename, (FLAGS_SET(flags, AT_SYMLINK_FOLLOW) ? 0 : AT_SYMLINK_NOFOLLOW) | (flags & AT_EMPTY_PATH) | - AT_NO_AUTOMOUNT, 0, &sx) < 0) { + AT_NO_AUTOMOUNT, STATX_TYPE, &sx) < 0) { if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno)) return -errno; /* If statx() is not available or forbidden, fall back to name_to_handle_at() below */ } else if (FLAGS_SET(sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) /* yay! */ return FLAGS_SET(sx.stx_attributes, STATX_ATTR_MOUNT_ROOT); + else if (FLAGS_SET(sx.stx_mask, STATX_TYPE) && S_ISLNK(sx.stx_mode)) + return false; /* symlinks are never mount points */ r = name_to_handle_at_loop(fd, filename, &h, &mount_id, flags); if (IN_SET(r, -ENOSYS, -EACCES, -EPERM, -EOVERFLOW, -EINVAL)) @@ -280,6 +282,8 @@ fallback_fstat: flags |= AT_SYMLINK_NOFOLLOW; if (fstatat(fd, filename, &a, flags) < 0) return -errno; + if (S_ISLNK(a.st_mode)) /* Symlinks are never mount points */ + return false; if (fstatat(fd, "", &b, AT_EMPTY_PATH) < 0) return -errno; From 9d0619dee729b9b481efa512295cf70cb5887fe1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Mar 2021 22:28:16 +0100 Subject: [PATCH 03/14] mountpoint-util: rebreak some comments --- src/basic/mountpoint-util.c | 24 +++++++++--------------- src/shared/mount-util.c | 5 ++--- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index 6d47419194..1d617e87b2 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -233,16 +233,13 @@ int fd_is_mount_point(int fd, const char *filename, int flags) { } else if (r < 0) return r; - /* The parent can do name_to_handle_at() but the - * directory we are interested in can't? If so, it - * must be a mount point. */ + /* The parent can do name_to_handle_at() but the directory we are interested in can't? If so, it must + * be a mount point. */ if (nosupp) return 1; - /* If the file handle for the directory we are - * interested in and its parent are identical, we - * assume this is the root directory, which is a mount - * point. */ + /* If the file handle for the directory we are interested in and its parent are identical, we assume + * this is the root directory, which is a mount point. */ if (h->handle_bytes == h_parent->handle_bytes && h->handle_type == h_parent->handle_type && @@ -265,17 +262,14 @@ fallback_fdinfo: if (mount_id != mount_id_parent) return 1; - /* Hmm, so, the mount ids are the same. This leaves one - * special case though for the root file system. For that, - * let's see if the parent directory has the same inode as we - * are interested in. Hence, let's also do fstat() checks now, - * too, but avoid the st_dev comparisons, since they aren't - * that useful on unionfs mounts. */ + /* Hmm, so, the mount ids are the same. This leaves one special case though for the root file + * system. For that, let's see if the parent directory has the same inode as we are interested + * in. Hence, let's also do fstat() checks now, too, but avoid the st_dev comparisons, since they + * aren't that useful on unionfs mounts. */ check_st_dev = false; fallback_fstat: - /* yay for fstatat() taking a different set of flags than the other - * _at() above */ + /* yay for fstatat() taking a different set of flags than the other _at() above */ if (flags & AT_SYMLINK_FOLLOW) flags &= ~AT_SYMLINK_FOLLOW; else diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 576e4054c2..53516c671b 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -84,9 +84,8 @@ int umount_recursive(const char *prefix, int flags) { int n = 0, r; bool again; - /* Try to umount everything recursively below a - * directory. Also, take care of stacked mounts, and keep - * unmounting them until they are gone. */ + /* Try to umount everything recursively below a directory. Also, take care of stacked mounts, and + * keep unmounting them until they are gone. */ do { _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; From 670e8efd6083bfef6186d33fa3b7dc90810d14ca Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 13:47:17 +0100 Subject: [PATCH 04/14] mount-until: make sure we'll exit bind_remount_recursive_with_mountinfo() loop eventually Just some robustness given that /proc/self/mountinfo was previously broken in the kernel. --- src/shared/mount-util.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 53516c671b..b9f46784db 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -204,6 +204,7 @@ int bind_remount_recursive_with_mountinfo( _cleanup_set_free_free_ Set *done = NULL; _cleanup_free_ char *simplified = NULL; + unsigned n_tries = 0; int r; assert(prefix); @@ -239,6 +240,9 @@ int bind_remount_recursive_with_mountinfo( char *x; unsigned long orig_flags; + if (n_tries++ >= 32) /* Let's not retry this loop forever */ + return -EBUSY; + todo = set_new(&path_hash_ops); if (!todo) return -ENOMEM; From c6111b85f77fa6207c0ac50a99cf5d05c2aa33c9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 13:49:06 +0100 Subject: [PATCH 05/14] path: drop simplification of path in bind_remount_recursive_with_mountinfo() We only use path-aware comparisons and hashops, hence no need to simplify, we can use the path as it is. --- src/shared/mount-util.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index b9f46784db..5a3d4b18d0 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -203,7 +203,6 @@ int bind_remount_recursive_with_mountinfo( FILE *proc_self_mountinfo) { _cleanup_set_free_free_ Set *done = NULL; - _cleanup_free_ char *simplified = NULL; unsigned n_tries = 0; int r; @@ -222,12 +221,6 @@ int bind_remount_recursive_with_mountinfo( * If the "deny_list" parameter is specified it may contain a list of subtrees to exclude from the * remount operation. Note that we'll ignore the deny list for the top-level path. */ - simplified = strdup(prefix); - if (!simplified) - return -ENOMEM; - - path_simplify(simplified, false); - done = set_new(&path_hash_ops); if (!done) return -ENOMEM; @@ -268,26 +261,26 @@ int bind_remount_recursive_with_mountinfo( if (!path || !type) continue; - if (!path_startswith(path, simplified)) + if (!path_startswith(path, prefix)) continue; /* Ignore this mount if it is deny-listed, but only if it isn't the top-level mount * we shall operate on. */ - if (!path_equal(path, simplified)) { + if (!path_equal(path, prefix)) { bool deny_listed = false; char **i; STRV_FOREACH(i, deny_list) { - if (path_equal(*i, simplified)) + if (path_equal(*i, prefix)) continue; - if (!path_startswith(*i, simplified)) + if (!path_startswith(*i, prefix)) continue; if (path_startswith(path, *i)) { deny_listed = true; log_debug("Not remounting %s deny-listed by %s, called for %s", - path, *i, simplified); + path, *i, prefix); break; } } @@ -302,7 +295,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, simplified); + top_autofs = top_autofs || path_equal(path, prefix); continue; } @@ -317,26 +310,26 @@ 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, simplified))) + (top_autofs || set_contains(done, prefix))) return 0; - if (!set_contains(done, simplified) && - !set_contains(todo, simplified)) { + if (!set_contains(done, prefix) && + !set_contains(todo, prefix)) { /* The prefix directory itself is not yet a mount, make it one. */ - r = mount_nofollow(simplified, simplified, NULL, MS_BIND|MS_REC, NULL); + r = mount_nofollow(prefix, prefix, NULL, MS_BIND|MS_REC, NULL); if (r < 0) return r; orig_flags = 0; - (void) get_mount_flags(table, simplified, &orig_flags); + (void) get_mount_flags(table, prefix, &orig_flags); - r = mount_nofollow(NULL, simplified, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL); + r = mount_nofollow(NULL, prefix, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL); if (r < 0) return r; log_debug("Made top-level directory %s a mount point.", prefix); - r = set_put_strdup(&done, simplified); + r = set_put_strdup(&done, prefix); if (r < 0) return r; } From d6bfab118d2e7c4c07566718175f4f7ec7a2387b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 13:56:55 +0100 Subject: [PATCH 06/14] mount-util: reorder a few things Let's always query one property, check it, and then query the next, preferring "cheap" ones over "slow" ones (i.e. cheap are the ones we can check directly, and slow are the ones we need to check with some loop of some kind). --- src/shared/mount-util.c | 43 +++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 5a3d4b18d0..6180d9c8c0 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -251,19 +251,33 @@ int bind_remount_recursive_with_mountinfo( const char *path, *type; r = mnt_table_next_fs(table, iter, &fs); - if (r == 1) + if (r == 1) /* EOF */ break; if (r < 0) return log_debug_errno(r, "Failed to get next entry from /proc/self/mountinfo: %m"); path = mnt_fs_get_target(fs); - type = mnt_fs_get_fstype(fs); - if (!path || !type) + if (!path) continue; if (!path_startswith(path, prefix)) continue; + type = mnt_fs_get_fstype(fs); + if (!type) + continue; + + /* Let's ignore autofs mounts. If they aren't triggered yet, we want to avoid + * triggering them, as we don't make any guarantees for future submounts anyway. If + * they are already triggered, then we will find another entry for this. */ + if (streq(type, "autofs")) { + top_autofs = top_autofs || path_equal(path, prefix); + continue; + } + + if (set_contains(done, path)) + continue; + /* Ignore this mount if it is deny-listed, but only if it isn't the top-level mount * we shall operate on. */ if (!path_equal(path, prefix)) { @@ -279,31 +293,18 @@ int bind_remount_recursive_with_mountinfo( if (path_startswith(path, *i)) { deny_listed = true; - log_debug("Not remounting %s deny-listed by %s, called for %s", - path, *i, prefix); + log_debug("Not remounting %s deny-listed by %s, called for %s", path, *i, prefix); break; } } + if (deny_listed) continue; } - /* Let's ignore autofs mounts. If they aren't - * triggered yet, we want to avoid triggering - * them, as we don't make any guarantees for - * future submounts anyway. If they are - * already triggered, then we will find - * another entry for this. */ - if (streq(type, "autofs")) { - top_autofs = top_autofs || path_equal(path, prefix); - continue; - } - - if (!set_contains(done, path)) { - r = set_put_strdup(&todo, path); - if (r < 0) - return r; - } + r = set_put_strdup(&todo, path); + if (r < 0) + return r; } /* If we have no submounts to process anymore and if From 5c5753b9ea5cc012586ae90d357d460dec4301a4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:02:29 +0100 Subject: [PATCH 07/14] mount-util: shortcut things after generating top-level bind mount Instead of marking the bind mount read-only right-away, let's just restart the loop, so that we'll pick it up like any other mount and then remount like that. --- src/shared/mount-util.c | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 6180d9c8c0..fb7e513acc 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -307,34 +307,25 @@ int bind_remount_recursive_with_mountinfo( return r; } - /* If we have no submounts to process anymore and if - * the root is either already done, or an autofs, we - * are done */ - if (set_isempty(todo) && - (top_autofs || set_contains(done, prefix))) - return 0; - + /* Check if the top-level directory was among what we have seen so far. For that check both + * 'done' and 'todo'. Also check 'top_autofs' because if the top-level dir is an autofs we'll + * not include it in either set but will set this bool. */ if (!set_contains(done, prefix) && - !set_contains(todo, prefix)) { + !(top_autofs || set_contains(todo, prefix))) { + /* The prefix directory itself is not yet a mount, make it one. */ r = mount_nofollow(prefix, prefix, NULL, MS_BIND|MS_REC, NULL); if (r < 0) return r; - orig_flags = 0; - (void) get_mount_flags(table, prefix, &orig_flags); - - r = mount_nofollow(NULL, prefix, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL); - if (r < 0) - return r; - - log_debug("Made top-level directory %s a mount point.", prefix); - - r = set_put_strdup(&done, prefix); - if (r < 0) - return r; + /* Immediately rescan, so that we pick up the new mount's flags */ + continue; } + /* If we have no submounts to process anymore, we are done */ + if (set_isempty(todo)) + return 0; + while ((x = set_steal_first(todo))) { r = set_consume(done, x); From 4b6ef52756ce1be1bd365c2bb63ad4f0c895c1a5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:06:22 +0100 Subject: [PATCH 08/14] mount-util: extend comment a bit, mention that we aren't atomic in behaviour --- src/shared/mount-util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index fb7e513acc..778cfba1ea 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -216,7 +216,9 @@ int bind_remount_recursive_with_mountinfo( * access, too. When mounts are stacked on the same mount point we only care for each individual * "top-level" mount on each point, as we cannot influence/access the underlying mounts anyway. We do * not have any effect on future submounts that might get propagated, they might be writable - * etc. This includes future submounts that have been triggered via autofs. + * etc. This includes future submounts that have been triggered via autofs. Also note that we can't + * operate atomically here. Mounts established while we process the tree might or might not get + * noticed and thus might or might not be covered. * * If the "deny_list" parameter is specified it may contain a list of subtrees to exclude from the * remount operation. Note that we'll ignore the deny list for the top-level path. */ From ba8dced2a60d8535bf6d685e2fd164e5ff3f056b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:07:32 +0100 Subject: [PATCH 09/14] mount-util: store mount flags in "todo" list in + handle submounts gracefully This replaces the "todo" set with a "todo" hash map that stores the mount flags we found. This makes an explicit call to get_mount_flags() unncessary, since we have the flags handy right-away, and lowers our work from O(n^2) to O(n). Nice! The "done" set is also improved slightly: we'll use more modern ways to allocate it, via set_ensure_consume(), and freeing-via-hash_ops. Finally, failures on submount remounts are now handled gracefully, there are just too many reasons why they might fail, given NFS, autofs, FUSE which weird access controls, where even root might lack the privs to do something. Fixes: #16156 --- src/shared/mount-util.c | 111 +++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 42 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 778cfba1ea..381fece872 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -202,7 +202,7 @@ int bind_remount_recursive_with_mountinfo( char **deny_list, FILE *proc_self_mountinfo) { - _cleanup_set_free_free_ Set *done = NULL; + _cleanup_set_free_ Set *done = NULL; unsigned n_tries = 0; int r; @@ -223,25 +223,15 @@ int bind_remount_recursive_with_mountinfo( * If the "deny_list" parameter is specified it may contain a list of subtrees to exclude from the * remount operation. Note that we'll ignore the deny list for the top-level path. */ - done = set_new(&path_hash_ops); - if (!done) - return -ENOMEM; - for (;;) { - _cleanup_set_free_free_ Set *todo = NULL; _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; + _cleanup_hashmap_free_ Hashmap *todo = NULL; bool top_autofs = false; - char *x; - unsigned long orig_flags; if (n_tries++ >= 32) /* Let's not retry this loop forever */ return -EBUSY; - todo = set_new(&path_hash_ops); - if (!todo) - return -ENOMEM; - rewind(proc_self_mountinfo); r = libmount_parse("/proc/self/mountinfo", proc_self_mountinfo, &table, &iter); @@ -249,8 +239,10 @@ int bind_remount_recursive_with_mountinfo( return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); for (;;) { + _cleanup_free_ char *d = NULL; + const char *path, *type, *opts; + unsigned long flags = 0; struct libmnt_fs *fs; - const char *path, *type; r = mnt_table_next_fs(table, iter, &fs); if (r == 1) /* EOF */ @@ -304,16 +296,31 @@ int bind_remount_recursive_with_mountinfo( continue; } - r = set_put_strdup(&todo, path); + opts = mnt_fs_get_vfs_options(fs); + if (opts) { + r = mnt_optstr_get_flags(opts, &flags, mnt_get_builtin_optmap(MNT_LINUX_MAP)); + if (r < 0) + log_debug_errno(r, "Could not get flags for '%s', ignoring: %m", path); + } + + d = strdup(path); + if (!d) + return -ENOMEM; + + r = hashmap_ensure_put(&todo, &path_hash_ops_free, d, ULONG_TO_PTR(flags)); + if (r == -EEXIST) + continue; if (r < 0) return r; + if (r > 0) + TAKE_PTR(d); } /* Check if the top-level directory was among what we have seen so far. For that check both * 'done' and 'todo'. Also check 'top_autofs' because if the top-level dir is an autofs we'll * not include it in either set but will set this bool. */ if (!set_contains(done, prefix) && - !(top_autofs || set_contains(todo, prefix))) { + !(top_autofs || hashmap_contains(todo, prefix))) { /* The prefix directory itself is not yet a mount, make it one. */ r = mount_nofollow(prefix, prefix, NULL, MS_BIND|MS_REC, NULL); @@ -325,46 +332,66 @@ int bind_remount_recursive_with_mountinfo( } /* If we have no submounts to process anymore, we are done */ - if (set_isempty(todo)) + if (hashmap_isempty(todo)) return 0; - while ((x = set_steal_first(todo))) { + for (;;) { + unsigned long flags; + char *x = NULL; - r = set_consume(done, x); + /* Take the first mount from our list of mounts to still process */ + flags = PTR_TO_ULONG(hashmap_steal_first_key_and_value(todo, (void**) &x)); + if (!x) + break; + + r = set_ensure_consume(&done, &path_hash_ops_free, x); if (IN_SET(r, 0, -EEXIST)) - continue; + continue; /* Already done */ if (r < 0) return r; - /* Deal with mount points that are obstructed by a later mount */ - r = path_is_mount_point(x, NULL, 0); - if (IN_SET(r, 0, -ENOENT)) - continue; + /* Now, remount this with the new flags set, but exclude MS_RELATIME from it. (It's + * the default anyway, thus redundant, and in userns we'll get an error if we try to + * explicitly enable it) */ + r = mount_nofollow(NULL, x, NULL, ((flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags) & ~MS_RELATIME, NULL); if (r < 0) { - if (!ERRNO_IS_PRIVILEGE(r)) + int q; + + /* OK, so the remount of this entry failed. We'll ultimately ignore this in + * almost all cases (there are simply so many reasons why this can fail, + * think autofs, NFS, FUSE, …), but let's generate useful debug messages at + * the very least. */ + + q = path_is_mount_point(x, NULL, 0); + if (IN_SET(q, 0, -ENOENT)) { + /* Hmm, whaaaa? The mount point is not actually a mount point? Then + * it is either obstructed by a later mount or somebody has been + * racing against us and removed it. Either way the mount point + * doesn't matter to us, let's ignore it hence. */ + log_debug_errno(r, "Mount point '%s' to remount is not a mount point anymore, ignoring remount failure: %m", x); + continue; + } + if (q < 0) /* Any other error on this? Just log and continue */ + log_debug_errno(q, "Failed to determine whether '%s' is a mount point or not, ignoring: %m", x); + + if (((flags ^ new_flags) & flags_mask & ~MS_RELATIME) == 0) { /* ignore MS_RELATIME while comparing */ + log_debug_errno(r, "Couldn't remount '%s', but the flags already match what we want, hence ignoring: %m", x); + continue; + } + + /* Make this fatal if this is the top-level mount */ + if (path_equal(x, prefix)) return r; - /* Even if root user invoke this, submounts under private FUSE or NFS mount points - * may not be acceessed. E.g., - * - * $ bindfs --no-allow-other ~/mnt/mnt ~/mnt/mnt - * $ bindfs --no-allow-other ~/mnt ~/mnt - * - * Then, root user cannot access the mount point ~/mnt/mnt. - * In such cases, the submounts are ignored, as we have no way to manage them. */ - log_debug_errno(r, "Failed to determine '%s' is mount point or not, ignoring: %m", x); + /* If this is not the top-level mount, then handle this gracefully: log but + * otherwise ignore. With NFS, FUSE, autofs there are just too many reasons + * this might fail without a chance for us to do anything about it, let's + * hence be strict on the top-level mount and lenient on the inner ones. */ + log_debug_errno(r, "Couldn't remount submount '%s' for unexpected reason, ignoring: %m", x); continue; } - /* Try to reuse the original flag set */ - orig_flags = 0; - (void) get_mount_flags(table, x, &orig_flags); - - r = mount_nofollow(NULL, x, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL); - if (r < 0) - return r; - - log_debug("Remounted %s read-only.", x); + log_debug("Remounted %s.", x); } } } From 2c5ff8ea76d8c3c9944567ae97f498925dfcd443 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:16:45 +0100 Subject: [PATCH 10/14] mount-util: fold what we need from get_mount_flags() bind_remount_one_with_mountinfo() And get rid of get_mount_flags() altogether. (This drops the statvfs() fallback that get_mount_flags() did. That fallback was incomplete however, and mostly hid errors. Our primary avenue to get mount flags is /proc/self/mountinfo and we should trust it, and fix bugs we might encounter with it, but not tape over it. Dropping the fallback is relevant in particular as it actually returned mount flags for any path, not just mount points, which was very icky.) --- src/shared/mount-util.c | 80 ++++++++--------------------------------- 1 file changed, 14 insertions(+), 66 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 381fece872..ab2f62c32f 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -131,68 +131,6 @@ int umount_recursive(const char *prefix, int flags) { return n; } -static int get_mount_flags( - struct libmnt_table *table, - const char *path, - unsigned long *ret) { - - _cleanup_close_ int fd = -1; - struct libmnt_fs *fs; - struct statvfs buf; - const char *opts; - int r; - - /* 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. - * - * This generally doesn't follow symlinks. */ - - fs = mnt_table_find_target(table, path, MNT_ITER_FORWARD); - if (!fs) { - log_debug("Could not find '%s' in mount table, ignoring.", path); - goto fallback; - } - - opts = mnt_fs_get_vfs_options(fs); - if (!opts) { - *ret = 0; - return 0; - } - - r = mnt_optstr_get_flags(opts, ret, mnt_get_builtin_optmap(MNT_LINUX_MAP)); - if (r != 0) { - log_debug_errno(r, "Could not get flags for '%s', ignoring: %m", path); - goto fallback; - } - - /* MS_RELATIME is default and trying to set it in an unprivileged container causes EPERM */ - *ret &= ~MS_RELATIME; - return 0; - -fallback: - fd = open(path, O_PATH|O_CLOEXEC|O_NOFOLLOW); - if (fd < 0) - return -errno; - - if (fstatvfs(fd, &buf) < 0) - return -errno; - - /* 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; -} - /* Use this function only if you do not have direct access to /proc/self/mountinfo but the caller can open it * for you. This is the case when /proc is masked or not mounted. Otherwise, use bind_remount_recursive. */ int bind_remount_recursive_with_mountinfo( @@ -419,7 +357,9 @@ int bind_remount_one_with_mountinfo( FILE *proc_self_mountinfo) { _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; - unsigned long orig_flags = 0; + unsigned long flags = 0; + struct libmnt_fs *fs; + const char *opts; int r; assert(path); @@ -435,10 +375,18 @@ int bind_remount_one_with_mountinfo( if (r < 0) return r; - /* Try to reuse the original flag set */ - (void) get_mount_flags(table, path, &orig_flags); + fs = mnt_table_find_target(table, path, MNT_ITER_FORWARD); + if (!fs) + return -EINVAL; /* Not a mount point we recognize */ - r = mount_nofollow(NULL, path, NULL, (orig_flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags, NULL); + opts = mnt_fs_get_vfs_options(fs); + if (opts) { + r = mnt_optstr_get_flags(opts, &flags, mnt_get_builtin_optmap(MNT_LINUX_MAP)); + if (r < 0) + log_debug_errno(r, "Could not get flags for '%s', ignoring: %m", path); + } + + r = mount_nofollow(NULL, path, NULL, ((flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags) & ~MS_RELATIME, NULL); if (r < 0) return r; From b23c6a6411fd58ac281642bcae915a7ea55db5c7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:17:20 +0100 Subject: [PATCH 11/14] mount-util: handle remount failures gracefully if flags already match In bind_remount_one_with_mountinfo() let's handle mount failures gracefully if the flags already match anyway. This isn't perfect, since it mixes up superblock and mount point flags, but it's close enough. --- src/shared/mount-util.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index ab2f62c32f..f226dc1aa6 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -387,8 +387,16 @@ int bind_remount_one_with_mountinfo( } r = mount_nofollow(NULL, path, NULL, ((flags & ~flags_mask)|MS_BIND|MS_REMOUNT|new_flags) & ~MS_RELATIME, NULL); - if (r < 0) - return r; + if (r < 0) { + if (((flags ^ new_flags) & flags_mask & ~MS_RELATIME) != 0) /* Ignore MS_RELATIME again, + * since kernel adds it in + * everywhere, because it's the + * default. */ + return r; + + /* Let's handle redundant remounts gracefully */ + log_debug_errno(r, "Failed to remount '%s' but flags already match what we want, ignoring: %m", path); + } return 0; } From 0338df47cfaeb023c7c2eaf6b698060fd133157b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 14:19:04 +0100 Subject: [PATCH 12/14] mount-util: generate failure if bind_remount_one_with_mountinfo() is called on non-existing path --- src/shared/mount-util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index f226dc1aa6..13f202d7e7 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -376,8 +376,12 @@ int bind_remount_one_with_mountinfo( return r; fs = mnt_table_find_target(table, path, MNT_ITER_FORWARD); - if (!fs) + if (!fs) { + if (laccess(path, F_OK) < 0) /* Hmm, it's not in the mount table, but does it exist at all? */ + return -errno; + return -EINVAL; /* Not a mount point we recognize */ + } opts = mnt_fs_get_vfs_options(fs); if (opts) { From 10cdbb8372238337e0789fc4fc40949f55caddb6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 13:44:56 +0100 Subject: [PATCH 13/14] test: add test for bind_remount_recursive() --- src/test/test-mount-util.c | 65 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c index 41df5588d0..29f462bfab 100644 --- a/src/test/test-mount-util.c +++ b/src/test/test-mount-util.c @@ -1,11 +1,20 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include +#include #include "alloc-util.h" +#include "fd-util.h" +#include "fileio.h" #include "mount-util.h" +#include "namespace-util.h" +#include "path-util.h" +#include "process-util.h" +#include "rm-rf.h" #include "string-util.h" +#include "strv.h" #include "tests.h" +#include "tmpfile-util.h" static void test_mount_option_mangle(void) { char *opts = NULL; @@ -61,10 +70,66 @@ static void test_mount_option_mangle(void) { assert_se(mount_option_mangle("rw,relatime,fmask=0022,dmask=0022,\"hogehoge", MS_RDONLY, &f, &opts) < 0); } +static void test_bind_remount_recursive(void) { + _cleanup_(rm_rf_physical_and_freep) char *tmp = NULL; + _cleanup_free_ char *subdir = NULL; + const char *p; + + if (geteuid() != 0) { + (void) log_tests_skipped("not running as root"); + return; + } + + assert_se(mkdtemp_malloc("/tmp/XXXXXX", &tmp) >= 0); + subdir = path_join(tmp, "subdir"); + assert_se(subdir); + assert_se(mkdir(subdir, 0755) >= 0); + + FOREACH_STRING(p, "/usr", "/sys", "/", tmp) { + pid_t pid; + + pid = fork(); + assert_se(pid >= 0); + + if (pid == 0) { + struct statvfs svfs; + /* child */ + assert_se(detach_mount_namespace() >= 0); + + /* Check that the subdir is writable (it must be because it's in /tmp) */ + assert_se(statvfs(subdir, &svfs) >= 0); + assert_se(!FLAGS_SET(svfs.f_flag, ST_RDONLY)); + + /* Make the subdir a bind mount */ + assert_se(mount_nofollow(subdir, subdir, NULL, MS_BIND|MS_REC, NULL) >= 0); + + /* Ensure it's still writable */ + assert_se(statvfs(subdir, &svfs) >= 0); + assert_se(!FLAGS_SET(svfs.f_flag, ST_RDONLY)); + + /* Now mark the path we currently run for read-only */ + assert_se(bind_remount_recursive(p, MS_RDONLY, MS_RDONLY, STRV_MAKE("/sys/kernel")) >= 0); + + /* Ensure that this worked on the top-level */ + assert_se(statvfs(p, &svfs) >= 0); + assert_se(FLAGS_SET(svfs.f_flag, ST_RDONLY)); + + /* And ensure this had an effect on the subdir exactly if we are talking about a path above the subdir */ + assert_se(statvfs(subdir, &svfs) >= 0); + assert_se(FLAGS_SET(svfs.f_flag, ST_RDONLY) == !!path_startswith(subdir, p)); + + _exit(EXIT_SUCCESS); + } + + assert_se(wait_for_terminate_and_check("test-remount-rec", pid, WAIT_LOG) == EXIT_SUCCESS); + } +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); test_mount_option_mangle(); + test_bind_remount_recursive(); return 0; } From 67d22a366567df874352099ff38083ffb3024d72 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Mar 2021 13:45:25 +0100 Subject: [PATCH 14/14] test: add explicit test for bind_remount_one_with_mountinfo() --- src/test/test-mount-util.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c index 29f462bfab..2e9116d359 100644 --- a/src/test/test-mount-util.c +++ b/src/test/test-mount-util.c @@ -125,11 +125,43 @@ static void test_bind_remount_recursive(void) { } } +static void test_bind_remount_one(void) { + pid_t pid; + + if (geteuid() != 0) { + (void) log_tests_skipped("not running as root"); + return; + } + + pid = fork(); + assert_se(pid >= 0); + + if (pid == 0) { + /* child */ + + _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; + + assert_se(detach_mount_namespace() >= 0); + + assert_se(fopen_unlocked("/proc/self/mountinfo", "re", &proc_self_mountinfo) >= 0); + + assert_se(bind_remount_one_with_mountinfo("/run", MS_RDONLY, MS_RDONLY, proc_self_mountinfo) >= 0); + assert_se(bind_remount_one_with_mountinfo("/proc/idontexist", MS_RDONLY, MS_RDONLY, proc_self_mountinfo) == -ENOENT); + assert_se(bind_remount_one_with_mountinfo("/proc/self", MS_RDONLY, MS_RDONLY, proc_self_mountinfo) == -EINVAL); + assert_se(bind_remount_one_with_mountinfo("/", MS_RDONLY, MS_RDONLY, proc_self_mountinfo) >= 0); + + _exit(EXIT_SUCCESS); + } + + assert_se(wait_for_terminate_and_check("test-remount-one", pid, WAIT_LOG) == EXIT_SUCCESS); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); test_mount_option_mangle(); test_bind_remount_recursive(); + test_bind_remount_one(); return 0; }