mount-util: simplify mount_switch_root() a bit

There's no need to fchdir() out of the rootfs and back into it around
the umount2(), hence don't.

This brings the logic closer to what the pivot_root() man page suggests.

While we are at it, always operate based on fds, once we opened the
original dir, and pass the path string along only for generating
messages (i.e. as "decoration").

Add tests for both code paths: the pivot_root() one and the MS_MOUNT.

diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c
index f30b5f1a7fcc174b65cc2f0b6cc2f599faf5adbc..b41672eb922ad595eece9857304e6d129d891a57 100644 (file)
--- a/src/shared/mount-util.c
+++ b/src/shared/mount-util.c
@@ -431,50 +431,46 @@ int bind_remount_one_with_mountinfo(
         return 0;
 }
 
-static int mount_switch_root_pivot(const char *path, int fd_newroot) {
-        _cleanup_close_ int fd_oldroot = -EBADF;
+static int mount_switch_root_pivot(int fd_newroot, const char *path) {
+        assert(fd_newroot >= 0);
+        assert(path);
 
-        fd_oldroot = open("/", O_PATH|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW);
-        if (fd_oldroot < 0)
-                return log_debug_errno(errno, "Failed to open old rootfs");
+        /* Change into the new rootfs. */
+        if (fchdir(fd_newroot) < 0)
+                return log_debug_errno(errno, "Failed to change into new rootfs '%s': %m", path);
 
         /* Let the kernel tuck the new root under the old one. */
         if (pivot_root(".", ".") < 0)
                 return log_debug_errno(errno, "Failed to pivot root to new rootfs '%s': %m", path);
 
-        /* At this point the new root is tucked under the old root. If we want
-         * to unmount it we cannot be fchdir()ed into it. So escape back to the
-         * old root. */
-        if (fchdir(fd_oldroot) < 0)
-                return log_debug_errno(errno, "Failed to change back to old rootfs: %m");
-
-        /* Note, usually we should set mount propagation up here but we'll
-         * assume that the caller has already done that. */
-
-        /* Get rid of the old root and reveal our brand new root. */
+        /* Get rid of the old root and reveal our brand new root. (This will always operate on the top-most
+         * mount on our cwd, regardless what our current directory actually points to.) */
         if (umount2(".", MNT_DETACH) < 0)
                 return log_debug_errno(errno, "Failed to unmount old rootfs: %m");
 
-        if (fchdir(fd_newroot) < 0)
-                return log_debug_errno(errno, "Failed to switch to new rootfs '%s': %m", path);
-
         return 0;
 }
 
-static int mount_switch_root_move(const char *path) {
-        if (mount(path, "/", NULL, MS_MOVE, NULL) < 0)
+static int mount_switch_root_move(int fd_newroot, const char *path) {
+        assert(fd_newroot >= 0);
+        assert(path);
+
+        /* Change into the new rootfs. */
+        if (fchdir(fd_newroot) < 0)
+                return log_debug_errno(errno, "Failed to change into new rootfs '%s': %m", path);
+
+        /* Move the new root fs */
+        if (mount(".", "/", NULL, MS_MOVE, NULL) < 0)
                 return log_debug_errno(errno, "Failed to move new rootfs '%s': %m", path);
 
+        /* Also change chroot dir */
         if (chroot(".") < 0)
                 return log_debug_errno(errno, "Failed to chroot to new rootfs '%s': %m", path);
 
-        if (chdir("/"))
-                return log_debug_errno(errno, "Failed to chdir to new rootfs '%s': %m", path);
-
         return 0;
 }
 
-int mount_switch_root(const char *path, unsigned long mount_propagation_flag) {
+int mount_switch_root_full(const char *path, unsigned long mount_propagation_flag, bool force_ms_move) {
         _cleanup_close_ int fd_newroot = -EBADF;
         int r;
 
@@ -485,19 +481,20 @@ int mount_switch_root(const char *path, unsigned long mount_propagation_flag) {
         if (fd_newroot < 0)
                 return log_debug_errno(errno, "Failed to open new rootfs '%s': %m", path);
 
-        /* Change into the new rootfs. */
-        if (fchdir(fd_newroot) < 0)
-                return log_debug_errno(errno, "Failed to change into new rootfs '%s': %m", path);
-
-        r = mount_switch_root_pivot(path, fd_newroot);
-        if (r < 0) {
-                /* Failed to pivot_root() fallback to MS_MOVE. For example, this may happen if the
-                 * rootfs is an initramfs in which case pivot_root() isn't supported. */
-                log_debug_errno(r, "Failed to pivot into new rootfs '%s': %m", path);
-                r = mount_switch_root_move(path);
+        if (!force_ms_move) {
+                r = mount_switch_root_pivot(fd_newroot, path);
+                if (r < 0) {
+                        log_debug_errno(r, "Failed to pivot into new rootfs '%s', will try to use MS_MOVE instead: %m", path);
+                        force_ms_move = true;
+                }
+        }
+        if (force_ms_move) {
+                /* Failed to pivot_root() fallback to MS_MOVE. For example, this may happen if the rootfs is
+                 * an initramfs in which case pivot_root() isn't supported. */
+                r = mount_switch_root_move(fd_newroot, path);
+                if (r < 0)
+                        return log_debug_errno(r, "Failed to switch to new rootfs '%s' with MS_MOVE: %m", path);
         }
-        if (r < 0)
-                return log_debug_errno(r, "Failed to switch to new rootfs '%s': %m", path);
 
         /* Finally, let's establish the requested propagation flags. */
         if (mount_propagation_flag == 0)

diff --git a/src/shared/mount-util.h b/src/shared/mount-util.h
index d1defcd8be482fb20d335e9951305f1dfcb4250d..d63fddeb10e20db541d83980307c381c0e1a51bc 100644 (file)
--- a/src/shared/mount-util.h
+++ b/src/shared/mount-util.h
@@ -21,7 +21,10 @@ static inline int bind_remount_recursive(const char *prefix, unsigned long new_f
 
 int bind_remount_one_with_mountinfo(const char *path, unsigned long new_flags, unsigned long flags_mask, FILE *proc_self_mountinfo);
 
-int mount_switch_root(const char *path, unsigned long mount_propagation_flag);
+int mount_switch_root_full(const char *path, unsigned long mount_propagation_flag, bool force_ms_move);
+static inline int mount_switch_root(const char *path, unsigned long mount_propagation_flag) {
+        return mount_switch_root_full(path, mount_propagation_flag, false);
+}
 
 DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(FILE*, endmntent, NULL);
 #define _cleanup_endmntent_ _cleanup_(endmntentp)

diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c
index 405cdf557a821ed362f5c29258b8645b815a6066..a395f0cc6d084072470b1b92f0e926f269aea677 100644 (file)
--- a/src/test/test-mount-util.c
+++ b/src/test/test-mount-util.c
@@ -16,6 +16,7 @@
 #include "namespace-util.h"
 #include "path-util.h"
 #include "process-util.h"
+#include "random-util.h"
 #include "rm-rf.h"
 #include "stat-util.h"
 #include "string-util.h"
@@ -388,6 +389,48 @@ TEST(make_mount_point_inode) {
         assert_se(!(S_IXOTH & st.st_mode));
 }
 
+TEST(make_mount_switch_root) {
+        _cleanup_(rm_rf_physical_and_freep) char *t = NULL;
+        _cleanup_free_ char *s = NULL;
+        int r;
+
+        if (geteuid() != 0 || have_effective_cap(CAP_SYS_ADMIN) <= 0) {
+                (void) log_tests_skipped("not running privileged");
+                return;
+        }
+
+        assert_se(mkdtemp_malloc(NULL, &t) >= 0);
+
+        assert_se(asprintf(&s, "%s/somerandomname%" PRIu64, t, random_u64()) >= 0);
+        assert_se(s);
+        assert_se(touch(s) >= 0);
+
+        for (int force_ms_move = 0; force_ms_move < 2; force_ms_move++) {
+                r = safe_fork("(switch-root",
+                              FORK_RESET_SIGNALS |
+                              FORK_CLOSE_ALL_FDS |
+                              FORK_DEATHSIG |
+                              FORK_WAIT |
+                              FORK_REOPEN_LOG |
+                              FORK_LOG |
+                              FORK_NEW_MOUNTNS |
+                              FORK_MOUNTNS_SLAVE,
+                              NULL);
+                assert_se(r >= 0);
+
+                if (r == 0) {
+                        assert_se(make_mount_point(t) >= 0);
+                        assert_se(mount_switch_root_full(t, /* mount_propagation_flag= */ 0, force_ms_move) >= 0);
+
+                        assert_se(access(ASSERT_PTR(strrchr(s, '/')), F_OK) >= 0);       /* absolute */
+                        assert_se(access(ASSERT_PTR(strrchr(s, '/')) + 1, F_OK) >= 0);   /* relative */
+                        assert_se(access(s, F_OK) < 0 && errno == ENOENT);               /* doesn't exist in our new environment */
+
+                        _exit(EXIT_SUCCESS);
+                }
+        }
+}
+
 static int intro(void) {
          /* Create a dummy network interface for testing remount_sysfs(). */
         (void) system("ip link add dummy-test-mnt type dummy");