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

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

[Oren Laadan]

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;
+		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

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

[Serge E. Hallyn]

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

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

[Serge E. Hallyn]

Quoting 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?

BTW since all I'm doing is nit-picking, I obviously agree with
the patch on the whole :)

There is no way for the task to be forked into a traced state
(without the parent being traced) right?  And, is the fact that
ctx->nr_total may end up less than the total number of active
tasks a problem at all?

thanks,
-serge

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

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting 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?
> 
> BTW since all I'm doing is nit-picking, I obviously agree with
> the patch on the whole :)
> 
> There is no way for the task to be forked into a traced state
> (without the parent being traced) right?  And, is the fact that

Not that I'm aware of.

> ctx->nr_total may end up less than the total number of active
> tasks a problem at all?

That's ok. A restart will fail if there are not enough tasks,
and will not-succeed (hang until interrupted) if there are
"too many" tasks.

The above is a protection against malicious use of sys_restart(),
not a fix to a correct use through (bug-less!) restart.c  :p

Oren.

> 
> thanks,
> -serge

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

[Oren Laadan]

Serge E. Hallyn wrote:
> 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?

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.

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

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

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> > 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?
> 
> 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 <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>


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