On 2018/10/12 下午8:02, fdman...@kernel.org wrote:
> From: Filipe Manana <fdman...@suse.com>
>
> 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: sta...@vger.kernel.org # 4.4+
> Signed-off-by: Filipe Manana <fdman...@suse.com>Reviewed-by: Qu Wenruo <w...@suse.com> 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, >
signature.asc
Description: OpenPGP digital signature
