Re: [PATCH] c/r: fix race of prepare_descendant() with an ongoing fork()

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> From: Oren Laadan <orenl-RdfvBDnrOiyVc3sceRu5cw@public.gmane.org>
> 
> 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 <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> ---
>  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?

> +		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
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers