pidns: fixup

I pushed the wrong branch to the pidns PR checkpoint-restore#1056 which resulted in the wrong patches getting merged. This is the actual result from the review. Signed-off-by: Adrian Reber <[email protected]>
adrianreber · Jul 17, 2020 · bdc3559 · bdc3559
1 parent 958b750
commit bdc3559
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 40 deletions.
diff --git a/criu/cr-restore.c b/criu/cr-restore.c
@@ -1335,20 +1335,21 @@ static bool needs_prep_creds(struct pstree_item *item)
 	return (!item->parent && ((root_ns_mask & CLONE_NEWUSER) || getuid()));
 }
 
-static int write_ns_last_pid(pid_t pid)
+static int set_next_pid(void *arg)
 {
 	char buf[32];
+	pid_t *pid = arg;
 	int len;
 	int fd;
 
 	fd = open_proc_rw(PROC_GEN, LAST_PID_PATH);
 	if (fd < 0)
 		return -1;
 
-	len = snprintf(buf, sizeof(buf), "%d", pid - 1);
+	len = snprintf(buf, sizeof(buf), "%d", *pid - 1);
 	if (write(fd, buf, len) != len) {
-		pr_perror("%d: Write %s to %s", pid, buf,
-			LAST_PID_PATH);
+		pr_perror("Failed to write %s to /proc/%s",
+			buf, LAST_PID_PATH);
 		close(fd);
 		return -1;
 	}
@@ -1416,7 +1417,10 @@ static inline int fork_with_pid(struct pstree_item *item)
 		int fd;
 
 		/* Not possible to restore into an empty PID namespace. */
-		BUG_ON(pid == INIT_PID);
+		if (pid == INIT_PID) {
+			pr_err("Unable to restore into an empty PID namespace\n");
+			return -1;
+		}
 
 		/*
 		 * Restoring into an existing namespace means that CLONE_NEWPID
@@ -1433,6 +1437,10 @@ static inline int fork_with_pid(struct pstree_item *item)
 
 		ret = switch_ns_by_fd(fd, &pid_ns_desc, NULL);
 		close(fd);
+		if (ret) {
+			pr_err("Unable to enter existing PID namespace\n");
+			return -1;
+		}
 
 		/*
 		 * If a process without a PID namespace is restored into
@@ -1444,8 +1452,6 @@ static inline int fork_with_pid(struct pstree_item *item)
 	}
 
 	if (!(clone_flags & CLONE_NEWPID)) {
-		pid_t helper_pid = -1;
-
 		lock_last_pid();
 
 		if (!kdat.has_clone3_set_tid) {
@@ -1456,38 +1462,23 @@ static inline int fork_with_pid(struct pstree_item *item)
 				 * so much easier and simpler. As long as CRIU supports
 				 * clone() this is needed.
 				 */
-				helper_pid = fork();
-				if (helper_pid < 0) {
-					pr_perror("Cannot fork ns_last_pid writer");
-					goto err_unlock;
-				}
-			}
-			if (helper_pid <= 0) {
-				ret = write_ns_last_pid(pid);
-				if (ret == -1 && helper_pid == -1)
-					goto err_unlock;
-				if (helper_pid == 0)
-					exit(ret);
+				ret = call_in_child_process(set_next_pid, (void *)&pid);
+			} else {
+				ret = set_next_pid((void *)&pid);
 			}
-			if (helper_pid > 0) {
-				/* We forked and this is the parent. */
-				int status;
-
-				ret = waitpid(-1, &status, 0);
-				if (ret < 0) {
-					pr_perror("Cannot wait for ns_last_pid writer");
-					goto err_unlock;
-				}
-				if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
-					pr_err("Writing ns_last_pid failed with error %d\n", status);
-					ret = -1;
-					goto err_unlock;
-				}
+			if (ret != 0) {
+				pr_err("Setting PID failed");
+				goto err_unlock;
 			}
 		}
 	} else {
-		if (!(pid_ns && pid_ns->ext_key))
-			BUG_ON(pid != INIT_PID);
+		if (!(pid_ns && pid_ns->ext_key)) {
+			if (pid != INIT_PID) {
+				pr_err("First PID in a PID namespace needs to be %d and not %d\n",
+					pid, INIT_PID);
+				return -1;
+			}
+		}
 	}
 
 	if (kdat.has_clone3_set_tid) {
@@ -2227,6 +2218,9 @@ static int restore_root_task(struct pstree_item *init)
 	 * this later.
 	 */
 
+	if (prepare_userns_hook())
+		return -1;
+
 	if (prepare_namespace_before_tasks())
 		return -1;
 
@@ -2243,9 +2237,6 @@ static int restore_root_task(struct pstree_item *init)
 		return -1;
 	}
 
-	if (prepare_userns_hook())
-		return -1;
-
 	__restore_switch_stage_nw(CR_STATE_ROOT_TASK);
 
 	ret = fork_with_pid(init);

diff --git a/criu/include/magic.h b/criu/include/magic.h
@@ -96,7 +96,7 @@
 #define FILES_MAGIC		0x56303138 /* Toropets */
 #define MEMFD_INODE_MAGIC	0x48453499 /* Dnipro */
 #define TIMENS_MAGIC		0x43114433 /* Beslan */
-#define PIDNS_MAGIC		0x12345678
+#define PIDNS_MAGIC		0x61157326 /* Surgut */
 
 #define IFADDR_MAGIC		RAW_IMAGE_MAGIC
 #define ROUTE_MAGIC		RAW_IMAGE_MAGIC

diff --git a/test/others/ns_ext/run.sh b/test/others/ns_ext/run.sh
@@ -26,20 +26,24 @@ CRIU=../../../criu/criu
 if [[ "$NS" == "net" ]]; then
 	setsid unshare -n bash -c 'unshare -n sh _run.sh pidfile2 & unshare -n sh _run.sh pidfile3 & ip link add xxx type veth && ip link add mymacvlan1 link xxx type macvlan mode bridge && . _run.sh pidfile' < /dev/zero &> output &
 elif [[ "$NS" == "pid" ]]; then
+	# Adding some random values to the command-line to easily grep the correct process later
 	RND1=$RANDOM
 	RND2=$RANDOM
-	setsid unshare -p -f setsid bash -c "setsid sh _run.sh pidfile2 $RND2 & . _run.sh pidfile $RND1" < /dev/zero &> output &
+	unshare -p -f bash -c "setsid sh _run.sh pidfile2 $RND2 & . _run.sh pidfile $RND1" < /dev/zero &> output &
 fi
 sleep 1
 while :; do
 	test -f pidfile && test -f pidfile2 && break;
 	sleep 0.1
 done
 
+# Figure out the PIDs of the relevant processes
 if [[ "$NS" == "net" ]]; then
 	pid=$(cat pidfile)
 	pid2=$(cat pidfile2)
 elif [[ "$NS" == "pid" ]]; then
+	# Unfortunately we cannot read out 'pidfile' as it contains the PID
+	# from within the PID namespace. We need to know the outside PID.
 	pid2=$(pgrep -f ". _run.sh pidfile $RND1" -n)
 	pid=$(pgrep -f "sh _run.sh pidfile2 $RND2" -n)
 fi
@@ -54,19 +58,35 @@ ino2=$(ls -iL $MNT2 | awk '{ print $1 }')
 exec 33< $MNT1
 exec 34< $MNT2
 $CRIU dump -v4 -t $pid -o dump.log -D images --external $NS[$ino]:test_ns --external $NS[$ino2]:test_ns2
+RESULT=$?
 cat images/dump.log | grep -B 5 Error || echo ok
+[ "$RESULT" != "0" ] && {
+	echo "CRIU dump failed"
+	echo FAIL
+	exit 1
+}
+
 $CRIU restore -v4 -o restore.log -D images --inherit-fd fd[33]:test_ns --inherit-fd fd[34]:test_ns2 -d
+RESULT=$?
 cat images/restore.log | grep -B 5 Error || echo ok
+[ "$RESULT" != "0" ] && {
+	echo "CRIU restore failed"
+	echo FAIL
+	exit 1
+}
+
 if [[ "$NS" == "pid" ]]; then
 	pid=$(pgrep -f "^sh _run.sh pidfile2 $RND2")
 fi
 new_ino=$(ls -iL /proc/$pid/ns/$NS | awk '{ print $1 }')
 new_ino2=$(ls -iL /proc/$pid2/ns/$NS | awk '{ print $1 }')
 [ "$ino" != "$new_ino" ] && {
+	echo "Inode of new NS is different"
 	echo FAIL
 	exit 1
 }
 [ "$ino2" != "$new_ino2" ] && {
+	echo "Inode of new NS is different"
 	echo FAIL
 	exit 1
 }