Re: [patch] inherited events not signalling parent on overflow

[Peter Zijlstra]

On Thu, 2015-06-11 at 00:30 -0400, Vince Weaver wrote:
> On Fri, 29 May 2015, Ingo Molnar wrote:
>  
> > * Vince Weaver <vincent.weaver@xxxxxxxxx> 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 <vincent.weaver@xxxxxxxxx>
> > > 
> > > 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);
 	}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/