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

[Paul E. McKenney]

On Wed, May 28, 2008 at 12:07:21PM +0200, Jens Axboe wrote:
> On Tue, May 27 2008, Paul E. McKenney wrote:
> > 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...
> 
> Thanks!

Glad it actually was of help!  ;-)

> > 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.
> 
> __call_for_each_cic() is always called under rcu_read_lock(), it merely
> exists to avoid a double rcu_read_lock(). Even if it is cheap. The
> convention follows the usual __lock_is_already_held() double under
> score, but I guess it could do with a comment! There are only two
> callers of the function, call_for_each_cic() which does the
> rcu_read_lock(), and cfq_free_io_context() which is called from ->dtor
> (and holds the rcu_read_lock() and ->trim which actually does not. That
> looks like it could be problematic, but it's only called when an io
> scheduler module is removed so not really critical. I'll add it, though!
> Actually, the task_lock() should be enough there. So no bug, but (again)
> it could do with a comment.

Sounds good!

> > 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.
> 
> So we have two callers of that, one is from the error path at init time
> and is obviously ok. The other does need rcu_barrier()! I'll add that.

OK, that does make my brain hurt less.  ;-)

> > 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?
> 
> ->ioc_data is part of the io_context, not cfq_io_context. And it can be
> shared now, so the correct locking for that would be ioc->lock and not
> the queue lock. __cfq_exit_single_io_context() is serialized in the
> sense that only one process gets to call the exit path.

Makes sense to me!

> > 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.)
> 
> There's no locking going into that function when coming from
> cfq_get_io_context(), the other paths (happen) to hold the queue lock
> already though.

So the call from cfq_get_io_context() needs an rcu_read_lock()?
Not seeing this in the patch below, but maybe you have it up a
function-call level or two?

> > 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()?
> 
> The data belonging to ->ioc_data (the cic, or per-process per-queue
> context) is only freed through call_rcu().

Ah, OK, got it.

> > Any of this at all helpful?
> 
> Very, perhaps with a few more rounds we can find some more bugs :-). I'm
> attaching a patch below, how does that look?

Looks much improved!  Very interested  to hear how it does with  the testing.

							Thanx, Paul

> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 4df3f05..75db529 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1142,6 +1142,9 @@ static void cfq_put_queue(struct cfq_queue *cfqq)
>  	kmem_cache_free(cfq_pool, cfqq);
>  }
> 
> +/*
> + * Must always be called with the rcu_read_lock() held
> + */
>  static void
>  __call_for_each_cic(struct io_context *ioc,
>  		    void (*func)(struct io_context *, struct cfq_io_context *))
> @@ -1197,6 +1200,11 @@ static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
>  	cfq_cic_free(cic);
>  }
> 
> +/*
> + * Must be called with rcu_read_lock() held or preemption otherwise disabled.
> + * Only two callers of this - ->dtor() which is called with the rcu_read_lock(),
> + * and ->trim() which is called with the task lock held
> + */
>  static void cfq_free_io_context(struct io_context *ioc)
>  {
>  	/*
> @@ -1502,20 +1510,24 @@ static struct cfq_io_context *
>  cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
>  {
>  	struct cfq_io_context *cic;
> +	unsigned long flags;
>  	void *k;
> 
>  	if (unlikely(!ioc))
>  		return NULL;
> 
> +	rcu_read_lock();
> +
>  	/*
>  	 * we maintain a last-hit cache, to avoid browsing over the tree
>  	 */
>  	cic = rcu_dereference(ioc->ioc_data);
> -	if (cic && cic->key == cfqd)
> +	if (cic && cic->key == cfqd) {
> +		rcu_read_unlock();
>  		return cic;
> +	}
> 
>  	do {
> -		rcu_read_lock();
>  		cic = radix_tree_lookup(&ioc->radix_root, (unsigned long) cfqd);
>  		rcu_read_unlock();
>  		if (!cic)
> @@ -1524,10 +1536,13 @@ cfq_cic_lookup(struct cfq_data *cfqd, struct io_context *ioc)
>  		k = cic->key;
>  		if (unlikely(!k)) {
>  			cfq_drop_dead_cic(cfqd, ioc, cic);
> +			rcu_read_lock();
>  			continue;
>  		}
> 
> +		spin_lock_irqsave(&ioc->lock, flags);
>  		rcu_assign_pointer(ioc->ioc_data, cic);
> +		spin_unlock_irqrestore(&ioc->lock, flags);
>  		break;
>  	} while (1);
> 
> @@ -2134,6 +2149,11 @@ static void *cfq_init_queue(struct request_queue *q)
> 
>  static void cfq_slab_kill(void)
>  {
> +	/*
> +	 * Make sure that all existing RCU callbacks have been processed
> +	 */
> +	rcu_barrier();
> +
>  	if (cfq_pool)
>  		kmem_cache_destroy(cfq_pool);
>  	if (cfq_ioc_pool)
> 
> -- 
> Jens Axboe
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/