On 2018/10/12 下午8:02, fdmanana@xxxxxxxxxx wrote:
> From: Filipe Manana <fdmanana@xxxxxxxx>
>
> At inode.c:evict_inode_truncate_pages(), when we iterate over the inode's
> extent states, we access an extent state record's "state" field after we
> unlocked the inode's io tree lock. This can lead to a use-after-free issue
> because after we unlock the io tree that extent state record might have
> been freed due to being merged into another adjacent extent state
> record (a previous inflight bio for a read operation finished in the
> meanwhile which unlocked a range in the io tree and cause a merge of
> extent state records, as explained in the comment before the while loop
> added in commit 6ca0709756710 ("Btrfs: fix hang during inode eviction due
> to concurrent readahead")).
>
> Fix this by keeping a copy of the extent state's flags in a local variable
> and using it after unlocking the io tree.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201189
> Fixes: b9d0b38928e2 ("btrfs: Add handler for invalidate page")
> CC: stable@xxxxxxxxxxxxxxx # 4.4+
> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
Indeed my fault, state should only be accessed with spinlock hold.
Thanks,
Qu
> ---
> fs/btrfs/inode.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3ea5339603cf..66c6c4103d2f 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5274,11 +5274,13 @@ static void evict_inode_truncate_pages(struct inode *inode)
> struct extent_state *cached_state = NULL;
> u64 start;
> u64 end;
> + unsigned state_flags;
>
> node = rb_first(&io_tree->state);
> state = rb_entry(node, struct extent_state, rb_node);
> start = state->start;
> end = state->end;
> + state_flags = state->state;
> spin_unlock(&io_tree->lock);
>
> lock_extent_bits(io_tree, start, end, &cached_state);
> @@ -5291,7 +5293,7 @@ static void evict_inode_truncate_pages(struct inode *inode)
> *
> * Note, end is the bytenr of last byte, so we need + 1 here.
> */
> - if (state->state & EXTENT_DELALLOC)
> + if (state_flags & EXTENT_DELALLOC)
> btrfs_qgroup_free_data(inode, NULL, start, end - start + 1);
>
> clear_extent_bit(io_tree, start, end,
>
Attachment:
signature.asc
Description: OpenPGP digital signature
