cr: define CHECKPOINT_SUBTREE flag and sysctl

Define a CHECKPOINT_SUBTREE flag for sys_checkpoint() which says it's ok if the the checkpointed set of tasks are not a fully isolated container without leaks.

Define a sysctl 'ckpt_subtree_allowed' which determines whether subtree checkpoints are ok. If that sysctl, ckpt_subtree_allowed, is 0, then the CHECKPOINT_SUBTREE flag may not be used. Also, if that sysctl is 0, then both sys_checkpoint() and sys_restart() always require CAP_SYS_ADMIN.

checkpoint/checkpoint.c +13 -0

+static int check_obj_isolated(struct cr_ctx *ctx, struct cr_objref *ref)
+{
+	struct uts_namespace *utsns;
+	struct ipc_namespace *ipcns;
+	struct file *file;
+	struct mm_struct *mm;
+	unsigned long cnt, cnt2;
+	int ret = 1;
+
+	/* note - one might think it worthwhile to put the ns
+	 * ones under #ifdefs for the CONFIG_X_NS, but instead
+	 * it CONFIG_CHECKPOINT should depend on all of those
+	 */

Quoted 10 lines · Serge E. Hallyn

Define a CHECKPOINT_SUBTREE flag for sys_checkpoint() which says it's ok if the the checkpointed set of tasks are not a fully isolated container without leaks.

Define a sysctl 'ckpt_subtree_allowed' which determines whether subtree checkpoints are ok. If that sysctl, ckpt_subtree_allowed, is 0, then the CHECKPOINT_SUBTREE flag may not be used. Also, if that sysctl is 0, then both sys_checkpoint() and sys_restart() always require CAP_SYS_ADMIN.

Whether subtree checkpoint is allowed and whether non-admin checkpoint is allowed are independent constraints, no? Should this really be a single flag?

Quoted 20 lines

checkpoint/checkpoint.c +20 -0

+static int check_obj_isolated(struct cr_ctx *ctx, struct cr_objref *ref)
+{
+	struct uts_namespace *utsns;
+	struct ipc_namespace *ipcns;
+	struct file *file;
+	struct mm_struct *mm;
+	unsigned long cnt, cnt2;
+	int ret = 1;
+
+	/* note - one might think it worthwhile to put the ns
+	 * ones under #ifdefs for the CONFIG_X_NS, but instead
+	 * it CONFIG_CHECKPOINT should depend on all of those
+	 */
+	/* note2: the objhash has taken a reference, so we account
+	 * for that */
+
+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;

checkpoint/checkpoint.c static int cr_ctx_checkpoint(struct cr_ctx *ctx, pid_t pid) · line 552

Quoted 3 lines

checkpoint/checkpoint.c +3 -0

+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

Quoted 12 lines · Nathan Lynch

Quoted 10 lines · Serge E. Hallyn

Define a CHECKPOINT_SUBTREE flag for sys_checkpoint() which says it's ok if the the checkpointed set of tasks are not a fully isolated container without leaks.

Define a sysctl 'ckpt_subtree_allowed' which determines whether subtree checkpoints are ok. If that sysctl, ckpt_subtree_allowed, is 0, then the CHECKPOINT_SUBTREE flag may not be used. Also, if that sysctl is 0, then both sys_checkpoint() and sys_restart() always require CAP_SYS_ADMIN.

Quoted 3 lines

Whether subtree checkpoint is allowed and whether non-admin checkpoint is allowed are independent constraints, no? Should this really be a single flag?

Well it's not about the flag, it's about the sysctl. So actually I don't have that right at checkpoint (but do at restart). It should just be:

	if (!ckpt_subtree_allowed && !capable(CAP_SYS_ADMIN))
		return -EPERM;

for both.

As for making it two sysctls, I don't really care. Fine by me...

There's no need for a sysctl disallowing the CHECKPOINT_SUBTREE flag. There should just be a unprivileged_checkpoint sysctl determining whether CAP_SYS_ADMIN is always needed. Then the optional CHECKPOINT_SUBTREE is always allowed.

That makes much more sense. Thanks, Nathan.

-serge

Quoted 13 lines · Nathan Lynch

checkpoint/checkpoint.c static int cr_ctx_checkpoint(struct cr_ctx *ctx, pid_t pid) · line 552

Quoted 7 lines · Serge E. Hallyn

checkpoint/checkpoint.c +7 -0

+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

It's got nothing to do with the refcounts changing.

It ensures that, at the end of the checkpoint, the resources (utsns in this case) had no users not accounted for by a checkpointed task. In other words, there was no information leak.

Now it's possible that at the *start* of the checkpoint there was another task, not being checkpointed and not frozen, in the utsns, and it exited before the leaks check took place.

Quoted 15 lines · Serge E. Hallyn

Quoted 13 lines · Nathan Lynch

Review

Quoted 7 lines · Serge E. Hallyn

(quoted diff) +7 -0

+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

Quoted 5 lines

It's got nothing to do with the refcounts changing.

It ensures that, at the end of the checkpoint, the resources (utsns in this case) had no users not accounted for by a checkpointed task. In other words, there was no information leak.

Okay, I had mistakenly believed this code was running in the subtree/non-container case. I reread your patch description and it indicates that these checks are made only in the case of container checkpoint. If I'm (finally) understanding the patch correctly, my concern is lessened. Comparing refcounts is still... unconventional.

Quoted 3 lines

Now it's possible that at the *start* of the checkpoint there was another task, not being checkpointed and not frozen, in the utsns, and it exited before the leaks check took place.

[Please excuse the obtuse queries below]

In which case the check would fail, yes? Can this scenario actually occur? I'm of the understanding that a container must be frozen before proceeding with checkpoint. If that's correct, how could a task in the container exit in the meantime?

Quoted 11 lines · Nathan Lynch

Quoted 3 lines · Serge E. Hallyn

Now it's possible that at the *start* of the checkpoint there was another task, not being checkpointed and not frozen, in the utsns, and it exited before the leaks check took place.

[Please excuse the obtuse queries below]

In which case the check would fail, yes? Can this scenario actually occur? I'm of the understanding that a container must be frozen before proceeding with checkpoint. If that's correct, how could a task in the container exit in the meantime?

Heh, because there is no such thing as a 'container'. There is a set of tasks in the same freezer control group, and it's possible that there is a task not in that cgroup which is in the same utsname as the rest of the tasks in that freezer cgroup.

Heh, because there is no such thing as a 'container'.

There is a set of tasks in the same freezer control group, and it's possible that there is a task not in that cgroup which is in the same utsname as the rest of the tasks in that freezer cgroup.

Quoted 1 line · Serge E. Hallyn

Heh, because there is no such thing as a 'container'.

Oh, yes, there is.

Set of tasks shares set of uts_ns, ipc_ns, mnt_ns, pid_ns and net_ns. No other task shares this set.
Pid_ns set has tree hierarchy.
All user_ns which come from credentials of set of tasks aren't shared with other tasks.

Such set of tasks logically forms a container.

Quoted 3 lines

There is a set of tasks in the same freezer control group, and it's possible that there is a task not in that cgroup which is in the same utsname as the rest of the tasks in that freezer cgroup.

Quoted 16 lines · Nathan Lynch

Quoted 15 lines · Serge E. Hallyn

Quoted 12 lines · Nathan Lynch

Review

Quoted 7 lines · Serge E. Hallyn

(quoted diff) +7 -0

+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

It's got nothing to do with the refcounts changing.

Quoted 8 lines

Quoted 3 lines

It ensures that, at the end of the checkpoint, the resources (utsns in this case) had no users not accounted for by a checkpointed task. In other words, there was no information leak.

Okay, I had mistakenly believed this code was running in the subtree/non-container case. I reread your patch description and it indicates that these checks are made only in the case of container checkpoint. If I'm (finally) understanding the patch correctly, my concern is lessened. Comparing refcounts is still... unconventional.

Yes, and there are cases where it won't be usable - for instance if opening a procfile increments the resource->use count. That should not be an issue for utsns, ipcns, files, or vmas, afaik.

Quoted 10 lines

Quoted 4 lines

Now it's possible that at the *start* of the checkpoint there was another task, not being checkpointed and not frozen, in the utsns, and it exited before the leaks check took place.

[Please excuse the obtuse queries below]

In which case the check would fail, yes? Can this scenario actually occur? I'm of the understanding that a container must be frozen before proceeding with checkpoint. If that's correct, how could a task in the container exit in the meantime?

Heh, because there is no such thing as a 'container'. There is a set of tasks in the same freezer control group, and it's possible that there is a task not in that cgroup which is in the same utsname as the rest of the tasks in that freezer cgroup.

Now from my POV the important thing is that: (1) when userspace does the right thing, then it gets the right thing, and (2) when userspace does the wrong thing, the kernel doesn't crash. This goes beyond that, aiming for something which probably isn't fully achievable, but still may be useful even if only mostly achievable: namely, warn the user when they do the wrong thing, in the believe that not doing so makes the implementation "a toy." While I don't agree with that analysis, in the end the only person whose opinion on that matters is the user's :) So I'm not opposed to giving them the option.

Quoted 26 lines · Serge E. Hallyn

Quoted 24 lines · Nathan Lynch

Quoted 13 lines · Serge E. Hallyn

Quoted 12 lines · Nathan Lynch

Review

Quoted 7 lines · Serge E. Hallyn

(quoted diff) +7 -0

+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

Quoted 5 lines

It's got nothing to do with the refcounts changing.

It ensures that, at the end of the checkpoint, the resources (utsns in this case) had no users not accounted for by a checkpointed task. In other words, there was no information leak.

Okay, I had mistakenly believed this code was running in the subtree/non-container case. I reread your patch description and it indicates that these checks are made only in the case of container checkpoint. If I'm (finally) understanding the patch correctly, my concern is lessened. Comparing refcounts is still... unconventional.

Quoted 3 lines

Yes, and there are cases where it won't be usable - for instance if opening a procfile increments the resource->use count. That should not be an issue for utsns, ipcns, files, or vmas, afaik.

Actually, one such case is if you have a FIFO - and a task outside the "container" (for whatever definition we choose) opens that FIFO because the right thingie is mounted in its (distinct) mounts namespace.

Also, unsure if unix domain sockets (those visible through the file system, not the "abstract" type) are otherwise isolated as well ?

Quoted 30 lines · Oren Laadan

Quoted 28 lines · Serge E. Hallyn

Quoted 16 lines · Nathan Lynch

Quoted 15 lines · Serge E. Hallyn

Quoted 12 lines · Nathan Lynch

Review

Quoted 7 lines · Serge E. Hallyn

(quoted diff) +7 -0

+	cnt = ref->users + 1;
+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");

I'm struggling to understand what guarantee a check such as this is supposed to be making. I see that it will catch *some* undesirable cases. But "current refcount equals old refcount" does not imply that "refcount has not changed in the meantime".

It's got nothing to do with the refcounts changing.

Quoted 8 lines

Quoted 3 lines

It ensures that, at the end of the checkpoint, the resources (utsns in this case) had no users not accounted for by a checkpointed task. In other words, there was no information leak.

Okay, I had mistakenly believed this code was running in the subtree/non-container case. I reread your patch description and it indicates that these checks are made only in the case of container checkpoint. If I'm (finally) understanding the patch correctly, my concern is lessened. Comparing refcounts is still... unconventional.

Yes, and there are cases where it won't be usable - for instance if opening a procfile increments the resource->use count. That should not be an issue for utsns, ipcns, files, or vmas, afaik.

Quoted 3 lines

Actually, one such case is if you have a FIFO - and a task outside the "container" (for whatever definition we choose) opens that FIFO because the right thingie is mounted in its (distinct) mounts namespace.

That'll affect the CR_OBJ_INODE object, right? (Not the CR_OBJ_FILE one).

Quoted 3 lines

Also, unsure if unix domain sockets (those visible through the file system, not the "abstract" type) are otherwise isolated as well ?

Yes, they are isolated by network namespace, to the chagrin of some people.

-serge

Hey Alexey and Oren,

here is my proposal for a patch on top of Oren's tree to do the leak checking by default (basically the same way it was done in Alexey's patchset). It also by default explicitly requires CAP_SYS_ADMIN for both checkpoint and restart.

I think the mm->exe_file save/restore is a separate feature addition, but it was required to cleanly pass the file use count checks...

Any chance this is acceptable to both of you?

thanks,
-serge

From 8cbfbbdde1e9eab01766fcfb193846ae12ec07d6 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/[email protected]>
Date: Thu, 23 Apr 2009 11:37:23 -0700
Subject: [PATCH 1/1] cr: define CHECKPOINT_SUBTREE flag and sysctl

Define a CHECKPOINT_SUBTREE flag for sys_checkpoint() which says it's ok if the the checkpointed set of tasks are not a fully isolated container without leaks.

Define a sysctl 'ckpt_subtree_allowed' which determines whether subtree checkpoints are ok. If that sysctl, ckpt_subtree_allowed, is 0, then the CHECKPOINT_SUBTREE flag may not be used. Also, if that sysctl is 0, then both sys_checkpoint() and sys_restart() always require CAP_SYS_ADMIN.

Finally, add a 'users' count to objhash items, and, for a !CHECKPOINT_SUBTREE checkpoint, return an error code if the actual objects' counts are higher, indicating leaks (references to the objects from a task not being checkpointed). Of course, by this time most of the checkpoint image has been written out to disk, so this is purely advisory. But then, it's probably naive to argue that anything more than an advisory 'this went wrong' error code is useful.

The comparison of the objhash user counts to object refcounts as a basis for checking for leaks comes from Alexey's OpenVZ-based c/r patchset.

TODO: I still need to figure out what to do about CONFIG_*_NS dependencies in the cr_check_leaks() fn.

checkpoint/checkpoint.c +11 -0

index 7382cc3..e42d3eb 100644
@@ -25,6 +25,7 @@
 #include <linux/checkpoint_hdr.h>
 
 #include "checkpoint_arch.h"
+#include "objhash.h"
 
 /* unique checkpoint identifier (FIXME: should be per-container ?) */
 static atomic_t cr_ctx_count = ATOMIC_INIT(0);

@@ -487,6 +488,9 @@ static int cr_get_container(struct cr_ctx *ctx, pid_t pid)
 	ctx->root_nsproxy = nsproxy;
 	ctx->root_init = is_container_init(task);
 
+	if (!(ctx->flags & CHECKPOINT_SUBTREE) && !ctx->root_init)
+		return -EBUSY;
+
 	return 0;
 
  out:

@@ -523,6 +527,96 @@ static int cr_ctx_checkpoint(struct cr_ctx *ctx, pid_t pid)
 	return 0;
 }
 
+static int check_obj_isolated(struct cr_ctx *ctx, struct cr_objref *ref)
+{
+	struct uts_namespace *utsns;
+	struct ipc_namespace *ipcns;
+	struct file *file;
+	struct mm_struct *mm;
+	unsigned long cnt, cnt2;

checkpoint/checkpoint.c +44 -0

+	switch (ref->type) {
+	case CR_OBJ_UTSNS:
+		utsns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&utsns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("uts namespace leak\n");
+			printk(KERN_NOTICE "%s: uts: %lu users count %lu\n",
+				__func__, cnt, cnt2);
+			ret = 0;
+		}
+		break;
+	case CR_OBJ_IPCNS:
+		ipcns = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&ipcns->kref.refcount);
+		if (cnt != cnt2) {
+			cr_debug("ipc namespace leak\n");
+			printk(KERN_NOTICE "%s: ipc: %lu users count %lu\n",
+				__func__, cnt, cnt2);
+			ret = 0;
+		}
+		break;
+	case CR_OBJ_FILE:
+		file = ref->ptr;
+		if (file == ctx->file)
+			cnt++;
+		cnt2 = (unsigned long) atomic_long_read(&file->f_count);
+		if (cnt != cnt2) {
+			cr_debug("file usage leak\n");
+			printk(KERN_NOTICE "%s: file: %lu users count %lu\n",
+				__func__, cnt, cnt2);
+			printk(KERN_NOTICE "%s: filename is %s (%s)\n",
+				__func__, file->f_dentry->d_iname,
+				file->f_dentry->d_name.name);
+			ret = 0;
+		}
+		break;
+	case CR_OBJ_MM:
+		mm = ref->ptr;
+		cnt2 = (unsigned long) atomic_read(&mm->mm_users);
+		if (cnt != cnt2) {
+			cr_debug("mm usage leak\n");
+			printk(KERN_NOTICE "%s: mm: %lu users count %lu\n",
+				__func__, cnt, cnt2);
+			ret = 0;

checkpoint/checkpoint.c +23 -0

+{
+	struct cr_objref *ref;
+	struct hlist_node *node;
+
+	if (ctx->flags & CHECKPOINT_SUBTREE)
+		return 0;
+
+	/* Make sure the uts and ipc namespaces aren't
+	 * shared with any tasks not being checkpointed */
+	hlist_for_each_entry(ref, node, &ctx->objhash->all_nodes, next) {
+		if (!check_obj_isolated(ctx, ref))
+			return -EBUSY;
+	}
+
+	/* TODO: check netns, etc */
+
+	return 0;
+}
+
 int do_checkpoint(struct cr_ctx *ctx, pid_t pid)
 {
 	int ret;

@@ -550,6 +644,10 @@ int do_checkpoint(struct cr_ctx *ctx, pid_t pid)
 	if (ret < 0)
 		goto out;
 
+	ret = cr_check_leaks(ctx);
+	if (ret < 0)
+		goto out;
+
 	ctx->crid = atomic_inc_return(&cr_ctx_count);
 
 	/* on success, return (unique) checkpoint identifier */

checkpoint/ckpt_file.c +3 -2

index 8be6abe..04ce1a0 100644
@@ -146,7 +146,8 @@ int cr_write_file(struct cr_ctx *ctx, struct file *file)
  * otherwise calls cr_write_file to dump the file pointer too.
  */
 static int
-cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd)
+cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd,
+		struct task_struct *t)
 {
 	struct cr_hdr h;
 	struct cr_hdr_fd_ent *hh;

@@ -236,7 +237,7 @@ int cr_write_fd_table(struct cr_ctx *ctx, struct task_struct *t)
 
 	cr_debug("nfds %d\n", nfds);
 	for (n = 0; n < nfds; n++) {
-		ret = cr_write_fd_ent(ctx, files, fdtable[n]);
+		ret = cr_write_fd_ent(ctx, files, fdtable[n], t);
 		if (ret < 0)
 			break;
 	}

checkpoint/ckpt_mem.c +3 -1

index 54b2674..834a78e 100644
@@ -616,10 +616,12 @@ static enum cr_vma_type cr_shared_vma_type(struct vm_area_struct *vma, int old)
  * cr_write_vma - classify the vma and dump its contents
  * @ctx: checkpoint context
  * @vma: vma object
+ * @task: owning task
  *
  * (see vma subtypes in checkpoint_hdr.h)
  */
-static int cr_write_vma(struct cr_ctx *ctx, struct vm_area_struct *vma)
+static int cr_write_vma(struct cr_ctx *ctx, struct vm_area_struct *vma,
+			struct task_struct *task)
 {
 	struct cr_hdr h;
 	struct cr_hdr_vma *hh;

checkpoint/ckpt_mem.c +16 -1

+	exe_new = cr_obj_add_ptr(ctx, mm->exe_file, &exefile_objref,
+				CR_OBJ_FILE, 0);
+	if (exe_new < 0) {
+		ret = exe_new;
+		goto mmput;
+	}
+
 	new = cr_obj_add_ptr(ctx, mm, &objref, CR_OBJ_MM, 0);
 	if (new < 0) {
 		ret = new;

@@ -764,6 +774,7 @@ int cr_write_mm(struct cr_ctx *ctx, struct task_struct *t)
 	down_read(&mm->mmap_sem);
 
 	hh->objref = objref;
+	hh->exefile_objref = exefile_objref;
 
 	hh->start_code = mm->start_code;
 	hh->end_code = mm->end_code;

@@ -786,10 +797,17 @@ int cr_write_mm(struct cr_ctx *ctx, struct task_struct *t)
 	if (ret < 0)
 		goto out;
 
+	if (exe_new) {
+		/* write out the exe_file */
+		ret = cr_write_file(ctx, mm->exe_file);
+		if (ret < 0)
+			goto out;
+	}
+
 	if (new) {
 		/* write the vma's */
 		for (vma = mm->mmap; vma; vma = vma->vm_next) {
-			ret = cr_write_vma(ctx, vma);
+			ret = cr_write_vma(ctx, vma, t);
 			if (ret < 0)
 				goto out;
 		}

checkpoint/rstr_mem.c +2 -0

index 9de770d..84a7255 100644
@@ -19,6 +19,7 @@
 #include <linux/mm.h>
 #include <linux/err.h>
 #include <linux/syscalls.h>
+#include <linux/proc_fs.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
 

@@ -519,6 +520,7 @@ int cr_read_mm(struct cr_ctx *ctx)
 	struct mm_struct *mm;
 	unsigned int nr;
 	int ret;
+	struct file *exe_file;
 
 	hh = cr_hbuf_get(ctx, sizeof(*hh));
 	if (!hh)

@@ -580,6 +582,24 @@ int cr_read_mm(struct cr_ctx *ctx)
 	mm->arg_end = hh->arg_end;

checkpoint/rstr_mem.c +10 -0

+		int fd = cr_read_file(ctx, hh->exefile_objref);
+		if (fd < 0) {
+			ret = fd;
+			up_write(&mm->mmap_sem);
+			goto out;
+		}
+		exe_file = fget(fd);
+		sys_close(fd);
+	}
+	set_mm_exe_file(mm, exe_file);
 	up_write(&mm->mmap_sem);
 
 	/* FIX: need also mm->flags */

include/linux/checkpoint.h +1 -1

index 82d2a40..175d727 100644
@@ -33,7 +33,7 @@ struct cr_ctx {
 	unsigned long flags;
 	unsigned long oflags;	/* restart: old flags */
 
-	struct file *file;
+	struct file *file;	/* file to write image to */
 	int total;		/* total read/written */
 
 	void *hbuf;		/* temporary buffer for headers */

On Fri, Apr 24, 2009 at 04:06:08PM -0500, Serge E. Hallyn wrote:

checkpoint/checkpoint.c static int cr_ctx_checkpoint(struct cr_ctx *ctx, pid_t pid) · line 546

Quoted 1 line

checkpoint/checkpoint.c +1 -0

+	cnt = ref->users + 1;

Perhaps this switch is another candidate for an fops-style function pointer.

Quoted 1 line

+static int cr_check_leaks(struct cr_ctx *ctx)

What are "leaks" ? ;)

How about cr_find_ref_leaks ? I suggest "find" because "check" confuses with "checkpoint". "ref" because we're not looking for memory leaks.

Quoted 2 lines

mm = get_task_mm(t);

Might add a check for exe_file == NULL here too (see below).

checkpoint/rstr_mem.c int cr_read_mm(struct cr_ctx *ctx) · line 592

Quoted 2 lines

checkpoint/rstr_mem.c +2 -0

+	if (!exe_file) {
+		/* really it can't be the case that it NOT be NULL... */

This comment isn't correct. Yes, *most* of the time exe_file should be non-NULL.

Some tools avoid pinning the filesystem with a reference to its executable file by copying the executable into private, anonymous, executable pages, and then unmaping the originals. This drops the last VMA reference to the file which causes the exe_file reference to be dropped as well.

It may provide an interesting testcase for checkpoint/restart since it would mean the executable couldn't be mapped from a checkpointed file -- we'd have to rely solely on VMA reconstruction for these.

include/linux/checkpoint.h struct cr_ctx { · line 68

Quoted 6 lines

/* cr_ctx: flags */

include/linux/checkpoint.h +3 -2

-#define CR_CTX_CKPT	0x1
-#define CR_CTX_RSTR	0x2
+#define CR_CTX_CKPT		0x1
+#define CR_CTX_RSTR		0x2
+#define CHECKPOINT_SUBTREE	0x4

The whitespace change somewhat obscures the introduction of the flag here.

Quoted 1 line · Matt Helsley

On Fri, Apr 24, 2009 at 04:06:08PM -0500, Serge E. Hallyn wrote:

Thanks for taking a look, Matt. Oren has done some nice work to it in his ckpt-v14, please take a look there.

Quoted 2 lines

checkpoint/checkpoint.c static int cr_ctx_checkpoint(struct cr_ctx *ctx, pid_t pid) · line 546

Quoted 1 line

checkpoint/checkpoint.c +1 -0

+	cnt = ref->users + 1;

Perhaps this switch is another candidate for an fops-style function pointer.

Yup, Oren did that :)

Quoted 5 lines

Quoted 1 line

+static int cr_check_leaks(struct cr_ctx *ctx)

What are "leaks" ? ;)

How about cr_find_ref_leaks ? I suggest "find" because "check" confuses with "checkpoint". "ref" because we're not looking for memory leaks.

Oren renamed it to ckpt_obj_contained()

...

Quoted 3 lines

Quoted 2 lines

mm = get_task_mm(t);

Might add a check for exe_file == NULL here too (see below).

Hmm, I think it's ok to store a NULL pointer in the objhash...

...

Quoted 4 lines

checkpoint/rstr_mem.c int cr_read_mm(struct cr_ctx *ctx) · line 592

Quoted 2 lines

checkpoint/rstr_mem.c +2 -0

+	if (!exe_file) {
+		/* really it can't be the case that it NOT be NULL... */

This comment isn't correct. Yes, *most* of the time exe_file should be non-NULL.

You misread the comment, though you're right about the code.

The comment says it can't be the case that it *not* be NULL here. That's because the object can't be in the objhash yet, because it always comes after the main mm entry in the layout.

Quoted 5 lines

Some tools avoid pinning the filesystem with a reference to its executable file by copying the executable into private, anonymous, executable pages, and then unmaping the originals. This drops the last VMA reference to the file which causes the exe_file reference to be dropped as well.

Quoted 3 lines

It may provide an interesting testcase for checkpoint/restart since it would mean the executable couldn't be mapped from a checkpointed file -- we'd have to rely solely on VMA reconstruction for these.

Yeah, taking another look I don't handle the restart case where exe_file is NULL. (It won't harm the kernel, just return an error from checkpoint I believe)

How does a userspace tool do that though? Once the file has been exec()d, userspace doesn't have an open fd to the executable anymore... Can you explain how it's done, and/or send me a testcase?

Quoted 7 lines

include/linux/checkpoint.h struct cr_ctx { · line 68

Quoted 6 lines

/* cr_ctx: flags */

include/linux/checkpoint.h +3 -2

-#define CR_CTX_CKPT	0x1
-#define CR_CTX_RSTR	0x2
+#define CR_CTX_CKPT		0x1
+#define CR_CTX_RSTR		0x2
+#define CHECKPOINT_SUBTREE	0x4

The whitespace change somewhat obscures the introduction of the flag here.

True. Guess that could've been a separate tiny patch.

thanks,
-serge

Hmmm... IIUC from the text above then:

	ptr = mmap(NULL, size, MMAP_EXEC | ... )
	memcpy(ptr, src, size);
	mremap(src, size, size, MREMAP_FIXED | ..., ptr);

Quoted 5 lines · Oren Laadan

Hmmm... IIUC from the text above then:

	ptr = mmap(NULL, size, MMAP_EXEC | ... )
	memcpy(ptr, src, size);
	mremap(src, size, size, MREMAP_FIXED | ..., ptr);

What is src and size?

Hmm, I guess you can get that from /proc/self/maps.

Cool.

Hey Alexey and Oren,

here is my proposal for a patch on top of Oren's tree to do the leak checking by default (basically the same way it was done in Alexey's patchset). It also by default explicitly requires CAP_SYS_ADMIN for both checkpoint and restart.

I think the mm->exe_file save/restore is a separate feature addition, but it was required to cleanly pass the file use count checks...

Any chance this is acceptable to both of you?