On Fri, 29 May 2015, Ingo Molnar wrote:
> * Vince Weaver <[email protected]> wrote:
> > If we inherit events, we inherit the signal state but not the fasync state, so
> > overflows in inherited children will never trigger the signal handler.
> >
> > Signed-off-by: Vince Weaver <[email protected]>
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 1a3bf48..7df4cf5 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> > child_event->overflow_handler_context
> > = parent_event->overflow_handler_context;
> >
> > + child_event->fasync = parent_event->fasync;
> > +
> > /*
> > * Precalculate sample_data sizes
> > */
This patch, while it does work well enough to enable self-monitored-sampling
of OpenMP programs, falls apart under fuzzing.
You end up with lots of
[25592.289382] kill_fasync: bad magic number in fasync_struct!
warnings and eventually I managed to lock up the system that way.
> Btw., if we do this (sensible looking) ABI fix, could we make it a new attr bit,
> so that PAPI can essentially query the kernel whether this gets propagated
> properly?
>
> That way old kernels 'intentionally' don't inherit the fasync handler and tooling
> can deterministically make use of this 'feature' on new kernels.
That would be useful. PAPI typically has to guess about feature support
(for workarounds) by using the kernel version number as a reference, and
this falls apart on kernels such as RHEL which backport a lot of
perf_event fixes/functionality.
Vince
On Thu, 2015-06-11 at 00:30 -0400, Vince Weaver wrote:
> On Fri, 29 May 2015, Ingo Molnar wrote:
>
> > * Vince Weaver <[email protected]> wrote:
>
> > > If we inherit events, we inherit the signal state but not the fasync state, so
> > > overflows in inherited children will never trigger the signal handler.
> > >
> > > Signed-off-by: Vince Weaver <[email protected]>
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 1a3bf48..7df4cf5 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -8626,6 +8630,8 @@ inherit_event(struct perf_event *parent_event,
> > > child_event->overflow_handler_context
> > > = parent_event->overflow_handler_context;
> > >
> > > + child_event->fasync = parent_event->fasync;
> > > +
> > > /*
> > > * Precalculate sample_data sizes
> > > */
>
> This patch, while it does work well enough to enable self-monitored-sampling
> of OpenMP programs, falls apart under fuzzing.
>
> You end up with lots of
>
> [25592.289382] kill_fasync: bad magic number in fasync_struct!
>
> warnings and eventually I managed to lock up the system that way.
Right, I had a peek earlier at how fasync worked but came away confused.
Today I seem to have had better luck. Installing fasync allocates memory
and sets filp->f_flags |= FASYNC, which upon the demise of the file
descriptor ensures the allocation is freed.
Now for perf, we can have the events stick around for a while after the
original FD is dead because of references from child events. With the
above patch these events would still have a pointer into this free'd
fasync. This is bad.
A further problem with the patch is that if the parent changes its
fasync state the children might lag and again have pointers into dead
space.
All is not lost though; does something like the below work?
---
kernel/events/core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1e33b9141f03..057f599ae0dc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4742,12 +4742,20 @@ static const struct file_operations perf_fops = {
* to user-space before waking everybody up.
*/
+static inline struct fasync_struct **perf_event_fasync(struct perf_event *event)
+{
+ /* only the parent has fasync state */
+ if (event->parent)
+ event = event->parent;
+ return &event->fasync;
+}
+
void perf_event_wakeup(struct perf_event *event)
{
ring_buffer_wakeup(event);
if (event->pending_kill) {
- kill_fasync(&event->fasync, SIGIO, event->pending_kill);
+ kill_fasync(perf_event_fasync(event), SIGIO, event->pending_kill);
event->pending_kill = 0;
}
}
@@ -6126,7 +6134,7 @@ static int __perf_event_overflow(struct perf_event *event,
else
perf_event_output(event, data, regs);
- if (event->fasync && event->pending_kill) {
+ if (*perf_event_fasync(event) && event->pending_kill) {
event->pending_wakeup = 1;
irq_work_queue(&event->pending);
}
On Thu, 11 Jun 2015, Peter Zijlstra wrote:
> Right, I had a peek earlier at how fasync worked but came away confused.
>
> Today I seem to have had better luck. Installing fasync allocates memory
> and sets filp->f_flags |= FASYNC, which upon the demise of the file
> descriptor ensures the allocation is freed.
>
> Now for perf, we can have the events stick around for a while after the
> original FD is dead because of references from child events. With the
> above patch these events would still have a pointer into this free'd
> fasync. This is bad.
>
> A further problem with the patch is that if the parent changes its
> fasync state the children might lag and again have pointers into dead
> space.
>
> All is not lost though; does something like the below work?
I had meant to reply to this earlier but maybe I forgot.
I've been running with this patch for a month now and haven't had
problems, and it fixes the issue of inherited signals. So it no one else
has issues with the patch it would be nice if it could be pushed upstream.
Thanks,
Vince
On Fri, Jul 31, 2015 at 12:42:06AM -0400, Vince Weaver wrote:
> On Thu, 11 Jun 2015, Peter Zijlstra wrote:
>
> > Right, I had a peek earlier at how fasync worked but came away confused.
> >
> > Today I seem to have had better luck. Installing fasync allocates memory
> > and sets filp->f_flags |= FASYNC, which upon the demise of the file
> > descriptor ensures the allocation is freed.
> >
> > Now for perf, we can have the events stick around for a while after the
> > original FD is dead because of references from child events. With the
> > above patch these events would still have a pointer into this free'd
> > fasync. This is bad.
> >
> > A further problem with the patch is that if the parent changes its
> > fasync state the children might lag and again have pointers into dead
> > space.
> >
> > All is not lost though; does something like the below work?
>
> I had meant to reply to this earlier but maybe I forgot.
>
> I've been running with this patch for a month now and haven't had
> problems, and it fixes the issue of inherited signals. So it no one else
> has issues with the patch it would be nice if it could be pushed upstream.
Great, thanks for testing. I'll go queue it.
Commit-ID: fed66e2cdd4f127a43fd11b8d92a99bdd429528c
Gitweb: http://git.kernel.org/tip/fed66e2cdd4f127a43fd11b8d92a99bdd429528c
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 11 Jun 2015 10:32:01 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 4 Aug 2015 09:57:52 +0200
perf: Fix fasync handling on inherited events
Vince reported that the fasync signal stuff doesn't work proper for
inherited events. So fix that.
Installing fasync allocates memory and sets filp->f_flags |= FASYNC,
which upon the demise of the file descriptor ensures the allocation is
freed and state is updated.
Now for perf, we can have the events stick around for a while after the
original FD is dead because of references from child events. So we
cannot copy the fasync pointer around. We can however consistently use
the parent's fasync, as that will be updated.
Reported-and-Tested-by: Vince Weaver <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Cc: <[email protected]>
Cc: Arnaldo Carvalho deMelo <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/1434011521.1495.71.camel@twins
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/events/core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 10d076b..072b8a6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4740,12 +4740,20 @@ static const struct file_operations perf_fops = {
* to user-space before waking everybody up.
*/
+static inline struct fasync_struct **perf_event_fasync(struct perf_event *event)
+{
+ /* only the parent has fasync state */
+ if (event->parent)
+ event = event->parent;
+ return &event->fasync;
+}
+
void perf_event_wakeup(struct perf_event *event)
{
ring_buffer_wakeup(event);
if (event->pending_kill) {
- kill_fasync(&event->fasync, SIGIO, event->pending_kill);
+ kill_fasync(perf_event_fasync(event), SIGIO, event->pending_kill);
event->pending_kill = 0;
}
}
@@ -6124,7 +6132,7 @@ static int __perf_event_overflow(struct perf_event *event,
else
perf_event_output(event, data, regs);
- if (event->fasync && event->pending_kill) {
+ if (*perf_event_fasync(event) && event->pending_kill) {
event->pending_wakeup = 1;
irq_work_queue(&event->pending);
}