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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html