[Devel] Re: [RFC v14][PATCH 53/54] Detect resource leaks for whole-containercheckpoint

[Oren Laadan]

On Thu, 7 May 2009, Matt Helsley wrote:

> On Wed, May 06, 2009 at 11:13:21PM -0700, Sukadev Bhattiprolu wrote:
> > Oren Laadan [orenl at cs.columbia.edu] wrote:
> 
> <snip>
> 
> > | > | diff --git a/checkpoint/memory.c b/checkpoint/memory.c
> > | > | index 7637c1e..5ae2b41 100644
> > | > | --- a/checkpoint/memory.c
> > | > | +++ b/checkpoint/memory.c
> > | > | @@ -687,6 +687,8 @@ static int do_checkpoint_mm(struct ckpt_ctx *ctx, struct mm_struct *mm)
> > | > |  			ret = exe_objref;
> > | > |  			goto out;
> > | > |  		}
> > | > | +		/* account for all references through vma/exe_file */
> > | > | +		ckpt_obj_users_inc(ctx, mm->exe_file, mm->num_exe_file_vmas);
> > | > 
> > | > Do we really need to add num_exe_file_vmas here ?
> > | > 
> > | > A quick look at all callers for added_exe_file_vma() seems to show that
> > | > those callers also do a get_file().
> > | 
> > | Each vma whose file is the same as mm->exe_file causes the refcount
> > | of that file to increase by 2: once by vma->vm_file, and once via
> > | added_exe_file_vma(). The c/r code calls ckpt_obj_checkpoint() only
> > | once, thus once one obj_file_grab() for that file. The code above
> > | accounts for the missing count.
> 
> Perhaps I'm misreading Oren's explanation, but the refcount on the file
> should not be:
> 
> 	2*#vmas(vm_file==mm->exe_file) + #fds(filp==mm->exe_file)

I left the #fds from the explanation, but of course they are counted.

> 
> It should be:
> 
> 	#vmas(vm_file==mm->exe_file) + #fds(filp==mm->exe_file) + 1(for mm->exe_file).

The current code counts #vmas (twice), #fds (once) and 1 (for 
mm->exe_file), for each process. Then there is +1 for the copy
that is kept in the objhash...

> 
> because added_exe_file_vma() increments num_exe_file_vmas but does not
> change the file reference count. So incrementing the obj count while
> walking the vmas and fds should bring the count 1 short of matching.
> 

Duh !  I recall having looked at it :/

The patch below should make it:

>From 9574933bbdafcbc6bee9d42fd215e80d0fb25348 Mon Sep 17 00:00:00 2001
From: Oren Laadan <orenl at cs.columbia.edu>
Date: Fri, 8 May 2009 02:41:19 -0400
Subject: [PATCH] c/r: fix users count of files that are pointed to by mm->exe_file

Drop the code that adds mm->num_exe_file_vmas to the users count of
the mm->exe_file. This is unnecessary because added_exe_file_vma()
increments num_exe_file_vmas but does not change the file reference
count. So incrementing the obj count while walking the vmas and fds
should bring the count 1 short of matching.

Signed-off-by: Oren Laadan <orenl at cs.columbia.edu>
---
 checkpoint/memory.c        |    2 --
 checkpoint/objhash.c       |    2 +-
 include/linux/checkpoint.h |    1 -
 3 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/checkpoint/memory.c b/checkpoint/memory.c
index fcf6481..e0ff4c1 100644
--- a/checkpoint/memory.c
+++ b/checkpoint/memory.c
@@ -687,8 +687,6 @@ static int do_checkpoint_mm(struct ckpt_ctx *ctx, struct mm_struct *mm)
 			ret = exe_objref;
 			goto out;
 		}
-		/* account for all references through vma/exe_file */
-		ckpt_obj_users_inc(ctx, mm->exe_file, mm->num_exe_file_vmas);
 	}
 
 	h->exefile_objref = exe_objref;
diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 87bc5e8..dc55047 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -394,7 +394,7 @@ int ckpt_obj_lookup_add(struct ckpt_ctx *ctx, void *ptr,
 }
 
 /* increment the 'users' count of an object */
-void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment)
+static void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment)
 {
 	struct ckpt_obj *obj;
 
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 8ee6304..d966f19 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -53,7 +53,6 @@ extern void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref,
 			    enum obj_type type);
 extern int ckpt_obj_lookup_add(struct ckpt_ctx *ctx, void *ptr,
 			       enum obj_type type, int *first);
-extern void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment);
 extern int ckpt_obj_insert(struct ckpt_ctx *ctx, void *ptr, int objref,
 			   enum obj_type type);
 
-- 
1.6.0.4

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers