Btrfs: fix use-after-free during inode eviction

PATCH In review [email protected] 3 participants 3 messages Oct 12, 2018 – Oct 19, 2018 Btrfs file system

lore

Share & tools

lore

[email protected]

author

2018-10-12 12:02 UTC #0 lore raw

PATCH 1/1 Btrfs: fix use-after-free during inode eviction

From: Filipe Manana <[email protected]>

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 [email protected] # 4.4+

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/btrfs/inode.c +3 -1

index 3ea5339603cf..66c6c4103d2f 100644
@@ -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 · 1 line

2.11.0

Qu Wenruo

Reviewed-by on v1 1/1

2018-10-12 13:13 UTC #1 lore raw

Quoted 13 lines · On 2018/10/12 下午8:02, [email protected] wrote:

From: Filipe Manana <[email protected]>
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")).

Quoted 7 lines

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 [email protected] # 4.4+
Signed-off-by Filipe Manana <[email protected]>

Reviewed-by Qu Wenruo <[email protected]>

Indeed my fault, state should only be accessed with spinlock hold.

Thanks,
Qu

Quoted 32 lines

---
 fs/btrfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/btrfs/inode.c +3 -1

index 3ea5339603cf..66c6c4103d2f 100644
@@ -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

Reviewed-by on v1 1/1

2018-10-19 10:26 UTC #2 lore raw

In reply to [email protected]

Quoted 13 lines · On Fri, Oct 12, 2018 at 01:02:48PM +0100, [email protected] wrote:

From: Filipe Manana <[email protected]>
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")).

Quoted 7 lines

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 [email protected] # 4.4+
Signed-off-by Filipe Manana <[email protected]>

Reviewed-by David Sterba <[email protected]>

Added to misc-next, thanks.