From patchwork Fri Jan 6 17:45:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 9501545 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DCCB060413 for ; Fri, 6 Jan 2017 17:45:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D211428500 for ; Fri, 6 Jan 2017 17:45:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C5CC028509; Fri, 6 Jan 2017 17:45:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM,T_DKIM_INVALID,T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E5EEF28500 for ; Fri, 6 Jan 2017 17:45:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752579AbdAFRpt (ORCPT ); Fri, 6 Jan 2017 12:45:49 -0500 Received: from mail-oi0-f51.google.com ([209.85.218.51]:34199 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752932AbdAFRpr (ORCPT ); Fri, 6 Jan 2017 12:45:47 -0500 Received: by mail-oi0-f51.google.com with SMTP id 3so440182581oih.1 for ; Fri, 06 Jan 2017 09:45:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=mU3kD0KmFK+IrcmLrb17prqtVBYhL6P9IwNX8M74cjU=; b=REpPLK9UR9qVQ6tMCTFnW5L24gyWgc3Kp0UhjfAAzz2XMuxMNJQBoISGYFFjS8SGZ2 ScuexA77LUT945NjLX7nWtlTQiJ+99SMtu5iExu4UJYz6z78cSaQuORNgzXwj49qct6x fjH05VGOk2zKXNsVCe9PnDXHxDYYQ2GaBvIWpwwG5KI4STgLt6jMUT0VRqNRCxbCBAOL eRYZAIMXO4LB0b3QpTS0wul20j6b0c8c3CF1M98HJvDC4MHkNr9DjgUM+drnLnmxwOtR o5U1kiCXZ4OgthkCgLF3fre3bNsFBDhhMZRDjuYDB3tCFoNhjhFXYbCIRQ1YHNeNQFTh aGQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=mU3kD0KmFK+IrcmLrb17prqtVBYhL6P9IwNX8M74cjU=; b=DCRw10CfZUXqOfX4v1FJWOu5kjay1CPbuOFmZWrz1TkQLSjEwiFklFJFpbuRBm57sZ aFhJDMCWgH44COAkWLSEgsh9faPjSmf1mAQ2eNuiY2ta964INT2AH4NmDuFl9hbTL26X /KmpIEnaHXITCuI6UdzFgi8o6pHcPkaPL56aqVu4kqv3U48b8/6z8xXLw00igLhkCDnx 9zxV/KzZLEviQTzpLKrbXuzOdSaJ7e5WtGVatZSwVjMnQHpEwz30yjFfvFx44nmUb/i5 cJ+bS4L5m6DxvctW3B4teHfo2w562W4E/DY8GgrzP17eAQPMYQV0ICwWXJRJXhYPs5aw ehUQ== X-Gm-Message-State: AIkVDXIAT3qGRQBRjlSA+yx5NMa1AjAZX8vHQ1yHiBc4lrwgv6J+7U1T+b/gPNHSVgyw3II7CnEcWvWg0rEOYgGe X-Received: by 10.157.60.69 with SMTP id j5mr1899255ote.224.1483724746587; Fri, 06 Jan 2017 09:45:46 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.11.130 with HTTP; Fri, 6 Jan 2017 09:45:45 -0800 (PST) In-Reply-To: <20170106102330.GB3533@quack2.suse.cz> References: <148366547505.38941.646379357860772670.stgit@dwillia2-desk3.amr.corp.intel.com> <20170106102330.GB3533@quack2.suse.cz> From: Dan Williams Date: Fri, 6 Jan 2017 09:45:45 -0800 Message-ID: Subject: Re: [PATCH] block: fix blk_get_backing_dev_info() crash, use bdev->bd_queue To: Jan Kara Cc: Jens Axboe , Andi Kleen , Rabin Vincent , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , linux-block@vger.kernel.org, Jeff Moyer , Wei Fang , Christoph Hellwig Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Jan 6, 2017 at 2:23 AM, Jan Kara wrote: > On Thu 05-01-17 17:17:55, Dan Williams wrote: >> The ->bd_queue member of struct block_device was added in commit >> 87192a2a49c4 ("vfs: cache request_queue in struct block_device") in >> v3.3. However, blk_get_backing_dev_info() has been using >> bdev_get_queue() and grabbing the request_queue through the gendisk >> since before the git era. >> >> At final __blkdev_put() time ->bd_disk is cleared while ->bd_queue is >> not. The queue remains valid until the final put of the parent disk. >> >> The following crash signature results from blk_get_backing_dev_info() >> trying to lookup the queue through ->bd_disk after the final put of the >> block device. Simply switch bdev_get_queue() to use ->bd_queue directly >> which is guaranteed to still be valid at invalidate_partition() time. >> >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000568 >> IP: blk_get_backing_dev_info+0x10/0x20 >> [..] >> Call Trace: >> __inode_attach_wb+0x3a7/0x5d0 >> __filemap_fdatawrite_range+0xf8/0x100 >> filemap_write_and_wait+0x40/0x90 >> fsync_bdev+0x54/0x60 >> ? bdget_disk+0x30/0x40 >> invalidate_partition+0x24/0x50 >> del_gendisk+0xfa/0x230 > > So we have a similar reports of the same problem. E.g.: > > http://www.spinics.net/lists/linux-fsdevel/msg105153.html > > However I kind of miss how your patch would fix all those cases. The > principial problem is that inode_to_bdi() called on block device inode > wants to get the backing_dev_info however on last close of a block device > we do put_disk() and thus the request queue containing backing_dev_info > does not have to be around at that time. In your case you are lucky enough > to have the containing disk still around but that's not the case for all > inode_to_bdi() users (see e.g. the report I referenced) and your patch > would change relatively straightforward NULL pointer dereference to rather > subtle use-after-free issue True. If there are other cases that don't hold their own queue reference this patch makes things worse. > so I disagree with going down this path. I still think this patch is the right thing to do, but it needs to come after the wider guarantee that having an active bdev reference guarantees that the queue and backing_dev_info are still allocated. > So what I think needs to be done is that we make backing_dev_info > independently allocated structure with different lifetime rules to gendisk > or request_queue - definitely I want it to live as long as block device > inode exists. However it needs more thought what the exact lifetime rules > will be. Hmm, why does it need to be separately allocated? Something like this, passes the libnvdimm unit tests: (non-whitespace damaged version attached) diff --git a/block/blk-core.c b/block/blk-core.c index 7c1a24fb09d8..6742c49b38e4 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -378,7 +378,8 @@ EXPORT_SYMBOL(blk_run_queue); void blk_put_queue(struct request_queue *q) { - kobject_put(&q->kobj); + if (q) + kobject_put(&q->kobj); } EXPORT_SYMBOL(blk_put_queue); diff --git a/fs/block_dev.c b/fs/block_dev.c index 05b553368bb4..7503110e37ff 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -589,12 +589,22 @@ static struct inode *bdev_alloc_inode(struct super_block *sb) return &ei->vfs_inode; } +static void bdev_release(struct work_struct *w) +{ + struct block_device *bdev = container_of(w, typeof(*bdev), bd_release); + struct bdev_inode *bdi = container_of(bdev, typeof(*bdi), bdev); + + blk_put_queue(bdev->bd_queue); + kmem_cache_free(bdev_cachep, bdi); +} + static void bdev_i_callback(struct rcu_head *head) { struct inode *inode = container_of(head, struct inode, i_rcu); - struct bdev_inode *bdi = BDEV_I(inode); + struct block_device *bdev = &BDEV_I(inode)->bdev; - kmem_cache_free(bdev_cachep, bdi); + /* blk_put_queue needs process context */ + schedule_work(&bdev->bd_release); } static void bdev_destroy_inode(struct inode *inode) @@ -613,6 +623,7 @@ static void init_once(void *foo) #ifdef CONFIG_SYSFS INIT_LIST_HEAD(&bdev->bd_holder_disks); #endif + INIT_WORK(&bdev->bd_release, bdev_release); inode_init_once(&ei->vfs_inode); /* Initialize mutex for freeze. */ mutex_init(&bdev->bd_fsfreeze_mutex); @@ -1268,6 +1279,8 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) mutex_lock_nested(&bdev->bd_mutex, for_part); if (!bdev->bd_openers) { bdev->bd_disk = disk; + if (!blk_get_queue(disk->queue)) + goto out_clear; bdev->bd_queue = disk->queue; bdev->bd_contains = bdev; @@ -1288,7 +1301,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) disk_put_part(bdev->bd_part); bdev->bd_part = NULL; bdev->bd_disk = NULL; - bdev->bd_queue = NULL; mutex_unlock(&bdev->bd_mutex); disk_unblock_events(disk); put_disk(disk); @@ -1364,7 +1376,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) disk_put_part(bdev->bd_part); bdev->bd_disk = NULL; bdev->bd_part = NULL; - bdev->bd_queue = NULL; if (bdev != bdev->bd_contains) __blkdev_put(bdev->bd_contains, mode, 1); bdev->bd_contains = NULL; diff --git a/include/linux/fs.h b/include/linux/fs.h index dc0478c07b2a..f084e4a2198b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -488,6 +488,7 @@ struct block_device { int bd_fsfreeze_count; /* Mutex for freeze */ struct mutex bd_fsfreeze_mutex; + struct work_struct bd_release; }; /*