From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758234AbcATHGs (ORCPT ); Wed, 20 Jan 2016 02:06:48 -0500 Received: from mga09.intel.com ([134.134.136.24]:12544 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757443AbcATHGk (ORCPT ); Wed, 20 Jan 2016 02:06:40 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,320,1449561600"; d="scan'208";a="894420545" From: Alexander Shishkin To: Peter Zijlstra 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 In-Reply-To: <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> <20160119200558.GC6357@twins.programming.kicks-ass.net> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Wed, 20 Jan 2016 09:04:28 +0200 Message-ID: <87io2owx37.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > 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(). Fair enough. > 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. We won't get to perf_release() before we're done with perf_mmap_close(), so that one's not really a problem. > 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. Yes, that one's very much an api abuse, it should clearly be using get_file()/fput() instead. Now that the code is there already, there's a slight chance that changing this will have userspace running into the fd limit and cause a regression. As a workaround we can probably introduce yet another magial owner to allow a userspace event to be 'stolen'. Since bpf is the only user of perf_event_get(), this can be somewhat easily arranged. Regards, -- Alex