Hi,
Thank you for comments.
On 02/15/12 04:11, Bart Van Assche wrote:
> On Feb 14 Stefan Richter wrote:
>> Or asked differently, what is supposed to serialize the ->queuedata
>> accesses?
AFAIU the racy check is ok because
sdev does not disappear as far as the device is opened.
The oopses in scsi_prep_fn happened just because they used queuedata
without checking.
>
> How about avoiding to modify q->queuedata before scsi_free_queue() has been
> invoked, e.g. as follows ?
I think this patch is good, too.
Since QUEUE_FLAG_DEAD is also racy, it is not much different
from queuedata check, IMO.
Maybe you want additional blk_queue_dead() check in prep_fn
to keep the current behaviour.
> Note: the kfree() call in scsi_host_dev_release() probably
> should be postponed until the last put on the queue has occurred.
>
>>From 4d29b62c686a718d34d3b3f634306376b40dbdb6 Mon Sep 17 00:00:00 2001
> From: Bart Van Assche <bvanassche@xxxxxxx>
> Date: Tue, 14 Feb 2012 13:52:21 +0100
> Subject: [PATCH] scsi-queuedata-null-deref-fix
>
> ---
> drivers/scsi/hosts.c | 4 ++--
> drivers/scsi/scsi_lib.c | 13 +++----------
> drivers/scsi/scsi_sysfs.c | 3 ---
> 3 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 351dc0b..5f34620 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -296,9 +296,9 @@ static void scsi_host_dev_release(struct device *dev)
> destroy_workqueue(shost->work_q);
> q = shost->uspace_req_q;
> if (q) {
> - kfree(q->queuedata);
> - q->queuedata = NULL;
> + void *qd = q->queuedata;
> scsi_free_queue(q);
> + kfree(qd);
> }
>
> scsi_destroy_command_freelist(shost);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f85cfa6..7c40303 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1491,7 +1491,9 @@ static void scsi_request_fn(struct request_queue *q)
> struct scsi_cmnd *cmd;
> struct request *req;
>
> - if (!sdev) {
> + BUG_ON(!sdev);
> +
> + if (test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)) {
> while ((req = blk_peek_request(q)) != NULL)
> scsi_kill_request(req, q);
> return;
> @@ -1700,15 +1702,6 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
>
> void scsi_free_queue(struct request_queue *q)
> {
> - unsigned long flags;
> -
> - WARN_ON(q->queuedata);
> -
> - /* cause scsi_request_fn() to kill all non-finished requests */
> - spin_lock_irqsave(q->queue_lock, flags);
> - q->request_fn(q);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> blk_cleanup_queue(q);
> }
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ea5658d..61dd107 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -976,9 +976,6 @@ void __scsi_remove_device(struct scsi_device *sdev)
> sdev->host->hostt->slave_destroy(sdev);
> transport_destroy_device(dev);
>
> - /* cause the request function to reject all I/O requests */
> - sdev->request_queue->queuedata = NULL;
> -
> /* Freeing the queue signals to block that we're done */
> scsi_free_queue(sdev->request_queue);
> put_device(dev);
--
Jun'ichi Nomura, NEC Corporation
--
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
