| From 281b08fa14ebbe97dadb8b3e5b12bc6d33b1012a Mon Sep 17 00:00:00 2001 |
| From: Josef Bacik <josef@toxicpanda.com> |
| Date: Fri, 17 Jan 2020 08:57:51 -0500 |
| Subject: [PATCH] btrfs: set trans->drity in btrfs_commit_transaction |
| |
| commit d62b23c94952e78211a383b7d90ef0afbd9a3717 upstream. |
| |
| If we abort a transaction we have the following sequence |
| |
| if (!trans->dirty && list_empty(&trans->new_bgs)) |
| return; |
| WRITE_ONCE(trans->transaction->aborted, err); |
| |
| The idea being if we didn't modify anything with our trans handle then |
| we don't really need to abort the whole transaction, maybe the other |
| trans handles are fine and we can carry on. |
| |
| However in the case of create_snapshot we add a pending_snapshot object |
| to our transaction and then commit the transaction. We don't actually |
| modify anything. sync() behaves the same way, attach to an existing |
| transaction and commit it. This means that if we have an IO error in |
| the right places we could abort the committing transaction with our |
| trans->dirty being not set and thus not set transaction->aborted. |
| |
| This is a problem because in the create_snapshot() case we depend on |
| pending->error being set to something, or btrfs_commit_transaction |
| returning an error. |
| |
| If we are not the trans handle that gets to commit the transaction, and |
| we're waiting on the commit to happen we get our return value from |
| cur_trans->aborted. If this was not set to anything because sync() hit |
| an error in the transaction commit before it could modify anything then |
| cur_trans->aborted would be 0. Thus we'd return 0 from |
| btrfs_commit_transaction() in create_snapshot. |
| |
| This is a problem because we then try to do things with |
| pending_snapshot->snap, which will be NULL because we didn't create the |
| snapshot, and then we'll get a NULL pointer dereference like the |
| following |
| |
| "BUG: kernel NULL pointer dereference, address: 00000000000001f0" |
| RIP: 0010:btrfs_orphan_cleanup+0x2d/0x330 |
| Call Trace: |
| ? btrfs_mksubvol.isra.31+0x3f2/0x510 |
| btrfs_mksubvol.isra.31+0x4bc/0x510 |
| ? __sb_start_write+0xfa/0x200 |
| ? mnt_want_write_file+0x24/0x50 |
| btrfs_ioctl_snap_create_transid+0x16c/0x1a0 |
| btrfs_ioctl_snap_create_v2+0x11e/0x1a0 |
| btrfs_ioctl+0x1534/0x2c10 |
| ? free_debug_processing+0x262/0x2a3 |
| do_vfs_ioctl+0xa6/0x6b0 |
| ? do_sys_open+0x188/0x220 |
| ? syscall_trace_enter+0x1f8/0x330 |
| ksys_ioctl+0x60/0x90 |
| __x64_sys_ioctl+0x16/0x20 |
| do_syscall_64+0x4a/0x1b0 |
| |
| In order to fix this we need to make sure anybody who calls |
| commit_transaction has trans->dirty set so that they properly set the |
| trans->transaction->aborted value properly so any waiters know bad |
| things happened. |
| |
| This was found while I was running generic/475 with my modified |
| fsstress, it reproduced within a few runs. I ran with this patch all |
| night and didn't see the problem again. |
| |
| CC: stable@vger.kernel.org # 4.4+ |
| Signed-off-by: Josef Bacik <josef@toxicpanda.com> |
| Reviewed-by: David Sterba <dsterba@suse.com> |
| Signed-off-by: David Sterba <dsterba@suse.com> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c |
| index 2db14fdd6bff..f48b8e91949f 100644 |
| --- a/fs/btrfs/transaction.c |
| +++ b/fs/btrfs/transaction.c |
| @@ -1929,6 +1929,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) |
| struct btrfs_transaction *prev_trans = NULL; |
| int ret; |
| |
| + /* |
| + * Some places just start a transaction to commit it. We need to make |
| + * sure that if this commit fails that the abort code actually marks the |
| + * transaction as failed, so set trans->dirty to make the abort code do |
| + * the right thing. |
| + */ |
| + trans->dirty = true; |
| + |
| /* Stop the commit early if ->aborted is set */ |
| if (unlikely(READ_ONCE(cur_trans->aborted))) { |
| ret = cur_trans->aborted; |
| -- |
| 2.7.4 |
| |