don't call pre_restore_task twice
PATCH 1/1 don't call pre_restore_task twice
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.
--- checkpoint/process.c | 4 ---- checkpoint/restart.c | 1 + 2 files changed, 1 insertions(+), 4 deletions(-)
checkpoint/process.c +0 -4
@@ -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 )
checkpoint/restart.c +1 -0
@@ -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); }
Signature · 1 line
1.5.4.3
A message that is not in the archive (sent off-list or not yet received): 20091007234750.GA6881-r/[email protected]
Matt Helsley
Reviewed-byQuoted 9 lines · Serge E. Hallyn
Pre_restore_taskis being called both before and insiderestore_task, causing a memory leak atcurrent->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.
Quoted 6 lines
Signed-off-by Serge E. Hallyn <serue-r/[email protected]>Reported-by Dan Smith <danms-r/[email protected]>Cc Dan Smith <danms-r/[email protected]>Cc Matt Helsley <matthltc-r/[email protected]>Signed-off-by Serge E. Hallyn <serue-r/[email protected]>
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, ¤t->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
Quoted 41 lines
--- checkpoint/process.c | 4 ---- checkpoint/restart.c | 1 + 2 files changed, 1 insertions(+), 4 deletions(-)checkpoint/process.c +0
-4@@ -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 )checkpoint/restart.c +1
-0@@ -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); }Signature · 7 lines
-- 1.5.4.3 _______________________________________________ Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org https://lists.linux-foundation.org/mailman/listinfo/containers
A message that is not in the archive (sent off-list or not yet received): 20091008030919.GH18101-52DBMbEzqgQ/wnmkkaCWp/[email protected]
Quoted 51 lines · Matt Helsley
Quoted 9 lines · Serge E. Hallyn
Pre_restore_taskis being called both before and insiderestore_task, causing a memory leak atcurrent->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.
Quoted 6 lines
Signed-off-by Serge E. Hallyn <serue-r/[email protected]>Reported-by Dan Smith <danms-r/[email protected]>Cc Dan Smith <danms-r/[email protected]>Cc Matt Helsley <matthltc-r/[email protected]>Signed-off-by Serge E. Hallyn <serue-r/[email protected]>Reviewed-by Matt Helsley <matthltc-r/[email protected]>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, ¤t->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();
Quoted 2 lines
But if we got
-ENOMEMfrompre_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
A message that is not in the archive (sent off-list or not yet received): 20091008141258.GA21486-r/[email protected]
Quoted 2 lines · Serge E. Hallyn
Quoted 1 line · Matt Helsley
On Wed, Oct 07, 2009 at 06:47:50PM -0500, Serge E. Hallyn wrote:
<snip>
Quoted 18 lines
Quoted 18 lines
void
post_restore_task(){ sigprocmask(SIG_SETMASK, ¤t->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();
Quoted 8 lines
Quoted 2 lines
But if we got
-ENOMEMfrompre_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
Dan Smith
Tested-bySH> 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/[email protected]> SH> Reported-by: Dan Smith <danms-r/[email protected]>
Thanks!
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: danms-r/[email protected]
A message that is not in the archive (sent off-list or not yet received): 871vldg3uz.fsf-FLMGYpZoEPULwtHQx/[email protected]
Quoted 9 lines · 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/[email protected]> SH> Reported-by: Dan Smith <danms-r/[email protected]>
Tested-by Dan Smith <danms-r/[email protected]>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
Pulled.
Quoted 47 lines · Serge E. Hallyn
Pre_restore_taskis being called both before and insiderestore_task, causing a memory leak atcurrent->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/[email protected]>Reported-by Dan Smith <danms-r/[email protected]>Cc Dan Smith <danms-r/[email protected]>Cc Matt Helsley <matthltc-r/[email protected]>Signed-off-by Serge E. Hallyn <serue-r/[email protected]>--- checkpoint/process.c | 4 ---- checkpoint/restart.c | 1 + 2 files changed, 1 insertions(+), 4 deletions(-)checkpoint/process.c +0
-4@@ -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 )checkpoint/restart.c +1
-0@@ -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); }