linux-kernel - Re: [PATCH RFC] perf: fix find_swevent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 13 May 2010 21:03:28 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu,
	a.p.zijlstra@...llo.nl, paulus@...ba.org, acme@...hat.com
Subject: Re: [PATCH RFC] perf: fix find_swevent_head() RCU lockdep splat

On Thu, May 13, 2010 at 11:25:56AM -0700, Paul E. McKenney wrote:
> This commit guesses at the perf_cpu_context locking design and deploys
> an rcu_dereference_check() accordingly.  The design appears to require
> that a given CPU be accessing its own per_cpu_context or that it be
> traversing under RCU protection.
> 
> Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> 
>  perf_event.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index a4fa381..002791c 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -4074,7 +4074,9 @@ find_swevent_head(struct perf_cpu_context *ctx, u64 type, u32 event_id)
>  
>  	hash = swevent_hash(type, event_id);
>  
> -	hlist = rcu_dereference(ctx->swevent_hlist);
> +	hlist = rcu_dereference_check(ctx->swevent_hlist,
> +				      rcu_read_lock_held() ||
> +				      ctx == &__get_cpu_var(perf_cpu_context));
>  	if (!hlist)
>  		return NULL;
>  

Hmm, that's not exactly that. It will always be the ctx of this cpu
but not always under rcu read lock. I mean touching the current cpu
ctx is not inherently safe.

In fact we have two paths:

perf_swevent_enable() gets the hlist and if it is called it means
that this hlist is not supposed to be NULL. If it is, it's a bug.

If we have created a software event, the hlist has been allocated
and perf_swevent_enable() is called later to activate this event.
May be I shouldn't use rcu_dereference() here but a simple dereference.
And the hlist can't be freed under us at this time so we don't need
rcu_read_lock().

OTOH, do_perf_sw_event() can be called anytime so it need this
rcu_read_lock().

On the perf_swevent_enable() path, what prevents the hlist to be
freed under us is the ctx->lock. Because we won't ever remove
an event from its context list outside this lock, and we might only
release the hlist after a software event gets removed from its
context list.

So either we do this:

hlist = rcu_dereference_check(ctx->swevent_hlist,
                              rcu_read_lock_held() ||
                              raw_spin_lock_is_held(&ctx->lock));

or:

hlist = ctx->swevent_hlist;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/