Re: switch_root: zap the last directory for the mount point of new-root

[Kang-Che Sung]

On Friday, July 19, 2019, Kang-Che Sung  wrote:
>
> There's side benefit for this patch: In case that overmount fails, we can
have
> a rootfs kept intact (instead of almost destroyed).
>

Correction. It's just a side effect, not a "benefit" worth talking about.

Speaking of, since we are now overmounting the root before zapping the
initramfs, I wonder if we can remove one check about whether the new root
is a mount point (this saves code size; mount() would fail with EINVAL in
that case).
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: switch_root: zap the last directory for the mount point of new-root

[Kang-Che Sung]

On Fri, Jul 19, 2019 at 3:27 PM 阿保 純一  wrote:
>
> As the author said in the comment of util-linux/switch_root.c, current 
> implementation leaves the mount point of new root-file-system without rmdir().
> As long as I experimented on a linux kernel, current process of "/" still 
> points old root-file-system even "/" is overmounted. So we can still access 
> and zap ititramfs after the directory is free from mount point.
>
> The patch below should zap the last directory left in the initramfs.
> It only swaps the timings of overmount and zapping.
>
> diff -Naur busybox-1.31.0.org/util-linux/switch_root.c 
> busybox-1.31.0/util-linux/switch_root.c
> --- busybox-1.31.0.org/util-linux/switch_root.c 2019-07-18 23:18:54.791346155 
> +0900
> +++ busybox-1.31.0/util-linux/switch_root.c 2019-07-18 23:21:33.867785730 
> +0900
> @@ -257,14 +257,14 @@
> }
>
> if (!dry_run) {
> -   // Zap everything out of rootdev
> -   delete_contents("/", rootdev);
> -
> // Overmount / with newdir and chroot into it
> if (mount(".", "/", NULL, MS_MOVE, NULL)) {
> // For example, fails when newroot is not a mountpoint
> bb_perror_msg_and_die("error moving root");
> }
> +
> +   // Zap everything out of rootdev
> +   delete_contents("/", rootdev);
> }
> xchroot(".");
> // The chdir is needed to recalculate "." and ".." links

There's side benefit for this patch: In case that overmount fails, we can have
a rootfs kept intact (instead of almost destroyed).

I think you should adjust the comment line:
// Zap everything out of (old) rootdev, where "/" still points to before chroot
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

switch_root: zap the last directory for the mount point of new-root

[阿保 純一]

As the author said in the comment of util-linux/switch_root.c, current 
implementation leaves the mount point of new root-file-system without rmdir().
As long as I experimented on a linux kernel, current process of "/" still 
points old root-file-system even "/" is overmounted. So we can still access and 
zap ititramfs after the directory is free from mount point.

The patch below should zap the last directory left in the initramfs.
It only swaps the timings of overmount and zapping.

diff -Naur busybox-1.31.0.org/util-linux/switch_root.c 
busybox-1.31.0/util-linux/switch_root.c
--- busybox-1.31.0.org/util-linux/switch_root.c 2019-07-18 23:18:54.791346155 
+0900
+++ busybox-1.31.0/util-linux/switch_root.c 2019-07-18 23:21:33.867785730 
+0900
@@ -257,14 +257,14 @@
}
 
if (!dry_run) {
-   // Zap everything out of rootdev
-   delete_contents("/", rootdev);
-
// Overmount / with newdir and chroot into it
if (mount(".", "/", NULL, MS_MOVE, NULL)) {
// For example, fails when newroot is not a mountpoint
bb_perror_msg_and_die("error moving root");
}
+
+   // Zap everything out of rootdev
+   delete_contents("/", rootdev);
}
xchroot(".");
// The chdir is needed to recalculate "." and ".." links
diff -Naur busybox-1.31.0.org/util-linux/switch_root.c busybox-1.31.0/util-linux/switch_root.c
--- busybox-1.31.0.org/util-linux/switch_root.c	2019-07-18 23:18:54.791346155 +0900
+++ busybox-1.31.0/util-linux/switch_root.c	2019-07-18 23:21:33.867785730 +0900
@@ -257,14 +257,14 @@
 	}
 
 	if (!dry_run) {
-		// Zap everything out of rootdev
-		delete_contents("/", rootdev);
-
 		// Overmount / with newdir and chroot into it
 		if (mount(".", "/", NULL, MS_MOVE, NULL)) {
 			// For example, fails when newroot is not a mountpoint
 			bb_perror_msg_and_die("error moving root");
 		}
+
+		// Zap everything out of rootdev
+		delete_contents("/", rootdev);
 	}
 	xchroot(".");
 	// The chdir is needed to recalculate "." and ".." links
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox