| Message ID | 50853089.6010801@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(-)