Btrfs: fix double free of fs root

Message ID	1467146678-29046-1-git-send-email-bo.li.liu@oracle.com (mailing list archive)
State	Accepted
Headers	show Return-Path: <linux-btrfs-owner@kernel.org> From: Liu Bo <bo.li.liu@oracle.com> To: linux-btrfs@vger.kernel.org Cc: Chandan Rajendra <chandan@linux.vnet.ibm.com> Subject: [PATCH] Btrfs: fix double free of fs root Date: Tue, 28 Jun 2016 13:44:38 -0700 Message-Id: <1467146678-29046-1-git-send-email-bo.li.liu@oracle.com> Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk

Message ID

1467146678-29046-1-git-send-email-bo.li.liu@oracle.com (mailing list archive)

State

Accepted

Headers

From: Liu Bo <bo.li.liu@oracle.com>
To: linux-btrfs@vger.kernel.org
Cc: Chandan Rajendra <chandan@linux.vnet.ibm.com>
Subject: [PATCH] Btrfs: fix double free of fs root
Date: Tue, 28 Jun 2016 13:44:38 -0700
Message-Id: <1467146678-29046-1-git-send-email-bo.li.liu@oracle.com>
Sender: linux-btrfs-owner@vger.kernel.org
Precedence: bulk

Commit Message

Liu Bo June 28, 2016, 8:44 p.m. UTC

I got this warning while mounting a btrfs image,

[ 3020.509606] ------------[ cut here ]------------
[ 3020.510107] WARNING: CPU: 3 PID: 5581 at lib/idr.c:1051 ida_remove+0xca/0x190
[ 3020.510853] ida_remove called for id=42 which is not allocated.
[ 3020.511466] Modules linked in:
[ 3020.511802] CPU: 3 PID: 5581 Comm: mount Not tainted 4.7.0-rc5+ #274
[ 3020.512438] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150714_191134- 04/01/2014
[ 3020.513385]  0000000000000286 0000000021295d86 ffff88006c66b8f0 ffffffff8182ba5a
[ 3020.514153]  0000000000000000 0000000000000009 ffff88006c66b930 ffffffff810e0ed7
[ 3020.514928]  0000041b00000000 ffffffff8289a8c0 ffff88007f437880 0000000000000000
[ 3020.515717] Call Trace:
[ 3020.515965]  [<ffffffff8182ba5a>] dump_stack+0xc9/0x13f
[ 3020.516487]  [<ffffffff810e0ed7>] __warn+0x147/0x160
[ 3020.517005]  [<ffffffff810e0f4f>] warn_slowpath_fmt+0x5f/0x80
[ 3020.517572]  [<ffffffff8182e6ca>] ida_remove+0xca/0x190
[ 3020.518075]  [<ffffffff813a2bcc>] free_anon_bdev+0x2c/0x60
[ 3020.518609]  [<ffffffff81657a9f>] free_fs_root+0x13f/0x160
[ 3020.519138]  [<ffffffff8165c679>] btrfs_get_fs_root+0x379/0x3d0
[ 3020.519710]  [<ffffffff81e6e975>] ? __mutex_unlock_slowpath+0x155/0x2c0
[ 3020.520366]  [<ffffffff816615b1>] open_ctree+0x2e91/0x3200
[ 3020.520965]  [<ffffffff8161ede2>] btrfs_mount+0x1322/0x15b0
[ 3020.521536]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
[ 3020.522167]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
[ 3020.522780]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
[ 3020.523305]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
[ 3020.523872]  [<ffffffff8161dee1>] btrfs_mount+0x421/0x15b0
[ 3020.524402]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
[ 3020.525045]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
[ 3020.525657]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
[ 3020.526289]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
[ 3020.526803]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
[ 3020.527365]  [<ffffffff813dc27a>] do_mount+0x41a/0x1770
[ 3020.527899]  [<ffffffff812e800d>] ? strndup_user+0x6d/0xc0
[ 3020.528447]  [<ffffffff812e7f68>] ? memdup_user+0x78/0xb0
[ 3020.528987]  [<ffffffff813ddad0>] SyS_mount+0x150/0x160
[ 3020.529493]  [<ffffffff81e72b7c>] entry_SYSCALL_64_fastpath+0x1f/0xbd

It turns out that we free fs root twice, btrfs_init_fs_root() calls
free_anon_bdev(root->anon_dev) and later then btrfs_get_fs_root() cals
free_fs_root which does another free_anon_bdev() and it ends up with the
above warning.

Instead of reset root->anon_dev to 0 after free_anon_bdev(), we can let
btrfs_init_fs_root() return directly since its callers have already done
the free job by calling free_fs_root().

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/disk-io.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

Comments

Chandan Rajendra June 30, 2016, 6:37 a.m. UTC | #1

On Tuesday, June 28, 2016 01:44:38 PM Liu Bo wrote:
> I got this warning while mounting a btrfs image,
> 
> [ 3020.509606] ------------[ cut here ]------------
> [ 3020.510107] WARNING: CPU: 3 PID: 5581 at lib/idr.c:1051 ida_remove+0xca/0x190
> [ 3020.510853] ida_remove called for id=42 which is not allocated.
> [ 3020.511466] Modules linked in:
> [ 3020.511802] CPU: 3 PID: 5581 Comm: mount Not tainted 4.7.0-rc5+ #274
> [ 3020.512438] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150714_191134- 04/01/2014
> [ 3020.513385]  0000000000000286 0000000021295d86 ffff88006c66b8f0 ffffffff8182ba5a
> [ 3020.514153]  0000000000000000 0000000000000009 ffff88006c66b930 ffffffff810e0ed7
> [ 3020.514928]  0000041b00000000 ffffffff8289a8c0 ffff88007f437880 0000000000000000
> [ 3020.515717] Call Trace:
> [ 3020.515965]  [<ffffffff8182ba5a>] dump_stack+0xc9/0x13f
> [ 3020.516487]  [<ffffffff810e0ed7>] __warn+0x147/0x160
> [ 3020.517005]  [<ffffffff810e0f4f>] warn_slowpath_fmt+0x5f/0x80
> [ 3020.517572]  [<ffffffff8182e6ca>] ida_remove+0xca/0x190
> [ 3020.518075]  [<ffffffff813a2bcc>] free_anon_bdev+0x2c/0x60
> [ 3020.518609]  [<ffffffff81657a9f>] free_fs_root+0x13f/0x160
> [ 3020.519138]  [<ffffffff8165c679>] btrfs_get_fs_root+0x379/0x3d0
> [ 3020.519710]  [<ffffffff81e6e975>] ? __mutex_unlock_slowpath+0x155/0x2c0
> [ 3020.520366]  [<ffffffff816615b1>] open_ctree+0x2e91/0x3200
> [ 3020.520965]  [<ffffffff8161ede2>] btrfs_mount+0x1322/0x15b0
> [ 3020.521536]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
> [ 3020.522167]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.522780]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
> [ 3020.523305]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
> [ 3020.523872]  [<ffffffff8161dee1>] btrfs_mount+0x421/0x15b0
> [ 3020.524402]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
> [ 3020.525045]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.525657]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.526289]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
> [ 3020.526803]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
> [ 3020.527365]  [<ffffffff813dc27a>] do_mount+0x41a/0x1770
> [ 3020.527899]  [<ffffffff812e800d>] ? strndup_user+0x6d/0xc0
> [ 3020.528447]  [<ffffffff812e7f68>] ? memdup_user+0x78/0xb0
> [ 3020.528987]  [<ffffffff813ddad0>] SyS_mount+0x150/0x160
> [ 3020.529493]  [<ffffffff81e72b7c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
> 
> It turns out that we free fs root twice, btrfs_init_fs_root() calls
> free_anon_bdev(root->anon_dev) and later then btrfs_get_fs_root() cals
> free_fs_root which does another free_anon_bdev() and it ends up with the
> above warning.
> 
> Instead of reset root->anon_dev to 0 after free_anon_bdev(), we can let
> btrfs_init_fs_root() return directly since its callers have already done
> the free job by calling free_fs_root().

Looks good to me. Thanks for fixing this.

Reviewed-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

David Sterba July 4, 2016, 4:52 p.m. UTC | #2

On Tue, Jun 28, 2016 at 01:44:38PM -0700, Liu Bo wrote:
> I got this warning while mounting a btrfs image,
> 
> [ 3020.509606] ------------[ cut here ]------------
> [ 3020.510107] WARNING: CPU: 3 PID: 5581 at lib/idr.c:1051 ida_remove+0xca/0x190
> [ 3020.510853] ida_remove called for id=42 which is not allocated.
> [ 3020.511466] Modules linked in:
> [ 3020.511802] CPU: 3 PID: 5581 Comm: mount Not tainted 4.7.0-rc5+ #274
> [ 3020.512438] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150714_191134- 04/01/2014
> [ 3020.513385]  0000000000000286 0000000021295d86 ffff88006c66b8f0 ffffffff8182ba5a
> [ 3020.514153]  0000000000000000 0000000000000009 ffff88006c66b930 ffffffff810e0ed7
> [ 3020.514928]  0000041b00000000 ffffffff8289a8c0 ffff88007f437880 0000000000000000
> [ 3020.515717] Call Trace:
> [ 3020.515965]  [<ffffffff8182ba5a>] dump_stack+0xc9/0x13f
> [ 3020.516487]  [<ffffffff810e0ed7>] __warn+0x147/0x160
> [ 3020.517005]  [<ffffffff810e0f4f>] warn_slowpath_fmt+0x5f/0x80
> [ 3020.517572]  [<ffffffff8182e6ca>] ida_remove+0xca/0x190
> [ 3020.518075]  [<ffffffff813a2bcc>] free_anon_bdev+0x2c/0x60
> [ 3020.518609]  [<ffffffff81657a9f>] free_fs_root+0x13f/0x160
> [ 3020.519138]  [<ffffffff8165c679>] btrfs_get_fs_root+0x379/0x3d0
> [ 3020.519710]  [<ffffffff81e6e975>] ? __mutex_unlock_slowpath+0x155/0x2c0
> [ 3020.520366]  [<ffffffff816615b1>] open_ctree+0x2e91/0x3200
> [ 3020.520965]  [<ffffffff8161ede2>] btrfs_mount+0x1322/0x15b0
> [ 3020.521536]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
> [ 3020.522167]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.522780]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
> [ 3020.523305]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
> [ 3020.523872]  [<ffffffff8161dee1>] btrfs_mount+0x421/0x15b0
> [ 3020.524402]  [<ffffffff81e60e74>] ? kmemleak_alloc_percpu+0x44/0x170
> [ 3020.525045]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.525657]  [<ffffffff8115f5e1>] ? lockdep_init_map+0x61/0x210
> [ 3020.526289]  [<ffffffff813a4f59>] mount_fs+0x49/0x2c0
> [ 3020.526803]  [<ffffffff813d840c>] vfs_kern_mount+0xac/0x1b0
> [ 3020.527365]  [<ffffffff813dc27a>] do_mount+0x41a/0x1770
> [ 3020.527899]  [<ffffffff812e800d>] ? strndup_user+0x6d/0xc0
> [ 3020.528447]  [<ffffffff812e7f68>] ? memdup_user+0x78/0xb0
> [ 3020.528987]  [<ffffffff813ddad0>] SyS_mount+0x150/0x160
> [ 3020.529493]  [<ffffffff81e72b7c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
> 
> It turns out that we free fs root twice, btrfs_init_fs_root() calls
> free_anon_bdev(root->anon_dev) and later then btrfs_get_fs_root() cals
> free_fs_root which does another free_anon_bdev() and it ends up with the
> above warning.
> 
> Instead of reset root->anon_dev to 0 after free_anon_bdev(), we can let
> btrfs_init_fs_root() return directly since its callers have already done
> the free job by calling free_fs_root().
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 60ce119..6c88c63 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1600,14 +1600,14 @@  int btrfs_init_fs_root(struct btrfs_root *root)
 
 	ret = get_anon_bdev(&root->anon_dev);
 	if (ret)
-		goto free_writers;
+		goto fail;
 
 	mutex_lock(&root->objectid_mutex);
 	ret = btrfs_find_highest_objectid(root,
 					&root->highest_objectid);
 	if (ret) {
 		mutex_unlock(&root->objectid_mutex);
-		goto free_root_dev;
+		goto fail;
 	}
 
 	ASSERT(root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
@@ -1615,14 +1615,8 @@  int btrfs_init_fs_root(struct btrfs_root *root)
 	mutex_unlock(&root->objectid_mutex);
 
 	return 0;
-
-free_root_dev:
-	free_anon_bdev(root->anon_dev);
-free_writers:
-	btrfs_free_subvolume_writers(root->subv_writers);
 fail:
-	kfree(root->free_ino_ctl);
-	kfree(root->free_ino_pinned);
+	/* the caller is responsible to call free_fs_root */
 	return ret;
 }

Btrfs: fix double free of fs root

Commit Message

Comments

Patch