From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751484AbbFKIcP (ORCPT ); Thu, 11 Jun 2015 04:32:15 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:40908 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750825AbbFKIcK convert rfc822-to-8bit (ORCPT ); Thu, 11 Jun 2015 04:32:10 -0400 Message-ID: <1434011521.1495.71.camel@twins> Subject: Re: [patch] inherited events not signalling parent on overflow From: Peter Zijlstra To: Vince Weaver Cc: Ingo Molnar , linux-kernel@vger.kernel.org, eranian@google.com, Paul Mackerras , Arnaldo Carvalho de Melo Date: Thu, 11 Jun 2015 10:32:01 +0200 In-Reply-To: References: <20150529063650.GA22568@gmail.com> Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2015-06-11 at 00:30 -0400, Vince Weaver wrote: > On Fri, 29 May 2015, Ingo Molnar wrote: > > > * Vince Weaver 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 > > > > > > 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); }