seize: Simplify creds checkers

Don't do comparisons of creds insize core seizing routine. Instead,
pull the creds on the caller's stack and compare them there if
required.

At the same time more the proc_status_creds_dumpable() to the place
where it's only needed and make it static.
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>

criu/cr-exec.c

@@ -119,7 +119,7 @@ int cr_exec(int pid, char **opt)
 	struct parasite_ctl *ctl;
 	struct vm_area_list vmas;
 	int ret, prev_state, exit_code = -1;
-	struct proc_status_creds *creds = NULL;
+	struct proc_status_creds creds;
 	unsigned long p_start;
 
 	if (!sys_name) {
@@ -141,19 +141,17 @@ int cr_exec(int pid, char **opt)
 	if (seize_catch_task(pid))
 		goto out;
 
+	/*
+	 * We don't seize a task's threads here, so there is no reason to
+	 * mess with creds in this use case anyway.
+	 */
+
 	prev_state = ret = seize_wait_task(pid, -1, &creds);
 	if (ret < 0) {
 		pr_err("Can't seize task %d\n", pid);
 		goto out;
 	}
 
-	/*
-	 * We don't seize a task's threads here, and there is no reason to
-	 * compare threads' creds in this use case anyway, so let's just free
-	 * the creds.
-	 */
-	free(creds);
-
 	ret = collect_mappings(pid, &vmas, NULL);
 	if (ret) {
 		pr_err("Can't collect vmas for %d\n", pid);

criu/include/proc_parse.h

@@ -94,9 +94,6 @@ struct proc_status_creds {
 	u32			cap_bnd[PROC_CAP_SIZE];
 };
 
-bool proc_status_creds_dumpable(struct proc_status_creds *parent,
-				struct proc_status_creds *child);
-
 #define INVALID_UID ((uid_t)-1)
 
 extern int parse_pid_stat(pid_t pid, struct proc_pid_stat *s);

criu/include/ptrace.h

@@ -76,7 +76,7 @@ struct ptrace_peeksiginfo_args {
 extern int processes_to_wait;
 
 extern int seize_catch_task(pid_t pid);
-extern int seize_wait_task(pid_t pid, pid_t ppid, struct proc_status_creds **creds);
+extern int seize_wait_task(pid_t pid, pid_t ppid, struct proc_status_creds *creds);
 extern int suspend_seccomp(pid_t pid);
 extern int unseize_task(pid_t pid, int orig_state, int state);
 extern int ptrace_peek_area(pid_t pid, void *dst, void *addr, long bytes);

criu/proc_parse.c

@@ -2511,55 +2511,6 @@ int aufs_parse(struct mount_info *new)
 	return ret;
 }
 
-bool proc_status_creds_dumpable(struct proc_status_creds *parent,
-				struct proc_status_creds *child)
-{
-	const size_t size = sizeof(struct proc_status_creds) -
-			offsetof(struct proc_status_creds, cap_inh);
-
-	/*
-	 * The comparison rules are the following
-	 *
-	 *  - CAPs can be different
-	 *  - seccomp filters should be passed via
-	 *    semantic comparison (FIXME) but for
-	 *    now we require them to be exactly
-	 *    identical
-	 *  - the rest of members must match
-	 */
-
-	if (memcmp(parent, child, size)) {
-		if (!pr_quelled(LOG_DEBUG)) {
-			pr_debug("Creds undumpable (parent:child)\n"
-				 "  uids:               %d:%d %d:%d %d:%d %d:%d\n"
-				 "  gids:               %d:%d %d:%d %d:%d %d:%d\n"
-				 "  state:              %d:%d"
-				 "  ppid:               %d:%d\n"
-				 "  sigpnd:             %llu:%llu\n"
-				 "  shdpnd:             %llu:%llu\n"
-				 "  seccomp_mode:       %d:%d\n"
-				 "  last_filter:        %u:%u\n",
-				 parent->uids[0], child->uids[0],
-				 parent->uids[1], child->uids[1],
-				 parent->uids[2], child->uids[2],
-				 parent->uids[3], child->uids[3],
-				 parent->gids[0], child->gids[0],
-				 parent->gids[1], child->gids[1],
-				 parent->gids[2], child->gids[2],
-				 parent->gids[3], child->gids[3],
-				 parent->state, child->state,
-				 parent->ppid, child->ppid,
-				 parent->sigpnd, child->sigpnd,
-				 parent->shdpnd, child->shdpnd,
-				 parent->seccomp_mode, child->seccomp_mode,
-				 parent->last_filter, child->last_filter);
-		}
-		return false;
-	}
-
-	return true;
-}
-
 int parse_children(pid_t pid, pid_t **_c, int *_n)
 {
 	pid_t *ch = NULL;

criu/ptrace.c

@@ -148,17 +148,11 @@ static int skip_sigstop(int pid, int nr_signals)
  * of it so the task would not know if it was saddled
  * up with someone else.
  */
-int seize_wait_task(pid_t pid, pid_t ppid, struct proc_status_creds **creds)
+int seize_wait_task(pid_t pid, pid_t ppid, struct proc_status_creds *creds)
 {
 	siginfo_t si;
 	int status, nr_sigstop;
 	int ret = 0, ret2, wait_errno = 0;
-	struct proc_status_creds cr;
-
-	/*
-	 * For the comparison below, let's zero out any padding.
-	 */
-	memzero(&cr, sizeof(struct proc_status_creds));
 
 	/*
 	 * It's ugly, but the ptrace API doesn't allow to distinguish
@@ -184,26 +178,26 @@ try_again:
 		wait_errno = errno;
 	}
 
-	ret2 = parse_pid_status(pid, &cr);
+	ret2 = parse_pid_status(pid, creds);
 	if (ret2)
 		goto err;
 
 	if (ret < 0 || WIFEXITED(status) || WIFSIGNALED(status)) {
-		if (cr.state != 'Z') {
+		if (creds->state != 'Z') {
 			if (pid == getpid())
 				pr_err("The criu itself is within dumped tree.\n");
 			else
 				pr_err("Unseizable non-zombie %d found, state %c, err %d/%d\n",
-						pid, cr.state, ret, wait_errno);
+						pid, creds->state, ret, wait_errno);
 			return -1;
 		}
 
 		return TASK_DEAD;
 	}
 
-	if ((ppid != -1) && (cr.ppid != ppid)) {
+	if ((ppid != -1) && (creds->ppid != ppid)) {
 		pr_err("Task pid reused while suspending (%d: %d -> %d)\n",
-				pid, ppid, cr.ppid);
+				pid, ppid, creds->ppid);
 		goto err;
 	}
 
@@ -235,25 +229,13 @@ try_again:
 		goto try_again;
 	}
 
-	if (*creds == NULL) {
-		*creds = xzalloc(sizeof(struct proc_status_creds));
-		if (!*creds)
-			goto err;
-
-		**creds = cr;
-
-	} else if (!proc_status_creds_dumpable(*creds, &cr)) {
-		pr_err("creds don't match %d %d\n", pid, ppid);
-		goto err;
-	}
-
-	if (cr.seccomp_mode != SECCOMP_MODE_DISABLED && suspend_seccomp(pid) < 0)
+	if (creds->seccomp_mode != SECCOMP_MODE_DISABLED && suspend_seccomp(pid) < 0)
 		goto err;
 
 	nr_sigstop = 0;
-	if (cr.sigpnd & (1 << (SIGSTOP - 1)))
+	if (creds->sigpnd & (1 << (SIGSTOP - 1)))
 		nr_sigstop++;
-	if (cr.shdpnd & (1 << (SIGSTOP - 1)))
+	if (creds->shdpnd & (1 << (SIGSTOP - 1)))
 		nr_sigstop++;
 	if (si.si_signo == SIGSTOP)
 		nr_sigstop++;

criu/seize.c

@@ -437,6 +437,7 @@ static int collect_children(struct pstree_item *item)
 	nr_inprogress = 0;
 	for (i = 0; i < nr_children; i++) {
 		struct pstree_item *c;
+		struct proc_status_creds *creds;
 		pid_t pid = ch[i];
 
 		/* Is it already frozen? */
@@ -462,7 +463,13 @@ static int collect_children(struct pstree_item *item)
 			/* fails when meets a zombie */
 			seize_catch_task(pid);
 
-		ret = seize_wait_task(pid, item->pid.real, &dmpi(c)->pi_creds);
+		creds = xzalloc(sizeof(*creds));
+		if (!creds) {
+			ret = -1;
+			goto free;
+		}
+
+		ret = seize_wait_task(pid, item->pid.real, creds);
 		if (ret < 0) {
 			/*
 			 * Here is a race window between parse_children() and seize(),
@@ -473,9 +480,11 @@ static int collect_children(struct pstree_item *item)
 			 */
 			ret = 0;
 			xfree(c);
+			xfree(creds);
 			continue;
 		}
 
+		dmpi(c)->pi_creds = creds;
 		c->pid.real = pid;
 		c->parent = item;
 		c->pid.state = ret;
@@ -591,6 +600,55 @@ static inline bool thread_collected(struct pstree_item *i, pid_t tid)
 	return false;
 }
 
+static bool creds_dumpable(struct proc_status_creds *parent,
+				struct proc_status_creds *child)
+{
+	const size_t size = sizeof(struct proc_status_creds) -
+			offsetof(struct proc_status_creds, cap_inh);
+
+	/*
+	 * The comparison rules are the following
+	 *
+	 *  - CAPs can be different
+	 *  - seccomp filters should be passed via
+	 *    semantic comparison (FIXME) but for
+	 *    now we require them to be exactly
+	 *    identical
+	 *  - the rest of members must match
+	 */
+
+	if (memcmp(parent, child, size)) {
+		if (!pr_quelled(LOG_DEBUG)) {
+			pr_debug("Creds undumpable (parent:child)\n"
+				 "  uids:               %d:%d %d:%d %d:%d %d:%d\n"
+				 "  gids:               %d:%d %d:%d %d:%d %d:%d\n"
+				 "  state:              %d:%d"
+				 "  ppid:               %d:%d\n"
+				 "  sigpnd:             %llu:%llu\n"
+				 "  shdpnd:             %llu:%llu\n"
+				 "  seccomp_mode:       %d:%d\n"
+				 "  last_filter:        %u:%u\n",
+				 parent->uids[0], child->uids[0],
+				 parent->uids[1], child->uids[1],
+				 parent->uids[2], child->uids[2],
+				 parent->uids[3], child->uids[3],
+				 parent->gids[0], child->gids[0],
+				 parent->gids[1], child->gids[1],
+				 parent->gids[2], child->gids[2],
+				 parent->gids[3], child->gids[3],
+				 parent->state, child->state,
+				 parent->ppid, child->ppid,
+				 parent->sigpnd, child->sigpnd,
+				 parent->shdpnd, child->shdpnd,
+				 parent->seccomp_mode, child->seccomp_mode,
+				 parent->last_filter, child->last_filter);
+		}
+		return false;
+	}
+
+	return true;
+}
+
 static int collect_threads(struct pstree_item *item)
 {
 	struct pid *threads = NULL;
@@ -618,6 +676,7 @@ static int collect_threads(struct pstree_item *item)
 	nr_inprogress = 0;
 	for (i = 0; i < nr_threads; i++) {
 		pid_t pid = threads[i].real;
+		struct proc_status_creds t_creds = {};
 
 		if (thread_collected(item, pid))
 			continue;
@@ -630,7 +689,7 @@ static int collect_threads(struct pstree_item *item)
 		if (!opts.freeze_cgroup && seize_catch_task(pid))
 			continue;
 
-		ret = seize_wait_task(pid, item_ppid(item), &dmpi(item)->pi_creds);
+		ret = seize_wait_task(pid, item_ppid(item), &t_creds);
 		if (ret < 0) {
 			/*
 			 * Here is a race window between parse_threads() and seize(),
@@ -651,6 +710,9 @@ static int collect_threads(struct pstree_item *item)
 			goto err;
 		}
 
+		if (!creds_dumpable(dmpi(item)->pi_creds, &t_creds))
+			goto err;
+
 		if (ret == TASK_STOPPED) {
 			nr_stopped++;
 		}
@@ -737,6 +799,7 @@ int collect_pstree(void)
 {
 	pid_t pid = root_item->pid.real;
 	int ret = -1;
+	struct proc_status_creds *creds;
 
 	timing_start(TIME_FREEZING);
 
@@ -755,11 +818,17 @@ int collect_pstree(void)
 		goto err;
 	}
 
-	ret = seize_wait_task(pid, -1, &dmpi(root_item)->pi_creds);
+	creds = xzalloc(sizeof(*creds));
+	if (!creds)
+		goto err;
+
+	ret = seize_wait_task(pid, -1, creds);
 	if (ret < 0)
 		goto err;
+
 	pr_info("Seized task %d, state %d\n", pid, ret);
 	root_item->pid.state = ret;
+	dmpi(root_item)->pi_creds = creds;
 
 	ret = collect_task(root_item);
 	if (ret < 0)