[1/2] seize: Simplify creds checkers

Submitted by Pavel Emelianov on Sept. 27, 2016, 5:45 p.m.

Details

Message ID 57EAB020.8090004@virtuozzo.com
State Accepted
Series "seize: Prepare seize_wait_task for comepl-ization"
Commit 79077fe4b1594fe7a4fae125a387731081622ee5
Headers show

Commit Message

Pavel Emelianov Sept. 27, 2016, 5:45 p.m.
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            | 14 ++++-----
 criu/include/proc_parse.h |  3 --
 criu/include/ptrace.h     |  2 +-
 criu/proc_parse.c         | 49 -------------------------------
 criu/ptrace.c             | 36 ++++++-----------------
 criu/seize.c              | 75 +++++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 88 insertions(+), 91 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-exec.c b/criu/cr-exec.c
index 6e7a5de..d1424fc 100644
--- a/criu/cr-exec.c
+++ b/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) {
@@ -135,19 +135,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);
diff --git a/criu/include/proc_parse.h b/criu/include/proc_parse.h
index fb0d643..ac7a780 100644
--- a/criu/include/proc_parse.h
+++ b/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);
diff --git a/criu/include/ptrace.h b/criu/include/ptrace.h
index bf416b3..b30d4ba 100644
--- a/criu/include/ptrace.h
+++ b/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);
diff --git a/criu/proc_parse.c b/criu/proc_parse.c
index 49d6c1a..cca63f4 100644
--- a/criu/proc_parse.c
+++ b/criu/proc_parse.c
@@ -2506,55 +2506,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;
diff --git a/criu/ptrace.c b/criu/ptrace.c
index 3d2f56e..a449487 100644
--- a/criu/ptrace.c
+++ b/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++;
diff --git a/criu/seize.c b/criu/seize.c
index e1e805a..0a9a750 100644
--- a/criu/seize.c
+++ b/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)