[PATCH 0/2] btrfs: anon_dev related fixes

[PATCH 0/2] btrfs: anon_dev related fixes

[Qu Wenruo]

The first one is a Sashiko bug report, that we have a very narrow window
to hit an anon_dev ID leak.

The second one is another small bug related to double freeing the same
anon_dev ID, exposed by LLM.

Qu Wenruo (2):
  btrfs: fix anon_dev leak when snapshot creation races with a root read
  btrfs: fix double free of anon_dev in btrfs_get_root_ref() on -EEXIST

 fs/btrfs/disk-io.c | 13 +++++++++++++
 fs/btrfs/ioctl.c   |  8 +-------
 2 files changed, 14 insertions(+), 7 deletions(-)

-- 
2.56.0

[PATCH 1/2] btrfs: fix anon_dev leak when snapshot creation races with a root read

[Qu Wenruo]

[BUG]
When create_snapshot() fails after the new snapshot root is already
cached (e.g. by a backref lookup), an anonymous block device ID can be
leaked.

[CAUSE]
The leak call chain looks like the following:

 create_snapshot()
 |- btrfs_commit_transaction()
 |  \- create_pending_snapshot()
 |     \- pending->snap = btrfs_get_new_fs_root()
 |        \- btrfs_get_root_ref()
 |           |- free_anon_bdev(*anon_dev);
 |           |- *anon_dev = 0;
 |           |  ^^^ found the cached root, freeing the preallocated
 |           |      anon_dev and retry
 |           |
 |           |- root = btrfs_lookup_fs_root()
 |           \- return root;
 |              ^^^ now pending->snap is the cache root.
 |
 |- btrfs_orphan_cleanup() fails
 \- fail:
    \- pending_snapshot->snap->anon_dev = 0
       ^^^ clears the cached root's ID which is not the preallocated ID.
           Futhermore, the old ID is not released, causing a leak.

[FIX]
Make the ownership of the preallocated ID explicit: btrfs_get_root_ref()
now sets *anon_dev to 0 whenever it consumes the ID.

This makes pending_snapshot->anon_dev non-zero only if it has not been
consumed, so the "prevent double freeing" hack in create_snapshot() is no
longer needed.

Remove it, along with the now redundant anon_dev resets in
create_snapshot() and create_subvol().

Thankfully this is really hard to hit, the window is pretty small, so
it's just to make the ID ownership more explicit and consistent.

Fixes: 2dfb1e43f57d ("btrfs: preallocate anon block device at first phase of snapshot creation")
Assisted-by: LLM (planning and coding)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 7 +++++++
 fs/btrfs/ioctl.c   | 8 +-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 78865704b45d..50888d1e3e1e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1297,6 +1297,8 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
  * @objectid:	root id
  * @anon_dev:	preallocated anonymous block device number for new roots,
  *		pass NULL for a new allocation.
+ *		If the ID is consumed, *@anon_dev is set to 0.
+ *		Otherwise the caller needs to release the ID.
  * @check_ref:	whether to check root item references, If true, return -ENOENT
  *		for orphan roots
  */
@@ -1390,6 +1392,9 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
 		}
 		goto fail;
 	}
+	/* The new root owns the preallocated anon_dev now. */
+	if (anon_dev)
+		*anon_dev = 0;
 	return root;
 fail:
 	/*
@@ -1424,6 +1429,8 @@ struct btrfs_root *btrfs_get_fs_root(struct btrfs_fs_info *fs_info,
  * @objectid:	tree objectid
  * @anon_dev:	if NULL, allocate a new anonymous block device or use the
  *		parameter value if not NULL
+ *		Check the comment of btrfs_get_root_ref() on the lifespan
+ *		of @anon_dev if not NULL.
  */
 struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info,
 					 u64 objectid, dev_t *anon_dev)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 651a780dd7b1..6cd12f586423 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -712,10 +712,8 @@ static noinline int create_subvol(const struct mnt_idmap *idmap,
 		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
-	/* anon_dev is owned by new_root now. */
-	anon_dev = 0;
 	BTRFS_I(new_inode_args.inode)->root = new_root;
-	/* ... and new_root is owned by new_inode_args.inode now. */
+	/* new_root is owned by new_inode_args.inode now. */
 
 	ret = btrfs_record_root_in_trans(trans, new_root);
 	if (unlikely(ret)) {
@@ -868,11 +866,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 
 	d_instantiate(dentry, inode);
 	ret = 0;
-	pending_snapshot->anon_dev = 0;
 fail:
-	/* Prevent double freeing of anon_dev */
-	if (ret && pending_snapshot->snap)
-		pending_snapshot->snap->anon_dev = 0;
 	btrfs_put_root(pending_snapshot->snap);
 	btrfs_block_rsv_release(fs_info, block_rsv, (u64)-1, NULL);
 	if (qgroup_reserved)
-- 
2.56.0

[PATCH 2/2] btrfs: fix double free of anon_dev in btrfs_get_root_ref() on -EEXIST

[Qu Wenruo]

[BUG]
If btrfs_get_root_ref() is given a preallocated anon_dev and loses the
race to insert the new root, the preallocated ID is freed twice.

[CAUSE]
The offending call trace looks like the following:

 btrfs_get_root_ref(*anon_dev = X)
 |- btrfs_init_fs_root()
 |  \- root->anon_dev = X
 |- btrfs_insert_fs_root() returns -EEXIST
 |- btrfs_put_root(root)
 |  \- free_anon_bdev(root->anon_dev)
 |     ^^^ ID X is freed here.
 |
 \- again:
    \- btrfs_lookup_fs_root() finds the existing root
       \- free_anon_bdev(*anon_dev)
          ^^^ ID X is freed again.

[FIX]
Clear root->anon_dev before putting the unpublished root, the same as
the fail: label does, so the ID stays owned by the caller.

The next retry either got the existing root and free the anon_dev, or
win the race (the root is removed by someone else) and reuse the
preallocated anon_dev.

Assisted-by: LLM (exposing the bug)
Fixes: 2dfb1e43f57d ("btrfs: preallocate anon block device at first phase of snapshot creation")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 50888d1e3e1e..7775fc768f7e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1387,6 +1387,12 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
 	ret = btrfs_insert_fs_root(fs_info, root);
 	if (ret) {
 		if (ret == -EEXIST) {
+			/*
+			 * The preallocated anon_dev is still owned by the
+			 * caller, don't let btrfs_put_root() free it.
+			 */
+			if (anon_dev && *anon_dev)
+				root->anon_dev = 0;
 			btrfs_put_root(root);
 			goto again;
 		}
-- 
2.56.0