* [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 related [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 related [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 related [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).