btrfs: fix memory leak in qgroup accounting
diff mbox series

Message ID 20200108120732.30451-1-johannes.thumshirn@wdc.com
State New
Headers show
Series
  • btrfs: fix memory leak in qgroup accounting
Related show

Commit Message

Johannes Thumshirn Jan. 8, 2020, 12:07 p.m. UTC
When running xfstests on the current btrfs I get the following splat from
kmemleak:
unreferenced object 0xffff88821b2404e0 (size 32):
  comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s)
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff  ...........&....
    10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff  ...&.... ..&....
  backtrace:
    [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs]
    [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs]
    [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs]
    [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs]
    [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs]
    [<00000000f0188930>] process_one_work+0x1cf/0x350
    [<00000000af5f2f8e>] worker_thread+0x28/0x3c0
    [<00000000b55a1add>] kthread+0x109/0x120
    [<00000000f88cbd17>] ret_from_fork+0x35/0x40

This corresponds to:
(gdb) l *(btrfs_find_all_roots_safe+0x41)
0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413).
1408
1409            tmp = ulist_alloc(GFP_NOFS);
1410            if (!tmp)
1411                    return -ENOMEM;
1412            *roots = ulist_alloc(GFP_NOFS);
1413            if (!*roots) {
1414                    ulist_free(tmp);
1415                    return -ENOMEM;
1416            }
1417

Following the lifetime of the allocated 'roots' ulist, it get's freed
again in btrfs_qgroup_account_extent().

But this does not happen if the function is called with the
'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent()
does a short leave and directly returns.

Instead of directly returning we should jump to the 'out_free' in order to
free all resources as expected.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Qu Wenruo Jan. 8, 2020, 12:16 p.m. UTC | #1
On 2020/1/8 下午8:07, Johannes Thumshirn wrote:
> When running xfstests on the current btrfs I get the following splat from
> kmemleak:
> unreferenced object 0xffff88821b2404e0 (size 32):
>   comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff  ...........&....
>     10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff  ...&.... ..&....
>   backtrace:
>     [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs]
>     [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs]
>     [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs]
>     [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs]
>     [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs]
>     [<00000000f0188930>] process_one_work+0x1cf/0x350
>     [<00000000af5f2f8e>] worker_thread+0x28/0x3c0
>     [<00000000b55a1add>] kthread+0x109/0x120
>     [<00000000f88cbd17>] ret_from_fork+0x35/0x40
> 
> This corresponds to:
> (gdb) l *(btrfs_find_all_roots_safe+0x41)
> 0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413).
> 1408
> 1409            tmp = ulist_alloc(GFP_NOFS);
> 1410            if (!tmp)
> 1411                    return -ENOMEM;
> 1412            *roots = ulist_alloc(GFP_NOFS);
> 1413            if (!*roots) {
> 1414                    ulist_free(tmp);
> 1415                    return -ENOMEM;
> 1416            }
> 1417
> 
> Following the lifetime of the allocated 'roots' ulist, it get's freed
> again in btrfs_qgroup_account_extent().
> 
> But this does not happen if the function is called with the
> 'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent()
> does a short leave and directly returns.
> 
> Instead of directly returning we should jump to the 'out_free' in order to
> free all resources as expected.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

The patch itself is OK.

Reviewed-by: Qu Wenruo <wqu@suse.com>


This means the qgroup get disabled when rescan is still running.
So I'm a little curious, could we just cancel the running rescan and
wait for it before disabling qgroup?

Thanks,
Qu

> ---
>  fs/btrfs/qgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index b046b04d7cce..7ebcdd201eed 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2416,7 +2416,7 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>  	int ret = 0;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
> -		return 0;
> +		goto out_free;
>  
>  	if (new_roots) {
>  		if (!maybe_fs_roots(new_roots))
>
Johannes Thumshirn Jan. 8, 2020, 12:31 p.m. UTC | #2
On 08/01/2020, 13:16, "Qu Wenruo" <quwenruo.btrfs@gmx.com> wrote:
[...]       

    The patch itself is OK.
    
    Reviewed-by: Qu Wenruo <wqu@suse.com>
    
    
    This means the qgroup get disabled when rescan is still running.
    So I'm a little curious, could we just cancel the running rescan and
    wait for it before disabling qgroup?
    
    
Maybe. I'm still not 100% certain what's the actual trigger. I see it most of the time
with btrfs/117, but running btrfs/117 alone doesn't trigger the memleak report.
I also saw the memleak report once with btrfs/116, but only once out of ~20 runs.
The good thing though is, it's 100% reproducible that we get the memleak report when
running the first 120 btrfs fstests .

P.S.: Sorry for the broken quoting in the reply, I still haven't configured my mail client correctly.

Byte,
	Johannes
Qu Wenruo Jan. 8, 2020, 12:38 p.m. UTC | #3
On 2020/1/8 下午8:31, Johannes Thumshirn wrote:
> On 08/01/2020, 13:16, "Qu Wenruo" <quwenruo.btrfs@gmx.com> wrote:
> [...]       
> 
>     The patch itself is OK.
>     
>     Reviewed-by: Qu Wenruo <wqu@suse.com>
>     
>     
>     This means the qgroup get disabled when rescan is still running.
>     So I'm a little curious, could we just cancel the running rescan and
>     wait for it before disabling qgroup?
>     
>     
> Maybe. I'm still not 100% certain what's the actual trigger. I see it most of the time
> with btrfs/117, but running btrfs/117 alone doesn't trigger the memleak report.
> I also saw the memleak report once with btrfs/116, but only once out of ~20 runs.
> The good thing though is, it's 100% reproducible that we get the memleak report when
> running the first 120 btrfs fstests.

No problem, since the patch itself is already good enough to solve all
the cases.

> 
> P.S.: Sorry for the broken quoting in the reply, I still haven't configured my mail client correctly.

Have fun in WDC!

Thanks,
Qu

> 
> Byte,
> 	Johannes
>
David Sterba Jan. 8, 2020, 4:55 p.m. UTC | #4
On Wed, Jan 08, 2020 at 09:07:32PM +0900, Johannes Thumshirn wrote:
> When running xfstests on the current btrfs I get the following splat from
> kmemleak:
> unreferenced object 0xffff88821b2404e0 (size 32):
>   comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff  ...........&....
>     10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff  ...&.... ..&....
>   backtrace:
>     [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs]
>     [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs]
>     [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs]
>     [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs]
>     [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs]
>     [<00000000f0188930>] process_one_work+0x1cf/0x350
>     [<00000000af5f2f8e>] worker_thread+0x28/0x3c0
>     [<00000000b55a1add>] kthread+0x109/0x120
>     [<00000000f88cbd17>] ret_from_fork+0x35/0x40
> 
> This corresponds to:
> (gdb) l *(btrfs_find_all_roots_safe+0x41)
> 0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413).
> 1408
> 1409            tmp = ulist_alloc(GFP_NOFS);
> 1410            if (!tmp)
> 1411                    return -ENOMEM;
> 1412            *roots = ulist_alloc(GFP_NOFS);
> 1413            if (!*roots) {
> 1414                    ulist_free(tmp);
> 1415                    return -ENOMEM;
> 1416            }
> 1417
> 
> Following the lifetime of the allocated 'roots' ulist, it get's freed
> again in btrfs_qgroup_account_extent().
> 
> But this does not happen if the function is called with the
> 'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent()
> does a short leave and directly returns.
> 
> Instead of directly returning we should jump to the 'out_free' in order to
> free all resources as expected.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Added to misc-next, with a short comment that we can't do a quick
return. Thanks.

Patch
diff mbox series

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b046b04d7cce..7ebcdd201eed 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2416,7 +2416,7 @@  int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	int ret = 0;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
-		return 0;
+		goto out_free;
 
 	if (new_roots) {
 		if (!maybe_fs_roots(new_roots))