criu: restore nested PID namespace hierarchies correctly

criu/criu/cr-restore.c

@@ -581,7 +581,29 @@ static int populate_pid_proc(void)
 	return 0;
 }
 
-static int __collect_child_pids(struct pstree_item *p, int state, unsigned int *n)
+/*
+ * The pid of `pi` as seen by `p`.  ns[0] is innermost, so stepping in
+ * (child depth - p's depth) entries gives p's view.  localpid() only
+ * matches when both live in the same namespace: a nested pidns init is 1
+ * to itself and something else entirely to its parent.
+ */
+static pid_t pid_in_parent_ns(struct pstree_item *p, struct pstree_item *pi)
+{
+	int idx = pi->pid->ns_level - p->pid->ns_level;
+
+	if (idx <= 0 || idx >= pi->pid->ns_level)
+		return localpid(pi);
+
+	return pi->pid->ns[idx].ns_pid;
+}
+
+/*
+ * `observer` is whoever waits on these children, which is not always p:
+ * children of helpers and zombies are reparented to init.  Pids are
+ * recorded in the observer's namespace.
+ */
+static int __collect_child_pids(struct pstree_item *p, struct pstree_item *observer, int state,
+				unsigned int *n)
 {
 	struct pstree_item *pi;
 
@@ -596,7 +618,15 @@ static int __collect_child_pids(struct pstree_item *p, int state, unsigned int *
 			return -1;
 
 		(*n)++;
-		*child = localpid(pi);
+		/*
+		 * Zombies only.  A helper's pid chain is synthesised by
+		 * get_or_create_helper_item(), so translating it through ns[]
+		 * would yield a pid that was never real.
+		 */
+		if (state == TASK_DEAD)
+			*child = pid_in_parent_ns(observer, pi);
+		else
+			*child = localpid(pi);
 	}
 
 	return 0;
@@ -617,12 +647,12 @@ static int collect_child_pids(int state, unsigned int *n)
 		for_each_pstree_item(pi) {
 			if (pi->pid->state != TASK_HELPER && pi->pid->state != TASK_DEAD)
 				continue;
-			if (__collect_child_pids(pi, state, n))
+			if (__collect_child_pids(pi, current, state, n))
 				return -1;
 		}
 	}
 
-	return __collect_child_pids(current, state, n);
+	return __collect_child_pids(current, current, state, n);
 }
 
 static int collect_helper_pids(struct task_restore_args *ta)
@@ -1546,11 +1576,18 @@ static inline int fork_with_pid_mode(struct pstree_item *item, bool parallel_sib
 
 	ca.item = item;
 	ca.clone_flags = rsti(item)->clone_flags;
-	if (opts.tfork.active && item != root_item &&
+	/*
+	 * Innermost pid 1 below the restore root means a nested pidns init,
+	 * which needs CLONE_NEWPID.  A zombie has no ids image, so
+	 * get_clone_mask() cannot derive the flag and clone3() would apply
+	 * the set_tid chain against the parent's namespace and hit EEXIST.
+	 * Not tfork-specific: plain dump/restore breaks the same way.
+	 */
+	if (item != root_item &&
 	    !(ca.clone_flags & CLONE_NEWPID) &&
 	    item->pid->ns_level > 1 &&
 	    item->pid->ns[0].ns_pid == INIT_PID) {
-		pr_info("tfork: repairing missing CLONE_NEWPID for pidns init uid=%d local=%d parent_local=%d level=%d\n",
+		pr_info("repairing missing CLONE_NEWPID for pidns init uid=%d local=%d parent_local=%d level=%d\n",
 			uid(item), pid,
 			item->parent ? localpid(item->parent) : -1,
 			item->pid->ns_level);
@@ -1606,45 +1643,13 @@ static inline int fork_with_pid_mode(struct pstree_item *item, bool parallel_sib
 		syscall_clone_flags |= CLONE_PARENT;
 
 	if (kdat.has_clone3_set_tid) {
-		if (opts.tfork.active && (syscall_clone_flags & CLONE_NEWPID)) {
-			pr_info("tfork: restore pidns init uid=%d local pid %d with fresh parent pid, dumped chain level=%d\n",
-				uid(item), pid, item->pid->ns_level);
+		if (item->pid->ns_level == 1)
 			ret = clone3_with_pid_noasan(restore_task_with_children, &ca,
 						     syscall_clone_flags & ~strip, SIGCHLD, pid);
-		} else if (item->pid->ns_level == 1)
-			ret = clone3_with_pid_noasan(restore_task_with_children, &ca,
-						     syscall_clone_flags & ~strip, SIGCHLD, pid);
-		else {
-			struct pid tfork_pid = {};
-			struct pid *restore_pid = item->pid;
-
-			if (opts.tfork.active && (root_ns_mask & CLONE_NEWPID) &&
-			    root_item && root_item->pid->ns_level > 1 &&
-			    item->pid->ns_level > 1) {
-				/*
-				 * Copy only scalar pid identity. struct pid also
-				 * embeds rb_node links owned by the dumped pid trees;
-				 * copying those nodes into a temporary stack object
-				 * corrupts the tree metadata if it ever gets reused.
-				 */
-				tfork_pid.item = item->pid->item;
-				tfork_pid.real = item->pid->real;
-				tfork_pid.local = item->pid->local;
-				tfork_pid.uid = item->pid->uid;
-				tfork_pid.state = item->pid->state;
-				tfork_pid.stop_signo = item->pid->stop_signo;
-				tfork_pid.ns_level = item->pid->ns_level - 1;
-				tfork_pid.leaf_ns_id = item->pid->leaf_ns_id;
-				memcpy(tfork_pid.ns, item->pid->ns, sizeof(tfork_pid.ns));
-				restore_pid = &tfork_pid;
-				pr_info("tfork: restore pid uid=%d local=%d with rebased pid chain level %d -> %d\n",
-					uid(item), pid, item->pid->ns_level,
-					restore_pid->ns_level);
-			}
+		else
 			ret = clone3_with_nested_pid_noasan(restore_task_with_children, &ca,
 							    syscall_clone_flags & ~strip,
-							    SIGCHLD, restore_pid);
-		}
+							    SIGCHLD, item->pid);
 	} else {
 		BUG_ON(item->pid->ns_level >= 1);
 		close_pid_proc();
@@ -1671,7 +1676,8 @@ static inline int fork_with_pid_mode(struct pstree_item *item, bool parallel_sib
 		goto err_unlock;
 	}
 
-	if (opts.tfork.active || item == root_item) {
+	if (item == root_item) {
+		/* Non-root tasks publish their real PID from __restore_task_with_children(). */
 		item->pid->real = ret;
 		pr_debug("PID: real %d virt %d\n", item->pid->real, localpid(item));
 	}

criu/criu/cr-tfork.c

@@ -1140,7 +1140,7 @@ int cr_tfork_tasks(pid_t pid)
 			list_for_each_entry(cgo_iter, &opts.new_cgroup_roots, node)
 				rpc_n_cg_root++;
 
-			rpc_max = 33 + 2 * (rpc_n_ifd + rpc_n_ext + rpc_n_cg_root
+			rpc_max = 32 + 2 * (rpc_n_ifd + rpc_n_ext + rpc_n_cg_root
 					    + opts.tfork.snap_mount_n)
 				+ rpc_n_copy_args + 2;
 			rpc_argv = calloc(rpc_max, sizeof(*rpc_argv));
@@ -1160,7 +1160,6 @@ int cr_tfork_tasks(pid_t pid)
 			rpc_argv[rpc_n++] = "-o";
 			rpc_argv[rpc_n++] = restore_log_arg;
 			rpc_argv[rpc_n++] = "-v2";
-			rpc_argv[rpc_n++] = "--keep-pid-hierarchy";
 
 			if (opts.root) {
 				rpc_argv[rpc_n++] = "--root";
@@ -1330,7 +1329,7 @@ int cr_tfork_tasks(pid_t pid)
 		buf[off] = '\0';
 		end = buf + off;
 
-		argv_max = 8 + 3;
+		argv_max = 8 + 2;
 		for (p = buf; p < end; p++)
 			if (*p == '\0')
 				argv_max++;
@@ -1375,11 +1374,10 @@ int cr_tfork_tasks(pid_t pid)
 			argv_new[argc_new++] = "--pidfile";
 			argv_new[argc_new++] = pidfile_arg;
 		}
-		if ((size_t)argc_new + 1 >= argv_max) {
+		if ((size_t)argc_new >= argv_max) {
 			pr_err("tfork restore argv overflow: used=%d max=%zu\n", argc_new, argv_max);
 			exit(1);
 		}
-		argv_new[argc_new++] = "--keep-pid-hierarchy";
 		argv_new[argc_new] = NULL;
 
 		execv("/proc/self/exe", argv_new);

criu/criu/pie/restorer.c

@@ -2485,18 +2485,6 @@ __visible long __export_restore_task(struct task_restore_args *args)
 				c_args.set_tid = ptr_to_u64(thread_args[i].tid_in_ns);
 				c_args.flags = clone_flags;
 				c_args.set_tid_size = thread_args[i].ns_level;
-				if (args->tfork_active && thread_args[i].ns_level > 0) {
-					/*
-					 * Preserve the TID visible in the clone's innermost PID namespace.
-					 * Outer namespace TIDs are allocated by the kernel so concurrent
-					 * copy helpers cannot collide with each other on the host.
-					 */
-					pr_debug("tfork: restore thread pid=%d with innermost tid=%d, set_tid_size %d -> 1\n",
-						thread_args[i].pid,
-						thread_args[i].tid_in_ns[0],
-						thread_args[i].ns_level);
-					c_args.set_tid_size = 1;
-				}
 				/* The kernel does stack + stack_size. */
 				c_args.stack = new_sp - RESTORE_STACK_SIZE;
 				c_args.stack_size = RESTORE_STACK_SIZE;

criu/test/others/tfork-ncopy-nested-pidns-threads.c

@@ -0,0 +1,154 @@
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#define READY_PATH "/tmp/tfork-ncopy-nested-pidns-ready"
+
+static void set_name(const char *name)
+{
+	if (prctl(PR_SET_NAME, name, 0, 0, 0)) {
+		perror("prctl(PR_SET_NAME)");
+		exit(1);
+	}
+}
+
+static void unblock_signals(void)
+{
+	sigset_t mask;
+	int ret;
+
+	sigemptyset(&mask);
+	ret = pthread_sigmask(SIG_SETMASK, &mask, NULL);
+	if (ret) {
+		errno = ret;
+		perror("pthread_sigmask");
+		exit(1);
+	}
+}
+
+static void park(void)
+{
+	for (;;)
+		pause();
+}
+
+static void *thread_main(void *arg)
+{
+	(void)arg;
+	set_name("tfork-thread");
+	park();
+	return NULL;
+}
+
+static void spawn_siblings(int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		pid_t pid = fork();
+
+		if (pid < 0) {
+			perror("fork sibling");
+			exit(1);
+		}
+		if (pid == 0) {
+			set_name("tfork-sibling");
+			park();
+			_exit(0);
+		}
+	}
+}
+
+static void nested_pidns_helper(int thread_count)
+{
+	pid_t nested_init;
+	int status;
+
+	set_name("tfork-helper");
+	if (unshare(CLONE_NEWPID)) {
+		perror("unshare(CLONE_NEWPID)");
+		exit(1);
+	}
+
+	nested_init = fork();
+	if (nested_init < 0) {
+		perror("fork nested init");
+		exit(1);
+	}
+	if (nested_init == 0) {
+		pthread_t *threads;
+		int fd, i;
+
+		set_name("tfork-ns-init");
+		threads = calloc(thread_count, sizeof(*threads));
+		if (!threads) {
+			perror("calloc threads");
+			_exit(1);
+		}
+		for (i = 0; i < thread_count; i++) {
+			int ret = pthread_create(&threads[i], NULL, thread_main, NULL);
+
+			if (ret) {
+				errno = ret;
+				perror("pthread_create");
+				_exit(1);
+			}
+		}
+
+		fd = open(READY_PATH, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+		if (fd < 0) {
+			perror("open ready file");
+			_exit(1);
+		}
+		dprintf(fd, "pid=%d threads=%d\n", getpid(), thread_count + 1);
+		close(fd);
+		park();
+		_exit(0);
+	}
+
+	if (waitpid(nested_init, &status, 0) != nested_init)
+		perror("waitpid nested init");
+	exit(1);
+}
+
+int main(int argc, char **argv)
+{
+	int siblings, threads;
+	pid_t helper;
+
+	if (argc != 3) {
+		fprintf(stderr, "usage: %s SIBLINGS THREADS\n", argv[0]);
+		return 2;
+	}
+	siblings = atoi(argv[1]);
+	threads = atoi(argv[2]);
+	if (siblings < 1 || threads < 1)
+		return 2;
+
+	/* CRIU's parasite bootstrap must be able to deliver its SIGTRAP. */
+	unblock_signals();
+	set_name("tfork-test-root");
+	spawn_siblings(siblings);
+	helper = fork();
+	if (helper < 0) {
+		perror("fork helper");
+		return 1;
+	}
+	if (helper == 0)
+		nested_pidns_helper(threads);
+
+	while (access(READY_PATH, F_OK))
+		usleep(10000);
+	park();
+	return 0;
+}