From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 265EA17E for ; Fri, 17 Jan 2025 00:03:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737072207; cv=none; b=BZnzNrklG0CaeuQIS4vwgluAwh7XIbQPOCN35Wm0EuJpYbrIgnF2hYZ9h7fE6fwe18l5D7nUGGXAjrtxkHwMKfjdP2+/7PFzU9HFPLOwjsiE+Itq+XUP6qoKVH1E9T06R9yZldZxWEvmmCn0vhF0L94iuPmL8NzdWqNRaxM+dW0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737072207; c=relaxed/simple; bh=Sm5CwBk1eZN8ldSlC2aPnwsbZKE2Xma2hUIyyK9U7I4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=YBkHvrpqwxgtS3UG/FaEhKthb7XRnfPDKZoIc8trN5CSk4LW1qUdRm6XiYcC3wb9x9NStwJv+6RoULu0ZlPNtUBLjUZ58TlH7iIbPLhVmQwLCoraqhUL0TehR7RrL7nRroZS06hjsbR9Xd8iWJFQ4CUZkfbp+2XzfUWWepziS0k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=ETq9QY/A; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="ETq9QY/A" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=NiteQQxvHzUaZoSdHjwr6yXzKKvhc0ixYuN2l0VE28c=; b=ETq9QY/AQ1MnJPF466PGLQlPro uWMthMD8fu4IkEBaSTYvzFC71anWuEloaJ5UTo2S/sih1Eg5bj0U7YROH2oCqwKrQKKxwpqZzzTw3 4e5NXMcgLv3l9uU5UmCyC4gRb4w6/6zC7ji6XdPLf6EQsOQwGyOlciWVN8fMkmLznN26NtsemONL5 0O5VKCN5CUGIGpONtqutBTTzEz8QEpZPzH+jUvks9KO0iwApzknbdYQoHInNNbwqCSBYFXkZcSWjh PM9L5vn/clwqEQxvS0od7OCZ79/Xt4fY1o2hIvhA4Wfk2qOoMd70Q2ochnugjxUoxPHxXQk/5dRRy Do7joypw==; Received: from 77-249-17-89.cable.dynamic.v4.ziggo.nl ([77.249.17.89] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.98 #2 (Red Hat Linux)) id 1tYZpV-00000004PFt-3z7q; Fri, 17 Jan 2025 00:03:18 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id AE1E8300777; Fri, 17 Jan 2025 01:03:16 +0100 (CET) Date: Fri, 17 Jan 2025 01:03:16 +0100 From: Peter Zijlstra To: Ravi Bangoria Cc: "mingo@kernel.org" , "lucas.demarchi@intel.com" , "linux-kernel@vger.kernel.org" , "willy@infradead.org" , "acme@kernel.org" , "namhyung@kernel.org" , "mark.rutland@arm.com" , "alexander.shishkin@linux.intel.com" , "jolsa@kernel.org" , "irogers@google.com" , "adrian.hunter@intel.com" , "kan.liang@linux.intel.com" Subject: Re: [PATCH 19/19] perf: Make perf_pmu_unregister() useable Message-ID: <20250117000316.GB33629@noisy.programming.kicks-ass.net> References: <20241104133909.669111662@infradead.org> <20241104135519.715883982@infradead.org> <20241217091216.GK35539@noisy.programming.kicks-ass.net> <20241217115219.GH12500@noisy.programming.kicks-ass.net> <8c31f7bd-871d-4a38-ad15-a16a116e1f39@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Fri, Jan 03, 2025 at 09:54:09AM +0530, Ravi Bangoria wrote: > Hi Peter, > > Sorry for the delay. Was on vacation. Yeah, me too :-) > Both of these are incorrect. They just reduce the race window, doesn't > actually solve the race. Anyway, I could spot few other races: > > 1) A race between event creation and perf_pmu_unregister(). Any event > create code path (perf_event_open(), perf_event_create_kernel_counter() > and inherit_event()) allocates event with perf_event_alloc() which adds > an event to the pmu->events list. However, the event is still immature, > for ex, event->ctx is still NULL. In the mean time, perf_pmu_unregister() > finds this event and tries to detach it. > > perf_event_open() perf_pmu_unregister() > event = perf_event_alloc() pmu_detach_event(event) > list_add(&event->pmu_list, &pmu->events); perf_event_ctx_lock(event) > /* perf_event_ctx_lock_nested(ctx) > * event->ctx is NULL. ctx = READ_ONCE(event->ctx); /* event->ctx is NULL */ > */ if (!refcount_inc_not_zero(&ctx->refcount)) { /* Crash */ > perf_install_in_context(ctx, event); Ah, that puts the lie to the guard(srcu) comment there, doesn't it :/ So the intent was for that SRCU section to cover the creation, so that perf_pmu_unregister() can take out the pmu to avoid creating more events, then srcu-sync to wait on all in-progress creation and then go detach everything. I suppose the simplest thing here is to grow that SRCU section. > 2) A race with perf_event_release_kernel(). perf_event_release_kernel() > prepares a separate "free_list" of all children events under ctx->mutex > and event->child_mutex. However, the "free_list" uses the same > "event->child_list" for entries. OTOH, perf_pmu_unregister() ultimately > calls __perf_remove_from_context() with DETACH_CHILD, which checks if > the event being removed is a child event, and if so, it will try to > detach the child from parent using list_del_init(&event->child_list); > i.e. two code path doing list_del on the same list entry. > > perf_event_release_kernel() perf_pmu_unregister() > /* Move children events to free_list */ ... > list_for_each_entry_safe(child, tmp, &free_list, child_list) { perf_remove_from_context() /* with DETACH_CHILD */ > ... __perf_remove_from_context() > list_del(&child->child_list); perf_child_detach() > list_del_init(&event->child_list); Bah, I had figured it was taken care of, because perf_event_exit_event() has a similar race. I'll try and figure out what to do there. > 3) A WARN(), not a race. perf_pmu_unregister() increments event->refcount > before detaching the event. If perf_pmu_unregister() picks up a child > event, perf_event_exit_event() called through perf_pmu_unregister() > will try to free it. Since event->refcount would be 2, free_event() > will trigger a WARN(). > > perf_pmu_unregister() > event = pmu_get_event() /* event->refcount => 2 */ > ... > perf_event_exit_event() > if (parent_event) { /* true, because `event` is a child */ > free_event(event); > if (WARN(atomic_long_cmpxchg(&event->refcount, 1, 0) != 1, > "unexpected event refcount: %ld; ptr=%p\n", > atomic_long_read(&event->refcount), event)) I'll make that something like: if (revoke) put_event(event); else free_event(event); or so. > 4) A race with perf_event_set_bpf_prog(). perf_event_set_bpf_prog() might > be in process of setting event->prog, where as perf_pmu_unregister(), > which internally calls perf_event_free_bpf_prog(), will clear the > event->prog pointer. > > perf_pmu_unregister() perf_event_set_bpf_prog() > ... perf_event_set_bpf_handler() > perf_event_free_bpf_prog() event->prog = prog; > event->prog = NULL; > > I've yet to inspect other code paths, so there might be more races. Weird, that should be serialized by perf_event_ctx_lock(), both __pmu_detach_event() and _perf_ioctl() are called under that. Thanks for going over this!