Re: [PATCH 2/4] scsi: Fix device removal NULL pointer dereference

[Hannes Reinecke <hare@suse.de>]

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@suse.de			      +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html