| Message ID | 20190711152304.11438-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free checksum hash on in close_ctree | expand |
On 2019/7/11 下午11:23, Johannes Thumshirn wrote: > fs_info::csum_hash gets initialized in btrfs_init_csum_hash() which is > called by open_ctree(). > > But it only gets freed if open_ctree() fails, not on normal operation. > > This leads to a memory leak like the following found by kmemleak: > unreferenced object 0xffff888132cb8720 (size 96): > comm "mount", pid 450, jiffies 4294912436 (age 17.584s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<000000000c9643d4>] crypto_create_tfm+0x2d/0xd0 > [<00000000ae577f68>] crypto_alloc_tfm+0x4b/0xb0 > [<000000002b5cdf30>] open_ctree+0xb84/0x2060 [btrfs] > [<0000000043204297>] btrfs_mount_root+0x552/0x640 [btrfs] > [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 > [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 > [<00000000f180080e>] fc_mount+0x9/0x30 > [<000000009e36cebd>] vfs_kern_mount.part.11+0x6a/0x80 > [<0000000004594c05>] btrfs_mount+0x174/0x910 [btrfs] > [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 > [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 > [<00000000b86e92c5>] do_mount+0x6b0/0x940 > [<0000000097464494>] ksys_mount+0x7b/0xd0 > [<0000000057213c80>] __x64_sys_mount+0x1c/0x20 > [<00000000cb689b5e>] do_syscall_64+0x43/0x130 > [<000000002194e289>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Free fs_info::csum_hash in close_ctree() to avoid the memory leak. > > Fixes: 6d97c6e31b55 ("btrfs: add boilerplate code for directly including the crypto framework") Not yet in upstream, thus I believe David could just fold this fix into the original commit. > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Reviewed-by: Qu Wenruo <wqu@suse.com> Although for the folding case, that reviewed-by won't make much sense. Thanks, Qu > --- > fs/btrfs/disk-io.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 41a2bd2e0c56..5f7ee70b3d1a 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4106,6 +4106,7 @@ void close_ctree(struct btrfs_fs_info *fs_info) > percpu_counter_destroy(&fs_info->dev_replace.bio_counter); > cleanup_srcu_struct(&fs_info->subvol_srcu); > > + btrfs_free_csum_hash(fs_info); > btrfs_free_stripe_hash_table(fs_info); > btrfs_free_ref_cache(fs_info); > } >
On Fri, Jul 12, 2019 at 03:34:45PM +0800, Qu Wenruo wrote: > Not yet in upstream, thus I believe David could just fold this fix into > the original commit. Right, I just didn't know if David's gonna rebase his branch before the pull request. AFAIK Linus doesn't really like recently rebased branches. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although for the folding case, that reviewed-by won't make much sense. Thanks anyways, Johannes
On Fri, Jul 12, 2019 at 11:21:38AM +0200, Johannes Thumshirn wrote: > On Fri, Jul 12, 2019 at 03:34:45PM +0800, Qu Wenruo wrote: > > Not yet in upstream, thus I believe David could just fold this fix into > > the original commit. > > Right, I just didn't know if David's gonna rebase his branch before the pull > request. AFAIK Linus doesn't really like recently rebased branches. The branch for pull request is frozen at least a week before the merge window starts and in exceptional and justified cases some changes are possible, but certainly not rebaasing half of the branch. As we found out, there are several problems with leaks, some of them known before the merge window, but we have the rc milestones to fix that.
On Thu, Jul 11, 2019 at 05:23:04PM +0200, Johannes Thumshirn wrote: > fs_info::csum_hash gets initialized in btrfs_init_csum_hash() which is > called by open_ctree(). > > But it only gets freed if open_ctree() fails, not on normal operation. > > This leads to a memory leak like the following found by kmemleak: > unreferenced object 0xffff888132cb8720 (size 96): > comm "mount", pid 450, jiffies 4294912436 (age 17.584s) > hex dump (first 32 bytes): > 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<000000000c9643d4>] crypto_create_tfm+0x2d/0xd0 > [<00000000ae577f68>] crypto_alloc_tfm+0x4b/0xb0 > [<000000002b5cdf30>] open_ctree+0xb84/0x2060 [btrfs] > [<0000000043204297>] btrfs_mount_root+0x552/0x640 [btrfs] > [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 > [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 > [<00000000f180080e>] fc_mount+0x9/0x30 > [<000000009e36cebd>] vfs_kern_mount.part.11+0x6a/0x80 > [<0000000004594c05>] btrfs_mount+0x174/0x910 [btrfs] > [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 > [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 > [<00000000b86e92c5>] do_mount+0x6b0/0x940 > [<0000000097464494>] ksys_mount+0x7b/0xd0 > [<0000000057213c80>] __x64_sys_mount+0x1c/0x20 > [<00000000cb689b5e>] do_syscall_64+0x43/0x130 > [<000000002194e289>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Free fs_info::csum_hash in close_ctree() to avoid the memory leak. > > Fixes: 6d97c6e31b55 ("btrfs: add boilerplate code for directly including the crypto framework") > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Added to 5.3 queue, thanks.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 41a2bd2e0c56..5f7ee70b3d1a 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4106,6 +4106,7 @@ void close_ctree(struct btrfs_fs_info *fs_info) percpu_counter_destroy(&fs_info->dev_replace.bio_counter); cleanup_srcu_struct(&fs_info->subvol_srcu); + btrfs_free_csum_hash(fs_info); btrfs_free_stripe_hash_table(fs_info); btrfs_free_ref_cache(fs_info); }
fs_info::csum_hash gets initialized in btrfs_init_csum_hash() which is called by open_ctree(). But it only gets freed if open_ctree() fails, not on normal operation. This leads to a memory leak like the following found by kmemleak: unreferenced object 0xffff888132cb8720 (size 96): comm "mount", pid 450, jiffies 4294912436 (age 17.584s) hex dump (first 32 bytes): 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<000000000c9643d4>] crypto_create_tfm+0x2d/0xd0 [<00000000ae577f68>] crypto_alloc_tfm+0x4b/0xb0 [<000000002b5cdf30>] open_ctree+0xb84/0x2060 [btrfs] [<0000000043204297>] btrfs_mount_root+0x552/0x640 [btrfs] [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 [<00000000f180080e>] fc_mount+0x9/0x30 [<000000009e36cebd>] vfs_kern_mount.part.11+0x6a/0x80 [<0000000004594c05>] btrfs_mount+0x174/0x910 [btrfs] [<00000000c99b10ea>] legacy_get_tree+0x22/0x40 [<0000000071a6495f>] vfs_get_tree+0x1f/0xc0 [<00000000b86e92c5>] do_mount+0x6b0/0x940 [<0000000097464494>] ksys_mount+0x7b/0xd0 [<0000000057213c80>] __x64_sys_mount+0x1c/0x20 [<00000000cb689b5e>] do_syscall_64+0x43/0x130 [<000000002194e289>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 Free fs_info::csum_hash in close_ctree() to avoid the memory leak. Fixes: 6d97c6e31b55 ("btrfs: add boilerplate code for directly including the crypto framework") Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/btrfs/disk-io.c | 1 + 1 file changed, 1 insertion(+)