On Mon, Mar 04, 2013 at 05:44:29PM +0800, Miao Xie wrote:
> There are several bugs at error path of create_snapshot() when the
> transaction commitment failed.
> - access the freed transaction handler. At the end of the
> transaction commitment, the transaction handler was freed, so we
> should not access it after the transaction commitment.
> - we were not aware of the error which happened during the snapshot
> creation if we submitted a async transaction commitment.
> - pending snapshot access vs pending snapshot free. when something
> wrong happened after we submitted a async transaction commitment,
> the transaction committer would cleanup the pending snapshots and
> free them. But the snapshot creators were not aware of it, they
> would access the freed pending snapshots.
>
> This patch fixes the above problems by:
> - remove the dangerous code that accessed the freed handler
> - assign ->error if the error happens during the snapshot creation
> - the transaction committer doesn't free the pending snapshots,
> just assigns the error number and evicts them before we unblock
> the transaction.
>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Miao Xie <miaox@xxxxxxxxxxxxxx>
> ---
> fs/btrfs/disk-io.c | 16 +++++-------
> fs/btrfs/ioctl.c | 6 +----
> fs/btrfs/transaction.c | 58 +++++++++++++++++++++++++++--------------------
> 3 files changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 02369a3..7d84651 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -62,7 +62,7 @@ static void btrfs_destroy_ordered_operations(struct btrfs_transaction *t,
> static void btrfs_destroy_ordered_extents(struct btrfs_root *root);
> static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
> struct btrfs_root *root);
> -static void btrfs_destroy_pending_snapshots(struct btrfs_transaction *t);
> +static void btrfs_evict_pending_snapshots(struct btrfs_transaction *t);
> static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root);
> static int btrfs_destroy_marked_extents(struct btrfs_root *root,
> struct extent_io_tree *dirty_pages,
> @@ -3687,7 +3687,7 @@ int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
> return ret;
> }
>
> -static void btrfs_destroy_pending_snapshots(struct btrfs_transaction *t)
> +static void btrfs_evict_pending_snapshots(struct btrfs_transaction *t)
> {
> struct btrfs_pending_snapshot *snapshot;
> struct list_head splice;
> @@ -3700,10 +3700,8 @@ static void btrfs_destroy_pending_snapshots(struct btrfs_transaction *t)
> snapshot = list_entry(splice.next,
> struct btrfs_pending_snapshot,
> list);
> -
> + snapshot->error = -ECANCELED;
ECANCELED or EROFS? Now that EROFS is why we're here.
Others look good.
thanks,
liubo
> list_del_init(&snapshot->list);
> -
> - kfree(snapshot);
> }
> }
>
> @@ -3840,6 +3838,8 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
> cur_trans->blocked = 1;
> wake_up(&root->fs_info->transaction_blocked_wait);
>
> + btrfs_evict_pending_snapshots(cur_trans);
> +
> cur_trans->blocked = 0;
> wake_up(&root->fs_info->transaction_wait);
>
> @@ -3849,8 +3849,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
> btrfs_destroy_delayed_inodes(root);
> btrfs_assert_delayed_root_empty(root);
>
> - btrfs_destroy_pending_snapshots(cur_trans);
> -
> btrfs_destroy_marked_extents(root, &cur_trans->dirty_pages,
> EXTENT_DIRTY);
> btrfs_destroy_pinned_extent(root,
> @@ -3894,6 +3892,8 @@ int btrfs_cleanup_transaction(struct btrfs_root *root)
> if (waitqueue_active(&root->fs_info->transaction_blocked_wait))
> wake_up(&root->fs_info->transaction_blocked_wait);
>
> + btrfs_evict_pending_snapshots(t);
> +
> t->blocked = 0;
> smp_mb();
> if (waitqueue_active(&root->fs_info->transaction_wait))
> @@ -3907,8 +3907,6 @@ int btrfs_cleanup_transaction(struct btrfs_root *root)
> btrfs_destroy_delayed_inodes(root);
> btrfs_assert_delayed_root_empty(root);
>
> - btrfs_destroy_pending_snapshots(t);
> -
> btrfs_destroy_delalloc_inodes(root);
>
> spin_lock(&root->fs_info->trans_lock);
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b908960..94c0e42 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -596,12 +596,8 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
> ret = btrfs_commit_transaction(trans,
> root->fs_info->extent_root);
> }
> - if (ret) {
> - /* cleanup_transaction has freed this for us */
> - if (trans->aborted)
> - pending_snapshot = NULL;
> + if (ret)
> goto fail;
> - }
>
> ret = pending_snapshot->error;
> if (ret)
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index f11c2e0..d8fce6f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1053,7 +1053,12 @@ int btrfs_defrag_root(struct btrfs_root *root)
>
> /*
> * new snapshots need to be created at a very specific time in the
> - * transaction commit. This does the actual creation
> + * transaction commit. This does the actual creation.
> + *
> + * Note:
> + * If the error which may affect the commitment of the current transaction
> + * happens, we should return the error number. If the error which just affect
> + * the creation of the pending snapshots, just return 0.
> */
> static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> struct btrfs_fs_info *fs_info,
> @@ -1072,7 +1077,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> struct extent_buffer *tmp;
> struct extent_buffer *old;
> struct timespec cur_time = CURRENT_TIME;
> - int ret;
> + int ret = 0;
> u64 to_reserve = 0;
> u64 index = 0;
> u64 objectid;
> @@ -1081,40 +1086,36 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>
> path = btrfs_alloc_path();
> if (!path) {
> - ret = pending->error = -ENOMEM;
> - return ret;
> + pending->error = -ENOMEM;
> + return 0;
> }
>
> new_root_item = kmalloc(sizeof(*new_root_item), GFP_NOFS);
> if (!new_root_item) {
> - ret = pending->error = -ENOMEM;
> + pending->error = -ENOMEM;
> goto root_item_alloc_fail;
> }
>
> - ret = btrfs_find_free_objectid(tree_root, &objectid);
> - if (ret) {
> - pending->error = ret;
> + pending->error = btrfs_find_free_objectid(tree_root, &objectid);
> + if (pending->error)
> goto no_free_objectid;
> - }
>
> btrfs_reloc_pre_snapshot(trans, pending, &to_reserve);
>
> if (to_reserve > 0) {
> - ret = btrfs_block_rsv_add(root, &pending->block_rsv,
> - to_reserve,
> - BTRFS_RESERVE_NO_FLUSH);
> - if (ret) {
> - pending->error = ret;
> + pending->error = btrfs_block_rsv_add(root,
> + &pending->block_rsv,
> + to_reserve,
> + BTRFS_RESERVE_NO_FLUSH);
> + if (pending->error)
> goto no_free_objectid;
> - }
> }
>
> - ret = btrfs_qgroup_inherit(trans, fs_info, root->root_key.objectid,
> - objectid, pending->inherit);
> - if (ret) {
> - pending->error = ret;
> + pending->error = btrfs_qgroup_inherit(trans, fs_info,
> + root->root_key.objectid,
> + objectid, pending->inherit);
> + if (pending->error)
> goto no_free_objectid;
> - }
>
> key.objectid = objectid;
> key.offset = (u64)-1;
> @@ -1142,7 +1143,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> dentry->d_name.len, 0);
> if (dir_item != NULL && !IS_ERR(dir_item)) {
> pending->error = -EEXIST;
> - goto fail;
> + goto dir_item_existed;
> } else if (IS_ERR(dir_item)) {
> ret = PTR_ERR(dir_item);
> btrfs_abort_transaction(trans, root, ret);
> @@ -1273,6 +1274,8 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> if (ret)
> btrfs_abort_transaction(trans, root, ret);
> fail:
> + pending->error = ret;
> +dir_item_existed:
> trans->block_rsv = rsv;
> trans->bytes_reserved = 0;
> no_free_objectid:
> @@ -1288,12 +1291,17 @@ root_item_alloc_fail:
> static noinline int create_pending_snapshots(struct btrfs_trans_handle *trans,
> struct btrfs_fs_info *fs_info)
> {
> - struct btrfs_pending_snapshot *pending;
> + struct btrfs_pending_snapshot *pending, *next;
> struct list_head *head = &trans->transaction->pending_snapshots;
> + int ret = 0;
>
> - list_for_each_entry(pending, head, list)
> - create_pending_snapshot(trans, fs_info, pending);
> - return 0;
> + list_for_each_entry_safe(pending, next, head, list) {
> + list_del(&pending->list);
> + ret = create_pending_snapshot(trans, fs_info, pending);
> + if (ret)
> + break;
> + }
> + return ret;
> }
>
> static void update_super_roots(struct btrfs_root *root)
> --
> 1.6.5.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
