From patchwork Fri Mar 27 17:28:48 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alasdair G Kergon X-Patchwork-Id: 14761 X-Patchwork-Delegate: agk@redhat.com Received: from hormel.redhat.com (hormel1.redhat.com [209.132.177.33]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n2RHSrYC025086 for ; Fri, 27 Mar 2009 17:28:53 GMT Received: from listman.util.phx.redhat.com (listman.util.phx.redhat.com [10.8.4.110]) by hormel.redhat.com (Postfix) with ESMTP id 047D561A4C7; Fri, 27 Mar 2009 13:28:53 -0400 (EDT) Received: from int-mx2.corp.redhat.com (nat-pool.util.phx.redhat.com [10.8.5.200]) by listman.util.phx.redhat.com (8.13.1/8.13.1) with ESMTP id n2RHSpRX001670 for ; Fri, 27 Mar 2009 13:28:51 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n2RHShnl028032; Fri, 27 Mar 2009 13:28:43 -0400 Received: from agk.fab.redhat.com (agk.fab.redhat.com [10.33.0.19]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n2RHSnTx011512; Fri, 27 Mar 2009 13:28:50 -0400 Received: from agk by agk.fab.redhat.com with local (Exim 4.34) id 1LnFrM-0000ZC-Df; Fri, 27 Mar 2009 17:28:48 +0000 Date: Fri, 27 Mar 2009 17:28:48 +0000 From: Alasdair G Kergon To: Neil Brown Subject: Re: [dm-devel] BUG/PATCH race between upgrade_mode and dm_table_any_congested Message-ID: <20090327172848.GT13664@agk.fab.redhat.com> Mail-Followup-To: Neil Brown , dm-devel@redhat.com References: <18887.9605.64321.547734@notabene.brown> Mime-Version: 1.0 Content-Disposition: inline In-Reply-To: <18887.9605.64321.547734@notabene.brown> User-Agent: Mutt/1.4.1i Organization: Red Hat UK Ltd. Registered in England and Wales, number 03798903. Registered Office: Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE. X-Scanned-By: MIMEDefang 2.58 on 172.16.27.26 X-loop: dm-devel@redhat.com Cc: dm-devel@redhat.com X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.5 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com On Mon, Mar 23, 2009 at 05:00:36PM +1100, Neil Brown wrote: > The code in current mainline is exactly the same so if we are correct > in our assessment, then the bug is still present. > Our current patch is below. It is a big ugly, and a better fix might > be a more thorough rewrite of the code. However I offer it incase it > is useful. Indeed this area of code is in need of a clean up. Here's an alternative version trying additionally to avoid the theoretical risk from overwriting the field with the same value. Alasdair From: Alasdair G Kergon upgrade_mode() sets bdev to NULL temporarily, and does not have any locking to exclude anything from seeing that NULL. In dm_table_any_congested() bdev_get_queue() can dereference that NULL and cause a reported oops. Fix this by not changing that field during the mode upgrade. Cc: Neil Brown Signed-off-by: Alasdair G Kergon --- drivers/md/dm-table.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel Index: linux-2.6.29/drivers/md/dm-table.c =================================================================== --- linux-2.6.29.orig/drivers/md/dm-table.c +++ linux-2.6.29/drivers/md/dm-table.c @@ -399,28 +399,30 @@ static int check_device_area(struct dm_d } /* - * This upgrades the mode on an already open dm_dev. Being + * This upgrades the mode on an already open dm_dev, being * careful to leave things as they were if we fail to reopen the - * device. + * device and not to touch the existing bdev field in case + * it is accessed concurrently inside dm_table_any_congested(). */ static int upgrade_mode(struct dm_dev_internal *dd, fmode_t new_mode, struct mapped_device *md) { int r; - struct dm_dev_internal dd_copy; - dev_t dev = dd->dm_dev.bdev->bd_dev; + struct dm_dev_internal dd_new, dd_old; - dd_copy = *dd; + dd_new = dd_old = *dd; + + dd_new.dm_dev.mode |= new_mode; + dd_new.dm_dev.bdev = NULL; + + r = open_dev(&dd_new, dd->dm_dev.bdev->bd_dev, md); + if (r) + return r; dd->dm_dev.mode |= new_mode; - dd->dm_dev.bdev = NULL; - r = open_dev(dd, dev, md); - if (!r) - close_dev(&dd_copy, md); - else - *dd = dd_copy; + close_dev(&dd_old, md); - return r; + return 0; } /*