[PATCH] btrfs: prevent deleting the wrong subvolume by ID

[PATCH] btrfs: prevent deleting the wrong subvolume by ID

[guolingxing]

When deleting a subvolume by ID, btrfs_ioctl_snap_destroy() first
resolves the requested ID to a name, then looks up that name again
under the parent directory lock.

A concurrent rename can change which subvolume is associated with
the name between these operations. Since the deletion path does not
verify the ID of the resolved subvolume, it can delete a different
subvolume while leaving the requested one intact.

This was reproduced by repeatedly exchanging the names of two
subvolumes using renameat2(RENAME_EXCHANGE) while deleting one of
them by ID. The wrong subvolume was deleted in 9 out of 14 independent
tests.

Fix this by checking that the resolved subvolume ID matches the
requested ID while holding the parent directory lock. Return -ENOENT
if the IDs differ, preventing deletion of the wrong subvolume.

Fixes: 949964c92843 ("btrfs: add new BTRFS_IOC_SNAP_DESTROY_V2 ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/btrfs/ioctl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 576cd76ca0bf..39eb7722998d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2380,6 +2380,17 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 
 	inode = d_inode(dentry);
 	dest = BTRFS_I(inode)->root;
+
+	/*
+	 * A concurrent rename may have changed which subvolume the name
+	 * refers to. Verify that the resolved ID still matches the requested ID.
+	 */
+	if (destroy_v2 && (vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID) &&
+	    btrfs_root_id(dest) != vol_args2->subvolid) {
+		ret = -ENOENT;
+		goto out_end_removing;
+	}
+
 	if (!capable(CAP_SYS_ADMIN)) {
 		/*
 		 * Regular user.  Only allow this with a special mount
-- 
2.34.1

Re: [PATCH] btrfs: prevent deleting the wrong subvolume by ID

[Qu Wenruo]

在 2026/10/18 18:07, guolingxing 写道:
> When deleting a subvolume by ID, btrfs_ioctl_snap_destroy() first
> resolves the requested ID to a name, then looks up that name again
> under the parent directory lock.
> 
> A concurrent rename can change which subvolume is associated with
> the name between these operations. Since the deletion path does not
> verify the ID of the resolved subvolume, it can delete a different
> subvolume while leaving the requested one intact.
> 
> This was reproduced by repeatedly exchanging the names of two
> subvolumes using renameat2(RENAME_EXCHANGE) while deleting one of
> them by ID. The wrong subvolume was deleted in 9 out of 14 independent
> tests.
> 
> Fix this by checking that the resolved subvolume ID matches the
> requested ID while holding the parent directory lock. Return -ENOENT
> if the IDs differ, preventing deletion of the wrong subvolume.

-ENOENT can be confusing to user space, the subvolumeid is still there.

-EAGAIN may be better?

Another thing is, if we got a incorrect subvolume tree, the better way 
may be retry the path resolution and lock again until we got either 
-ENOENT from path resolution.

But the current code base makes is pretty hard to retry. A refactor may 
be needed if you want to that retry path.

> 
> Fixes: 949964c92843 ("btrfs: add new BTRFS_IOC_SNAP_DESTROY_V2 ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
> ---
>   fs/btrfs/ioctl.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 576cd76ca0bf..39eb7722998d 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2380,6 +2380,17 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>   
>   	inode = d_inode(dentry);
>   	dest = BTRFS_I(inode)->root;
> +
> +	/*
> +	 * A concurrent rename may have changed which subvolume the name
> +	 * refers to. Verify that the resolved ID still matches the requested ID.
> +	 */
> +	if (destroy_v2 && (vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID) &&
> +	    btrfs_root_id(dest) != vol_args2->subvolid) {
> +		ret = -ENOENT;
> +		goto out_end_removing;
> +	}
> +
>   	if (!capable(CAP_SYS_ADMIN)) {
>   		/*
>   		 * Regular user.  Only allow this with a special mount

Re:Re: [PATCH] btrfs: prevent deleting the wrong subvolume by ID

[郭玲兴]

Thanks for the review!
I agree that -ENOENT is misleading here, as the requested subvolume may still exist. I'll change it to -EAGAIN in v2.
An internal retry would be preferable, but it would require refactoring the current path resolution and cleanup flow. I'd prefer to keep this fix minimal for now and leave the retry logic for a separate change.
I'll update the patch and rerun the reproducer.







From: Qu Wenruo <wqu@suse.com>
 Date: 2026-10-18 17:09:57
To:guolingxing <guolingxing@tju.edu.cn>,David Sterba <dsterba@suse.com>,Chris Mason <mason@kernel.org>
 cc: Marcos Paulo de Souza <mpdesouza@suse.com>,linux-btrfs@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: Re: [PATCH] btrfs: prevent deleting the wrong subvolume by ID>
>
>在 2026/10/18 18:07, guolingxing 写道:
>> When deleting a subvolume by ID, btrfs_ioctl_snap_destroy() first
>> resolves the requested ID to a name, then looks up that name again
>> under the parent directory lock.
>> 
>> A concurrent rename can change which subvolume is associated with
>> the name between these operations. Since the deletion path does not
>> verify the ID of the resolved subvolume, it can delete a different
>> subvolume while leaving the requested one intact.
>> 
>> This was reproduced by repeatedly exchanging the names of two
>> subvolumes using renameat2(RENAME_EXCHANGE) while deleting one of
>> them by ID. The wrong subvolume was deleted in 9 out of 14 independent
>> tests.
>> 
>> Fix this by checking that the resolved subvolume ID matches the
>> requested ID while holding the parent directory lock. Return -ENOENT
>> if the IDs differ, preventing deletion of the wrong subvolume.
>
>-ENOENT can be confusing to user space, the subvolumeid is still there.
>
>-EAGAIN may be better?
>
>Another thing is, if we got a incorrect subvolume tree, the better way 
>may be retry the path resolution and lock again until we got either 
>-ENOENT from path resolution.
>
>But the current code base makes is pretty hard to retry. A refactor may 
>be needed if you want to that retry path.
>
>> 
>> Fixes: 949964c92843 ("btrfs: add new BTRFS_IOC_SNAP_DESTROY_V2 ioctl")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
>> ---
>>   fs/btrfs/ioctl.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>> 
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 576cd76ca0bf..39eb7722998d 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2380,6 +2380,17 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>>   
>>   	inode = d_inode(dentry);
>>   	dest = BTRFS_I(inode)->root;
>> +
>> +	/*
>> +	 * A concurrent rename may have changed which subvolume the name
>> +	 * refers to. Verify that the resolved ID still matches the requested ID.
>> +	 */
>> +	if (destroy_v2 && (vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID) &&
>> +	    btrfs_root_id(dest) != vol_args2->subvolid) {
>> +		ret = -ENOENT;
>> +		goto out_end_removing;
>> +	}
>> +
>>   	if (!capable(CAP_SYS_ADMIN)) {
>>   		/*
>>   		 * Regular user.  Only allow this with a special mount
>
>

[PATCH v2] btrfs: prevent deleting the wrong subvolume by ID

[guolingxing]

When deleting a subvolume by ID, btrfs_ioctl_snap_destroy() first
resolves the requested ID to a name, then looks up that name again
under the parent directory lock.

A concurrent rename can change which subvolume is associated with
the name between these operations. Since the deletion path does not
verify the ID of the resolved subvolume, it can delete a different
subvolume while leaving the requested one intact.

This was reproduced by repeatedly exchanging the names of two
subvolumes using renameat2(RENAME_EXCHANGE) while deleting one of
them by ID. The wrong subvolume was deleted in 9 out of 14 independent
tests.

Fix this by checking that the resolved subvolume ID matches the
requested ID while holding the parent directory lock. Return -EAGAIN
on a mismatch so that userspace can retry the operation instead of
being told that the requested subvolume does not exist.

Fixes: 949964c92843 ("btrfs: add new BTRFS_IOC_SNAP_DESTROY_V2 ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
Changes since v1:
- Return -EAGAIN instead of -ENOENT on a subvolume ID mismatch,
  as suggested by Qu Wenruo.


Testing:
- Unpatched kernel: 9 wrong deletions in 14 independent runs.
- v2 patched kernel: 0 wrong deletions in 30 independent runs.
- EAGAIN was observed in 14 runs; all 30 runs successfully deleted
  the requested subvolume while preserving the other one.

 fs/btrfs/ioctl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 576cd76ca0bf..d82a7f3f3d14 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2380,6 +2380,17 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 
 	inode = d_inode(dentry);
 	dest = BTRFS_I(inode)->root;
+
+	/*
+	 * A concurrent rename may have changed which subvolume the name
+	 * refers to. Verify that the resolved ID still matches the requested ID.
+	 */
+	if (destroy_v2 && (vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID) &&
+	    btrfs_root_id(dest) != vol_args2->subvolid) {
+		ret = -EAGAIN;
+		goto out_end_removing;
+	}
+
 	if (!capable(CAP_SYS_ADMIN)) {
 		/*
 		 * Regular user.  Only allow this with a special mount
-- 
2.34.1

Re: [PATCH v2] btrfs: prevent deleting the wrong subvolume by ID

[Qu Wenruo]

在 2026/10/19 02:08, guolingxing 写道:
> When deleting a subvolume by ID, btrfs_ioctl_snap_destroy() first
> resolves the requested ID to a name, then looks up that name again
> under the parent directory lock.
> 
> A concurrent rename can change which subvolume is associated with
> the name between these operations. Since the deletion path does not
> verify the ID of the resolved subvolume, it can delete a different
> subvolume while leaving the requested one intact.
> 
> This was reproduced by repeatedly exchanging the names of two
> subvolumes using renameat2(RENAME_EXCHANGE) while deleting one of
> them by ID. The wrong subvolume was deleted in 9 out of 14 independent
> tests.
> 
> Fix this by checking that the resolved subvolume ID matches the
> requested ID while holding the parent directory lock. Return -EAGAIN
> on a mismatch so that userspace can retry the operation instead of
> being told that the requested subvolume does not exist.
> 
> Fixes: 949964c92843 ("btrfs: add new BTRFS_IOC_SNAP_DESTROY_V2 ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: guolingxing <guolingxing@tju.edu.cn>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> Changes since v1:
> - Return -EAGAIN instead of -ENOENT on a subvolume ID mismatch,
>    as suggested by Qu Wenruo.
> 
> 
> Testing:
> - Unpatched kernel: 9 wrong deletions in 14 independent runs.
> - v2 patched kernel: 0 wrong deletions in 30 independent runs.
> - EAGAIN was observed in 14 runs; all 30 runs successfully deleted
>    the requested subvolume while preserving the other one.
> 
>   fs/btrfs/ioctl.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 576cd76ca0bf..d82a7f3f3d14 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2380,6 +2380,17 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>   
>   	inode = d_inode(dentry);
>   	dest = BTRFS_I(inode)->root;
> +
> +	/*
> +	 * A concurrent rename may have changed which subvolume the name
> +	 * refers to. Verify that the resolved ID still matches the requested ID.
> +	 */
> +	if (destroy_v2 && (vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID) &&
> +	    btrfs_root_id(dest) != vol_args2->subvolid) {
> +		ret = -EAGAIN;
> +		goto out_end_removing;
> +	}
> +
>   	if (!capable(CAP_SYS_ADMIN)) {
>   		/*
>   		 * Regular user.  Only allow this with a special mount