[PATCH 2/2] Btrfs: do not delete a subvolume which is in a R/O subvolume

[PATCH 2/2] Btrfs: do not delete a subvolume which is in a R/O subvolume

[Miao Xie]

Step to reproduce:
 # mkfs.btrfs <disk>
 # mount <disk> <mnt>
 # btrfs sub create <mnt>/subv0
 # btrfs sub snap <mnt> <mnt>/subv0/snap0
 # change <mnt>/subv0 from R/W to R/O
 # btrfs sub del <mnt>/subv0/snap0

We deleted the snapshot successfully. I think we should not be able to delete
the snapshot since the parent subvolume is R/O.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 29fb07c..54c56c7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2061,13 +2061,13 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		err = -EINVAL;
 		if (root == dest)
 			goto out_dput;
-
-		/* check if subvolume may be deleted by a non-root user */
-		err = btrfs_may_delete(dir, dentry, 1);
-		if (err)
-			goto out_dput;
 	}
 
+	/* check if subvolume may be deleted by a user */
+	err = btrfs_may_delete(dir, dentry, 1);
+	if (err)
+		goto out_dput;
+
 	if (btrfs_ino(inode) != BTRFS_FIRST_FREE_OBJECTID) {
 		err = -EINVAL;
 		goto out_dput;
-- 
1.7.6.5

Re: [PATCH 2/2] Btrfs: do not delete a subvolume which is in a R/O subvolume

[cwillu]

On Mon, Oct 22, 2012 at 5:39 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
> Step to reproduce:
>  # mkfs.btrfs <disk>
>  # mount <disk> <mnt>
>  # btrfs sub create <mnt>/subv0
>  # btrfs sub snap <mnt> <mnt>/subv0/snap0
>  # change <mnt>/subv0 from R/W to R/O
>  # btrfs sub del <mnt>/subv0/snap0
>
> We deleted the snapshot successfully. I think we should not be able to delete
> the snapshot since the parent subvolume is R/O.

snap0 isn't read-only in that case, right?  From a user interaction
standpoint, this seems like it just forces a user to rm -rf rather
btrfs sub del, which strikes me as a bit ham-handed when all we really
care about is leaving a (the?) directory entry where snap0 used to be.

Re: [PATCH 2/2] Btrfs: do not delete a subvolume which is in a R/O subvolume

[Miao Xie]

On Mon, 22 Oct 2012 05:57:12 -0600, cwillu wrote:
> On Mon, Oct 22, 2012 at 5:39 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
>> Step to reproduce:
>>  # mkfs.btrfs <disk>
>>  # mount <disk> <mnt>
>>  # btrfs sub create <mnt>/subv0
>>  # btrfs sub snap <mnt> <mnt>/subv0/snap0
>>  # change <mnt>/subv0 from R/W to R/O
>>  # btrfs sub del <mnt>/subv0/snap0
>>
>> We deleted the snapshot successfully. I think we should not be able to delete
>> the snapshot since the parent subvolume is R/O.
> 
> snap0 isn't read-only in that case, right?  From a user interaction
> standpoint, this seems like it just forces a user to rm -rf rather
> btrfs sub del, which strikes me as a bit ham-handed when all we really
> care about is leaving a (the?) directory entry where snap0 used to be.
> 

I don't think we can identify "btrfs sub del" with "rm -rf", because "rm -rf"
will check the permission of the parent directory of each file/directory which
is going to be deleted, but "btrfs sub del" doesn't do it, it will see all the
file/directory in the subvolume as one, so I think it seems like a special
"rmdir". From this standpoint, deleting a snapshot whose parent subvolume
is readonly should be forbidden.

Thanks
Miao

Re: [PATCH 2/2] Btrfs: do not delete a subvolume which is in a R/O subvolume

[cwillu]

On Wed, Oct 24, 2012 at 4:03 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
> On Mon, 22 Oct 2012 05:57:12 -0600, cwillu wrote:
>> On Mon, Oct 22, 2012 at 5:39 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
>>> Step to reproduce:
>>>  # mkfs.btrfs <disk>
>>>  # mount <disk> <mnt>
>>>  # btrfs sub create <mnt>/subv0
>>>  # btrfs sub snap <mnt> <mnt>/subv0/snap0
>>>  # change <mnt>/subv0 from R/W to R/O
>>>  # btrfs sub del <mnt>/subv0/snap0
>>>
>>> We deleted the snapshot successfully. I think we should not be able to delete
>>> the snapshot since the parent subvolume is R/O.
>>
>> snap0 isn't read-only in that case, right?  From a user interaction
>> standpoint, this seems like it just forces a user to rm -rf rather
>> btrfs sub del, which strikes me as a bit ham-handed when all we really
>> care about is leaving a (the?) directory entry where snap0 used to be.
>>
>
> I don't think we can identify "btrfs sub del" with "rm -rf", because "rm -rf"
> will check the permission of the parent directory of each file/directory which
> is going to be deleted, but "btrfs sub del" doesn't do it, it will see all the
> file/directory in the subvolume as one, so I think it seems like a special
> "rmdir". From this standpoint, deleting a snapshot whose parent subvolume
> is readonly should be forbidden.

Sorry; reading back, I misunderstood you to mean that subv0 was marked
as a readonly subvolume, as opposed to marking the mountpoint
readonly.  The former can't work at all (it would make the pair
undeletable, as the subv0 can't be deleted while it contains another
subvolume).

I'm still not sure that the latter is quite right, but I care a lot
less as one could always remount it rw (unlike ro subvolumes, as I
understand them).