• Filipe Manana's avatar
    btrfs: fix space cache memory leak after transaction abort · bbc37d6e
    Filipe Manana authored
    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: default avatarJosef Bacik <josef@toxicpanda.com>
    Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
    Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
    bbc37d6e
free-space-cache.c 107 KB