[PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Sylvain 'ythier' Hitier]

Date:   Fri Sep 26 20:56:07 2014 +0000

fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

Currently, in copy_process(), a failure of either sched_fork() or
perf_event_init_task() does trigger perf_event_free_task()!

Moreover, the label bad_fork_cleanup_policy does more than what its name
implies, because it includes perf_event_free_task()!

Let's explain the change with a grASCIIcally-enhanced kind-of-diff which
provides an adequate context.
                                        // Read vertically this column
                                        //   |  |  |  |  |  |  |  |  |
                                        //   v  v  v  v  v  v  v  v  v
 {
//SNIP//
    if (clone_flags & CLONE_THREAD)
        threadgroup_change_begin(current);
//SNIP//
 #ifdef CONFIG_NUMA
    p->mempolicy = mpol_dup(p->mempolicy);
    if (IS_ERR(p->mempolicy)) {
//SNIP//
        goto bad_fork_cleanup_threadgroup_lock;
    }
 #endif
//SNIP//
    retval = sched_fork(clone_flags, p);
    if (retval)
//                                      // mustn't perf_event_free_task()
        goto bad_fork_cleanup_policy;
    retval = perf_event_init_task(p);
    if (retval)
//                                      // mustn't perf_event_free_task()
        goto bad_fork_cleanup_policy;
    retval = audit_alloc(p);
    if (retval)
//                                      // must perf_event_free_task()
//                                      @@ Hence change this way:
-       goto bad_fork_cleanup_policy;
+       goto bad_fork_cleanup_perf;
//SNIP//
 bad_fork_cleanup_audit:
    audit_free(p);
//                                      // let's clean perf up
//                                      @@ Hence change this way:
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
    perf_event_free_task(p);
//                                      // no (longer) need to clean perf up
//                                      @@ Hence change this way:
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
    mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock:
 #endif
    if (clone_flags & CLONE_THREAD)
        threadgroup_change_end(current);
//SNIP//
 }

Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
---
 kernel/fork.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 0cf9cdb..a91e47d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1360,7 +1360,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		goto bad_fork_cleanup_policy;
 	retval = audit_alloc(p);
 	if (retval)
-		goto bad_fork_cleanup_policy;
+		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
 	retval = copy_semundo(clone_flags, p);
@@ -1566,8 +1566,9 @@ bad_fork_cleanup_semundo:
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
 	perf_event_free_task(p);
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
 	mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock:

Regards,
Sylvain "ythier" Hitier

-- 
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Oleg Nesterov]

On 09/26, Sylvain 'ythier' Hitier wrote:
>
>     retval = sched_fork(clone_flags, p);
>     if (retval)
> //                                      // mustn't perf_event_free_task()
>         goto bad_fork_cleanup_policy;

Agreed, this is wrong. Good catch.

but, unless I missed something,

>     retval = perf_event_init_task(p);
>     if (retval)
> //                                      // mustn't perf_event_free_task()
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

this is not right and thus the patch is not right too.

Suppose that perf_event_init_task() -> perf_event_init_context(ctxn => 0)
succeeds and then perf_event_init_context(ctxn => 1) fails, we need
perf_event_free_task() to cleanup ->perf_event_ctxp[0].

So if perf_event_init_task() fails, we still need "goto bad_fork_cleanup_perf".

No?

Or, probably better, we need to change perf_event_init_context() to call
perf_event_free_task() on failure.

Or. We can simply move memset(child->perf_event_ctxp, 0, ...) from
perf_event_init_context() up. This reminds that we really need to cleanup
copy_process(), in particular I think it asks for the new copy_xxx() helper
which should do misc simple initializations which can't fail.

What do you think?

Oleg.

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Peter Zijlstra]

On Sat, Sep 27, 2014 at 08:07:25PM +0200, Oleg Nesterov wrote:
> On 09/26, Sylvain 'ythier' Hitier wrote:
> >
> >     retval = sched_fork(clone_flags, p);
> >     if (retval)
> > //                                      // mustn't perf_event_free_task()
> >         goto bad_fork_cleanup_policy;
> 
> Agreed, this is wrong. Good catch.
> 
> but, unless I missed something,

Ah, indeed. It was meant to be a no-op there, but its before we do that
memset, so its still the inherited values, and we don't want to clean
those up I think.

> >     retval = perf_event_init_task(p);
> >     if (retval)
> > //                                      // mustn't perf_event_free_task()
>                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> this is not right and thus the patch is not right too.

Agreed

> Suppose that perf_event_init_task() -> perf_event_init_context(ctxn => 0)
> succeeds and then perf_event_init_context(ctxn => 1) fails, we need
> perf_event_free_task() to cleanup ->perf_event_ctxp[0].
> 
> So if perf_event_init_task() fails, we still need "goto bad_fork_cleanup_perf".
> 
> No?

Yep

> Or, probably better, we need to change perf_event_init_context() to call
> perf_event_free_task() on failure.
> 
> Or. We can simply move memset(child->perf_event_ctxp, 0, ...) from
> perf_event_init_context() up. This reminds that we really need to cleanup
> copy_process(), in particular I think it asks for the new copy_xxx() helper
> which should do misc simple initializations which can't fail.
> 
> What do you think?

I prefer the former, as the latter scatters the perf specific bits over
more places. Something like so then?



---
Subject: perf: Fix perf bug in fork()

Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by
calling perf_event_free_task() when failing sched_fork() we will not yet
have done the memset() on ->perf_event_ctxp[] and will therefore try and
'free' the inherited contexts, which are still in use by the parent
process. This is bad..

Suggested-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a232b40..4a0dbb2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8078,8 +8078,10 @@ int perf_event_init_task(struct task_struct *child)
 
 	for_each_task_context_nr(ctxn) {
 		ret = perf_event_init_context(child, ctxn);
-		if (ret)
+		if (ret) {
+			perf_event_free_task(child);
 			return ret;
+		}
 	}
 
 	return 0;
diff --git a/kernel/fork.c b/kernel/fork.c
index ad64248..b6cc3f2 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1367,7 +1367,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		goto bad_fork_cleanup_policy;
 	retval = audit_alloc(p);
 	if (retval)
-		goto bad_fork_cleanup_policy;
+		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
 	retval = copy_semundo(clone_flags, p);
@@ -1573,8 +1573,9 @@ bad_fork_cleanup_semundo:
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
 	perf_event_free_task(p);
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
 	mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock:

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Subject: perf: Fix perf bug in fork()
> 
> Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by
> calling perf_event_free_task() when failing sched_fork() we will not yet
> have done the memset() on ->perf_event_ctxp[] and will therefore try and
> 'free' the inherited contexts, which are still in use by the parent
> process. This is bad..
> 
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Reported-by: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Could this fix a couple of fuzzer triggered perf crashes perhaps?

Thanks,

	Ingo

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Peter Zijlstra]

On Mon, Sep 29, 2014 at 02:07:22PM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Subject: perf: Fix perf bug in fork()
> > 
> > Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by
> > calling perf_event_free_task() when failing sched_fork() we will not yet
> > have done the memset() on ->perf_event_ctxp[] and will therefore try and
> > 'free' the inherited contexts, which are still in use by the parent
> > process. This is bad..
> > 
> > Suggested-by: Oleg Nesterov <oleg@redhat.com>
> > Reported-by: Oleg Nesterov <oleg@redhat.com>
> > Reported-by: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Could this fix a couple of fuzzer triggered perf crashes perhaps?

It could indeed I suppose.. you never know what paths those fuzzers
manage to hit.

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Andrew Morton]

On Mon, 29 Sep 2014 16:00:48 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 29, 2014 at 02:07:22PM +0200, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Subject: perf: Fix perf bug in fork()
> > > 
> > > Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by
> > > calling perf_event_free_task() when failing sched_fork() we will not yet
> > > have done the memset() on ->perf_event_ctxp[] and will therefore try and
> > > 'free' the inherited contexts, which are still in use by the parent
> > > process. This is bad..
> > > 
> > > Suggested-by: Oleg Nesterov <oleg@redhat.com>
> > > Reported-by: Oleg Nesterov <oleg@redhat.com>
> > > Reported-by: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > Could this fix a couple of fuzzer triggered perf crashes perhaps?
> 
> It could indeed I suppose.. you never know what paths those fuzzers
> manage to hit.

The patch isn't in linux-next and didn't cc stable.  I think I'll
squirt it Linuswards later this week unless someone stops me..

Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()

[Oleg Nesterov]

On 09/29, Peter Zijlstra wrote:
>
> Something like so then?

Yes, thanks, I believ this is correct.

Oleg.

[tip:perf/urgent] perf: Fix perf bug in fork()

[tip-bot for Peter Zijlstra]

Commit-ID:  9c2b9d30e28559a78c9e431cdd7f2c6bf5a9ee67
Gitweb:     http://git.kernel.org/tip/9c2b9d30e28559a78c9e431cdd7f2c6bf5a9ee67
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 29 Sep 2014 12:12:01 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 3 Oct 2014 05:41:08 +0200

perf: Fix perf bug in fork()

Oleg noticed that a cleanup by Sylvain actually uncovered a bug; by
calling perf_event_free_task() when failing sched_fork() we will not yet
have done the memset() on ->perf_event_ctxp[] and will therefore try and
'free' the inherited contexts, which are still in use by the parent
process.

This is bad and might explain some outstanding fuzzer failures ...

Suggested-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Aaron Tomlin <atomlin@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Daeseok Youn <daeseok.youn@gmail.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Vladimir Davydov <vdavydov@parallels.com>
Cc: <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/20140929101201.GE5430@worktop
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 4 +++-
 kernel/fork.c        | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index afdd9e1..658f232 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7956,8 +7956,10 @@ int perf_event_init_task(struct task_struct *child)
 
 	for_each_task_context_nr(ctxn) {
 		ret = perf_event_init_context(child, ctxn);
-		if (ret)
+		if (ret) {
+			perf_event_free_task(child);
 			return ret;
+		}
 	}
 
 	return 0;
diff --git a/kernel/fork.c b/kernel/fork.c
index 0cf9cdb..a91e47d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1360,7 +1360,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		goto bad_fork_cleanup_policy;
 	retval = audit_alloc(p);
 	if (retval)
-		goto bad_fork_cleanup_policy;
+		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
 	retval = copy_semundo(clone_flags, p);
@@ -1566,8 +1566,9 @@ bad_fork_cleanup_semundo:
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
-bad_fork_cleanup_policy:
+bad_fork_cleanup_perf:
 	perf_event_free_task(p);
+bad_fork_cleanup_policy:
 #ifdef CONFIG_NUMA
 	mpol_put(p->mempolicy);
 bad_fork_cleanup_threadgroup_lock: