* [perf] perf_event_open() sometimes returning 0
@ 2020-01-02 18:55 Vince Weaver
2020-01-02 19:22 ` Vince Weaver
0 siblings, 1 reply; 8+ messages in thread
From: Vince Weaver @ 2020-01-02 18:55 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim
Hello
so I was tracking down some odd behavior in the perf_fuzzer which turns
out to be because perf_even_open() sometimes returns 0 (indicating a file
descriptor of 0) even though as far as I can tell stdin is still open.
Before I waste too much time trying to track this down, is this a known
issue?
Some sample strace output:
perf_event_open({type=PERF_TYPE_RAW, size=0x78 /* PERF_ATTR_SIZE_??? */, config=0x1313, ...}, 0, 3, -1, PERF_FLAG_FD_NO_GROUP) = 0
perf_event_open({type=PERF_TYPE_SOFTWARE, size=0x78 /* PERF_ATTR_SIZE_??? */, config=PERF_COUNT_SW_DUMMY, ...}, -1, 3, -1, PERF_FLAG_FD_NO_GROUP|PERF_FLAG_FD_OUTPUT) = 0
perf_event_open({type=PERF_TYPE_SOFTWARE, size=0x78 /* PERF_ATTR_SIZE_??? */, config=PERF_COUNT_SW_PAGE_FAULTS_MIN, ...}, 158266, 2, -1, PERF_FLAG_FD_CLOEXEC) = 0
perf_event_open({type=PERF_TYPE_HW_CACHE, size=0x78 /* PERF_ATTR_SIZE_??? */, config=PERF_COUNT_HW_CACHE_DTLB|PERF_COUNT_HW_CACHE_OP_READ<<8|PERF_COUNT_HW_CACHE_RESULT_MISS<<16, ...}, 0, 4, -1, PERF_FLAG_FD_NO_GROUP|PERF_FLAG_FD_CLOEXEC) = 0
perf_event_open({type=PERF_TYPE_RAW, size=0x78 /* PERF_ATTR_SIZE_??? */, config=0, ...}, -1, 0, -1, PERF_FLAG_FD_OUTPUT|PERF_FLAG_FD_CLOEXEC) = 0
On my Haswell system (running current git) I can reproduce things with a
single call:
memset(&pe[0],0,sizeof(struct perf_event_attr));
pe[0].type=PERF_TYPE_RAW;
pe[0].size=120;
pe[0].config=0x0ULL;
pe[0].sample_period=0x4777c3ULL;
pe[0].sample_type=PERF_SAMPLE_STREAM_ID; /* 200 */
pe[0].read_format=PERF_FORMAT_TOTAL_TIME_RUNNING|PERF_FORMAT_ID|PERF_FORMAT_GROUP; /* e */
pe[0].inherit=1;
pe[0].exclude_hv=1;
pe[0].exclude_idle=1;
pe[0].enable_on_exec=1;
pe[0].watermark=1;
pe[0].precise_ip=0; /* arbitrary skid */
pe[0].mmap_data=1;
pe[0].exclude_guest=1;
pe[0].exclude_callchain_kernel=1;
pe[0].mmap2=1;
pe[0].comm_exec=1;
pe[0].context_switch=1;
pe[0].bpf_event=1;
pe[0].wakeup_watermark=47545;
pe[0].bp_type=HW_BREAKPOINT_EMPTY;
pe[0].branch_sample_type=PERF_SAMPLE_BRANCH_KERNEL|PERF_SAMPLE_BRANCH_ANY_RETURN|PERF_SAMPLE_BRANCH_COND|0x800ULL;
pe[0].sample_regs_user=42ULL;
pe[0].sample_stack_user=0xfffffffd;
pe[0].aux_watermark=25443;
pe[0].aux_sample_size=8192;
fd[0]=perf_event_open(&pe[0],
-1, /* current thread */
0, /* Only cpu 0 */
-1, /* New Group Leader */
PERF_FLAG_FD_OUTPUT|PERF_FLAG_FD_CLOEXEC /*a*/ );
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [perf] perf_event_open() sometimes returning 0
2020-01-02 18:55 [perf] perf_event_open() sometimes returning 0 Vince Weaver
@ 2020-01-02 19:22 ` Vince Weaver
2020-01-06 12:03 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Mark Rutland
0 siblings, 1 reply; 8+ messages in thread
From: Vince Weaver @ 2020-01-02 19:22 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim
On Thu, 2 Jan 2020, Vince Weaver wrote:
> so I was tracking down some odd behavior in the perf_fuzzer which turns
> out to be because perf_even_open() sometimes returns 0 (indicating a file
> descriptor of 0) even though as far as I can tell stdin is still open.
error is triggered if aux_sample_size has non-zero value.
seems to be this line in kernel/events/core.c:
if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
goto err_locked;
(note, err is never set)
Vince
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0)
2020-01-02 19:22 ` Vince Weaver
@ 2020-01-06 12:03 ` Mark Rutland
2020-01-06 18:09 ` Vince Weaver
2020-01-07 7:39 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Alexander Shishkin
0 siblings, 2 replies; 8+ messages in thread
From: Mark Rutland @ 2020-01-06 12:03 UTC (permalink / raw)
To: Vince Weaver, Alexander Shishkin, Peter Zijlstra
Cc: linux-kernel, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Namhyung Kim
On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
> On Thu, 2 Jan 2020, Vince Weaver wrote:
>
> > so I was tracking down some odd behavior in the perf_fuzzer which turns
> > out to be because perf_even_open() sometimes returns 0 (indicating a file
> > descriptor of 0) even though as far as I can tell stdin is still open.
Yikes.
> error is triggered if aux_sample_size has non-zero value.
>
> seems to be this line in kernel/events/core.c:
>
>
> if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
> goto err_locked;
>
>
> (note, err is never set)
Looks like that was introduced in commit:
ab43762ef010967e ("perf: Allow normal events to output AUX data")
I guess we should return -EINVAL in this case.
Vince, does the below (untested) patch work for you?
Thanks,
Mark.
---->8----
From c79f31b42aaf85f3a924af9218794b3bc8b79892 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Mon, 6 Jan 2020 11:51:06 +0000
Subject: [PATCH] perf: correctly handle failed perf_get_aux_event()
Vince reports a worrying issue:
| so I was tracking down some odd behavior in the perf_fuzzer which turns
| out to be because perf_even_open() sometimes returns 0 (indicating a file
| descriptor of 0) even though as far as I can tell stdin is still open.
... and further the cause:
| error is triggered if aux_sample_size has non-zero value.
|
| seems to be this line in kernel/events/core.c:
|
| if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
| goto err_locked;
|
| (note, err is never set)
This seems to be a thinko in commit:
ab43762ef010967e ("perf: Allow normal events to output AUX data")
... and we should probably return -EINVAL here, as this should only
happen when the new event is mis-configured or does not have a
compatible aux_event group leader.
Fixes: ab43762ef010967e ("perf: Allow normal events to output AUX data")
Reported-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
kernel/events/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a1f8bde19b56..2173c23c25b4 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11465,8 +11465,10 @@ SYSCALL_DEFINE5(perf_event_open,
}
}
- if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
+ if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader)) {
+ err = -EINVAL;
goto err_locked;
+ }
/*
* Must be under the same ctx::mutex as perf_install_in_context(),
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0)
2020-01-06 12:03 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Mark Rutland
@ 2020-01-06 18:09 ` Vince Weaver
2020-01-16 16:45 ` Vince Weaver
2020-01-07 7:39 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Alexander Shishkin
1 sibling, 1 reply; 8+ messages in thread
From: Vince Weaver @ 2020-01-06 18:09 UTC (permalink / raw)
To: Mark Rutland
Cc: Vince Weaver, Alexander Shishkin, Peter Zijlstra, linux-kernel,
Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim
On Mon, 6 Jan 2020, Mark Rutland wrote:
> On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
> > On Thu, 2 Jan 2020, Vince Weaver wrote:
> >
> Vince, does the below (untested) patch work for you?
yes, this patch fixes things for me.
Tested-by: Vince Weaver <vincent.weaver@maine.edu>
> ---->8----
> From c79f31b42aaf85f3a924af9218794b3bc8b79892 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Mon, 6 Jan 2020 11:51:06 +0000
> Subject: [PATCH] perf: correctly handle failed perf_get_aux_event()
>
> Vince reports a worrying issue:
>
> | so I was tracking down some odd behavior in the perf_fuzzer which turns
> | out to be because perf_even_open() sometimes returns 0 (indicating a file
> | descriptor of 0) even though as far as I can tell stdin is still open.
>
> ... and further the cause:
>
> | error is triggered if aux_sample_size has non-zero value.
> |
> | seems to be this line in kernel/events/core.c:
> |
> | if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
> | goto err_locked;
> |
> | (note, err is never set)
>
> This seems to be a thinko in commit:
>
> ab43762ef010967e ("perf: Allow normal events to output AUX data")
>
> ... and we should probably return -EINVAL here, as this should only
> happen when the new event is mis-configured or does not have a
> compatible aux_event group leader.
>
> Fixes: ab43762ef010967e ("perf: Allow normal events to output AUX data")
> Reported-by: Vince Weaver <vincent.weaver@maine.edu>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
> kernel/events/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a1f8bde19b56..2173c23c25b4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11465,8 +11465,10 @@ SYSCALL_DEFINE5(perf_event_open,
> }
> }
>
> - if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
> + if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader)) {
> + err = -EINVAL;
> goto err_locked;
> + }
>
> /*
> * Must be under the same ctx::mutex as perf_install_in_context(),
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0)
2020-01-06 18:09 ` Vince Weaver
@ 2020-01-16 16:45 ` Vince Weaver
2020-01-18 18:05 ` [PATCH, v5.4] perf: Correctly handle failed perf_get_aux_event() Ingo Molnar
0 siblings, 1 reply; 8+ messages in thread
From: Vince Weaver @ 2020-01-16 16:45 UTC (permalink / raw)
To: Vince Weaver
Cc: Mark Rutland, Alexander Shishkin, Peter Zijlstra, linux-kernel,
Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim
On Mon, 6 Jan 2020, Vince Weaver wrote:
> On Mon, 6 Jan 2020, Mark Rutland wrote:
>
> > On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
> > > On Thu, 2 Jan 2020, Vince Weaver wrote:
> > >
> > Vince, does the below (untested) patch work for you?
>
>
> yes, this patch fixes things for me.
>
> Tested-by: Vince Weaver <vincent.weaver@maine.edu>
>
is this patch going to make it upstream? It's a fairly major correctness
bug with perf_event_open().
Vince
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH, v5.4] perf: Correctly handle failed perf_get_aux_event()
2020-01-16 16:45 ` Vince Weaver
@ 2020-01-18 18:05 ` Ingo Molnar
2020-01-19 13:28 ` Greg Kroah-Hartman
0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2020-01-18 18:05 UTC (permalink / raw)
To: Vince Weaver
Cc: Mark Rutland, Alexander Shishkin, Peter Zijlstra, linux-kernel,
Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim,
Greg Kroah-Hartman, stable kernel team
* Vince Weaver <vincent.weaver@maine.edu> wrote:
> On Mon, 6 Jan 2020, Vince Weaver wrote:
>
> > On Mon, 6 Jan 2020, Mark Rutland wrote:
> >
> > > On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
> > > > On Thu, 2 Jan 2020, Vince Weaver wrote:
> > > >
> > > Vince, does the below (untested) patch work for you?
> >
> >
> > yes, this patch fixes things for me.
> >
> > Tested-by: Vince Weaver <vincent.weaver@maine.edu>
> >
>
> is this patch going to make it upstream? It's a fairly major correctness
> bug with perf_event_open().
I just sent it to Linus.
In hindsight this should have been marked Cc: stable for v5.4 - we should
forward it to Greg once Linus has pulled it:
da9ec3d3dd0f: ("perf: Correctly handle failed perf_get_aux_event()")
Note that in the v5.4 cherry-pick there's a conflict due to interaction
with another recent commit - I've attached the ported fix against v5.4,
but have only test built it.
Thanks,
Ingo
==============>
From 703595681c934d2a88a91e8a41f7f63eeb1573e0 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Sat, 18 Jan 2020 19:03:55 +0100
Subject: [PATCH] perf: Correctly handle failed perf_get_aux_event()
Vince reports a worrying issue:
| so I was tracking down some odd behavior in the perf_fuzzer which turns
| out to be because perf_even_open() sometimes returns 0 (indicating a file
| descriptor of 0) even though as far as I can tell stdin is still open.
... and further the cause:
| error is triggered if aux_sample_size has non-zero value.
|
| seems to be this line in kernel/events/core.c:
|
| if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
| goto err_locked;
|
| (note, err is never set)
This seems to be a thinko in commit:
ab43762ef010967e ("perf: Allow normal events to output AUX data")
... and we should probably return -EINVAL here, as this should only
happen when the new event is mis-configured or does not have a
compatible aux_event group leader.
Fixes: ab43762ef010967e ("perf: Allow normal events to output AUX data")
Reported-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Vince Weaver <vincent.weaver@maine.edu>
(cherry picked from commit da9ec3d3dd0f1240a48920be063448a2242dbd90)
---
kernel/events/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 00a014670ed0..291fe3e2165f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11184,8 +11184,10 @@ SYSCALL_DEFINE5(perf_event_open,
}
}
- if (event->attr.aux_output && !perf_get_aux_event(event, group_leader))
+ if (event->attr.aux_output && !perf_get_aux_event(event, group_leader)) {
+ err = -EINVAL;
goto err_locked;
+ }
/*
* Must be under the same ctx::mutex as perf_install_in_context(),
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH, v5.4] perf: Correctly handle failed perf_get_aux_event()
2020-01-18 18:05 ` [PATCH, v5.4] perf: Correctly handle failed perf_get_aux_event() Ingo Molnar
@ 2020-01-19 13:28 ` Greg Kroah-Hartman
0 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-01-19 13:28 UTC (permalink / raw)
To: Ingo Molnar
Cc: Vince Weaver, Mark Rutland, Alexander Shishkin, Peter Zijlstra,
linux-kernel, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Namhyung Kim, stable kernel team
On Sat, Jan 18, 2020 at 07:05:29PM +0100, Ingo Molnar wrote:
>
> * Vince Weaver <vincent.weaver@maine.edu> wrote:
>
> > On Mon, 6 Jan 2020, Vince Weaver wrote:
> >
> > > On Mon, 6 Jan 2020, Mark Rutland wrote:
> > >
> > > > On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
> > > > > On Thu, 2 Jan 2020, Vince Weaver wrote:
> > > > >
> > > > Vince, does the below (untested) patch work for you?
> > >
> > >
> > > yes, this patch fixes things for me.
> > >
> > > Tested-by: Vince Weaver <vincent.weaver@maine.edu>
> > >
> >
> > is this patch going to make it upstream? It's a fairly major correctness
> > bug with perf_event_open().
>
> I just sent it to Linus.
>
> In hindsight this should have been marked Cc: stable for v5.4 - we should
> forward it to Greg once Linus has pulled it:
>
> da9ec3d3dd0f: ("perf: Correctly handle failed perf_get_aux_event()")
>
>
> Note that in the v5.4 cherry-pick there's a conflict due to interaction
> with another recent commit - I've attached the ported fix against v5.4,
> but have only test built it.
Thanks for the backport, now queued up.
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0)
2020-01-06 12:03 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Mark Rutland
2020-01-06 18:09 ` Vince Weaver
@ 2020-01-07 7:39 ` Alexander Shishkin
1 sibling, 0 replies; 8+ messages in thread
From: Alexander Shishkin @ 2020-01-07 7:39 UTC (permalink / raw)
To: Mark Rutland, Vince Weaver, Peter Zijlstra
Cc: linux-kernel, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Namhyung Kim, alexander.shishkin
Mark Rutland <mark.rutland@arm.com> writes:
> On Thu, Jan 02, 2020 at 02:22:47PM -0500, Vince Weaver wrote:
>> On Thu, 2 Jan 2020, Vince Weaver wrote:
>>
>> > so I was tracking down some odd behavior in the perf_fuzzer which turns
>> > out to be because perf_even_open() sometimes returns 0 (indicating a file
>> > descriptor of 0) even though as far as I can tell stdin is still open.
>
> Yikes.
>
>> error is triggered if aux_sample_size has non-zero value.
>>
>> seems to be this line in kernel/events/core.c:
>>
>>
>> if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
>> goto err_locked;
>>
>>
>> (note, err is never set)
>
> Looks like that was introduced in commit:
>
> ab43762ef010967e ("perf: Allow normal events to output AUX data")
>
> I guess we should return -EINVAL in this case.
That's right. Thanks for looking into this!
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a1f8bde19b56..2173c23c25b4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11465,8 +11465,10 @@ SYSCALL_DEFINE5(perf_event_open,
> }
> }
>
> - if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader))
> + if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader)) {
> + err = -EINVAL;
> goto err_locked;
> + }
>
FWIW,
Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Thanks,
--
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-19 13:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-02 18:55 [perf] perf_event_open() sometimes returning 0 Vince Weaver
2020-01-02 19:22 ` Vince Weaver
2020-01-06 12:03 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Mark Rutland
2020-01-06 18:09 ` Vince Weaver
2020-01-16 16:45 ` Vince Weaver
2020-01-18 18:05 ` [PATCH, v5.4] perf: Correctly handle failed perf_get_aux_event() Ingo Molnar
2020-01-19 13:28 ` Greg Kroah-Hartman
2020-01-07 7:39 ` [PATCH] perf: correctly handle failed perf_get_aux_event() (was: Re: [perf] perf_event_open() sometimes returning 0) Alexander Shishkin
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.