* [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted @ 2009-12-30 11:28 Liming Wang 2009-12-30 12:28 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Liming Wang @ 2009-12-30 11:28 UTC (permalink / raw) To: Ingo Molnar Cc: Frederic Weisbecker, Paul Mackerras, Thomas Gleixner, Peter Zijlstra, linux-kernel, Liming Wang If the parent has no entry in group_list, child_ctx will not be allocated, which will lead dereference of a NULL child_ctx. Signed-off-by: Liming Wang <liming.wang@windriver.com> --- kernel/perf_event.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 5b987b4..3664c4b 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -5126,6 +5126,8 @@ int perf_event_init_task(struct task_struct *child) */ mutex_lock(&parent_ctx->mutex); + if (list_empty(&parent_ctx->group_list)) + goto exit; /* * We dont have to disable NMIs - we are only looking at * the list, not manipulating it: -- 1.6.0.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted 2009-12-30 11:28 [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted Liming Wang @ 2009-12-30 12:28 ` Peter Zijlstra 2009-12-30 14:36 ` Wang Liming 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2009-12-30 12:28 UTC (permalink / raw) To: Liming Wang Cc: Ingo Molnar, Frederic Weisbecker, Paul Mackerras, Thomas Gleixner, linux-kernel On Wed, 2009-12-30 at 19:28 +0800, Liming Wang wrote: > If the parent has no entry in group_list, child_ctx will not be > allocated, which will lead dereference of a NULL child_ctx. That changelog sucks. Best I can make of it is that there is a race where the parent gets his context instantiated and we manage to get the mutex before the other thread manages to add the first event. Then we observe parent_event_ctx but have an empty list. Is that it? In that case, would it not be better to change the if (inherited_all) condition to if (inherited_all && child_ctx) ? > Signed-off-by: Liming Wang <liming.wang@windriver.com> > --- > kernel/perf_event.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/kernel/perf_event.c b/kernel/perf_event.c > index 5b987b4..3664c4b 100644 > --- a/kernel/perf_event.c > +++ b/kernel/perf_event.c > @@ -5126,6 +5126,8 @@ int perf_event_init_task(struct task_struct *child) > */ > mutex_lock(&parent_ctx->mutex); > > + if (list_empty(&parent_ctx->group_list)) > + goto exit; > /* > * We dont have to disable NMIs - we are only looking at > * the list, not manipulating it: ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted 2009-12-30 12:28 ` Peter Zijlstra @ 2009-12-30 14:36 ` Wang Liming 2009-12-30 15:08 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Wang Liming @ 2009-12-30 14:36 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Frederic Weisbecker, Paul Mackerras, Thomas Gleixner, linux-kernel Peter Zijlstra wrote: > On Wed, 2009-12-30 at 19:28 +0800, Liming Wang wrote: >> If the parent has no entry in group_list, child_ctx will not be >> allocated, which will lead dereference of a NULL child_ctx. > > That changelog sucks. Sorry the changelog is too simple. > > Best I can make of it is that there is a race where the parent gets his > context instantiated and we manage to get the mutex before the other > thread manages to add the first event. > > Then we observe parent_event_ctx but have an empty list. > > Is that it? I didn't find this case. In my case, if I perf record a existing process with "--pid" and finish record, and if later the recorded process forks a process, the condition will occur. Liming Wang > > In that case, would it not be better to change the if (inherited_all) > condition to if (inherited_all && child_ctx) ? > >> Signed-off-by: Liming Wang <liming.wang@windriver.com> >> --- >> kernel/perf_event.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/kernel/perf_event.c b/kernel/perf_event.c >> index 5b987b4..3664c4b 100644 >> --- a/kernel/perf_event.c >> +++ b/kernel/perf_event.c >> @@ -5126,6 +5126,8 @@ int perf_event_init_task(struct task_struct *child) >> */ >> mutex_lock(&parent_ctx->mutex); >> >> + if (list_empty(&parent_ctx->group_list)) >> + goto exit; >> /* >> * We dont have to disable NMIs - we are only looking at >> * the list, not manipulating it: > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted 2009-12-30 14:36 ` Wang Liming @ 2009-12-30 15:08 ` Peter Zijlstra 2009-12-30 15:02 ` Wang Liming 2009-12-31 14:30 ` [tip:perf/urgent] perf: Fix NULL deref in inheritance code tip-bot for Peter Zijlstra 0 siblings, 2 replies; 6+ messages in thread From: Peter Zijlstra @ 2009-12-30 15:08 UTC (permalink / raw) To: Wang Liming Cc: Ingo Molnar, Frederic Weisbecker, Paul Mackerras, Thomas Gleixner, linux-kernel On Wed, 2009-12-30 at 22:36 +0800, Wang Liming wrote: > > Best I can make of it is that there is a race where the parent gets his > > context instantiated and we manage to get the mutex before the other > > thread manages to add the first event. > > > > Then we observe parent_event_ctx but have an empty list. > > > > Is that it? > I didn't find this case. > In my case, if I perf record a existing process with "--pid" and finish record, > and if later the recorded process forks a process, the condition will occur. Ah, right, that will lead to the same state, since closing the last counter will not remove the context. Does the below also fix your issue? --- Subject: perf: Fix NULL deref in inheritance code From: Peter Zijlstra <a.p.zijlstra@chello.nl> Date: Wed Dec 30 16:00:35 CET 2009 Liming found a NULL deref when a task has a perf context but no counters when it forks. This can occur in two cases, a race during construction where the fork hits after installing the context but before the first counter gets inserted, or more reproducably, a fork after the last counter is closed (which leaves the context around). CC: stable@kernel.org Reported-by: Wang Liming <liming.wang@windriver.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> --- kernel/perf_event.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) Index: linux-2.6/kernel/perf_event.c =================================================================== --- linux-2.6.orig/kernel/perf_event.c +++ linux-2.6/kernel/perf_event.c @@ -5149,7 +5149,7 @@ int perf_event_init_task(struct task_str GFP_KERNEL); if (!child_ctx) { ret = -ENOMEM; - goto exit; + break; } __perf_event_init_context(child_ctx, child); @@ -5165,7 +5165,7 @@ int perf_event_init_task(struct task_str } } - if (inherited_all) { + if (child_ctx && inherited_all) { /* * Mark the child context as a clone of the parent * context, or of whatever the parent is a clone of. @@ -5185,7 +5185,6 @@ int perf_event_init_task(struct task_str get_ctx(child_ctx->parent_ctx); } -exit: mutex_unlock(&parent_ctx->mutex); perf_unpin_context(parent_ctx); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted 2009-12-30 15:08 ` Peter Zijlstra @ 2009-12-30 15:02 ` Wang Liming 2009-12-31 14:30 ` [tip:perf/urgent] perf: Fix NULL deref in inheritance code tip-bot for Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: Wang Liming @ 2009-12-30 15:02 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Frederic Weisbecker, Paul Mackerras, Thomas Gleixner, linux-kernel Peter Zijlstra wrote: > On Wed, 2009-12-30 at 22:36 +0800, Wang Liming wrote: >>> Best I can make of it is that there is a race where the parent gets his >>> context instantiated and we manage to get the mutex before the other >>> thread manages to add the first event. >>> >>> Then we observe parent_event_ctx but have an empty list. >>> >>> Is that it? >> I didn't find this case. >> In my case, if I perf record a existing process with "--pid" and finish record, >> and if later the recorded process forks a process, the condition will occur. > > Ah, right, that will lead to the same state, since closing the last > counter will not remove the context. > > Does the below also fix your issue? Yes, it's OK to me. Thanks a lot! Liming Wang > > --- > Subject: perf: Fix NULL deref in inheritance code > From: Peter Zijlstra <a.p.zijlstra@chello.nl> > Date: Wed Dec 30 16:00:35 CET 2009 > > Liming found a NULL deref when a task has a perf context but no counters > when it forks. > > This can occur in two cases, a race during construction where the fork hits > after installing the context but before the first counter gets inserted, or > more reproducably, a fork after the last counter is closed (which leaves the > context around). > > CC: stable@kernel.org > Reported-by: Wang Liming <liming.wang@windriver.com> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > kernel/perf_event.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > Index: linux-2.6/kernel/perf_event.c > =================================================================== > --- linux-2.6.orig/kernel/perf_event.c > +++ linux-2.6/kernel/perf_event.c > @@ -5149,7 +5149,7 @@ int perf_event_init_task(struct task_str > GFP_KERNEL); > if (!child_ctx) { > ret = -ENOMEM; > - goto exit; > + break; > } > > __perf_event_init_context(child_ctx, child); > @@ -5165,7 +5165,7 @@ int perf_event_init_task(struct task_str > } > } > > - if (inherited_all) { > + if (child_ctx && inherited_all) { > /* > * Mark the child context as a clone of the parent > * context, or of whatever the parent is a clone of. > @@ -5185,7 +5185,6 @@ int perf_event_init_task(struct task_str > get_ctx(child_ctx->parent_ctx); > } > > -exit: > mutex_unlock(&parent_ctx->mutex); > > perf_unpin_context(parent_ctx); > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] perf: Fix NULL deref in inheritance code 2009-12-30 15:08 ` Peter Zijlstra 2009-12-30 15:02 ` Wang Liming @ 2009-12-31 14:30 ` tip-bot for Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: tip-bot for Peter Zijlstra @ 2009-12-31 14:30 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, paulus, hpa, mingo, a.p.zijlstra, fweisbec, stable, tglx, liming.wang, mingo Commit-ID: 05cbaa2853cdfc255fdd04e65a82bfe9208c4e52 Gitweb: http://git.kernel.org/tip/05cbaa2853cdfc255fdd04e65a82bfe9208c4e52 Author: Peter Zijlstra <a.p.zijlstra@chello.nl> AuthorDate: Wed, 30 Dec 2009 16:00:35 +0100 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Thu, 31 Dec 2009 13:11:31 +0100 perf: Fix NULL deref in inheritance code Liming found a NULL deref when a task has a perf context but no counters when it forks. This can occur in two cases, a race during construction where the fork hits after installing the context but before the first counter gets inserted, or more reproducably, a fork after the last counter is closed (which leaves the context around). Reported-by: Wang Liming <liming.wang@windriver.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Paul Mackerras <paulus@samba.org> CC: <stable@kernel.org> LKML-Reference: <1262185684.7135.222.camel@laptop> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- kernel/perf_event.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 03cc061..58ed1da 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -5148,7 +5148,7 @@ int perf_event_init_task(struct task_struct *child) GFP_KERNEL); if (!child_ctx) { ret = -ENOMEM; - goto exit; + break; } __perf_event_init_context(child_ctx, child); @@ -5164,7 +5164,7 @@ int perf_event_init_task(struct task_struct *child) } } - if (inherited_all) { + if (child_ctx && inherited_all) { /* * Mark the child context as a clone of the parent * context, or of whatever the parent is a clone of. @@ -5184,7 +5184,6 @@ int perf_event_init_task(struct task_struct *child) get_ctx(child_ctx->parent_ctx); } -exit: mutex_unlock(&parent_ctx->mutex); perf_unpin_context(parent_ctx); ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-12-31 14:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-30 11:28 [PATCH 1/1] perf_event: add check for group_list if the parent isn't counted Liming Wang 2009-12-30 12:28 ` Peter Zijlstra 2009-12-30 14:36 ` Wang Liming 2009-12-30 15:08 ` Peter Zijlstra 2009-12-30 15:02 ` Wang Liming 2009-12-31 14:30 ` [tip:perf/urgent] perf: Fix NULL deref in inheritance code 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