[RFC][PATCH 1/4] checkpoint/restart: fix code to handle open symlinks

[RFC][PATCH 1/4] checkpoint/restart: fix code to handle open symlinks

[Dave Hansen]

There's no such thing as an opened symlink.


---

 linux-2.6.git-dave/checkpoint/ckpt_file.c         |    3 ---
 linux-2.6.git-dave/checkpoint/rstr_file.c         |    1 -
 linux-2.6.git-dave/include/linux/checkpoint_hdr.h |    1 -
 3 files changed, 5 deletions(-)

diff -puN checkpoint/ckpt_file.c~fix-no-opened-symlinks checkpoint/ckpt_file.c
--- linux-2.6.git/checkpoint/ckpt_file.c~fix-no-opened-symlinks	2008-12-02 10:13:34.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:14:08.000000000 -0800
@@ -104,9 +104,6 @@ static int cr_write_fd_data(struct cr_ct
 	case S_IFDIR:
 		fd_type = CR_FD_DIR;
 		break;
-	case S_IFLNK:
-		fd_type = CR_FD_LINK;
-		break;
 	default:
 		cr_hbuf_put(ctx, sizeof(*hh));
 		return -EBADF;
diff -puN checkpoint/rstr_file.c~fix-no-opened-symlinks checkpoint/rstr_file.c
--- linux-2.6.git/checkpoint/rstr_file.c~fix-no-opened-symlinks	2008-12-02 10:13:34.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/rstr_file.c	2008-12-02 10:14:07.000000000 -0800
@@ -94,7 +94,6 @@ cr_read_fd_data(struct cr_ctx *ctx, stru
 	switch (hh->fd_type) {
 	case CR_FD_FILE:
 	case CR_FD_DIR:
-	case CR_FD_LINK:
 		file = cr_read_open_fname(ctx, hh->f_flags, hh->f_mode);
 		break;
 	default:
diff -puN include/linux/checkpoint_hdr.h~fix-no-opened-symlinks include/linux/checkpoint_hdr.h
--- linux-2.6.git/include/linux/checkpoint_hdr.h~fix-no-opened-symlinks	2008-12-02 10:13:34.000000000 -0800
+++ linux-2.6.git-dave/include/linux/checkpoint_hdr.h	2008-12-02 10:13:34.000000000 -0800
@@ -137,7 +137,6 @@ struct cr_hdr_fd_ent {
 enum  fd_type {
 	CR_FD_FILE = 1,
 	CR_FD_DIR,
-	CR_FD_LINK
 };
 
 struct cr_hdr_fd_data {
_

[RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking

[Dave Hansen]

The existing ctx->vfsroot is dangerous.  We take it from
the root task via: ctx->root_task->fs->root and don't take
a ref on the 'fs' in there.  We also take a ref on the
fs.root which is worthless.

So, replace ctx->vfsroot with a reference to the 'fs'
instead.  This gives us easy access to fs.root later on,
and also lets us do proper refcounting.

This, of course, eventually needs to be done per-process
but this should at least remove a potential oops.

---

 linux-2.6.git-dave/checkpoint/checkpoint.c    |    7 ++-----
 linux-2.6.git-dave/checkpoint/ckpt_file.c     |    2 +-
 linux-2.6.git-dave/checkpoint/ckpt_mem.c      |    2 +-
 linux-2.6.git-dave/checkpoint/sys.c           |    4 ++--
 linux-2.6.git-dave/include/linux/checkpoint.h |    2 +-
 5 files changed, 7 insertions(+), 10 deletions(-)

diff -puN checkpoint/checkpoint.c~fix-cr_ctx_checkpoint-locking checkpoint/checkpoint.c
--- linux-2.6.git/checkpoint/checkpoint.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/checkpoint.c	2008-12-02 10:21:52.000000000 -0800
@@ -511,12 +511,9 @@ static int cr_ctx_checkpoint(struct cr_c
 	 * assume checkpointer is in container's root vfs
 	 * FIXME: this works for now, but will change with real containers
 	 */
-	task_lock(ctx->root_task);
-	ctx->vfsroot = &ctx->root_task->fs->root;
-	task_unlock(ctx->root_task);
-	if (!ctx->vfsroot)
+	ctx->fs = get_fs_struct(ctx->root_task);
+	if (!ctx->fs)
 		return -EAGAIN;
-	path_get(ctx->vfsroot);
 
 	return 0;
 }
diff -puN checkpoint/ckpt_file.c~fix-cr_ctx_checkpoint-locking checkpoint/ckpt_file.c
--- linux-2.6.git/checkpoint/ckpt_file.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:46:56.000000000 -0800
@@ -117,7 +117,7 @@ static int cr_write_fd_data(struct cr_ct
 	if (ret < 0)
 		return ret;
 
-	return cr_write_fname(ctx, &file->f_path, ctx->vfsroot);
+	return cr_write_fname(ctx, &file->f_path, &ctx->fs.root);
 }
 
 /**
diff -puN checkpoint/ckpt_mem.c~fix-cr_ctx_checkpoint-locking checkpoint/ckpt_mem.c
--- linux-2.6.git/checkpoint/ckpt_mem.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/ckpt_mem.c	2008-12-02 10:21:52.000000000 -0800
@@ -436,7 +436,7 @@ static int cr_write_vma(struct cr_ctx *c
 
 	/* save the file name, if relevant */
 	if (vma->vm_file) {
-		ret = cr_write_fname(ctx, &vma->vm_file->f_path, ctx->vfsroot);
+		ret = cr_write_fname(ctx, &vma->vm_file->f_path, &ctx->fs.root);
 		if (ret < 0)
 			return ret;
 	}
diff -puN checkpoint/sys.c~fix-cr_ctx_checkpoint-locking checkpoint/sys.c
--- linux-2.6.git/checkpoint/sys.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/sys.c	2008-12-02 10:21:52.000000000 -0800
@@ -152,8 +152,8 @@ static void cr_ctx_free(struct cr_ctx *c
 
 	kfree(ctx->hbuf);
 
-	if (ctx->vfsroot)
-		path_put(ctx->vfsroot);
+	if (ctx->fs)
+		put_fs_struct(ctx->fs);
 
 	cr_pgarr_free(ctx);
 	cr_objhash_free(ctx);
diff -puN include/linux/checkpoint.h~fix-cr_ctx_checkpoint-locking include/linux/checkpoint.h
--- linux-2.6.git/include/linux/checkpoint.h~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
+++ linux-2.6.git-dave/include/linux/checkpoint.h	2008-12-02 10:21:52.000000000 -0800
@@ -41,7 +41,7 @@ struct cr_ctx {
 
 	struct list_head pgarr_list;	/* page array to dump VMA contents */
 
-	struct path *vfsroot;	/* container root (FIXME) */
+	struct fs_struct *fs;		/* container root/pwd (FIXME) */
 
 	/* [multi-process checkpoint] */
 	struct task_struct **tasks_arr; /* array of all tasks [checkpoint] */
_

[RFC][PATCH 3/4] checkpoint/restart: fix 'struct file' references

[Dave Hansen]

We must hold a reference to 'file' when it get passed in to
fd_install().  After fd_install() the entry in the fdtable
takes on the reference.  If we don't hold a reference to
it, another thread can come along after fd_install() and
fput() it before we've done the get_file().

In cr_read_fd_data(), the cr_obj_add_ref() code does the
get_file() internally.  But, we need to do this (and get
the ref) before we do the cr_attach_file().  Otherwise,
the file can go away after the cr_attach_file() and before
the cr_obj_add_ref().


---

 linux-2.6.git-dave/checkpoint/rstr_file.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff -puN checkpoint/rstr_file.c~fix-refs-order-in-cr_attach_get_file checkpoint/rstr_file.c
--- linux-2.6.git/checkpoint/rstr_file.c~fix-refs-order-in-cr_attach_get_file	2008-12-02 10:22:17.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/rstr_file.c	2008-12-02 10:22:17.000000000 -0800
@@ -59,8 +59,8 @@ static int cr_attach_get_file(struct fil
 
 	if (fd >= 0) {
 		fsnotify_open(file->f_path.dentry);
-		fd_install(fd, file);
 		get_file(file);
+		fd_install(fd, file);
 	}
 	return fd;
 }
@@ -107,6 +107,11 @@ cr_read_fd_data(struct cr_ctx *ctx, stru
 
 	/* FIX: need to restore uid, gid, owner etc */
 
+	/* register new <objref, file> tuple in hash table */
+	ret = cr_obj_add_ref(ctx, file, parent, CR_OBJ_FILE, 0);
+	if (ret < 0)
+		goto out;
+
 	fd = cr_attach_file(file);	/* no need to cleanup 'file' below */
 	if (fd < 0) {
 		filp_close(file, NULL);
@@ -114,10 +119,6 @@ cr_read_fd_data(struct cr_ctx *ctx, stru
 		goto out;
 	}
 
-	/* register new <objref, file> tuple in hash table */
-	ret = cr_obj_add_ref(ctx, file, parent, CR_OBJ_FILE, 0);
-	if (ret < 0)
-		goto out;
 	ret = sys_fcntl(fd, F_SETFL, hh->f_flags & CR_SETFL_MASK);
 	if (ret < 0)
 		goto out;
_

[RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()

[Dave Hansen]

I think having all the allocations in one place, plus the
reduction in the number of lines speaks for itself.  In
any case, this is last in the series and can be dropped if
you don't like it.

---

 linux-2.6.git-dave/checkpoint/ckpt_file.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff -puN checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc checkpoint/ckpt_file.c
--- linux-2.6.git/checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc	2008-12-02 10:26:53.000000000 -0800
+++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:26:53.000000000 -0800
@@ -38,6 +38,7 @@ int cr_scan_fds(struct files_struct *fil
 	int i, n = 0;
 	int tot = CR_DEFAULT_FDTABLE;
 
+retry:
 	fds = kmalloc(tot * sizeof(*fds), GFP_KERNEL);
 	if (!fds)
 		return -ENOMEM;
@@ -55,18 +56,12 @@ int cr_scan_fds(struct files_struct *fil
 		if (!fcheck_files(files, i))
 			continue;
 		if (n == tot) {
-			/*
-			 * fcheck_files() is safe with drop/re-acquire
-			 * of the lock, because it tests:  fd < max_fds
-			 */
+			/* we undershot the size of fds[] */
 			spin_unlock(&files->file_lock);
 			rcu_read_unlock();
 			tot *= 2;	/* won't overflow: kmalloc will fail */
-			fds = krealloc(fds, tot * sizeof(*fds), GFP_KERNEL);
-			if (!fds)
-				return -ENOMEM;
-			rcu_read_lock();
-			spin_lock(&files->file_lock);
+			kfree(fds);
+			goto retry;
 		}
 		fds[n++] = i;
 	}
_

Re: [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking

[Oren Laadan]

any reason why you want to reference the entire ->fs instead of, as I
suggested, make a *copy* of ->fs->root and then reference its contents ?


Dave Hansen wrote:
> The existing ctx->vfsroot is dangerous.  We take it from
> the root task via: ctx->root_task->fs->root and don't take
> a ref on the 'fs' in there.  We also take a ref on the
> fs.root which is worthless.
> 
> So, replace ctx->vfsroot with a reference to the 'fs'
> instead.  This gives us easy access to fs.root later on,
> and also lets us do proper refcounting.
> 
> This, of course, eventually needs to be done per-process
> but this should at least remove a potential oops.
> 
> ---
> 
>  linux-2.6.git-dave/checkpoint/checkpoint.c    |    7 ++-----
>  linux-2.6.git-dave/checkpoint/ckpt_file.c     |    2 +-
>  linux-2.6.git-dave/checkpoint/ckpt_mem.c      |    2 +-
>  linux-2.6.git-dave/checkpoint/sys.c           |    4 ++--
>  linux-2.6.git-dave/include/linux/checkpoint.h |    2 +-
>  5 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff -puN checkpoint/checkpoint.c~fix-cr_ctx_checkpoint-locking checkpoint/checkpoint.c
> --- linux-2.6.git/checkpoint/checkpoint.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/checkpoint.c	2008-12-02 10:21:52.000000000 -0800
> @@ -511,12 +511,9 @@ static int cr_ctx_checkpoint(struct cr_c
>  	 * assume checkpointer is in container's root vfs
>  	 * FIXME: this works for now, but will change with real containers
>  	 */
> -	task_lock(ctx->root_task);
> -	ctx->vfsroot = &ctx->root_task->fs->root;
> -	task_unlock(ctx->root_task);
> -	if (!ctx->vfsroot)
> +	ctx->fs = get_fs_struct(ctx->root_task);
> +	if (!ctx->fs)
>  		return -EAGAIN;
> -	path_get(ctx->vfsroot);
>  
>  	return 0;
>  }
> diff -puN checkpoint/ckpt_file.c~fix-cr_ctx_checkpoint-locking checkpoint/ckpt_file.c
> --- linux-2.6.git/checkpoint/ckpt_file.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:46:56.000000000 -0800
> @@ -117,7 +117,7 @@ static int cr_write_fd_data(struct cr_ct
>  	if (ret < 0)
>  		return ret;
>  
> -	return cr_write_fname(ctx, &file->f_path, ctx->vfsroot);
> +	return cr_write_fname(ctx, &file->f_path, &ctx->fs.root);
>  }
>  
>  /**
> diff -puN checkpoint/ckpt_mem.c~fix-cr_ctx_checkpoint-locking checkpoint/ckpt_mem.c
> --- linux-2.6.git/checkpoint/ckpt_mem.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/ckpt_mem.c	2008-12-02 10:21:52.000000000 -0800
> @@ -436,7 +436,7 @@ static int cr_write_vma(struct cr_ctx *c
>  
>  	/* save the file name, if relevant */
>  	if (vma->vm_file) {
> -		ret = cr_write_fname(ctx, &vma->vm_file->f_path, ctx->vfsroot);
> +		ret = cr_write_fname(ctx, &vma->vm_file->f_path, &ctx->fs.root);
>  		if (ret < 0)
>  			return ret;
>  	}
> diff -puN checkpoint/sys.c~fix-cr_ctx_checkpoint-locking checkpoint/sys.c
> --- linux-2.6.git/checkpoint/sys.c~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/sys.c	2008-12-02 10:21:52.000000000 -0800
> @@ -152,8 +152,8 @@ static void cr_ctx_free(struct cr_ctx *c
>  
>  	kfree(ctx->hbuf);
>  
> -	if (ctx->vfsroot)
> -		path_put(ctx->vfsroot);
> +	if (ctx->fs)
> +		put_fs_struct(ctx->fs);
>  
>  	cr_pgarr_free(ctx);
>  	cr_objhash_free(ctx);
> diff -puN include/linux/checkpoint.h~fix-cr_ctx_checkpoint-locking include/linux/checkpoint.h
> --- linux-2.6.git/include/linux/checkpoint.h~fix-cr_ctx_checkpoint-locking	2008-12-02 10:21:52.000000000 -0800
> +++ linux-2.6.git-dave/include/linux/checkpoint.h	2008-12-02 10:21:52.000000000 -0800
> @@ -41,7 +41,7 @@ struct cr_ctx {
>  
>  	struct list_head pgarr_list;	/* page array to dump VMA contents */
>  
> -	struct path *vfsroot;	/* container root (FIXME) */
> +	struct fs_struct *fs;		/* container root/pwd (FIXME) */
>  
>  	/* [multi-process checkpoint] */
>  	struct task_struct **tasks_arr; /* array of all tasks [checkpoint] */
> _
> 

Re: [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking

[Dave Hansen]

On Tue, 2008-12-02 at 17:22 -0500, Oren Laadan wrote:
> any reason why you want to reference the entire ->fs instead of, as I
> suggested, make a *copy* of ->fs->root and then reference its
> contents ?

This looked simpler to me.  We're going to have to rip this sucker out
anyway, so I went for what I thought was the smallest amount of code.

-- Dave

Re: [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> I think having all the allocations in one place, plus the
> reduction in the number of lines speaks for itself.  In
> any case, this is last in the series and can be dropped if
> you don't like it.
> 
> ---
> 
>  linux-2.6.git-dave/checkpoint/ckpt_file.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff -puN checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc checkpoint/ckpt_file.c
> --- linux-2.6.git/checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc	2008-12-02 10:26:53.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:26:53.000000000 -0800
> @@ -38,6 +38,7 @@ int cr_scan_fds(struct files_struct *fil
>  	int i, n = 0;

Heh, well I think you'll need to initialize n after the retry: label.

Otherwise, I can see the appeal of this...

>  	int tot = CR_DEFAULT_FDTABLE;
> 
> +retry:
>  	fds = kmalloc(tot * sizeof(*fds), GFP_KERNEL);
>  	if (!fds)
>  		return -ENOMEM;
> @@ -55,18 +56,12 @@ int cr_scan_fds(struct files_struct *fil
>  		if (!fcheck_files(files, i))
>  			continue;
>  		if (n == tot) {
> -			/*
> -			 * fcheck_files() is safe with drop/re-acquire
> -			 * of the lock, because it tests:  fd < max_fds
> -			 */
> +			/* we undershot the size of fds[] */
>  			spin_unlock(&files->file_lock);
>  			rcu_read_unlock();
>  			tot *= 2;	/* won't overflow: kmalloc will fail */
> -			fds = krealloc(fds, tot * sizeof(*fds), GFP_KERNEL);
> -			if (!fds)
> -				return -ENOMEM;
> -			rcu_read_lock();
> -			spin_lock(&files->file_lock);
> +			kfree(fds);
> +			goto retry;
>  		}
>  		fds[n++] = i;
>  	}
> _
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()

[Oren Laadan]

(as discussed in the LKML thread) as far as I can see the existing code is
safe, and this code is not more correct (for restart) in terms of races with
changes to the file table ?

Dave Hansen wrote:
> I think having all the allocations in one place, plus the
> reduction in the number of lines speaks for itself.  In
> any case, this is last in the series and can be dropped if
> you don't like it.
> 
> ---
> 
>  linux-2.6.git-dave/checkpoint/ckpt_file.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff -puN checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc checkpoint/ckpt_file.c
> --- linux-2.6.git/checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc	2008-12-02 10:26:53.000000000 -0800
> +++ linux-2.6.git-dave/checkpoint/ckpt_file.c	2008-12-02 10:26:53.000000000 -0800
> @@ -38,6 +38,7 @@ int cr_scan_fds(struct files_struct *fil
>  	int i, n = 0;
>  	int tot = CR_DEFAULT_FDTABLE;
>  
> +retry:
>  	fds = kmalloc(tot * sizeof(*fds), GFP_KERNEL);
>  	if (!fds)
>  		return -ENOMEM;
> @@ -55,18 +56,12 @@ int cr_scan_fds(struct files_struct *fil
>  		if (!fcheck_files(files, i))
>  			continue;
>  		if (n == tot) {
> -			/*
> -			 * fcheck_files() is safe with drop/re-acquire
> -			 * of the lock, because it tests:  fd < max_fds
> -			 */
> +			/* we undershot the size of fds[] */
>  			spin_unlock(&files->file_lock);
>  			rcu_read_unlock();
>  			tot *= 2;	/* won't overflow: kmalloc will fail */
> -			fds = krealloc(fds, tot * sizeof(*fds), GFP_KERNEL);
> -			if (!fds)
> -				return -ENOMEM;
> -			rcu_read_lock();
> -			spin_lock(&files->file_lock);
> +			kfree(fds);
> +			goto retry;
>  		}
>  		fds[n++] = i;
>  	}
> _
> 

Re: [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()

[Dave Hansen]

On Wed, 2008-12-03 at 04:48 -0500, Oren Laadan wrote:
> (as discussed in the LKML thread) as far as I can see the existing code is
> safe, and this code is not more correct (for restart) in terms of races with
> changes to the file table ?

Agreed.  This just saves a few lines of code.

-- Dave