Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932673Ab2BOC1g (ORCPT ); Tue, 14 Feb 2012 21:27:36 -0500 Received: from TYO202.gate.nec.co.jp ([202.32.8.206]:38966 "EHLO tyo202.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755814Ab2BOC1e (ORCPT ); Tue, 14 Feb 2012 21:27:34 -0500 Message-ID: <4F3B17C7.9040107@ce.jp.nec.com> Date: Wed, 15 Feb 2012 11:26:15 +0900 From: "Jun'ichi Nomura" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 MIME-Version: 1.0 To: Bart Van Assche , Stefan Richter CC: jbottomley@parallels.com, linux-scsi@vger.kernel.org, Huajun Li , Axel Theilmann , linux-kernel@vger.kernel.org Subject: Re: Yet another hot unplug NULL pointer dereference (was Re: status of oops in sd_revalidate_disk?) References: <4EE8E419.8010000@pre-sense.de> <20120214143800.69703df9@stein> <2173642.Y3HxI15Hxl@asus> In-Reply-To: <2173642.Y3HxI15Hxl@asus> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3472 Lines: 108 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 > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/