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*/ );
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Alexander Shishkin <[email protected]>
---
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
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 <[email protected]>
> ---->8----
> From c79f31b42aaf85f3a924af9218794b3bc8b79892 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> ---
> 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
>
>
Mark Rutland <[email protected]> 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 <[email protected]>
Thanks,
--
Alex
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 <[email protected]>
>
is this patch going to make it upstream? It's a fairly major correctness
bug with perf_event_open().
Vince
* Vince Weaver <[email protected]> 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 <[email protected]>
> >
>
> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Alexander Shishkin <[email protected]>
Tested-by: Vince Weaver <[email protected]>
(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(),
On Sat, Jan 18, 2020 at 07:05:29PM +0100, Ingo Molnar wrote:
>
> * Vince Weaver <[email protected]> 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 <[email protected]>
> > >
> >
> > 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