All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
@ 2014-09-26 21:06 Sylvain 'ythier' Hitier
  2014-09-27 18:07 ` Oleg Nesterov
  0 siblings, 1 reply; 8+ messages in thread
From: Sylvain 'ythier' Hitier @ 2014-09-26 21:06 UTC (permalink / raw)
  To: linux-kernel, Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra

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!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-26 21:06 [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task() Sylvain 'ythier' Hitier
@ 2014-09-27 18:07 ` Oleg Nesterov
  2014-09-29 10:12   ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2014-09-27 18:07 UTC (permalink / raw)
  To: Sylvain 'ythier' Hitier
  Cc: linux-kernel, Andrew Morton, Ingo Molnar, Peter Zijlstra

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-27 18:07 ` Oleg Nesterov
@ 2014-09-29 10:12   ` Peter Zijlstra
  2014-09-29 12:07     ` Ingo Molnar
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Peter Zijlstra @ 2014-09-29 10:12 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Sylvain 'ythier' Hitier, linux-kernel, Andrew Morton,
	Ingo Molnar

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:

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-29 10:12   ` Peter Zijlstra
@ 2014-09-29 12:07     ` Ingo Molnar
  2014-09-29 14:00       ` Peter Zijlstra
  2014-09-29 22:28     ` Oleg Nesterov
  2014-10-03  5:27     ` [tip:perf/urgent] perf: Fix perf bug in fork() tip-bot for Peter Zijlstra
  2 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2014-09-29 12:07 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Oleg Nesterov, Sylvain 'ythier' Hitier, linux-kernel,
	Andrew Morton, Vince Weaver


* 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-29 12:07     ` Ingo Molnar
@ 2014-09-29 14:00       ` Peter Zijlstra
  2014-10-01 22:44         ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2014-09-29 14:00 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Oleg Nesterov, Sylvain 'ythier' Hitier, linux-kernel,
	Andrew Morton, Vince Weaver

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-29 10:12   ` Peter Zijlstra
  2014-09-29 12:07     ` Ingo Molnar
@ 2014-09-29 22:28     ` Oleg Nesterov
  2014-10-03  5:27     ` [tip:perf/urgent] perf: Fix perf bug in fork() tip-bot for Peter Zijlstra
  2 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2014-09-29 22:28 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Sylvain 'ythier' Hitier, linux-kernel, Andrew Morton,
	Ingo Molnar

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

Yes, thanks, I believ this is correct.

Oleg.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task()
  2014-09-29 14:00       ` Peter Zijlstra
@ 2014-10-01 22:44         ` Andrew Morton
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2014-10-01 22:44 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Oleg Nesterov, Sylvain 'ythier' Hitier,
	linux-kernel, Vince Weaver

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..


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [tip:perf/urgent] perf: Fix perf bug in fork()
  2014-09-29 10:12   ` Peter Zijlstra
  2014-09-29 12:07     ` Ingo Molnar
  2014-09-29 22:28     ` Oleg Nesterov
@ 2014-10-03  5:27     ` tip-bot for Peter Zijlstra
  2 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Peter Zijlstra @ 2014-10-03  5:27 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, torvalds, peterz, acme, atomlin, riel, akpm,
	sylvain.hitier, tglx, oleg, vdavydov, rientjes, linux-kernel,
	hpa, paulus, daeseok.youn, stable, keescook

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:

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-10-03  5:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26 21:06 [PATCH] fork.c: copy_process(): fix cleanup WRT perf_event_free_task() Sylvain 'ythier' Hitier
2014-09-27 18:07 ` Oleg Nesterov
2014-09-29 10:12   ` Peter Zijlstra
2014-09-29 12:07     ` Ingo Molnar
2014-09-29 14:00       ` Peter Zijlstra
2014-10-01 22:44         ` Andrew Morton
2014-09-29 22:28     ` Oleg Nesterov
2014-10-03  5:27     ` [tip:perf/urgent] perf: Fix perf bug in fork() tip-bot for Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.