Btrfs: fix use-after-free during inode eviction
diff mbox series

Message ID 20181012120248.32262-1-fdmanana@kernel.org
State New
Headers show
Series
  • Btrfs: fix use-after-free during inode eviction
Related show

Commit Message

Filipe Manana Oct. 12, 2018, 12:02 p.m. UTC
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>
---
 fs/btrfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Qu Wenruo Oct. 12, 2018, 1:13 p.m. UTC | #1
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,
>
David Sterba Oct. 19, 2018, 10:26 a.m. UTC | #2
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.

Patch
diff mbox series

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,