| Message ID | 20181012120248.32262-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: fix use-after-free during inode eviction | expand |
On 2018/10/12 下午8:02, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@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: stable@vger.kernel.org # 4.4+ > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@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, >
On Fri, Oct 12, 2018 at 01:02:48PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@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: stable@vger.kernel.org # 4.4+ > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Added to misc-next, thanks.
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,