From: Filipe Manana <fdmanana@suse.com>

Date: Mon, 5 Apr 2021 12:32:16 +0100

Git-commit: 061dde8245356d8864d29e25207aa4daa0be4d3c

Patch-mainline: v5.13-rc1

Subject: [PATCH] btrfs: fix race between transaction aborts and fsyncs leading

 to use-after-free

References: bsc#1186441

There is a race between a task aborting a transaction during a commit,

a task doing an fsync and the transaction kthread, which leads to an

use-after-free of the log root tree. When this happens, it results in a

stack trace like the following:

  BTRFS info (device dm-0): forced readonly

  BTRFS warning (device dm-0): Skipping commit of aborted transaction.

  BTRFS: error (device dm-0) in cleanup_transaction:1958: errno=-5 IO failure

  BTRFS warning (device dm-0): lost page write due to IO error on /dev/mapper/error-test (-5)

  BTRFS warning (device dm-0): Skipping commit of aborted transaction.

  BTRFS warning (device dm-0): direct IO failed ino 261 rw 0,0 sector 0xa4e8 len 4096 err no 10

  BTRFS error (device dm-0): error writing primary super block to device 1

  BTRFS warning (device dm-0): direct IO failed ino 261 rw 0,0 sector 0x12e000 len 4096 err no 10

  BTRFS warning (device dm-0): direct IO failed ino 261 rw 0,0 sector 0x12e008 len 4096 err no 10

  BTRFS warning (device dm-0): direct IO failed ino 261 rw 0,0 sector 0x12e010 len 4096 err no 10

  BTRFS: error (device dm-0) in write_all_supers:4110: errno=-5 IO failure (1 errors while writing supers)

  BTRFS: error (device dm-0) in btrfs_sync_log:3308: errno=-5 IO failure

  general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b68: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI

  CPU: 2 PID: 2458471 Comm: fsstress Not tainted 5.12.0-rc5-btrfs-next-84 #1

  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014

  RIP: 0010:__mutex_lock+0x139/0xa40

  Code: c0 74 19 (...)

  RSP: 0018:ffff9f18830d7b00 EFLAGS: 00010202

  RAX: 6b6b6b6b6b6b6b68 RBX: 0000000000000001 RCX: 0000000000000002

  RDX: ffffffffb9c54d13 RSI: 0000000000000000 RDI: 0000000000000000

  RBP: ffff9f18830d7bc0 R08: 0000000000000000 R09: 0000000000000000

  R10: ffff9f18830d7be0 R11: 0000000000000001 R12: ffff8c6cd199c040

  R13: ffff8c6c95821358 R14: 00000000fffffffb R15: ffff8c6cbcf01358

  FS:  00007fa9140c2b80(0000) GS:ffff8c6fac600000(0000) knlGS:0000000000000000

  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033

  CR2: 00007fa913d52000 CR3: 000000013d2b4003 CR4: 0000000000370ee0

  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000

  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

  Call Trace:

   ? __btrfs_handle_fs_error+0xde/0x146 [btrfs]

   ? btrfs_sync_log+0x7c1/0xf20 [btrfs]

   ? btrfs_sync_log+0x7c1/0xf20 [btrfs]

   btrfs_sync_log+0x7c1/0xf20 [btrfs]

   btrfs_sync_file+0x40c/0x580 [btrfs]

   do_fsync+0x38/0x70

   __x64_sys_fsync+0x10/0x20

   do_syscall_64+0x33/0x80

   entry_SYSCALL_64_after_hwframe+0x44/0xae

  RIP: 0033:0x7fa9142a55c3

  Code: 8b 15 09 (...)

  RSP: 002b:00007fff26278d48 EFLAGS: 00000246 ORIG_RAX: 000000000000004a

  RAX: ffffffffffffffda RBX: 0000563c83cb4560 RCX: 00007fa9142a55c3

  RDX: 00007fff26278cb0 RSI: 00007fff26278cb0 RDI: 0000000000000005

  RBP: 0000000000000005 R08: 0000000000000001 R09: 00007fff26278d5c

  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000340

  R13: 00007fff26278de0 R14: 00007fff26278d96 R15: 0000563c83ca57c0

  Modules linked in: btrfs dm_zero dm_snapshot dm_thin_pool (...)

  ---[ end trace ee2f1b19327d791d ]---

The steps that lead to this crash are the following:

1) We are at transaction N;

2) We have two tasks with a transaction handle attached to transaction N.

   Task A and Task B. Task B is doing an fsync;

3) Task B is at btrfs_sync_log(), and has saved fs_info->log_root_tree

   into a local variable named 'log_root_tree' at the top of

   btrfs_sync_log(). Task B is about to call write_all_supers(), but

   before that...

4) Task A calls btrfs_commit_transaction(), and after it sets the

   transaction state to TRANS_STATE_COMMIT_START, an error happens before

   it waits for the transaction's 'num_writers' counter to reach a value

   of 1 (no one else attached to the transaction), so it jumps to the

   label "cleanup_transaction";

5) Task A then calls cleanup_transaction(), where it aborts the

   transaction, setting BTRFS_FS_STATE_TRANS_ABORTED on fs_info->fs_state,

   setting the ->aborted field of the transaction and the handle to an

   errno value and also setting BTRFS_FS_STATE_ERROR on fs_info->fs_state.

   After that, at cleanup_transaction(), it deletes the transaction from

   the list of transactions (fs_info->trans_list), sets the transaction

   to the state TRANS_STATE_COMMIT_DOING and then waits for the number

   of writers to go down to 1, as it's currently 2 (1 for task A and 1

   for task B);

6) The transaction kthread is running and sees that BTRFS_FS_STATE_ERROR

   is set in fs_info->fs_state, so it calls btrfs_cleanup_transaction().

   There it sees the list fs_info->trans_list is empty, and then proceeds

   into calling btrfs_drop_all_logs(), which frees the log root tree with

   a call to btrfs_free_log_root_tree();

7) Task B calls write_all_supers() and, shortly after, under the label

   'out_wake_log_root', it deferences the pointer stored in

   'log_root_tree', which was already freed in the previous step by the

   transaction kthread. This results in a use-after-free leading to a

   crash.

Fix this by deleting the transaction from the list of transactions at

cleanup_transaction() only after setting the transaction state to

TRANS_STATE_COMMIT_DOING and waiting for all existing tasks that are

attached to the transaction to release their transaction handles.

This makes the transaction kthread wait for all the tasks attached to

the transaction to be done with the transaction before dropping the

log roots and doing other cleanups.

Fixes: ef67963dac255b ("btrfs: drop logs when we've aborted a transaction")

CC: stable@vger.kernel.org # 5.10+

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Signed-off-by: Filipe Manana <fdmanana@suse.com>

Signed-off-by: David Sterba <dsterba@suse.com>

---

 fs/btrfs/transaction.c | 12 +++++++++++-

 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c

index 62f5195d0076..f75de9f6c0ad 100644

--- a/fs/btrfs/transaction.c

+++ b/fs/btrfs/transaction.c

@@ -1986,7 +1986,6 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans, int err)

 	 */

 	BUG_ON(list_empty(&cur_trans->list));

-	list_del_init(&cur_trans->list);

 	if (cur_trans == fs_info->running_transaction) {

 		cur_trans->state = TRANS_STATE_COMMIT_DOING;

 		spin_unlock(&fs_info->trans_lock);

@@ -1995,6 +1994,17 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans, int err)

 		spin_lock(&fs_info->trans_lock);

 	}

+

+	/*

+	 * Now that we know no one else is still using the transaction we can

+	 * remove the transaction from the list of transactions. This avoids

+	 * the transaction kthread from cleaning up the transaction while some

+	 * other task is still using it, which could result in a use-after-free

+	 * on things like log trees, as it forces the transaction kthread to

+	 * wait for this transaction to be cleaned up by us.

+	 */

+	list_del_init(&cur_trans->list);

+

 	spin_unlock(&fs_info->trans_lock);

 	btrfs_cleanup_one_transaction(trans->transaction, fs_info);

-- 

2.26.2