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

[Bart Van Assche <bvanassche@acm.org>]

On 06/26/12 09:13, Mike Christie wrote:

> 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.


The above makes a lot of sense to me - I'll have a closer look at it.

However, it seems to me that we are paying a price for
scsi_dispatch_cmd() being called without the queue lock held, namely
increased complexity of the SCSI core.

Bart.