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