[PATCH 1/1] don't call pre_restore_task twice

[PATCH 1/1] don't call pre_restore_task twice

[Serge E. Hallyn]

Pre_restore_task is being called both before and inside
restore_task, causing a memory leak at
current->checkpoint_data.

Only call it once, outside restore_task.

This fixes a memory leak spotted by Dan Smith, and the
actual bug was deduced by Matt Helsley.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/process.c |    4 ----
 checkpoint/restart.c |    1 +
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/checkpoint/process.c b/checkpoint/process.c
index 424f688..c51e036 100644
--- a/checkpoint/process.c
+++ b/checkpoint/process.c
@@ -864,10 +864,6 @@ int restore_task(struct ckpt_ctx *ctx)
 {
 	int ret;
 
-	ret = pre_restore_task(ctx);
-	if (ret < 0)
-		goto out;
-
 	ret = restore_task_struct(ctx);
 	ckpt_debug("task %d\n", ret);
 	if (ret < 0)
diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 3a58a76..fc94374 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -967,6 +967,7 @@ static int do_restore_task(void)
 	 */
 	if (zombie) {
 		restore_debug_exit(ctx);
+		post_restore_task(ctx);
 		ckpt_ctx_put(ctx);
 		do_exit(current->exit_code);
 	}
-- 
1.5.4.3

Re: [PATCH 1/1] don't call pre_restore_task twice

[Matt Helsley]

On Wed, Oct 07, 2009 at 06:47:50PM -0500, Serge E. Hallyn wrote:
> Pre_restore_task is being called both before and inside
> restore_task, causing a memory leak at
> current->checkpoint_data.
> 
> Only call it once, outside restore_task.
> 
> This fixes a memory leak spotted by Dan Smith, and the
> actual bug was deduced by Matt Helsley.
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

However, I think I spotted another problem:

int pre_restore_task()
{
        sigset_t sigset;

        /* task-specific restart data: freed from post_restore_task() */
        current->checkpoint_data = kzalloc(sizeof(struct ckpt_data),
GFP_KERNEL);
        if (!current->checkpoint_data)
                return -ENOMEM;
...
}

void post_restore_task()
{
	sigprocmask(SIG_SETMASK, &current->checkpoint_data->blocked, NULL);
...
}

then in do_restore_coord():

if (ctx->uflags & RESTART_TASKSELF) {
                ret = pre_restore_task();
                ckpt_debug("pre restore task: %d\n", ret);
                if (ret < 0)
                        goto out;
...
 out:
        if (ctx->uflags & RESTART_TASKSELF)
                post_restore_task();

But if we got -ENOMEM from pre_restore_task() then I think there will be a
NULL dereference.


Cheers,
	-Matt Helsley

> ---
>  checkpoint/process.c |    4 ----
>  checkpoint/restart.c |    1 +
>  2 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/checkpoint/process.c b/checkpoint/process.c
> index 424f688..c51e036 100644
> --- a/checkpoint/process.c
> +++ b/checkpoint/process.c
> @@ -864,10 +864,6 @@ int restore_task(struct ckpt_ctx *ctx)
>  {
>  	int ret;
> 
> -	ret = pre_restore_task(ctx);
> -	if (ret < 0)
> -		goto out;
> -
>  	ret = restore_task_struct(ctx);
>  	ckpt_debug("task %d\n", ret);
>  	if (ret < 0)
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 3a58a76..fc94374 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -967,6 +967,7 @@ static int do_restore_task(void)
>  	 */
>  	if (zombie) {
>  		restore_debug_exit(ctx);
> +		post_restore_task(ctx);
>  		ckpt_ctx_put(ctx);
>  		do_exit(current->exit_code);
>  	}


> -- 
> 1.5.4.3
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 1/1] don't call pre_restore_task twice

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> On Wed, Oct 07, 2009 at 06:47:50PM -0500, Serge E. Hallyn wrote:
> > Pre_restore_task is being called both before and inside
> > restore_task, causing a memory leak at
> > current->checkpoint_data.
> > 
> > Only call it once, outside restore_task.
> > 
> > This fixes a memory leak spotted by Dan Smith, and the
> > actual bug was deduced by Matt Helsley.
> > 
> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > 
> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> However, I think I spotted another problem:
> 
> int pre_restore_task()
> {
>         sigset_t sigset;
> 
>         /* task-specific restart data: freed from post_restore_task() */
>         current->checkpoint_data = kzalloc(sizeof(struct ckpt_data),
> GFP_KERNEL);
>         if (!current->checkpoint_data)
>                 return -ENOMEM;
> ...
> }
> 
> void post_restore_task()
> {
> 	sigprocmask(SIG_SETMASK, &current->checkpoint_data->blocked, NULL);
> ...
> }
> 
> then in do_restore_coord():
> 
> if (ctx->uflags & RESTART_TASKSELF) {
>                 ret = pre_restore_task();
>                 ckpt_debug("pre restore task: %d\n", ret);
>                 if (ret < 0)
>                         goto out;
> ...
>  out:
>         if (ctx->uflags & RESTART_TASKSELF)
>                 post_restore_task();
> 
> But if we got -ENOMEM from pre_restore_task() then I think there will be a
> NULL dereference.

But the very first thing post_restore_task() does is

	/* can happen if restart failed early */
	if (!current->checkpoint_data)
		return;

-serge

Re: [PATCH 1/1] don't call pre_restore_task twice

[Dan Smith]

SH> This fixes a memory leak spotted by Dan Smith, and the
SH> actual bug was deduced by Matt Helsley.

SH> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
SH> Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Tested-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Thanks!

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 1/1] don't call pre_restore_task twice

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> SH> This fixes a memory leak spotted by Dan Smith, and the
> SH> actual bug was deduced by Matt Helsley.
> 
> SH> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> SH> Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Tested-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Thanks!

Thanks, pushed for now to git://git.kernel.org/pub/scm/linux/kernel/git/sergeh/linux-cr.git
branch ckpt-v18-dev.oct8

-serge

Re: [PATCH 1/1] don't call pre_restore_task twice

[Matt Helsley]

On Thu, Oct 08, 2009 at 09:12:58AM -0500, Serge E. Hallyn wrote:
> Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> > On Wed, Oct 07, 2009 at 06:47:50PM -0500, Serge E. Hallyn wrote:

<snip>

> > void post_restore_task()
> > {
> > 	sigprocmask(SIG_SETMASK, &current->checkpoint_data->blocked, NULL);
> > ...
> > }
> > 
> > then in do_restore_coord():
> > 
> > if (ctx->uflags & RESTART_TASKSELF) {
> >                 ret = pre_restore_task();
> >                 ckpt_debug("pre restore task: %d\n", ret);
> >                 if (ret < 0)
> >                         goto out;
> > ...
> >  out:
> >         if (ctx->uflags & RESTART_TASKSELF)
> >                 post_restore_task();
> > 
> > But if we got -ENOMEM from pre_restore_task() then I think there will be a
> > NULL dereference.
> 
> But the very first thing post_restore_task() does is
> 
> 	/* can happen if restart failed early */
> 	if (!current->checkpoint_data)
> 		return;

Ah, good. That's a fairly recent change so it wasn't in my git tree.  Looks 
like it was added Oct 3rd by:

commit: bb091acbf0a47bd61323322a4e0a784d1ee0c24f
"c/r: threads sync on restart (fix regression from commit afbe522c...)"

Cheers,
	-Matt Helsley

Re: [PATCH 1/1] don't call pre_restore_task twice

[Oren Laadan]

Pulled.

Serge E. Hallyn wrote:
> Pre_restore_task is being called both before and inside
> restore_task, causing a memory leak at
> current->checkpoint_data.
> 
> Only call it once, outside restore_task.
> 
> This fixes a memory leak spotted by Dan Smith, and the
> actual bug was deduced by Matt Helsley.
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Reported-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/process.c |    4 ----
>  checkpoint/restart.c |    1 +
>  2 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/checkpoint/process.c b/checkpoint/process.c
> index 424f688..c51e036 100644
> --- a/checkpoint/process.c
> +++ b/checkpoint/process.c
> @@ -864,10 +864,6 @@ int restore_task(struct ckpt_ctx *ctx)
>  {
>  	int ret;
>  
> -	ret = pre_restore_task(ctx);
> -	if (ret < 0)
> -		goto out;
> -
>  	ret = restore_task_struct(ctx);
>  	ckpt_debug("task %d\n", ret);
>  	if (ret < 0)
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 3a58a76..fc94374 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -967,6 +967,7 @@ static int do_restore_task(void)
>  	 */
>  	if (zombie) {
>  		restore_debug_exit(ctx);
> +		post_restore_task(ctx);
>  		ckpt_ctx_put(ctx);
>  		do_exit(current->exit_code);
>  	}