lkml.org 
[lkml]   [2020]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.8 174/255] btrfs: fix space cache memory leak after transaction abort
    Date
    From: Filipe Manana <fdmanana@suse.com>

    commit bbc37d6e475eee8ffa2156ec813efc6bbb43c06d upstream.

    If a transaction aborts it can cause a memory leak of the pages array of
    a block group's io_ctl structure. The following steps explain how that can
    happen:

    1) Transaction N is committing, currently in state TRANS_STATE_UNBLOCKED
    and it's about to start writing out dirty extent buffers;

    2) Transaction N + 1 already started and another task, task A, just called
    btrfs_commit_transaction() on it;

    3) Block group B was dirtied (extents allocated from it) by transaction
    N + 1, so when task A calls btrfs_start_dirty_block_groups(), at the
    very beginning of the transaction commit, it starts writeback for the
    block group's space cache by calling btrfs_write_out_cache(), which
    allocates the pages array for the block group's io_ctl with a call to
    io_ctl_init(). Block group A is added to the io_list of transaction
    N + 1 by btrfs_start_dirty_block_groups();

    4) While transaction N's commit is writing out the extent buffers, it gets
    an IO error and aborts transaction N, also setting the file system to
    RO mode;

    5) Task A has already returned from btrfs_start_dirty_block_groups(), is at
    btrfs_commit_transaction() and has set transaction N + 1 state to
    TRANS_STATE_COMMIT_START. Immediately after that it checks that the
    filesystem was turned to RO mode, due to transaction N's abort, and
    jumps to the "cleanup_transaction" label. After that we end up at
    btrfs_cleanup_one_transaction() which calls btrfs_cleanup_dirty_bgs().
    That helper finds block group B in the transaction's io_list but it
    never releases the pages array of the block group's io_ctl, resulting in
    a memory leak.

    In fact at the point when we are at btrfs_cleanup_dirty_bgs(), the pages
    array points to pages that were already released by us at
    __btrfs_write_out_cache() through the call to io_ctl_drop_pages(). We end
    up freeing the pages array only after waiting for the ordered extent to
    complete through btrfs_wait_cache_io(), which calls io_ctl_free() to do
    that. But in the transaction abort case we don't wait for the space cache's
    ordered extent to complete through a call to btrfs_wait_cache_io(), so
    that's why we end up with a memory leak - we wait for the ordered extent
    to complete indirectly by shutting down the work queues and waiting for
    any jobs in them to complete before returning from close_ctree().

    We can solve the leak simply by freeing the pages array right after
    releasing the pages (with the call to io_ctl_drop_pages()) at
    __btrfs_write_out_cache(), since we will never use it anymore after that
    and the pages array points to already released pages at that point, which
    is currently not a problem since no one will use it after that, but not a
    good practice anyway since it can easily lead to use-after-free issues.

    So fix this by freeing the pages array right after releasing the pages at
    __btrfs_write_out_cache().

    This issue can often be reproduced with test case generic/475 from fstests
    and kmemleak can detect it and reports it with the following trace:

    unreferenced object 0xffff9bbf009fa600 (size 512):
    comm "fsstress", pid 38807, jiffies 4298504428 (age 22.028s)
    hex dump (first 32 bytes):
    00 a0 7c 4d 3d ed ff ff 40 a0 7c 4d 3d ed ff ff ..|M=...@.|M=...
    80 a0 7c 4d 3d ed ff ff c0 a0 7c 4d 3d ed ff ff ..|M=.....|M=...
    backtrace:
    [<00000000f4b5cfe2>] __kmalloc+0x1a8/0x3e0
    [<0000000028665e7f>] io_ctl_init+0xa7/0x120 [btrfs]
    [<00000000a1f95b2d>] __btrfs_write_out_cache+0x86/0x4a0 [btrfs]
    [<00000000207ea1b0>] btrfs_write_out_cache+0x7f/0xf0 [btrfs]
    [<00000000af21f534>] btrfs_start_dirty_block_groups+0x27b/0x580 [btrfs]
    [<00000000c3c23d44>] btrfs_commit_transaction+0xa6f/0xe70 [btrfs]
    [<000000009588930c>] create_subvol+0x581/0x9a0 [btrfs]
    [<000000009ef2fd7f>] btrfs_mksubvol+0x3fb/0x4a0 [btrfs]
    [<00000000474e5187>] __btrfs_ioctl_snap_create+0x119/0x1a0 [btrfs]
    [<00000000708ee349>] btrfs_ioctl_snap_create_v2+0xb0/0xf0 [btrfs]
    [<00000000ea60106f>] btrfs_ioctl+0x12c/0x3130 [btrfs]
    [<000000005c923d6d>] __x64_sys_ioctl+0x83/0xb0
    [<0000000043ace2c9>] do_syscall_64+0x33/0x80
    [<00000000904efbce>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

    CC: stable@vger.kernel.org # 4.9+
    Reviewed-by: Josef Bacik <josef@toxicpanda.com>
    Signed-off-by: Filipe Manana <fdmanana@suse.com>
    Signed-off-by: David Sterba <dsterba@suse.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    fs/btrfs/disk-io.c | 1 +
    fs/btrfs/free-space-cache.c | 2 +-
    2 files changed, 2 insertions(+), 1 deletion(-)

    --- a/fs/btrfs/disk-io.c
    +++ b/fs/btrfs/disk-io.c
    @@ -4574,6 +4574,7 @@ static void btrfs_cleanup_bg_io(struct b
    cache->io_ctl.inode = NULL;
    iput(inode);
    }
    + ASSERT(cache->io_ctl.pages == NULL);
    btrfs_put_block_group(cache);
    }

    --- a/fs/btrfs/free-space-cache.c
    +++ b/fs/btrfs/free-space-cache.c
    @@ -1186,7 +1186,6 @@ static int __btrfs_wait_cache_io(struct
    ret = update_cache_item(trans, root, inode, path, offset,
    io_ctl->entries, io_ctl->bitmaps);
    out:
    - io_ctl_free(io_ctl);
    if (ret) {
    invalidate_inode_pages2(inode->i_mapping);
    BTRFS_I(inode)->generation = 0;
    @@ -1346,6 +1345,7 @@ static int __btrfs_write_out_cache(struc
    * them out later
    */
    io_ctl_drop_pages(io_ctl);
    + io_ctl_free(io_ctl);

    unlock_extent_cached(&BTRFS_I(inode)->io_tree, 0,
    i_size_read(inode) - 1, &cached_state);

    \
     
     \ /
      Last update: 2020-09-01 17:47    [W:4.334 / U:0.256 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site