Re: 2.6.26-rc4: RIP __call_for_each_cic+0x20/0x50

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, May 27, 2008 at 03:35:10PM +0200, Jens Axboe wrote:
> On Tue, May 27 2008, Alexey Dobriyan wrote:
> > On Sat, May 10, 2008 at 02:37:19PM +0400, Alexey Dobriyan wrote:
> > > > > > > @@ -41,8 +41,8 @@ int put_io_context(struct io_context *ioc)
> > > > > > >  		rcu_read_lock();
> > > > > > >  		if (ioc->aic && ioc->aic->dtor)
> > > > > > >  			ioc->aic->dtor(ioc->aic);
> > > > > > > -		rcu_read_unlock();
> > > > > > >  		cfq_dtor(ioc);
> > > > > > > +		rcu_read_unlock();
> > > > > > >  
> > > > > > >  		kmem_cache_free(iocontext_cachep, ioc);
> > > > > > >  		return 1;
> > > > > > 
> > > > > > This helps in sense that 3 times bulk cross-compiles finish to the end.
> > > > > > You'll hear me if another such oops will resurface.
> > > > > 
> > > > > Still looking good?
> > > > 
> > > > Yup!
> > > 
> > > And this with patch in mainline, again with PREEMPT_RCU.
> > 
> > Ping, this happened again with 2.6.26-rc4 and PREEMPT_RCU.
> 
> Worrisome... Paul, would you mind taking a quick look at cfq
> and see if you can detect why breaks with preempt rcu? It's
> clearly a use-after-free symptom, but I don't see how it can
> happen.

Some quick and probably off-the-mark questions...

o	What is the purpose of __call_for_each_cic()?  When called
	from call_for_each_cic(), it is under rcu_read_lock(), as
	required, but it is also called from cfq_free_io_context(),
	which is assigned to the ->dtor and ->exit members of the
	cfq_io_context struct.  What protects calls through these
	members?

	(This is for the ->cic_list field of the cfq_io_context structure.
	One possibility is that the io_context's ->lock member is held,
	but I don't see this.  Not that I looked all that hard...)

	My suggestion would be to simply change all invocations of
	__call_for_each_cic() to instead invoke call_for_each_cic().
	The rcu_read_lock()/rcu_read_unlock() pair is pretty
	lightweight, even in CONFIG_PREEMPT_RCU.

o	When calling cfq_slab_kill(), for example from cfq_exit(),
	what ensures that all previous RCU callbacks have completed?
	
	I suspect that you need an rcu_barrier() at the beginning
	of cfq_slab_kill(), but I could be missing something.

o	Updates to the ->ioc_data field of the cfq_io_context
	seem to be protected by the request_queue ->queue_lock
	field.  This seems very strange to me.  It is OK if every
	cfq_io_context is associated with only one request_queue
	structure -- is this the case?

o	What protects the first rcu_dereference() in cfq_cic_lookup()?
	There needs to be either an enclose rcu_read_lock() on the
	one hand or the ->queue_lock needs to be held.

	(My guess is the latter, given the later rcu_assign_pointer()
	in this same function, but I don't see a lock acquisition
	in the immediate vicinity -- might be further up the function
	call stack, though.)

o	Why is there no grace period associated with the ioc_data?
	For example, what happens to the old value of ->ioc_data
	after the rcu_assign_pointer() in cfq_cic_lookup()?  Readers
	might still be referencing the old version, right?  If so,
	how do we avoid messing them up?

	Or are we somehow leveraging the call_rcu() in cfq_cic_free()?

Any of this at all helpful?

							Thanx, Paul