* [PATCH] perf: Fix vmalloc ring buffer free function
@ 2013-02-26 15:34 Jiri Olsa
2013-02-27 13:02 ` Frederic Weisbecker
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2013-02-26 15:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Namhyung Kim, Paul Mackerras, Peter Zijlstra,
Arnaldo Carvalho de Melo
If we allocate perf ring buffer with the size of single page,
we will get memory corruption when releasing it. It's caused
by rb_free_work function (the CONFIG_PERF_USE_VMALLOC option
variant).
For single page sized ring buffer the page_order is -1 (because
nr_pages is 0). This needs to be recognized in the rb_free_work
function and set 'nr' to 0 in this case, so only the user page
gets freed.
Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
kernel/events/ring_buffer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 23cb34f..21159fb 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -336,7 +336,8 @@ static void rb_free_work(struct work_struct *work)
int i, nr;
rb = container_of(work, struct ring_buffer, work);
- nr = 1 << page_order(rb);
+ /* -1 if there's only user page */
+ nr = page_order(rb) == -1 ? 0 : 1 << page_order(rb);
base = rb->user_page;
for (i = 0; i < nr + 1; i++)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf: Fix vmalloc ring buffer free function
2013-02-26 15:34 [PATCH] perf: Fix vmalloc ring buffer free function Jiri Olsa
@ 2013-02-27 13:02 ` Frederic Weisbecker
2013-02-27 13:14 ` Jiri Olsa
0 siblings, 1 reply; 3+ messages in thread
From: Frederic Weisbecker @ 2013-02-27 13:02 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Corey Ashford, Ingo Molnar, Namhyung Kim,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
2013/2/26 Jiri Olsa <jolsa@redhat.com>:
> If we allocate perf ring buffer with the size of single page,
> we will get memory corruption when releasing it. It's caused
> by rb_free_work function (the CONFIG_PERF_USE_VMALLOC option
> variant).
>
> For single page sized ring buffer the page_order is -1 (because
> nr_pages is 0). This needs to be recognized in the rb_free_work
> function and set 'nr' to 0 in this case, so only the user page
> gets freed.
>
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
> kernel/events/ring_buffer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 23cb34f..21159fb 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -336,7 +336,8 @@ static void rb_free_work(struct work_struct *work)
> int i, nr;
>
> rb = container_of(work, struct ring_buffer, work);
> - nr = 1 << page_order(rb);
> + /* -1 if there's only user page */
> + nr = page_order(rb) == -1 ? 0 : 1 << page_order(rb);
So we allow for 0 sized ring buffer? I'm not entirely convinced this
is a good idea. Besides this above case, perf_output_begin() looks
dangerous in the case of CONFIG_PERF_USE_VMALLOC due to page_order()
being -1. At least rb->nr_pages sould be zero in that case.
> base = rb->user_page;
> for (i = 0; i < nr + 1; i++)
> --
> 1.7.11.7
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf: Fix vmalloc ring buffer free function
2013-02-27 13:02 ` Frederic Weisbecker
@ 2013-02-27 13:14 ` Jiri Olsa
0 siblings, 0 replies; 3+ messages in thread
From: Jiri Olsa @ 2013-02-27 13:14 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: linux-kernel, Corey Ashford, Ingo Molnar, Namhyung Kim,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
On Wed, Feb 27, 2013 at 02:02:39PM +0100, Frederic Weisbecker wrote:
> 2013/2/26 Jiri Olsa <jolsa@redhat.com>:
> > If we allocate perf ring buffer with the size of single page,
> > we will get memory corruption when releasing it. It's caused
> > by rb_free_work function (the CONFIG_PERF_USE_VMALLOC option
> > variant).
> >
> > For single page sized ring buffer the page_order is -1 (because
> > nr_pages is 0). This needs to be recognized in the rb_free_work
> > function and set 'nr' to 0 in this case, so only the user page
> > gets freed.
> >
> > Reported-by: Jan Stancek <jstancek@redhat.com>
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> > kernel/events/ring_buffer.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > index 23cb34f..21159fb 100644
> > --- a/kernel/events/ring_buffer.c
> > +++ b/kernel/events/ring_buffer.c
> > @@ -336,7 +336,8 @@ static void rb_free_work(struct work_struct *work)
> > int i, nr;
> >
> > rb = container_of(work, struct ring_buffer, work);
> > - nr = 1 << page_order(rb);
> > + /* -1 if there's only user page */
> > + nr = page_order(rb) == -1 ? 0 : 1 << page_order(rb);
>
> So we allow for 0 sized ring buffer? I'm not entirely convinced this
> is a good idea. Besides this above case, perf_output_begin() looks
We actually have test perf case 'tools/perf/tests/rdpmc.c' using
just the user page for counter id and other stuff..
> dangerous in the case of CONFIG_PERF_USE_VMALLOC due to page_order()
> being -1. At least rb->nr_pages sould be zero in that case.
right, seems to suffer the same way as rb_free_work.. will check
thanks,
jirka
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-27 13:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-26 15:34 [PATCH] perf: Fix vmalloc ring buffer free function Jiri Olsa
2013-02-27 13:02 ` Frederic Weisbecker
2013-02-27 13:14 ` Jiri Olsa
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.