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]

Message-ID: <20100520070121.GA5309@nowhere>
Date:	Thu, 20 May 2010 09:01:24 +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 04:43:27PM -0700, Paul E. McKenney wrote:
> On Thu, May 13, 2010 at 10:59:26PM +0200, Frederic Weisbecker wrote:
> > >  	hash = swevent_hash(type, event_id);
> > >  
> > > +	return &hlist->heads[hash];
> > > +}
> > > +
> > > +static inline struct hlist_head *
> > > +find_swevent_head_rcu(struct perf_cpu_context *ctx, u64 type, u32 event_id)
> > > +{
> > > +	struct swevent_hlist *hlist;
> > > +
> > >  	hlist = rcu_dereference(ctx->swevent_hlist);
> 
> This is appropriate if find_swevent_head_rcu() is always invoked in an
> RCU read-side critical section.  (Which at first glance does appear to
> be the intent, just checking.)



Exactly!


 
> > > -	if (!hlist)
> > > -		return NULL;
> > >  
> > > -	return &hlist->heads[hash];
> > > +	return __find_swevent_head(hlist, type, event_id);
> > > +}
> > > +
> > > +static inline struct hlist_head *
> > > +find_swevent_head(struct perf_cpu_context *ctx, u64 type,
> > > +		  u32 event_id, struct perf_event *event)
> > > +{
> > > +	struct swevent_hlist *hlist;
> > > +
> > > +	hlist = rcu_dereference_check(ctx->swevent_hlist,
> > > +				      lockdep_is_held(&event->ctx->lock));
> 
> This could be invoked with either the event->ctx->lock held or in
> an RCU read-side critical section.  If this is always called with
> the update-side lock held, you can (but don't need to) instead say:
> 
> 	hlist = rcu_dereference_protected(ctx->swevent_hlist,
> 				          lockdep_is_held(&event->ctx->lock));
> 
> This is slightly faster, as it drops the volatile casts.  In many cases,
> you won't care, but in case this code path needs ultimate performance.
> 
> Also I thought it was event->ctx.lock, but at this point I trust your eyes
> more than my own.  ;-)
> 
> 							Thanx, Paul



This is indeed never called from the read side.

In fact the situation is a bit complicated.
The true update side is:

	creation_of_perf_event() {
		mutex_lock(ctx->mutex);
		alloc/rcu_assign_pointer the hlist
		mutex_unlock(ctx->mutex);
	}
	/* the event for which we've allocated the hlist
	   can be scheduled only once the above is
	   finished */

And, in a serialized way, we can have:

	schedule_perf_event() {
		spin_lock(ctx->lock);
		rcu_deref hlist
		spin_unlock(ctx->lock);
	}

Scheduling an event that derefs the hlist will not happen if we haven't
created and assigned the hlist before.
And also scheduling an event that derefs the hlist won't happen after
the hlist has been destroyed (released), it also can't be destroyed
concurrently with the scheduling.

This is why I consider the event scheduling as part of the update side,
since it is serialized with it, the ctx->lock guarantees that.

OTOH, we can have the read side concurrently anytime, hence the need
for rcu_read_lock() there.
	
So, I guess it's justified to use rcu_dereference_protected() here.

Thanks.

--
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/