checkpoint/restart: fix code to handle open symlinks

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

checkpoint/checkpoint.c renamed from checkpoint/checkpoint.c~fix-cr_ctx_checkpoint-locking +2 -5

@@ -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

checkpoint/ckpt_file.c renamed from checkpoint/ckpt_file.c~fix-cr_ctx_checkpoint-locking +1 -1

@@ -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

checkpoint/ckpt_mem.c renamed from checkpoint/ckpt_mem.c~fix-cr_ctx_checkpoint-locking +1 -1

@@ -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

checkpoint/sys.c renamed from checkpoint/sys.c~fix-cr_ctx_checkpoint-locking +2 -2

@@ -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

include/linux/checkpoint.h renamed from include/linux/checkpoint.h~fix-cr_ctx_checkpoint-locking +1 -1

@@ -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] */

_

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 ?

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

checkpoint/ckpt_file.c renamed from checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc +4 -9

 	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

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

checkpoint/ckpt_file.c renamed from checkpoint/ckpt_file.c~fix-cr_scan_fds-realloc +4 -9

@@ -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;
 	}

_

(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 ?