Quoting Oren Laadan ([email protected]): > > > Serge E. Hallyn wrote: > > Quoting Oren Laadan ([email protected]): > >> From: Oren Laadan <[email protected]> > >> > >> If prepare_descendants() is walking a tree and one of the tasks is > >> forking, one of two bads can happen. If the child doesn't inherit the > >> ->ctx, it breaks the assumption that the entire subtree is prepared. > >> If the child inherits the ->ctx, it will have one without having taken > >> a reference. > >> > >> This patch closed this race by explicitly getting and referencing the > >> ->ctx for a child process should the parent have one, atomically under > >> the tasklist_lock. > >> > >> Signed-off-by: Oren Laadan <[email protected]> > >> --- > >> kernel/fork.c | 11 ++++++++--- > >> 1 files changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/kernel/fork.c b/kernel/fork.c > >> index 9f13d7b..57118e4 100644 > >> --- a/kernel/fork.c > >> +++ b/kernel/fork.c > >> @@ -62,6 +62,7 @@ > >> #include <linux/fs_struct.h> > >> #include <linux/magic.h> > >> #include <linux/perf_counter.h> > >> +#include <linux/checkpoint.h> > >> > >> #include <asm/pgtable.h> > >> #include <asm/pgalloc.h> > >> @@ -1148,9 +1149,6 @@ static struct task_struct *copy_process(unsigned > >> long clone_flags, > >> INIT_LIST_HEAD(&p->pi_state_list); > >> p->pi_state_cache = NULL; > >> #endif > >> -#ifdef CONFIG_CHECKPOINT > >> - p->checkpoint_ctx = NULL; > >> -#endif > >> /* > >> * sigaltstack should be cleared when sharing the same VM > >> */ > >> @@ -1188,6 +1186,13 @@ static struct task_struct *copy_process(unsigned > >> long clone_flags, > >> /* Need tasklist lock for parent etc handling! */ > >> write_lock_irq(&tasklist_lock); > >> > >> +#ifdef CONFIG_CHECKPOINT > >> + /* If parent is restarting, child should be too */ > >> + if (unlikely(current->checkpoint_ctx)) { > >> + p->checkpoint_ctx = current->checkpoint_ctx; > > > > Won't break anything, but technically p->checkpoint_ctx will > > already be copied from current->checkpoint_ctx, so only the > > ckpt_ctx_get() is necessary, so this could really read > > > > if (p->checkpoint_ctx) > > ckpt_ctx_get(p->checkpoint_ctx); > > > > Right? > > That's what I initially thought. However, it _is_ possible that > the following occurs: > > 1) Task A starts a fork an does some of copy_process(): so > @p is NULL, but copy_process() isn't complete > > 2) An ancestor of task A calls prepare_descendants(), which > then places a valid checkpoint_ctx on A.
Huh. Guess so. Thanks. Acked-by: Serge Hallyn <[email protected]> > That is why I placed this test-and-set snippet while holding > tasklist_lock (write): it ensure that the child has whatever > the parent has, because prepare_descendants() uses the same > lock. > > Oren. > > > > >> + ckpt_ctx_get(p->checkpoint_ctx); > >> + } > >> +#endif > >> /* > >> * The task hasn't been attached yet, so its cpus_allowed mask will > >> * not be changed, nor will its assigned CPU. > >> -- > >> 1.6.0.4 > >> > >> _______________________________________________ > >> Containers mailing list > >> [email protected] > >> https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Containers mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
