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