Hello,
On Wed, Oct 10, 2012 at 05:09:04PM +0200, Bart Van Assche wrote:
> Some request_fn implementations, e.g. scsi_request_fn(), unlock
> the queue lock. Make sure that blk_cleanup_queue() waits until all
> active request_fn invocations have finished. This fixes a potential
> use-after-free at the end of scsi_request_fn(). Also, change the
> type of the 'drain' variable from bool to int to avoid that the
> highest bits of the request counters get ignored.
Similar comment. It would be great if you better separate what's
broken and how it's fixed. Kinda difficult to digest.
> @@ -308,7 +308,9 @@ void __blk_run_queue_uncond(struct request_queue *q)
> if (unlikely(blk_queue_dead(q)))
> return;
>
> + q->driver_active++;
> q->request_fn(q);
> + q->driver_active--;
Maybe q->request_fn_active is a better name?
> -void blk_drain_queue(struct request_queue *q, bool drain_all)
> +static void blk_drain_queue(struct request_queue *q, bool drain_all)
> {
> int i;
>
> while (true) {
> - bool drain = false;
> + int drain = 0;
I don't think this is necessary. bool conversion works fine
regardless how high the bits are. Isn't avoiding signed/unsigned
autocast maze one of the reasons why we're using bool to begin with?
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f29a1a9..0e15374 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)
> struct scsi_cmnd *cmd;
> struct request *req;
>
> - if(!get_device(&sdev->sdev_gendev))
> - /* We must be tearing the block queue down already */
> - return;
> -
> /*
> * To start with, we keep looping until the queue is empty, or until
> * the host is no longer able to accept any more requests.
> @@ -1629,11 +1625,7 @@ out_delay:
> if (sdev->device_busy == 0)
> blk_delay_queue(q, SCSI_QUEUE_DELAY);
> out:
> - /* must be careful here...if we trigger the ->remove() function
> - * we cannot be holding the q lock */
> - spin_unlock_irq(q->queue_lock);
> - put_device(&sdev->sdev_gendev);
> - spin_lock_irq(q->queue_lock);
> + ;
I think moving this out to a separate patch would be better.
Thanks.
--
tejun
--
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
