btrfs: do not corrupt the fs with rename exchange on a subvol
diff mbox series

Message ID 20191115204306.3446-1-josef@toxicpanda.com
State New
Headers show
Series
  • btrfs: do not corrupt the fs with rename exchange on a subvol
Related show

Commit Message

Josef Bacik Nov. 15, 2019, 8:43 p.m. UTC
Testing with the new fsstress support for subvolumes uncovered a pretty
bad problem with rename exchange on subvolumes.  We're modifying two
different subvolumes, but we only start the transaction on one of them,
so the other one is not added to the dirty root list.  This is caught by
btrfs_cow_block() with a warning because the root has not been updated,
however if we do not modify this root again we'll end up pointing at an
invalid root because the root item is never updated.

Fix this by making sure we add the destination root to the trans list,
the same as we do with normal renames.  This fixes the corruption.

Fixes: cdd1fedf8261 ("btrfs: add support for RENAME_EXCHANGE and RENAME_WHITEOUT")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Filipe Manana Nov. 15, 2019, 9:39 p.m. UTC | #1
On Fri, Nov 15, 2019 at 8:44 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Testing with the new fsstress support for subvolumes uncovered a pretty
> bad problem with rename exchange on subvolumes.  We're modifying two
> different subvolumes, but we only start the transaction on one of them,
> so the other one is not added to the dirty root list.  This is caught by
> btrfs_cow_block() with a warning because the root has not been updated,
> however if we do not modify this root again we'll end up pointing at an
> invalid root because the root item is never updated.
>
> Fix this by making sure we add the destination root to the trans list,
> the same as we do with normal renames.  This fixes the corruption.
>
> Fixes: cdd1fedf8261 ("btrfs: add support for RENAME_EXCHANGE and RENAME_WHITEOUT")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

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

Looks good, thanks.

> ---
>  fs/btrfs/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f6fc47525a52..56032c518b26 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9575,6 +9575,9 @@ static int btrfs_rename_exchange(struct inode *old_dir,
>                 goto out_notrans;
>         }
>
> +       if (dest != root)
> +               btrfs_record_root_in_trans(trans, dest);
> +
>         /*
>          * We need to find a free sequence number both in the source and
>          * in the destination directory for the exchange.
> --
> 2.23.0
>
David Sterba Nov. 18, 2019, 7:11 p.m. UTC | #2
On Fri, Nov 15, 2019 at 03:43:06PM -0500, Josef Bacik wrote:
> Testing with the new fsstress support for subvolumes uncovered a pretty
> bad problem with rename exchange on subvolumes.  We're modifying two
> different subvolumes, but we only start the transaction on one of them,
> so the other one is not added to the dirty root list.  This is caught by
> btrfs_cow_block() with a warning because the root has not been updated,
> however if we do not modify this root again we'll end up pointing at an
> invalid root because the root item is never updated.
> 
> Fix this by making sure we add the destination root to the trans list,
> the same as we do with normal renames.  This fixes the corruption.
> 
> Fixes: cdd1fedf8261 ("btrfs: add support for RENAME_EXCHANGE and RENAME_WHITEOUT")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, thanks.

Patch
diff mbox series

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f6fc47525a52..56032c518b26 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9575,6 +9575,9 @@  static int btrfs_rename_exchange(struct inode *old_dir,
 		goto out_notrans;
 	}
 
+	if (dest != root)
+		btrfs_record_root_in_trans(trans, dest);
+
 	/*
 	 * We need to find a free sequence number both in the source and
 	 * in the destination directory for the exchange.