* [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking
2008-12-02 18:57 [RFC][PATCH 1/4] checkpoint/restart: fix code to handle open symlinks Dave Hansen
@ 2008-12-02 18:57 ` Dave Hansen
2008-12-02 22:22 ` Oren Laadan
2008-12-02 18:57 ` [RFC][PATCH 3/4] checkpoint/restart: fix 'struct file' references Dave Hansen
2008-12-02 18:57 ` [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds() Dave Hansen
2 siblings, 1 reply; 9+ messages in thread
From: Dave Hansen @ 2008-12-02 18:57 UTC (permalink / raw)
To: Oren Laadan; +Cc: containers, 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] */
_
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking
2008-12-02 18:57 ` [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking Dave Hansen
@ 2008-12-02 22:22 ` Oren Laadan
[not found] ` <4935B52D.6050706-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Oren Laadan @ 2008-12-02 22:22 UTC (permalink / raw)
To: Dave Hansen; +Cc: containers
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] */
> _
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC][PATCH 3/4] checkpoint/restart: fix 'struct file' references
2008-12-02 18:57 [RFC][PATCH 1/4] checkpoint/restart: fix code to handle open symlinks Dave Hansen
2008-12-02 18:57 ` [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking Dave Hansen
@ 2008-12-02 18:57 ` Dave Hansen
2008-12-02 18:57 ` [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds() Dave Hansen
2 siblings, 0 replies; 9+ messages in thread
From: Dave Hansen @ 2008-12-02 18:57 UTC (permalink / raw)
To: Oren Laadan; +Cc: containers, 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;
_
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()
2008-12-02 18:57 [RFC][PATCH 1/4] checkpoint/restart: fix code to handle open symlinks Dave Hansen
2008-12-02 18:57 ` [RFC][PATCH 2/4] checkpoint/restart: fix cr_ctx_checkpoint() locking Dave Hansen
2008-12-02 18:57 ` [RFC][PATCH 3/4] checkpoint/restart: fix 'struct file' references Dave Hansen
@ 2008-12-02 18:57 ` Dave Hansen
2008-12-03 4:21 ` Serge E. Hallyn
2008-12-03 9:48 ` Oren Laadan
2 siblings, 2 replies; 9+ messages in thread
From: Dave Hansen @ 2008-12-02 18:57 UTC (permalink / raw)
To: Oren Laadan; +Cc: containers, 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;
}
_
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()
2008-12-02 18:57 ` [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds() Dave Hansen
@ 2008-12-03 4:21 ` Serge E. Hallyn
2008-12-03 9:48 ` Oren Laadan
1 sibling, 0 replies; 9+ messages in thread
From: Serge E. Hallyn @ 2008-12-03 4:21 UTC (permalink / raw)
To: Dave Hansen; +Cc: containers
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds()
2008-12-02 18:57 ` [RFC][PATCH 4/4] checkpoint/restart: simplify cr_scan_fds() Dave Hansen
2008-12-03 4:21 ` Serge E. Hallyn
@ 2008-12-03 9:48 ` Oren Laadan
[not found] ` <493655E2.7040304-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
1 sibling, 1 reply; 9+ messages in thread
From: Oren Laadan @ 2008-12-03 9:48 UTC (permalink / raw)
To: Dave Hansen; +Cc: containers
(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;
> }
> _
>
^ permalink raw reply [flat|nested] 9+ messages in thread