Fix -K check.ndk-sysroot-r21

The -K check is not currently very useful, and was incomplete:
|skip_remount| was being set to 1 even in the cases where the remount
was happening with different flags (and therefore not being skipped.)

Fix this by renaming the |skip_remount| variable to |change_remount|
so that it better reflects what we want to test, and adding a clearer
error message (with a suggestion to explicitly add '-v'.)

Also fix some long lines and a comment.

Bug: chromium:1008895
Test: Unit tests pass.
Test: minijail0 -p -K -- /bin/id prints error.
Test: minijail0 -p -Kslave -- /bin/id prints error.
Test: minijail0 -pv -K -- /bin/id runs successfully.
Test: minijail0 -pv -Kslave -- /bin/id runs successfully.
Change-Id: I19b4bf4f495bd5ec40338f2502854d4054be4c5c

3 files changed, 24 insertions, 15 deletions

diff --git a/libminijail.c b/libminijail.c
index 29f6ef48..cd6ffe76 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -1794,14 +1794,17 @@ static void write_ugid_maps_or_die(const struct minijail *j)
 	if (j->uidmap && write_proc_file(j->initpid, j->uidmap, "uid_map") != 0)
 		kill_child_and_die(j, "failed to write uid_map");
 	if (j->gidmap && j->flags.disable_setgroups) {
-		/* Older kernels might not have the /proc/<pid>/setgroups files. */
+		/*
+		 * Older kernels might not have the /proc/<pid>/setgroups files.
+		 */
 		int ret = write_proc_file(j->initpid, "deny", "setgroups");
 		if (ret != 0) {
 			if (ret == -ENOENT) {
 				/* See http://man7.org/linux/man-pages/man7/user_namespaces.7.html. */
 				warn("could not disable setgroups(2)");
 			} else
-				kill_child_and_die(j, "failed to disable setgroups(2)");
+				kill_child_and_die(
+				    j, "failed to disable setgroups(2)");
 		}
 	}
 	if (j->gidmap && write_proc_file(j->initpid, j->gidmap, "gid_map") != 0)
@@ -2161,12 +2164,15 @@ void API minijail_enter(const struct minijail *j)
 			pdie("unshare(CLONE_NEWNS) failed");
 		/*
 		 * By default, remount all filesystems as private, unless
-		 * - Passed a specific remount mode, in which case remount with that,
-		 * - Asked not to remount at all, in which case skip the mount(2) call.
+		 * - Passed a specific remount mode, in which case remount with
+		 *   that,
+		 * - Asked not to remount at all, in which case skip the
+		 *   mount(2) call.
 		 * https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt
 		 */
 		if (j->remount_mode) {
-			if (mount(NULL, "/", NULL, MS_REC | j->remount_mode, NULL))
+			if (mount(NULL, "/", NULL, MS_REC | j->remount_mode,
+				  NULL))
 				pdie("mount(NULL, /, NULL, MS_REC | MS_PRIVATE,"
 				     " NULL) failed");
 		}
diff --git a/minijail0_cli.c b/minijail0_cli.c
index c1d053ee..277c2224 100644
--- a/minijail0_cli.c
+++ b/minijail0_cli.c
@@ -582,7 +582,7 @@ int parse_args(struct minijail *j, int argc, char *const argv[],
 	int forward = 1;
 	int binding = 0;
 	int chroot = 0, pivot_root = 0;
-	int mount_ns = 0, skip_remount = 0;
+	int mount_ns = 0, change_remount = 0;
 	int inherit_suppl_gids = 0, keep_suppl_gids = 0;
 	int caps = 0, ambient_caps = 0;
 	int seccomp = -1;
@@ -681,11 +681,12 @@ int parse_args(struct minijail *j, int argc, char *const argv[],
 			add_mount(j, optarg);
 			break;
 		case 'K':
-			if (optarg)
+			if (optarg) {
 				set_remount_mode(j, optarg);
-			else
+			} else {
 				minijail_skip_remount_private(j);
-			skip_remount = 1;
+			}
+			change_remount = 1;
 			break;
 		case 'P':
 			use_pivot_root(j, optarg, &pivot_root, chroot);
@@ -909,12 +910,14 @@ int parse_args(struct minijail *j, int argc, char *const argv[],
 	}
 
 	/*
-	 * Remounting / as MS_PRIVATE only happens when entering a new mount
-	 * namespace, so skipping it only applies in that case.
+	 * / is only remounted when entering a new mount namespace, so unless
+	 * that's set there is no need for the -K/-K<mode> flags.
 	 */
-	if (skip_remount && !mount_ns) {
-		fprintf(stderr, "Can't skip marking mounts as MS_PRIVATE"
-				" without mount namespaces.\n");
+	if (change_remount && !mount_ns) {
+		fprintf(stderr, "No need to use -K (skip remounting '/') or "
+				"-K<mode> (remount '/' as <mode>)\n"
+				"without -v (new mount namespace).\n"
+				"Do you need to add '-v' explicitly?\n");
 		exit(1);
 	}
 
diff --git a/system_unittest.cc b/system_unittest.cc
index 470421db..4156e6e6 100644
--- a/system_unittest.cc
+++ b/system_unittest.cc
@@ -260,7 +260,7 @@ TEST(setup_mount_destination, dest_exists) {
   EXPECT_EQ(0, setup_mount_destination(nullptr, "/dev", 0, 0, false, nullptr));
 }
 
-// Mount flags should be obtained for bind-mounts
+// Mount flags should be obtained for bind-mounts.
 TEST(setup_mount_destination, mount_flags) {
   struct statvfs stvfs_buf;
   ASSERT_EQ(0, statvfs("/proc", &stvfs_buf));