On 06/26/2012 11:13 AM, Mike Christie wrote: > On 06/26/2012 01:46 AM, Bart Van Assche wrote: >>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >>>>>> index 6dfb978..c26ef49 100644 >>>>>> --- a/drivers/scsi/scsi_lib.c >>>>>> +++ b/drivers/scsi/scsi_lib.c >>>>>> @@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q) >>>>>> LIST_HEAD(starved_list); >>>>>> unsigned long flags; >>>>>> >>>>>> - /* if the device is dead, sdev will be NULL, so no queue to run */ >>>>>> - if (!sdev) >>>>>> - return; >>>>>> - >>>>>> + BUG_ON(!sdev); >>>> >>>> Needs to be a blk_queue_dead() check as well. >> >> Callers of scsi_run_queue() don't hold the queue lock. Does it make >> sense to test whether the queue is dead without the queue lock being held ? >> > > I think there is still another bug in this path when it is called from > the requeue path. If scsi_requeue_command requeues a command and that > gets executed by some other thread before scsi_requeue_command calls > scsi_run_queue then we could end up accessing freed memory. > > It looks possible some other thread is doing > blk_cleanup_queue->blk_drain_queue and that calls blk_run_queue and that > kills/fails the IO that scsi_requeue_command had queued. Then > blk_cleanup_queue could complete and we could end up doing the last put > on the device and freeing the queue and sdev before scsi_requeue_command > can call scsi_run_queue. scsi_run_queue could then be accessing freed > memory. > > I think we need a get/put: > > scsi_requeue_command.... > > get_device(&sdev->sdev_gendev); > > spin_lock_irqsave(q->queue_lock, flags); > scsi_unprep_request(req); > blk_requeue_request(q, req); > spin_unlock_irqrestore(q->queue_lock, flags); > > scsi_run_queue(q); > > put_device(&sdev->sdev_gendev); > > This will prevent some other path from freeing the queue/sdev from under us. <clapping hands> Congrats, Mike. This looks like the bug I have been hunting since nearly a year now. (and which I've been pestering folks at the Storage Summit :-) So yeah, definitely a good idea. I'll give it a shout and see if it improves things. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@xxxxxxx +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
