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@xxxxxxxx>
 Date: 2026-10-18 17:09:57
To:guolingxing <guolingxing@xxxxxxxxxx>,David Sterba <dsterba@xxxxxxxx>,Chris Mason <mason@xxxxxxxxxx>
 cc: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>,linux-btrfs@xxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
>> Signed-off-by: guolingxing <guolingxing@xxxxxxxxxx>
>> ---
>>   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
>
>