[PATCH v2 0/2] btrfs: subvolume deletion vs. snapshot fixes

[PATCH v2 0/2] btrfs: subvolume deletion vs. snapshot fixes

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi,

This small series fixes a couple of bugs that can happen when trying to
snapshot a deleted subvolume. Patch 1 fixes a filesystem abort that we
hit in production. Patch 2 fixes another issue that Sweet Tea spotted
when reviewing patch 1.

An fstest was sent previously [1].

Thanks!

Changes from v1 [2]:

- Rebased on latest misc-next.
- Added patch 2.

1: https://lore.kernel.org/linux-btrfs/62415ffc97ff2db4fa65cdd6f9db6ddead8105cd.1703010806.git.osandov@osandov.com/
2: https://lore.kernel.org/linux-btrfs/068014bd3e90668525c295660862db2932e25087.1703010314.git.osandov@fb.com/

Omar Sandoval (2):
  btrfs: don't abort filesystem when attempting to snapshot deleted
    subvolume
  btrfs: avoid copying BTRFS_ROOT_SUBVOL_DEAD flag to snapshot of
    subvolume being deleted

 fs/btrfs/inode.c | 22 +++++++++++++---------
 fs/btrfs/ioctl.c |  3 +++
 2 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] btrfs: don't abort filesystem when attempting to snapshot deleted subvolume

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

If the source file descriptor to the snapshot ioctl refers to a deleted
subvolume, we get the following abort:

  ------------[ cut here ]------------
  BTRFS: Transaction aborted (error -2)
  WARNING: CPU: 0 PID: 833 at fs/btrfs/transaction.c:1875 create_pending_snapshot+0x1040/0x1190 [btrfs]
  Modules linked in: pata_acpi btrfs ata_piix libata scsi_mod virtio_net blake2b_generic xor net_failover virtio_rng failover scsi_common rng_core raid6_pq libcrc32c
  CPU: 0 PID: 833 Comm: t_snapshot_dele Not tainted 6.7.0-rc6 #2
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-1.fc39 04/01/2014
  RIP: 0010:create_pending_snapshot+0x1040/0x1190 [btrfs]
  Code: e9 44 fe ff ff 44 89 e6 48 c7 c7 f8 59 7b c0 e8 d6 f4 a3 f7 0f 0b e9 4c fa ff ff 44 89 e6 48 c7 c7 f8 59 7b c0 e8 c0 f4 a3 f7 <0f> 0b e9 ef fe ff ff 44 89 e6 48 c7 c7 f8 59 7b c0 e8 aa f4 a3 f7
  RSP: 0018:ffffa09c01337af8 EFLAGS: 00010282
  RAX: 0000000000000000 RBX: ffff9982053e7c78 RCX: 0000000000000027
  RDX: ffff99827dc20848 RSI: 0000000000000001 RDI: ffff99827dc20840
  RBP: ffffa09c01337c00 R08: 0000000000000000 R09: ffffa09c01337998
  R10: 0000000000000003 R11: ffffffffb96da248 R12: fffffffffffffffe
  R13: ffff99820535bb28 R14: ffff99820b7bd000 R15: ffff99820381ea80
  FS:  00007fe20aadabc0(0000) GS:ffff99827dc00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000559a120b502f CR3: 00000000055b6000 CR4: 00000000000006f0
  Call Trace:
   <TASK>
   ? create_pending_snapshot+0x1040/0x1190 [btrfs]
   ? __warn+0x81/0x130
   ? create_pending_snapshot+0x1040/0x1190 [btrfs]
   ? report_bug+0x171/0x1a0
   ? handle_bug+0x3a/0x70
   ? exc_invalid_op+0x17/0x70
   ? asm_exc_invalid_op+0x1a/0x20
   ? create_pending_snapshot+0x1040/0x1190 [btrfs]
   ? create_pending_snapshot+0x1040/0x1190 [btrfs]
   create_pending_snapshots+0x92/0xc0 [btrfs]
   btrfs_commit_transaction+0x66b/0xf40 [btrfs]
   btrfs_mksubvol+0x301/0x4d0 [btrfs]
   btrfs_mksnapshot+0x80/0xb0 [btrfs]
   __btrfs_ioctl_snap_create+0x1c2/0x1d0 [btrfs]
   btrfs_ioctl_snap_create_v2+0xc4/0x150 [btrfs]
   btrfs_ioctl+0x8a6/0x2650 [btrfs]
   ? kmem_cache_free+0x22/0x340
   ? do_sys_openat2+0x97/0xe0
   __x64_sys_ioctl+0x97/0xd0
   do_syscall_64+0x46/0xf0
   entry_SYSCALL_64_after_hwframe+0x6e/0x76
  RIP: 0033:0x7fe20abe83af
  Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
  RSP: 002b:00007ffe6eff1360 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
  RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fe20abe83af
  RDX: 00007ffe6eff23c0 RSI: 0000000050009417 RDI: 0000000000000003
  RBP: 0000000000000003 R08: 0000000000000000 R09: 00007fe20ad16cd0
  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
  R13: 00007ffe6eff13c0 R14: 00007fe20ad45000 R15: 0000559a120b6d58
   </TASK>
  ---[ end trace 0000000000000000 ]---
  BTRFS: error (device vdc: state A) in create_pending_snapshot:1875: errno=-2 No such entry
  BTRFS info (device vdc: state EA): forced readonly
  BTRFS warning (device vdc: state EA): Skipping commit of aborted transaction.
  BTRFS: error (device vdc: state EA) in cleanup_transaction:2055: errno=-2 No such entry

This happens because create_pending_snapshot() initializes the new root
item as a copy of the source root item. This includes the refs field,
which is 0 for a deleted subvolume. The call to btrfs_insert_root()
therefore inserts a root with refs == 0. btrfs_get_new_fs_root() then
finds the root and returns -ENOENT if refs == 0, which causes
create_pending_snapshot() to abort.

Fix it by checking the source root's refs before attempting the snapshot
(but after locking subvol_sem to avoid racing with deletion).

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a1743904202b..0af214c8bef4 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -790,6 +790,9 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 		return -EOPNOTSUPP;
 	}
 
+	if (btrfs_root_refs(&root->root_item) == 0)
+		return -ENOENT;
+
 	if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state))
 		return -EINVAL;
 
-- 
2.43.0

[PATCH v2 2/2] btrfs: avoid copying BTRFS_ROOT_SUBVOL_DEAD flag to snapshot of subvolume being deleted

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Sweet Tea spotted a race between subvolume deletion and snapshotting
that can result in the root item for the snapshot having the
BTRFS_ROOT_SUBVOL_DEAD flag set. The race is:

Thread 1                                      | Thread 2
----------------------------------------------|----------
btrfs_delete_subvolume                        |
  btrfs_set_root_flags(BTRFS_ROOT_SUBVOL_DEAD)|
                                              |btrfs_mksubvol
                                              |  down_read(subvol_sem)
                                              |  create_snapshot
                                              |    ...
                                              |    create_pending_snapshot
                                              |      copy root item from source
  down_write(subvol_sem)                      |

This flag is only checked in send and swap activate, which this would
cause to fail mysteriously.

create_snapshot() now checks the root refs to reject a deleted
subvolume, so we can fix this by locking subvol_sem earlier so that the
BTRFS_ROOT_SUBVOL_DEAD flag and the root refs are updated atomically.

Reported-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b3e39610cc95..7bcc1c03437a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4458,6 +4458,8 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	u64 root_flags;
 	int ret;
 
+	down_write(&fs_info->subvol_sem);
+
 	/*
 	 * Don't allow to delete a subvolume with send in progress. This is
 	 * inside the inode lock so the error handling that has to drop the bit
@@ -4469,25 +4471,25 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 		btrfs_warn(fs_info,
 			   "attempt to delete subvolume %llu during send",
 			   dest->root_key.objectid);
-		return -EPERM;
+		ret = -EPERM;
+		goto out_up_write;
 	}
 	if (atomic_read(&dest->nr_swapfiles)) {
 		spin_unlock(&dest->root_item_lock);
 		btrfs_warn(fs_info,
 			   "attempt to delete subvolume %llu with active swapfile",
 			   root->root_key.objectid);
-		return -EPERM;
+		ret = -EPERM;
+		goto out_up_write;
 	}
 	root_flags = btrfs_root_flags(&dest->root_item);
 	btrfs_set_root_flags(&dest->root_item,
 			     root_flags | BTRFS_ROOT_SUBVOL_DEAD);
 	spin_unlock(&dest->root_item_lock);
 
-	down_write(&fs_info->subvol_sem);
-
 	ret = may_destroy_subvol(dest);
 	if (ret)
-		goto out_up_write;
+		goto out_undead;
 
 	btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
 	/*
@@ -4497,7 +4499,7 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	 */
 	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 5, true);
 	if (ret)
-		goto out_up_write;
+		goto out_undead;
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
@@ -4563,15 +4565,17 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	inode->i_flags |= S_DEAD;
 out_release:
 	btrfs_subvolume_release_metadata(root, &block_rsv);
-out_up_write:
-	up_write(&fs_info->subvol_sem);
+out_undead:
 	if (ret) {
 		spin_lock(&dest->root_item_lock);
 		root_flags = btrfs_root_flags(&dest->root_item);
 		btrfs_set_root_flags(&dest->root_item,
 				root_flags & ~BTRFS_ROOT_SUBVOL_DEAD);
 		spin_unlock(&dest->root_item_lock);
-	} else {
+	}
+out_up_write:
+	up_write(&fs_info->subvol_sem);
+	if (!ret) {
 		d_invalidate(dentry);
 		btrfs_prune_dentries(dest);
 		ASSERT(dest->send_in_progress == 0);
-- 
2.43.0

Re: [PATCH v2 0/2] btrfs: subvolume deletion vs. snapshot fixes

[Sweet Tea Dorminy]

On 1/4/24 14:48, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi,
> 
> This small series fixes a couple of bugs that can happen when trying to
> snapshot a deleted subvolume. Patch 1 fixes a filesystem abort that we
> hit in production. Patch 2 fixes another issue that Sweet Tea spotted
> when reviewing patch 1.
> 
> An fstest was sent previously [1].
> 
> Thanks!
> 
> Changes from v1 [2]:
> 
> - Rebased on latest misc-next.
> - Added patch 2.
> 
> 1: https://lore.kernel.org/linux-btrfs/62415ffc97ff2db4fa65cdd6f9db6ddead8105cd.1703010806.git.osandov@osandov.com/
> 2: https://lore.kernel.org/linux-btrfs/068014bd3e90668525c295660862db2932e25087.1703010314.git.osandov@fb.com/
> 
> Omar Sandoval (2):
>    btrfs: don't abort filesystem when attempting to snapshot deleted
>      subvolume
>    btrfs: avoid copying BTRFS_ROOT_SUBVOL_DEAD flag to snapshot of
>      subvolume being deleted
> 
>   fs/btrfs/inode.c | 22 +++++++++++++---------
>   fs/btrfs/ioctl.c |  3 +++
>   2 files changed, 16 insertions(+), 9 deletions(-)
> 

For the series:
Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Re: [PATCH v2 0/2] btrfs: subvolume deletion vs. snapshot fixes

[Anand Jain]

On 1/5/24 03:48, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi,
> 
> This small series fixes a couple of bugs that can happen when trying to
> snapshot a deleted subvolume. Patch 1 fixes a filesystem abort that we
> hit in production. Patch 2 fixes another issue that Sweet Tea spotted
> when reviewing patch 1.
> 
> An fstest was sent previously [1].
> 
> Thanks!
> 
> Changes from v1 [2]:
> 
> - Rebased on latest misc-next.
> - Added patch 2.
> 
> 1: https://lore.kernel.org/linux-btrfs/62415ffc97ff2db4fa65cdd6f9db6ddead8105cd.1703010806.git.osandov@osandov.com/
> 2: https://lore.kernel.org/linux-btrfs/068014bd3e90668525c295660862db2932e25087.1703010314.git.osandov@fb.com/
> 
> Omar Sandoval (2):
>    btrfs: don't abort filesystem when attempting to snapshot deleted
>      subvolume
>    btrfs: avoid copying BTRFS_ROOT_SUBVOL_DEAD flag to snapshot of
>      subvolume being deleted
> 

Looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


Thx, Anand


>   fs/btrfs/inode.c | 22 +++++++++++++---------
>   fs/btrfs/ioctl.c |  3 +++
>   2 files changed, 16 insertions(+), 9 deletions(-)
> 

Re: [PATCH v2 0/2] btrfs: subvolume deletion vs. snapshot fixes

[David Sterba]

On Thu, Jan 04, 2024 at 11:48:45AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi,
> 
> This small series fixes a couple of bugs that can happen when trying to
> snapshot a deleted subvolume. Patch 1 fixes a filesystem abort that we
> hit in production. Patch 2 fixes another issue that Sweet Tea spotted
> when reviewing patch 1.
> 
> An fstest was sent previously [1].
> 
> Thanks!
> 
> Changes from v1 [2]:
> 
> - Rebased on latest misc-next.
> - Added patch 2.
> 
> 1: https://lore.kernel.org/linux-btrfs/62415ffc97ff2db4fa65cdd6f9db6ddead8105cd.1703010806.git.osandov@osandov.com/
> 2: https://lore.kernel.org/linux-btrfs/068014bd3e90668525c295660862db2932e25087.1703010314.git.osandov@fb.com/
> 
> Omar Sandoval (2):
>   btrfs: don't abort filesystem when attempting to snapshot deleted
>     subvolume
>   btrfs: avoid copying BTRFS_ROOT_SUBVOL_DEAD flag to snapshot of
>     subvolume being deleted

Added to misc-next, thanks.