From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757356Ab2CZNBB (ORCPT ); Mon, 26 Mar 2012 09:01:01 -0400 Received: from merlin.infradead.org ([205.233.59.134]:52270 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756899Ab2CZNBA convert rfc822-to-8bit (ORCPT ); Mon, 26 Mar 2012 09:01:00 -0400 Message-ID: <1332765676.16159.108.camel@twins> Subject: Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm From: Peter Zijlstra To: Ari Savolainen Cc: Paul Mackerras , Ingo Molnar , Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Stephane Eranian , "Paul E. McKenney" Date: Mon, 26 Mar 2012 14:41:16 +0200 In-Reply-To: References: <1332409996.18960.511.camel@twins> Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Mailer: Evolution 3.2.2- Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2012-03-22 at 13:36 +0200, Ari Savolainen wrote: > 22. maaliskuuta 2012 11.53 Peter Zijlstra kirjoitti: > > On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote: > >> The warning below is printed when executing a command like > >> sudo perf record su - user -c "echo hello" > >> > >> It's fixed by moving the call of perf_event_comm to be protected > >> by the task lock. > > > > That seems like a rather poor solution since it increases the lock hold > > time for no explained reason. > > > >> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage! > > > >> [] lockdep_rcu_suspicious+0xe5/0x100 > >> [] perf_event_comm+0x37a/0x4d0 > > > > So where exactly is this, perf_event_comm_event() takes rcu_read_lock() > > so I presume its before that. > > I think the warning comes from this source-level call path: > > perf_event_comm -> > perf_event_enable_on_exec -> > perf_cgroup_sched_out -> > perf_cgroup_from_task -> > task_subsys_state -> > task_subsys_state_check > > It seems there that path does not take rcu_read_lock(). Where should > rcu_read_lock/unlock be added? In perf_group_sched_out around the > calls of perf_cgroup_from_task? Like this: Ah, ok. So IIRC this too is not needed. As the comment near perf_cgroup_from_task() says, we hold explicit references to the cgroup. Ideally we'd come up with a better validation condition but all variants I could come up with make the code ugly and might actually generate worse code, the current true simply shuts it up. Stephane any thoughts? --- kernel/events/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index a6a9ec4..e423261 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -240,7 +240,7 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx, static inline struct perf_cgroup * perf_cgroup_from_task(struct task_struct *task) { - return container_of(task_subsys_state(task, perf_subsys_id), + return container_of(task_subsys_state_check(task, perf_subsys_id, true), struct perf_cgroup, css); }