On Tue, Mar 24, 2020 at 2:48 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> Dave reported a problem where we were panicing with generic/475 with
> misc-5.7. This is because we were doing IO after we had stopped all of
> the worker threads, because we do the log tree cleanup on roots at drop
> time. Cleaning up the log tree may need to do reads if we happened to
> have evicted the blocks from memory.
Here the "may need" is actually a "will always need", because before
calling btrfs_free_fs_roots() at close_ctree(),
we drop all the extent buffers from memory from the btree inode
through the call to invalidate_inode_pages2().
So this causes a use-after-free on the workqueues used for reads while
traversing the log trees during the log dropping, since the workqueues
were freed before right after invalidate_inode_pages2(),
everytime we abort a transaction and we have at least one log root
around that is big enough to not consist of only one leaf.
>
> Because of this simply add a helper to btrfs_cleanup_transaction() that
> will go through and drop all of the log roots. This gets run before we
> do the close_ctree() work, and thus we are allowed to do any reads that
> we would need. I ran this through many iterations of generic/475 with
> constrained memory and I did not see the issue.
>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
Fixes: 8c38938c7bb096 ("btrfs: move the root freeing stuff into btrfs_put_root")
Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
Thanks.
>
> ---
> fs/btrfs/disk-io.c | 36 ++++++++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a6cb5cbbdb9f..d10c7be10f3b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2036,9 +2036,6 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
> for (i = 0; i < ret; i++)
> btrfs_drop_and_free_fs_root(fs_info, gang[i]);
> }
> -
> - if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> - btrfs_free_log_root_tree(NULL, fs_info);
> }
>
> static void btrfs_init_scrub(struct btrfs_fs_info *fs_info)
> @@ -3888,7 +3885,7 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
> spin_unlock(&fs_info->fs_roots_radix_lock);
>
> if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> - btrfs_free_log(NULL, root);
> + ASSERT(root->log_root == NULL);
> if (root->reloc_root) {
> btrfs_put_root(root->reloc_root);
> root->reloc_root = NULL;
> @@ -4211,6 +4208,36 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info)
> up_write(&fs_info->cleanup_work_sem);
> }
>
> +static void btrfs_drop_all_logs(struct btrfs_fs_info *fs_info)
> +{
> + struct btrfs_root *gang[8];
> + u64 root_objectid = 0;
> + int ret;
> +
> + spin_lock(&fs_info->fs_roots_radix_lock);
> + while ((ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> + (void **)gang, root_objectid,
> + ARRAY_SIZE(gang))) != 0) {
> + int i;
> +
> + for (i = 0; i < ret; i++)
> + gang[i] = btrfs_grab_root(gang[i]);
> + spin_unlock(&fs_info->fs_roots_radix_lock);
> +
> + for (i = 0; i < ret; i++) {
> + if (!gang[i])
> + continue;
> + root_objectid = gang[i]->root_key.objectid;
> + btrfs_free_log(NULL, gang[i]);
> + btrfs_put_root(gang[i]);
> + }
> + root_objectid++;
> + spin_lock(&fs_info->fs_roots_radix_lock);
> + }
> + spin_unlock(&fs_info->fs_roots_radix_lock);
> + btrfs_free_log_root_tree(NULL, fs_info);
> +}
> +
> static void btrfs_destroy_ordered_extents(struct btrfs_root *root)
> {
> struct btrfs_ordered_extent *ordered;
> @@ -4603,6 +4630,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
> btrfs_destroy_delayed_inodes(fs_info);
> btrfs_assert_delayed_root_empty(fs_info);
> btrfs_destroy_all_delalloc_inodes(fs_info);
> + btrfs_drop_all_logs(fs_info);
> mutex_unlock(&fs_info->transaction_kthread_mutex);
>
> return 0;
> --
> 2.17.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
