From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757485AbcASUGL (ORCPT ); Tue, 19 Jan 2016 15:06:11 -0500 Received: from casper.infradead.org ([85.118.1.10]:38669 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754656AbcASUGC (ORCPT ); Tue, 19 Jan 2016 15:06:02 -0500 Date: Tue, 19 Jan 2016 21:05:58 +0100 From: Peter Zijlstra To: Alexander Shishkin Cc: Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Jiri Olsa , alexei.starovoitov@gmail.com Subject: Re: [PATCH v2] perf: Synchronously cleanup child events Message-ID: <20160119200558.GC6357@twins.programming.kicks-ass.net> References: <20160118144410.GS6357@twins.programming.kicks-ass.net> <1453216354-9282-1-git-send-email-alexander.shishkin@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1453216354-9282-1-git-send-email-alexander.shishkin@linux.intel.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 19, 2016 at 05:12:34PM +0200, Alexander Shishkin wrote: > +static void __put_event(struct perf_event *event) > { > struct perf_event_context *ctx; > > if (!is_kernel_event(event)) > perf_remove_from_owner(event); > > +int perf_event_release_kernel(struct perf_event *event) > { > + struct perf_event *child, *tmp; > + LIST_HEAD(child_list); > > + if (!is_kernel_event(event)) > + perf_remove_from_owner(event); > > + event->owner = NULL; > > +retry: > + /* > + * If this is the last reference, we're done here, otherwise > + * we must have raced with inherit_event(), in which case, repeat > + */ > + if (!put_event_last(event)) > + goto retry; > > + return 0; > +} So I think there's a number of problems still :-( I all starts with having two perf_remove_from_owner() calls (as I mentioned on IRC), this doesn't make sense. I think the moment you close the file and userspace looses control over it, we should drop the owner bit, which is exactly the one remove_from_owner in perf_release(). If, for some magical reason, the event lives on after that (and we'll get to that), it should live on owner-less. Now, assume someone has such a magical reference, then our put_event_last() goto again loop will never terminate, this seems like a bad thing. The most obvious place that generates such magical references would be the bpf arraymap doing perf_event_get() on things. There are a few other places that take temp references (perf_mmap_close), but those are 'short' lived and while ugly will not cause massive grief. The BPF one OTOH is a real problem here. And looking at the BPF stuff, that code seems to assume perf_event_kernel_release() := put_event(), so this patch breaks that too. Alexei, is there a reason the arraymap stuff needs a perf event ref as opposed to a file ref? I'm forever a little confused on how perf<->bpf works.