perf_evlist__mmap_consume() uses perf_mmap__empty() to judge whether
perf_mmap is empty and can be released. But the result is inverted so
fix it.
Signed-off-by: He Kuang <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 82bf224..76ef7ee 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -695,7 +695,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
static bool perf_mmap__empty(struct perf_mmap *md)
{
- return perf_mmap__read_head(md) != md->prev;
+ return perf_mmap__read_head(md) == md->prev;
}
static void perf_evlist__mmap_get(struct perf_evlist *evlist, int idx)
--
2.3.3.220.g9ab698f
After perf_evlist__filter_pollfd() filters out fds and releases
perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
then perf_evlist__mmap_consume() will do the final unmap. In this
condition, perf_evlist__mmap_read() will crash by referencing invalid
mmap. Put refcnt check before use.
Can be reproduced as following:
$ perf trace --duration 1.0 ls
...
perf: Segmentation fault
Obtained 14 stack frames.
./perf(dump_stack+0x2e) [0x503c2d]
./perf(sighandler_dump_stack+0x2e)
[0x503d0c]
/lib64/libc.so.6(+0x34df0) [0x7f5fd9a4adf0]
./perf() [0x4a8fda]
./perf(perf_evlist__mmap_read+0x56)
[0x4aae93]
./perf() [0x470b28]
./perf(cmd_trace+0xada) [0x4727bd]
./perf() [0x49c4f4]
./perf() [0x49c74d]
./perf() [0x49c899]
./perf(main+0x23b)
[0x49cbfa]
/lib64/libc.so.6(__libc_start_main+0xf5)
[0x7f5fd9a377b5]
./perf() [0x434ea5]
[(nil)]
Signed-off-by: He Kuang <[email protected]>
---
tools/perf/util/evlist.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 76ef7ee..9d36433 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -634,11 +634,18 @@ static struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
{
struct perf_mmap *md = &evlist->mmap[idx];
- unsigned int head = perf_mmap__read_head(md);
- unsigned int old = md->prev;
- unsigned char *data = md->base + page_size;
+ unsigned int head;
+ unsigned int old;
+ unsigned char *data;
union perf_event *event = NULL;
+ if (md == NULL || md->refcnt == 0)
+ return NULL;
+
+ head = perf_mmap__read_head(md);
+ old = md->prev;
+ data = md->base + page_size;
+
if (evlist->overwrite) {
/*
* If we're further behind than half the buffer, there's a chance
--
2.3.3.220.g9ab698f
Em Tue, Apr 07, 2015 at 05:31:10PM +0800, He Kuang escreveu:
> perf_evlist__mmap_consume() uses perf_mmap__empty() to judge whether
> perf_mmap is empty and can be released. But the result is inverted so
> fix it.
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> @@ -695,7 +695,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
>
> static bool perf_mmap__empty(struct perf_mmap *md)
> {
> - return perf_mmap__read_head(md) != md->prev;
> + return perf_mmap__read_head(md) == md->prev;
> }
>
> static void perf_evlist__mmap_get(struct perf_evlist *evlist, int idx)
Argh, thanks, good spotting, applying...
- Arnaldo
Em Tue, Apr 07, 2015 at 05:31:11PM +0800, He Kuang escreveu:
> After perf_evlist__filter_pollfd() filters out fds and releases
> perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
> then perf_evlist__mmap_consume() will do the final unmap. In this
> condition, perf_evlist__mmap_read() will crash by referencing invalid
> mmap. Put refcnt check before use.
>
> Can be reproduced as following:
After applying 1/2 in this series and trying to reproduce I couldn't, it
works, looking at the code...
Let me get my head around this, idea was that after all fds associated
with a mmap would be closed, i.e. the perf_mmap->refcnt hits zero, then
we would have to drain whatever was left in the mmap, but looking again
that doesn't look like that is what is doing, becaue in filter_pollfd we
will munmap it before being able to "drain" it, as all mmaps were
closed, thus filter_pollfd returned zero...
Reading on, thanks for the patch!
- Arnaldo
> $ perf trace --duration 1.0 ls
> ...
> perf: Segmentation fault
> Obtained 14 stack frames.
> ./perf(dump_stack+0x2e) [0x503c2d]
> ./perf(sighandler_dump_stack+0x2e)
> [0x503d0c]
> /lib64/libc.so.6(+0x34df0) [0x7f5fd9a4adf0]
> ./perf() [0x4a8fda]
> ./perf(perf_evlist__mmap_read+0x56)
> [0x4aae93]
> ./perf() [0x470b28]
> ./perf(cmd_trace+0xada) [0x4727bd]
> ./perf() [0x49c4f4]
> ./perf() [0x49c74d]
> ./perf() [0x49c899]
> ./perf(main+0x23b)
> [0x49cbfa]
> /lib64/libc.so.6(__libc_start_main+0xf5)
> [0x7f5fd9a377b5]
> ./perf() [0x434ea5]
> [(nil)]
>
> Signed-off-by: He Kuang <[email protected]>
> ---
> tools/perf/util/evlist.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 76ef7ee..9d36433 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -634,11 +634,18 @@ static struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
> union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> {
> struct perf_mmap *md = &evlist->mmap[idx];
> - unsigned int head = perf_mmap__read_head(md);
> - unsigned int old = md->prev;
> - unsigned char *data = md->base + page_size;
> + unsigned int head;
> + unsigned int old;
> + unsigned char *data;
> union perf_event *event = NULL;
>
> + if (md == NULL || md->refcnt == 0)
> + return NULL;
> +
> + head = perf_mmap__read_head(md);
> + old = md->prev;
> + data = md->base + page_size;
> +
> if (evlist->overwrite) {
> /*
> * If we're further behind than half the buffer, there's a chance
> --
> 2.3.3.220.g9ab698f
Hi, Arnaldo
On 2015/4/7 20:36, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 07, 2015 at 05:31:11PM +0800, He Kuang escreveu:
>> After perf_evlist__filter_pollfd() filters out fds and releases
>> perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
>> then perf_evlist__mmap_consume() will do the final unmap. In this
>> condition, perf_evlist__mmap_read() will crash by referencing invalid
>> mmap. Put refcnt check before use.
>>
>> Can be reproduced as following:
> After applying 1/2 in this series and trying to reproduce I couldn't, it
> works, looking at the code...
>
> Let me get my head around this, idea was that after all fds associated
> with a mmap would be closed, i.e. the perf_mmap->refcnt hits zero, then
> we would have to drain whatever was left in the mmap, but looking again
> that doesn't look like that is what is doing, becaue in filter_pollfd we
> will munmap it before being able to "drain" it, as all mmaps were
> closed, thus filter_pollfd returned zero...
In function __perf_evlist__mmap(), refcnt is initialized to 2, see commit:
823969860329 ("perf evlist: Refcount mmaps")
After filter_pollfd, perf_mmap->refcnt is 1 not 0.
perf_evlist__filter_pollfd() -- refcnt=1
draining = true
if (perf_evlist__mmap_read() != NULL)
perf_evlist__mmap_consume() -- unmap, refcnt = 0
perf_evlist__mmap_read() -- segfault
else
exit
I noticed that this issue also exists in builtin-record.c, but it
checks before mmap_read():
if (rec->evlist->mmap[i].base) {
if (record__mmap_read(rec, i, draining) != 0) {
So we can either do the check outside
builtin-trace.c:perf_evlist__mmap_read() like what
builtin-record.c do or inside. What's your opinion?
>
> Reading on, thanks for the patch!
>
> - Arnaldo
>
>
>> $ perf trace --duration 1.0 ls
>> ...
>> perf: Segmentation fault
>> Obtained 14 stack frames.
>> ./perf(dump_stack+0x2e) [0x503c2d]
>> ./perf(sighandler_dump_stack+0x2e)
>> [0x503d0c]
>> /lib64/libc.so.6(+0x34df0) [0x7f5fd9a4adf0]
>> ./perf() [0x4a8fda]
>> ./perf(perf_evlist__mmap_read+0x56)
>> [0x4aae93]
>> ./perf() [0x470b28]
>> ./perf(cmd_trace+0xada) [0x4727bd]
>> ./perf() [0x49c4f4]
>> ./perf() [0x49c74d]
>> ./perf() [0x49c899]
>> ./perf(main+0x23b)
>> [0x49cbfa]
>> /lib64/libc.so.6(__libc_start_main+0xf5)
>> [0x7f5fd9a377b5]
>> ./perf() [0x434ea5]
>> [(nil)]
>>
>> Signed-off-by: He Kuang <[email protected]>
>> ---
>> tools/perf/util/evlist.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 76ef7ee..9d36433 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -634,11 +634,18 @@ static struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
>> union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
>> {
>> struct perf_mmap *md = &evlist->mmap[idx];
>> - unsigned int head = perf_mmap__read_head(md);
>> - unsigned int old = md->prev;
>> - unsigned char *data = md->base + page_size;
>> + unsigned int head;
>> + unsigned int old;
>> + unsigned char *data;
>> union perf_event *event = NULL;
>>
>> + if (md == NULL || md->refcnt == 0)
>> + return NULL;
>> +
>> + head = perf_mmap__read_head(md);
>> + old = md->prev;
>> + data = md->base + page_size;
>> +
>> if (evlist->overwrite) {
>> /*
>> * If we're further behind than half the buffer, there's a chance
>> --
>> 2.3.3.220.g9ab698f
>
Commit-ID: 8ea92ceb748535799e3e9f35afb85bdc23bf6d7c
Gitweb: http://git.kernel.org/tip/8ea92ceb748535799e3e9f35afb85bdc23bf6d7c
Author: He Kuang <[email protected]>
AuthorDate: Tue, 7 Apr 2015 17:31:10 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 8 Apr 2015 09:06:58 -0300
perf evlist: Fix inverted logic in perf_mmap__empty
perf_evlist__mmap_consume() uses perf_mmap__empty() to judge whether
perf_mmap is empty and can be released. But the result is inverted so
fix it.
Signed-off-by: He Kuang <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Wang Nan <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 82bf224..76ef7ee 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -695,7 +695,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
static bool perf_mmap__empty(struct perf_mmap *md)
{
- return perf_mmap__read_head(md) != md->prev;
+ return perf_mmap__read_head(md) == md->prev;
}
static void perf_evlist__mmap_get(struct perf_evlist *evlist, int idx)
Hi, Arnaldo
On 2015/4/8 11:15, He Kuang wrote:
> Hi, Arnaldo
> On 2015/4/7 20:36, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Apr 07, 2015 at 05:31:11PM +0800, He Kuang escreveu:
>>> After perf_evlist__filter_pollfd() filters out fds and releases
>>> perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
>>> then perf_evlist__mmap_consume() will do the final unmap. In this
>>> condition, perf_evlist__mmap_read() will crash by referencing invalid
>>> mmap. Put refcnt check before use.
>>>
>>> Can be reproduced as following:
>> After applying 1/2 in this series and trying to reproduce I couldn't, it
>> works, looking at the code...
>>
>> Let me get my head around this, idea was that after all fds associated
>> with a mmap would be closed, i.e. the perf_mmap->refcnt hits zero, then
>> we would have to drain whatever was left in the mmap, but looking again
>> that doesn't look like that is what is doing, becaue in filter_pollfd we
>> will munmap it before being able to "drain" it, as all mmaps were
>> closed, thus filter_pollfd returned zero...
>
> In function __perf_evlist__mmap(), refcnt is initialized to 2, see
> commit:
> 823969860329 ("perf evlist: Refcount mmaps")
>
> After filter_pollfd, perf_mmap->refcnt is 1 not 0.
>
> perf_evlist__filter_pollfd() -- refcnt=1
> draining = true
> if (perf_evlist__mmap_read() != NULL)
> perf_evlist__mmap_consume() -- unmap, refcnt = 0
> perf_evlist__mmap_read() -- segfault
> else
> exit
>
> I noticed that this issue also exists in builtin-record.c, but it
> checks before mmap_read():
>
> if (rec->evlist->mmap[i].base) {
> if (record__mmap_read(rec, i, draining) != 0) {
>
> So we can either do the check outside
> builtin-trace.c:perf_evlist__mmap_read() like what
> builtin-record.c do or inside. What's your opinion?
I found the issue is still there, so ping...
>>
>> Reading on, thanks for the patch!
>>
>> - Arnaldo
>>
>>> $ perf trace --duration 1.0 ls
>>> ...
>>> perf: Segmentation fault
>>> Obtained 14 stack frames.
>>> ./perf(dump_stack+0x2e) [0x503c2d]
>>> ./perf(sighandler_dump_stack+0x2e)
>>> [0x503d0c]
>>> /lib64/libc.so.6(+0x34df0) [0x7f5fd9a4adf0]
>>> ./perf() [0x4a8fda]
>>> ./perf(perf_evlist__mmap_read+0x56)
>>> [0x4aae93]
>>> ./perf() [0x470b28]
>>> ./perf(cmd_trace+0xada) [0x4727bd]
>>> ./perf() [0x49c4f4]
>>> ./perf() [0x49c74d]
>>> ./perf() [0x49c899]
>>> ./perf(main+0x23b)
>>> [0x49cbfa]
>>> /lib64/libc.so.6(__libc_start_main+0xf5)
>>> [0x7f5fd9a377b5]
>>> ./perf() [0x434ea5]
>>> [(nil)]
>>>
>>> Signed-off-by: He Kuang <[email protected]>
>>> ---
>>> tools/perf/util/evlist.c | 13 ++++++++++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index 76ef7ee..9d36433 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -634,11 +634,18 @@ static struct perf_evsel
>>> *perf_evlist__event2evsel(struct perf_evlist *evlist,
>>> union perf_event *perf_evlist__mmap_read(struct perf_evlist
>>> *evlist, int idx)
>>> {
>>> struct perf_mmap *md = &evlist->mmap[idx];
>>> - unsigned int head = perf_mmap__read_head(md);
>>> - unsigned int old = md->prev;
>>> - unsigned char *data = md->base + page_size;
>>> + unsigned int head;
>>> + unsigned int old;
>>> + unsigned char *data;
>>> union perf_event *event = NULL;
>>> + if (md == NULL || md->refcnt == 0)
>>> + return NULL;
>>> +
>>> + head = perf_mmap__read_head(md);
>>> + old = md->prev;
>>> + data = md->base + page_size;
>>> +
>>> if (evlist->overwrite) {
>>> /*
>>> * If we're further behind than half the buffer, there's a
>>> chance
>>> --
>>> 2.3.3.220.g9ab698f
>>
>
>
Em Mon, May 11, 2015 at 08:11:14PM +0800, He Kuang escreveu:
> Hi, Arnaldo
>
> On 2015/4/8 11:15, He Kuang wrote:
> >Hi, Arnaldo
> >On 2015/4/7 20:36, Arnaldo Carvalho de Melo wrote:
> >>Em Tue, Apr 07, 2015 at 05:31:11PM +0800, He Kuang escreveu:
> >>>After perf_evlist__filter_pollfd() filters out fds and releases
> >>>perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
> >>>then perf_evlist__mmap_consume() will do the final unmap. In this
> >>>condition, perf_evlist__mmap_read() will crash by referencing invalid
> >>>mmap. Put refcnt check before use.
> >>>
> >>>Can be reproduced as following:
> >>After applying 1/2 in this series and trying to reproduce I couldn't, it
> >>works, looking at the code...
> >>
> >>Let me get my head around this, idea was that after all fds associated
> >>with a mmap would be closed, i.e. the perf_mmap->refcnt hits zero, then
> >>we would have to drain whatever was left in the mmap, but looking again
> >>that doesn't look like that is what is doing, becaue in filter_pollfd we
> >>will munmap it before being able to "drain" it, as all mmaps were
> >>closed, thus filter_pollfd returned zero...
> >
> >In function __perf_evlist__mmap(), refcnt is initialized to 2, see commit:
> > 823969860329 ("perf evlist: Refcount mmaps")
> >
> >After filter_pollfd, perf_mmap->refcnt is 1 not 0.
> >
> > perf_evlist__filter_pollfd() -- refcnt=1
> > draining = true
> > if (perf_evlist__mmap_read() != NULL)
> > perf_evlist__mmap_consume() -- unmap, refcnt = 0
> > perf_evlist__mmap_read() -- segfault
> >else
> >exit
> >
> >I noticed that this issue also exists in builtin-record.c, but it
> >checks before mmap_read():
> >
> >if (rec->evlist->mmap[i].base) {
> > if (record__mmap_read(rec, i, draining) != 0) {
> >
> >So we can either do the check outside
> >builtin-trace.c:perf_evlist__mmap_read() like what
> >builtin-record.c do or inside. What's your opinion?
>
> I found the issue is still there, so ping...
Right, I noticed it as well sometimes, will apply the bandaid and leave
properly researching it for later.
- Arnaldo
Em Mon, May 11, 2015 at 10:47:34AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, May 11, 2015 at 08:11:14PM +0800, He Kuang escreveu:
> > >So we can either do the check outside
> > >builtin-trace.c:perf_evlist__mmap_read() like what
> > >builtin-record.c do or inside. What's your opinion?
> >
> > I found the issue is still there, so ping...
>
> Right, I noticed it as well sometimes, will apply the bandaid and leave
> properly researching it for later.
Trying to research it now...
- Arnaldo