Btrfs: do not delete a subvolume which is in a R/O subvolume

PATCH In review Miao Xie 2 participants 4 messages Oct 22, 2012 – Oct 24, 2012 Btrfs file system

lore

Share & tools

lore

Miao Xie

author

2012-10-22 11:39 UTC #0 lore raw

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 <[email protected]>

---
 fs/btrfs/ioctl.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

fs/btrfs/ioctl.c +5 -5

index 29fb07c..54c56c7 100644
@@ -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;

Signature · 1 line

1.7.6.5

cwillu

2012-10-22 11:57 UTC #1 lore raw

Quoted 10 lines · 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.

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.

Miao Xie

author

2012-10-24 10:03 UTC #2 lore raw

Quoted 9 lines · cwillu

Quoted 8 lines · 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

Quoted 8 lines

Quoted 2 lines
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

cwillu

2012-10-24 10:13 UTC #3 lore raw

Quoted 19 lines · Miao Xie

Quoted 9 lines · cwillu
Quoted 8 lines · 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
Quoted 8 lines
Quoted 2 lines
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.

Quoted 6 lines

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).