[PATCH 1/3] restart: coordinator in new pidns to always report status via pipe

[PATCH 1/3] restart: coordinator in new pidns to always report status via pipe

[Oren Laadan]

Serge Hallyn reports:
  "another question: if i run 'restart < out' and sys_restart returns
  due to a -EPERM on some object, then restart.c returns 1.  but if i
  'restart --pids', then it reports the error and returns 0.  unless i
  add --copy-status to the flags.  that seems inconsistent?"

It was with a subtree checkpoint in a child pidns, root-task is not
pid 1, So, the restarts calls ckpt_coordinator_pidns() execution.

In commit 2000bbb4b9... "restart: fix race in ckpt_coordinator_pidns
and --no-wait" adds a pipe for a coordinator in a new pids to report
success/failure of the restart operation back to the parent when the
parent does not wish to wait.

IOW, the coordinator's exit value is overloaded - used once to report
success/failure and once (optionally) to report root-tasks exit status.

This patch fixes this by extending the previous commit to make the
coordinator-pidns always report the restart status via the pipe, and
only use the exit status for --wait --copy-status case.

Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 restart.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/restart.c b/restart.c
index 35c54ea..5871bbf 100644
--- a/restart.c
+++ b/restart.c
@@ -942,10 +942,12 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 	ckpt_dbg("forking coordinator in new pidns\n");
 
 	/*
-	 * We won't wait for (collect) the coordinator, so we use a
-	 * pipe instead for the coordinator to report success/failure.
+	 * The coordinator report restart susccess/failure via pipe.
+	 * (It cannot use return value, because the in the default
+	 * --wait --copy-status case it is already used to report the
+	 * root-task's return value).
 	 */
-	if (!ctx->args->wait && pipe(ctx->pipe_coord)) {
+	if (pipe(ctx->pipe_coord) < 0) {
 		perror("pipe");
 		return -1;
 	}
@@ -981,10 +983,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 		return -1;
 
 	ctx->args->copy_status = copy;
-	if (ctx->args->wait)
-		return ckpt_collect_child(ctx);
-	else
-		return ckpt_coordinator_status(ctx);
+	return ckpt_coordinator_status(ctx);
 }
 #else
 static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
@@ -1040,13 +1039,13 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
 		 * around and be reaper until all tasks are gone.
 		 * Otherwise, container will die as soon as we exit.
 		 */
-		if (!ctx->args->wait) {
-			/* report status because parent won't wait for us */
-			if (write(ctx->pipe_coord[1], &ret, sizeof(ret)) < 0) {
-				perror("failed to report status");
-				exit(1);
-			}
+
+		/* Report success/failure to the parent */
+		if (write(ctx->pipe_coord[1], &ret, sizeof(ret)) < 0) {
+			perror("failed to report status");
+			exit(1);
 		}
+
 		ret = ckpt_pretend_reaper(ctx);
 	} else if (ctx->args->wait) {
 		ret = ckpt_collect_child(ctx);
-- 
1.6.0.4

[PATCH 2/3] restart: add --warn-* and --fail-* command line switches

[Oren Laadan]

Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 restart.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/restart.c b/restart.c
index 5871bbf..2efe879 100644
--- a/restart.c
+++ b/restart.c
@@ -79,6 +79,10 @@ static char usage_str[] =
 "     --inspect          inspect image on-the-fly for error records\n"
 "  -v,--verbose          verbose output\n"
 "  -d,--debug            debugging output\n"
+"     --warn-COND        warn on condition COND, but proceed anyways\n"
+"     --fail-COND        warn on condition COND, and abort operation\n"
+"  	  COND=any:        any condition\n"
+"  	  COND=pidzero:    task with sid/pgid zero in a --no-pidns restart\n"
 "";
 
 /*
@@ -359,8 +363,13 @@ struct args {
 	int infd;
 	char *logfile;
 	int logfd;
+	long warn;
+	long fail;
 };
 
+#define CKPT_COND_PIDZERO  0x1
+#define CKPT_COND_ANY      ULONG_MAX
+
 static void usage(char *str)
 {
 	fprintf(stderr, "%s", str);
@@ -379,6 +388,37 @@ static int str2num(char *str)
 	return num;
 }
 
+static long cond_to_mask(const char *cond)
+{
+	static struct {
+		char *cond;
+		long mask;
+	} conditions[] = {
+		{"pidzero", CKPT_COND_PIDZERO},
+		{"any", CKPT_COND_ANY},
+		{NULL, 0}
+	};
+
+	int i;
+
+	for (i = 0; conditions[i].cond; i++)
+		if (!strcmp(cond, conditions[i].cond))
+			return conditions[i].mask;
+
+	printf("restart: invalid warn/fail condition '%s'\n", cond);
+	exit(1);
+}
+
+static inline int ckpt_cond_warn(struct ckpt_ctx *ctx, long mask)
+{
+	return (ctx->args->warn & mask);
+}
+		
+static inline int ckpt_cond_fail(struct ckpt_ctx *ctx, long mask)
+{
+	return (ctx->args->fail & mask);
+}
+
 static void parse_args(struct args *args, int argc, char *argv[])
 {
 	static struct option opts[] = {
@@ -401,10 +441,13 @@ static void parse_args(struct args *args, int argc, char *argv[])
 		{ "freezer",	required_argument,	NULL, 'F' },
 		{ "verbose",	no_argument,		NULL, 'v' },
 		{ "debug",	no_argument,		NULL, 'd' },
+		{ "warn-pidzero",	no_argument,	NULL, 9 },
+		{ "fail-pidzero",	no_argument,	NULL, 10 },
 		{ NULL,		0,			NULL, 0 }
 	};
 	static char optc[] = "hdvpPwWF:r:i:l:";
 
+	int optind;
 	int sig;
 
 	/* defaults */
@@ -414,7 +457,7 @@ static void parse_args(struct args *args, int argc, char *argv[])
 	args->logfd = -1;
 
 	while (1) {
-		int c = getopt_long(argc, argv, optc, opts, NULL);
+		int c = getopt_long(argc, argv, optc, opts, &optind);
 		if (c == -1)
 			break;
 		switch (c) {
@@ -494,6 +537,12 @@ static void parse_args(struct args *args, int argc, char *argv[])
 		case 'F':
 			args->freezer = optarg;
 			break;
+		case 9:
+			args->warn |= cond_to_mask(&opts[optind].name[5]);
+			break;
+		case 10:
+			args->fail |= cond_to_mask(&opts[optind].name[5]);
+			break;
 		default:
 			usage(usage_str);
 		}
@@ -1171,15 +1220,21 @@ static int ckpt_setup_task(struct ckpt_ctx *ctx, pid_t pid, pid_t ppid)
 	return 0;
 }
 
-static inline int ckpt_valid_pid(pid_t pid, int pidns, char *which, int i)
+static int ckpt_valid_pid(struct ckpt_ctx *ctx, pid_t pid, char *which, int i)
 {
 	if (pid < 0) {
 		ckpt_err("Invalid %s %d (for task#%d)\n", which, pid, i);
 		return 0;
 	}
-	if (!pidns && pid == 0) {
-		ckpt_err("Zero %s (task#%d) requires pid-ns\n", which, i + 1);
-		return 0;
+	if (!ctx->args->pidns && pid == 0) {
+		if (ckpt_cond_fail(ctx, CKPT_COND_PIDZERO)) {
+			ckpt_err("[err] task # %d with %s zero"
+				 " (requires --pidns)\n", i + 1, which);
+			return 0;
+		} else if (ckpt_cond_warn(ctx, CKPT_COND_PIDZERO)) {
+			ckpt_err("[warn] task # %d with %s zero"
+				 " (consider --pidns)\n", i + 1, which);
+		}
 	}
 	return 1;
 }
@@ -1188,7 +1243,6 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 {
 	struct ckpt_pids *pids_arr = ctx->pids_arr;
 	int pids_nr = ctx->pids_nr;
-	int pidns = ctx->args->pidns;
 	struct task *task;
 	pid_t root_pid;
 	pid_t root_sid;
@@ -1225,13 +1279,13 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 
 		task->flags = 0;
 
-		if (!ckpt_valid_pid(pids_arr[i].vpid, pidns, "pid", i))
+		if (!ckpt_valid_pid(ctx, pids_arr[i].vpid, "pid", i))
 			return -1;
-		else if (!ckpt_valid_pid(pids_arr[i].vtgid, pidns, "tgid", i))
+		else if (!ckpt_valid_pid(ctx, pids_arr[i].vtgid, "tgid", i))
 			return -1;
-		else if (!ckpt_valid_pid(pids_arr[i].vsid, pidns, "sid", i))
+		else if (!ckpt_valid_pid(ctx, pids_arr[i].vsid, "sid", i))
 			return -1;
-		else if (!ckpt_valid_pid(pids_arr[i].vpgid, pidns, "pgid", i))
+		else if (!ckpt_valid_pid(ctx, pids_arr[i].vpgid, "pgid", i))
 			return -1;
 
 		/* zero references to root_sid (root_sid != root_pid) */
-- 
1.6.0.4

[PATCH 3/3] restart: allow subtree restart of image with sid/pgid zero

[Oren Laadan]

In a checkpoint of a child namespace, it is possible that sid or pgid
will be 0 (meaning, they are from a process outside that namespace).

Restarting from such an image with --pidns is easy, and works well.
However, restarting with --no-pids didn't work because we can't deal
with zero pids.

This patch fixes the problem for subtree restart (--no-pidns). If it
finds sid/pgid that are zero, it creates a special ghost task, and
substitutes its pid for all those 0's.

(Also get rid of un-suqashing of sid/pgid == 0 in ckpt_adjust_pids()
because there should no longer be any 0's there).

Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 restart.c |  127 +++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 77 insertions(+), 50 deletions(-)

diff --git a/restart.c b/restart.c
index 2efe879..0c438c2 100644
--- a/restart.c
+++ b/restart.c
@@ -250,9 +250,6 @@ struct task zero_task;
 #define TASK_NEWPID	0x20	/* starts a new pid namespace */
 #define TASK_DEAD	0x40	/* dead task (dummy) */
 
-#define TASK_ZERO_SID	0x100	/* sid was temporarily zeroed */
-#define TASK_ZERO_PGID	0x200	/* pgid was temporarily zeroed */
-
 struct ckpt_ctx {
 	pid_t root_pid;
 	int pipe_in;
@@ -1239,6 +1236,41 @@ static int ckpt_valid_pid(struct ckpt_ctx *ctx, pid_t pid, char *which, int i)
 	return 1;
 }
 
+static int ckpt_alloc_pid(struct ckpt_ctx *ctx)
+{
+	int n = 0;
+
+	/*
+	 * allocate an unused pid for the placeholder
+	 * (this will become inefficient if pid-space is exhausted)
+	 */
+	do {
+		if (ctx->tasks_pid == INT_MAX)
+			ctx->tasks_pid = 2;
+		else
+			ctx->tasks_pid++;
+
+		if (n++ == INT_MAX) {	/* ohhh... */
+			ckpt_err("pid namsepace exhausted");
+			return -1;
+		}
+	} while (hash_lookup(ctx, ctx->tasks_pid));
+
+	return ctx->tasks_pid;
+}
+
+static int ckpt_zero_pid(struct ckpt_ctx *ctx)
+{
+	pid_t pid;
+
+	pid = ckpt_alloc_pid(ctx);
+	if (pid < 0)
+		return -1;
+	if (ckpt_setup_task(ctx, pid, ctx->pids_arr[0].vpid) < 0)
+		return -1;
+	return pid;
+}
+
 static int ckpt_init_tree(struct ckpt_ctx *ctx)
 {
 	struct ckpt_pids *pids_arr = ctx->pids_arr;
@@ -1246,6 +1278,7 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 	struct task *task;
 	pid_t root_pid;
 	pid_t root_sid;
+	pid_t zero_pid = 0;
 	int i;
 
 	root_pid = pids_arr[0].vpid;
@@ -1257,16 +1290,20 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 	 * same as root_pid or 0), and root_sid was inherited from an
 	 * ancestor of that subtree.
 	 *
-	 * So we make the root-task also inherit sid from its ancestor
-	 * (== coordinator), whatever 'restart' task currently has.
-	 * For that, we force the root-task's sid and all references
-	 * to it from other tasks (via sid and pgid), to 0. Later, the
-	 * feeder will substitute the cooridnator's sid for them.
+	 * If we restart with --pidns, make the root-task also inherit
+	 * sid from its ancestor (== coordinator), whatever 'restart'
+	 * task currently has.  For that, we force the root-task's sid
+	 * and all references to it from other tasks (via sid and
+	 * pgid), to 0. Later, the feeder will substitute the
+	 * cooridnator's sid for them.
 	 *
 	 * (Note that this still works even if the coordinator's sid
 	 * is "used" by a restarting task: a new-pidns restart will
 	 * fail because the pid is in use, and in an old-pidns restart
 	 * the task will be assigned a new pid anyway).
+	 *
+	 * If we restart with --no-pidns, we'll add a ghost task below
+	 * whose pid will be used instead of these zeroed entried.
 	 */
 
 	/* forcing root_sid to -1, will make comparisons below fail */
@@ -1288,15 +1325,10 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 		else if (!ckpt_valid_pid(ctx, pids_arr[i].vpgid, "pgid", i))
 			return -1;
 
-		/* zero references to root_sid (root_sid != root_pid) */
-		if (pids_arr[i].vsid == root_sid) {
-			task->flags |= TASK_ZERO_SID;
+		if (pids_arr[i].vsid == root_sid)
 			pids_arr[i].vsid = 0;
-		}
-		if (pids_arr[i].vpgid == root_sid) {
-			task->flags |= TASK_ZERO_PGID;
+		if (pids_arr[i].vpgid == root_sid)
 			pids_arr[i].vpgid = 0;
-		}
 
 		task->pid = pids_arr[i].vpid;
 		task->ppid = pids_arr[i].vppid;
@@ -1324,6 +1356,9 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 
 		sid = pids_arr[i].vsid;
 
+		/* Remember if we find any vsid/vpgid - see below */
+		if (pids_arr[i].vsid == 0 || pids_arr[i].vpgid == 0)
+			zero_pid = 1;
 		/*
 		 * An unaccounted-for sid belongs to a task that was a
 		 * session leader and died. We can safe set its parent
@@ -1360,6 +1395,27 @@ static int ckpt_init_tree(struct ckpt_ctx *ctx)
 			return -1;
 	}
 
+	/*
+	 * Zero sid/pgid is disallowed in --no-pidns mode. If there
+	 * were any, we invent a new ghost-zero task and substitute
+	 * its pid for those any sid/pgid.
+	 */
+	if (zero_pid && !ctx->args->pidns) {
+		zero_pid = ckpt_zero_pid(ctx);
+		if (zero_pid < 0)
+			return -1;
+		for (i = 0; i < pids_nr; i++) {
+			if (pids_arr[i].vsid == 0) {
+				pids_arr[i].vsid = zero_pid;
+				pids_arr[i].vppid = zero_pid;
+			}
+			if (pids_arr[i].vpgid == 0) {
+				pids_arr[i].vpgid = zero_pid;
+				pids_arr[i].vppid = zero_pid;
+			}
+		}
+	}
+
 	/* mark root task(s), and set its "creator" to be zero_task */
 	ckpt_init_task(ctx)->flags |= TASK_ROOT;
 	ckpt_init_task(ctx)->creator = &zero_task;
@@ -1534,7 +1590,7 @@ static int ckpt_placeholder_task(struct ckpt_ctx *ctx, struct task *task)
 {
 	struct task *session = hash_lookup(ctx, task->sid);
 	struct task *holder = &ctx->tasks_arr[ctx->tasks_nr++];
-	int n = 0;
+	pid_t pid;
 
 	if (ctx->tasks_nr > ctx->tasks_max) {
 		/* shouldn't happen, beacuse we prepared enough */
@@ -1542,27 +1598,15 @@ static int ckpt_placeholder_task(struct ckpt_ctx *ctx, struct task *task)
 		return -1;
 	}
 
-	/*
-	 * allocate an unused pid for the placeholder
-	 * (this will become inefficient if pid-space is exhausted)
-	 */
-	do {
-		if (ctx->tasks_pid == INT_MAX)
-			ctx->tasks_pid = 2;
-		else
-			ctx->tasks_pid++;
-
-		if (n++ == INT_MAX) {	/* ohhh... */
-			ckpt_err("pid namsepace exhausted");
-			return -1;
-		}
-	} while (hash_lookup(ctx, ctx->tasks_pid));
+	pid = ckpt_alloc_pid(ctx);
+	if (pid < 0)
+		return -1;
 
 	holder->flags = TASK_DEAD;
 
-	holder->pid = ctx->tasks_pid;
+	holder->pid = pid;
 	holder->ppid = ckpt_init_task(ctx)->pid;
-	holder->tgid = holder->pid;
+	holder->tgid = pid;
 	holder->sid = task->sid;
 
 	holder->children = NULL;
@@ -2097,23 +2141,6 @@ static int ckpt_adjust_pids(struct ckpt_ctx *ctx)
 		}
 	}
 
-	if (!ctx->args->pidns) {
-		/*
-		 * If a task's {sid,pgid} was zeroed out (in ckpt_init_tree)
-		 * then substitute the coordinator's sid for it now. (This
-		 * should leave no more 0's in restart of subtree-checkpoint).
-		 *
-		 * NOTE: thanks to the construction of tasks_arr[], the first
-		 * ctx->pid_nr entries in both arrays match (the same pids).
-		 */
-		for (m = 0; m < ctx->pids_nr; m++) {
-			if (ctx->tasks_arr[m].flags & TASK_ZERO_SID)
-				ctx->copy_arr[m].vsid = coord_sid;
-			if (ctx->tasks_arr[m].flags & TASK_ZERO_PGID)
-				ctx->copy_arr[m].vpgid = coord_sid;
-		}
-	}
-
 	memcpy(ctx->pids_arr, ctx->copy_arr, len);
 
 #ifdef CHECKPOINT_DEBUG
-- 
1.6.0.4

Re: [PATCH 1/3] restart: coordinator in new pidns to always report status via pipe

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> Serge Hallyn reports:
>   "another question: if i run 'restart < out' and sys_restart returns
>   due to a -EPERM on some object, then restart.c returns 1.  but if i
>   'restart --pids', then it reports the error and returns 0.  unless i
>   add --copy-status to the flags.  that seems inconsistent?"
> 
> It was with a subtree checkpoint in a child pidns, root-task is not
> pid 1, So, the restarts calls ckpt_coordinator_pidns() execution.
> 
> In commit 2000bbb4b9... "restart: fix race in ckpt_coordinator_pidns
> and --no-wait" adds a pipe for a coordinator in a new pids to report
> success/failure of the restart operation back to the parent when the
> parent does not wish to wait.
> 
> IOW, the coordinator's exit value is overloaded - used once to report
> success/failure and once (optionally) to report root-tasks exit status.
> 
> This patch fixes this by extending the previous commit to make the
> coordinator-pidns always report the restart status via the pipe, and
> only use the exit status for --wait --copy-status case.
> 
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

Seems to be working fine for me.

Tested-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

to all 3.

thanks,
-serge

> ---
>  restart.c |   25 ++++++++++++-------------
>  1 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/restart.c b/restart.c
> index 35c54ea..5871bbf 100644
> --- a/restart.c
> +++ b/restart.c
> @@ -942,10 +942,12 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
>  	ckpt_dbg("forking coordinator in new pidns\n");
> 
>  	/*
> -	 * We won't wait for (collect) the coordinator, so we use a
> -	 * pipe instead for the coordinator to report success/failure.
> +	 * The coordinator report restart susccess/failure via pipe.
> +	 * (It cannot use return value, because the in the default
> +	 * --wait --copy-status case it is already used to report the
> +	 * root-task's return value).
>  	 */
> -	if (!ctx->args->wait && pipe(ctx->pipe_coord)) {
> +	if (pipe(ctx->pipe_coord) < 0) {
>  		perror("pipe");
>  		return -1;
>  	}
> @@ -981,10 +983,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
>  		return -1;
> 
>  	ctx->args->copy_status = copy;
> -	if (ctx->args->wait)
> -		return ckpt_collect_child(ctx);
> -	else
> -		return ckpt_coordinator_status(ctx);
> +	return ckpt_coordinator_status(ctx);
>  }
>  #else
>  static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> @@ -1040,13 +1039,13 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
>  		 * around and be reaper until all tasks are gone.
>  		 * Otherwise, container will die as soon as we exit.
>  		 */
> -		if (!ctx->args->wait) {
> -			/* report status because parent won't wait for us */
> -			if (write(ctx->pipe_coord[1], &ret, sizeof(ret)) < 0) {
> -				perror("failed to report status");
> -				exit(1);
> -			}
> +
> +		/* Report success/failure to the parent */
> +		if (write(ctx->pipe_coord[1], &ret, sizeof(ret)) < 0) {
> +			perror("failed to report status");
> +			exit(1);
>  		}
> +
>  		ret = ckpt_pretend_reaper(ctx);
>  	} else if (ctx->args->wait) {
>  		ret = ckpt_collect_child(ctx);
> -- 
> 1.6.0.4