Re: Kernel crash in icq_free_icq_rcu

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Jan 18, 2012 at 02:03:22PM +0800, Shaohua Li wrote:
> Subject: block: fix NULL icq_cache reference
> 
> CPU0:					CPU1:
> 					switch from cfq to noop
> 					   elv_quiesce_start
> C: get_request
> A:   ioc_create_icq
>       alloc icq with cfq
> 					   B: do elevator switch
> 					       ioc_clear_queue
> 					   elv_quiesce_end
>       insert icq to ioc
> 					switch from noop to cfq
> 					    elv_quiesce_start
> 					    do elevator switch
> 					       ioc_clear_queue
> 					    elv_quiesce_end
> CPU0 leaves some icq to ioc list after elevator is switching from cfq to noop.
> in the second ioc_clear_queue, the ioc has icq in its list, but current
> elevator is noop. so ioc_exit_icq will get a NULL et->icq_cache.
> 
> In above cases, if A runs after B, ioc_create_icq will have a NULL
> et->icq_cache, this will trigger another crash.
> 
> Note, get_request caches et without lock hold. Between C and A, a elevator
> switch can start. But we will have elvpriv++, elv_quiesce_start will drain
> all requests first. So this will not trigger crash.

Thanks a lot for tracking it down.

Hmmm... but I'm having a difficult time following the description.
Maybe we can simplify a bit?  e.g. sth like the following?

  Once a queue is quiesced, it's not supposed to have any elvpriv data
  or icq's, and elevator switching depends on that.  Request alloc
  path followed the rule for elvpriv data but forgot apply it to
  icq's leading to the following crash during elevator switch.

  <oops log>

  Fix it by not allocating icq's if ELVPRIV is not set for the
  request.

> Index: linux/block/blk-core.c
> ===================================================================
> --- linux.orig/block/blk-core.c	2012-01-18 12:44:13.000000000 +0800
> +++ linux/block/blk-core.c	2012-01-18 12:45:28.000000000 +0800
> @@ -872,11 +872,11 @@ retry:
>  	spin_unlock_irq(q->queue_lock);
>  
>  	/* create icq if missing */
> -	if (unlikely(et->icq_cache && !icq))
> +	if (unlikely(et->icq_cache && !icq && (rw_flags & REQ_ELVPRIV)))
>  		icq = ioc_create_icq(q, gfp_mask);
>  
>  	/* rqs are guaranteed to have icq on elv_set_request() if requested */
> -	if (likely(!et->icq_cache || icq))
> +	if (likely(!et->icq_cache || icq || !(rw_flags & REQ_ELVPRIV)))
>  		rq = blk_alloc_request(q, icq, rw_flags, gfp_mask);

Hmmm... I was trying to avoid adding a goto label with the double
testing but with REQ_ELVPRIV test added, it looks more confusing.
Maybe something like the following is better?

	/* rqs are guaranteed to have icq on elv_set_request() if requested */
	if ((rw_flags & REQ_ELVPRIV) && unlikely(et->icq_cache && !icq)) {
		icq = ioc_create_icq(q, gfp_mask);
		if (!icq)
			goto fail_icq;
	}
	rq = blk_alloc_request(q, icq, rw_flags, gfp_mask);
 fail_icq:

Thanks.

-- 
tejun