| Message ID | 5164078e-4e15-d6df-7356-fa4f5d70a2db@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/20/2018 07:45 AM, Misono, Tomohiro wrote: > Deletion of subvolume by non-privileged user is completely restricted > by default because we can delete a subvolume even if it is not empty > and may cause data loss. In other words, when user_subvol_rm_allowed > mount option is used, a user can delete a subvolume containing the > directory which cannot be deleted directly by the user. > > However, there should be no harm to allow users to delete empty subvolumes > when rmdir(2) would have been allowed if they were normal directories. > This patch allows deletion of empty subvolume by default. Instead of modifying the ioctl, what about allowing rmdir(2) to work for an _empty_ subvolume (and all the permission check are satisfied) ? > > Note that user_subvol_rm_allowed option requires write+exec permission > of the subvolume to be deleted, but they are not required for empty > subvolume. > > The comment in the code is also updated accordingly. > > Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com> > --- > fs/btrfs/ioctl.c | 55 +++++++++++++++++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 111ee282b777..838406a7a7f5 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2366,36 +2366,43 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > dest = BTRFS_I(inode)->root; > if (!capable(CAP_SYS_ADMIN)) { > /* > - * Regular user. Only allow this with a special mount > - * option, when the user has write+exec access to the > - * subvol root, and when rmdir(2) would have been > - * allowed. > + * By default, regular user is only allowed to delete > + * empty subvols when rmdir(2) would have been allowed > + * if they were normal directories. > * > - * Note that this is _not_ check that the subvol is > - * empty or doesn't contain data that we wouldn't > + * If the mount option 'user_subvol_rm_allowed' is set, > + * it allows users to delete non-empty subvols when the > + * user has write+exec access to the subvol root and when > + * rmdir(2) would have been allowed (except the emptiness > + * check). > + * > + * Note that this option does _not_ check that if the subvol > + * is empty or doesn't contain data that the user wouldn't > * otherwise be able to delete. > * > - * Users who want to delete empty subvols should try > - * rmdir(2). > + * Users who want to delete empty subvols created by > + * snapshot (ino number == 2) can use rmdir(2). > */ > - err = -EPERM; > - if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) > - goto out_dput; > + err = -ENOTEMPTY; > + if (inode->i_size != BTRFS_EMPTY_DIR_SIZE) { > + if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) > + goto out_dput; > > - /* > - * Do not allow deletion if the parent dir is the same > - * as the dir to be deleted. That means the ioctl > - * must be called on the dentry referencing the root > - * of the subvol, not a random directory contained > - * within it. > - */ > - err = -EINVAL; > - if (root == dest) > - goto out_dput; > + /* > + * Do not allow deletion if the parent dir is the same > + * as the dir to be deleted. That means the ioctl > + * must be called on the dentry referencing the root > + * of the subvol, not a random directory contained > + * within it. > + */ > + err = -EINVAL; > + if (root == dest) > + goto out_dput; > > - err = inode_permission(inode, MAY_WRITE | MAY_EXEC); > - if (err) > - goto out_dput; > + err = inode_permission(inode, MAY_WRITE | MAY_EXEC); > + if (err) > + goto out_dput; > + } > } > > /* check if subvolume may be deleted by a user */ >
On 20.03.2018 22:06, Goffredo Baroncelli wrote: > On 03/20/2018 07:45 AM, Misono, Tomohiro wrote: >> Deletion of subvolume by non-privileged user is completely restricted >> by default because we can delete a subvolume even if it is not empty >> and may cause data loss. In other words, when user_subvol_rm_allowed >> mount option is used, a user can delete a subvolume containing the >> directory which cannot be deleted directly by the user. >> >> However, there should be no harm to allow users to delete empty subvolumes >> when rmdir(2) would have been allowed if they were normal directories. >> This patch allows deletion of empty subvolume by default. > > Instead of modifying the ioctl, what about allowing rmdir(2) to work for an _empty_ subvolume (and all the permission check are satisfied) ? I'm inclined to agree with Goffredo. user_subvol_rm_allowed flag really looks like a hack ontop of the ioctl. I'd rather we modify the generic behavior. > > > >> >> Note that user_subvol_rm_allowed option requires write+exec permission >> of the subvolume to be deleted, but they are not required for empty >> subvolume. >> >> The comment in the code is also updated accordingly. >> >> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com> >> --- >> fs/btrfs/ioctl.c | 55 +++++++++++++++++++++++++++++++------------------------ >> 1 file changed, 31 insertions(+), 24 deletions(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 111ee282b777..838406a7a7f5 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -2366,36 +2366,43 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, >> dest = BTRFS_I(inode)->root; >> if (!capable(CAP_SYS_ADMIN)) { >> /* >> - * Regular user. Only allow this with a special mount >> - * option, when the user has write+exec access to the >> - * subvol root, and when rmdir(2) would have been >> - * allowed. >> + * By default, regular user is only allowed to delete >> + * empty subvols when rmdir(2) would have been allowed >> + * if they were normal directories. >> * >> - * Note that this is _not_ check that the subvol is >> - * empty or doesn't contain data that we wouldn't >> + * If the mount option 'user_subvol_rm_allowed' is set, >> + * it allows users to delete non-empty subvols when the >> + * user has write+exec access to the subvol root and when >> + * rmdir(2) would have been allowed (except the emptiness >> + * check). >> + * >> + * Note that this option does _not_ check that if the subvol >> + * is empty or doesn't contain data that the user wouldn't >> * otherwise be able to delete. >> * >> - * Users who want to delete empty subvols should try >> - * rmdir(2). >> + * Users who want to delete empty subvols created by >> + * snapshot (ino number == 2) can use rmdir(2). >> */ >> - err = -EPERM; >> - if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) >> - goto out_dput; >> + err = -ENOTEMPTY; >> + if (inode->i_size != BTRFS_EMPTY_DIR_SIZE) { >> + if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) >> + goto out_dput; >> >> - /* >> - * Do not allow deletion if the parent dir is the same >> - * as the dir to be deleted. That means the ioctl >> - * must be called on the dentry referencing the root >> - * of the subvol, not a random directory contained >> - * within it. >> - */ >> - err = -EINVAL; >> - if (root == dest) >> - goto out_dput; >> + /* >> + * Do not allow deletion if the parent dir is the same >> + * as the dir to be deleted. That means the ioctl >> + * must be called on the dentry referencing the root >> + * of the subvol, not a random directory contained >> + * within it. >> + */ >> + err = -EINVAL; >> + if (root == dest) >> + goto out_dput; >> >> - err = inode_permission(inode, MAY_WRITE | MAY_EXEC); >> - if (err) >> - goto out_dput; >> + err = inode_permission(inode, MAY_WRITE | MAY_EXEC); >> + if (err) >> + goto out_dput; >> + } >> } >> >> /* check if subvolume may be deleted by a user */ >> > > -- 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 2018-03-21 03:46, Nikolay Borisov wrote: > > > On 20.03.2018 22:06, Goffredo Baroncelli wrote: >> On 03/20/2018 07:45 AM, Misono, Tomohiro wrote: >>> Deletion of subvolume by non-privileged user is completely restricted >>> by default because we can delete a subvolume even if it is not empty >>> and may cause data loss. In other words, when user_subvol_rm_allowed >>> mount option is used, a user can delete a subvolume containing the >>> directory which cannot be deleted directly by the user. >>> >>> However, there should be no harm to allow users to delete empty subvolumes >>> when rmdir(2) would have been allowed if they were normal directories. >>> This patch allows deletion of empty subvolume by default. >> >> Instead of modifying the ioctl, what about allowing rmdir(2) to work for an _empty_ subvolume (and all the permission check are satisfied) ? > > I'm inclined to agree with Goffredo. user_subvol_rm_allowed flag really > looks like a hack ontop of the ioctl. I'd rather we modify the generic > behavior. I agree as well, with the addendum that I'd love to see a new ioctl that does proper permissions checks. While letting rmdir(2) work for an empty subvolume with the appropriate permissions would be great (it will let rm -r work correctly), it doesn't address the usefulness of being able to just `btrfs subvolume delete` and not have to wait for the command to finish before you can reuse the name. -- 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 03/21/2018 12:47 PM, Austin S. Hemmelgarn wrote:
> I agree as well, with the addendum that I'd love to see a new ioctl that does proper permissions checks. While letting rmdir(2) work for an empty subvolume with the appropriate permissions would be great (it will let rm -r work correctly), it doesn't address the usefulness of being able to just `btrfs subvolume delete` and not have to wait for the command to finish before you can reuse the name.
How this could work ?
If you want to check all the subvolumes files permissions, this will require some time: you need to traverse all the subvolume-filesystem; and only if all the checks are passed, you can delete the subvolume.
Unfortunately I think that only two options exist:
- don't check permissions, and you can quick remove a subvolume
- check all the permissions, i.e. check all the files permissions, and only if all the permissions are OK, you can delete the subvolume. However this cannot be a "quick" subvolume delete
BR
G.Baroncelli
On 2018-03-21 16:38, Goffredo Baroncelli wrote: > On 03/21/2018 12:47 PM, Austin S. Hemmelgarn wrote: >> I agree as well, with the addendum that I'd love to see a new ioctl that does proper permissions checks. While letting rmdir(2) work for an empty subvolume with the appropriate permissions would be great (it will let rm -r work correctly), it doesn't address the usefulness of being able to just `btrfs subvolume delete` and not have to wait for the command to finish before you can reuse the name. > > How this could work ? > > If you want to check all the subvolumes files permissions, this will require some time: you need to traverse all the subvolume-filesystem; and only if all the checks are passed, you can delete the subvolume. > > Unfortunately I think that only two options exist: > - don't check permissions, and you can quick remove a subvolume > - check all the permissions, i.e. check all the files permissions, and only if all the permissions are OK, you can delete the subvolume. However this cannot be a "quick" subvolume delete Why exactly would you need to check everything? What I'm talking about is having behavior like `user_subvol_rm_allowed` be the default, with an additional check emulating the regular dentry removal check (namely that the user has appropriate permissions on the parent directory) so that people can't delete things like their own home directories. We're already _way_ beyond POSIX semantics here because we're debating the handling of permissions for an ioctl that takes a different fd than what it functionally operates on, so I see no reason whatsoever that we need to enforce POSIX semantics to that degree. -- 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 03/22/2018 01:15 PM, Austin S. Hemmelgarn wrote: > On 2018-03-21 16:38, Goffredo Baroncelli wrote: >> On 03/21/2018 12:47 PM, Austin S. Hemmelgarn wrote: >>> I agree as well, with the addendum that I'd love to see a new ioctl that does proper permissions checks. While letting rmdir(2) work for an empty subvolume with the appropriate permissions would be great (it will let rm -r work correctly), it doesn't address the usefulness of being able to just `btrfs subvolume delete` and not have to wait for the command to finish before you can reuse the name. >> >> How this could work ? >> >> If you want to check all the subvolumes files permissions, this will require some time: you need to traverse all the subvolume-filesystem; and only if all the checks are passed, you can delete the subvolume. >> >> Unfortunately I think that only two options exist: >> - don't check permissions, and you can quick remove a subvolume >> - check all the permissions, i.e. check all the files permissions, and only if all the permissions are OK, you can delete the subvolume. However this cannot be a "quick" subvolume delete > > Why exactly would you need to check everything? What I'm talking about is having behavior like `user_subvol_rm_allowed` be the default, with an additional check emulating the regular dentry removal check (namely that the user has appropriate permissions on the parent directory) so that people can't delete things like their own home directories. We're already _way_ beyond POSIX semantics here because we're debating the handling of permissions for an ioctl that takes a different fd than what it functionally operates on, so I see no reason whatsoever that we need to enforce POSIX semantics to that degree. > Why "user_subvol_rm_allowed" doesn't satisfy your needing ? Does not perform enough permission checking ? BR G.Baroncelli
On 2018/03/21 16:46, Nikolay Borisov wrote: > > > On 20.03.2018 22:06, Goffredo Baroncelli wrote: >> On 03/20/2018 07:45 AM, Misono, Tomohiro wrote: >>> Deletion of subvolume by non-privileged user is completely restricted >>> by default because we can delete a subvolume even if it is not empty >>> and may cause data loss. In other words, when user_subvol_rm_allowed >>> mount option is used, a user can delete a subvolume containing the >>> directory which cannot be deleted directly by the user. >>> >>> However, there should be no harm to allow users to delete empty subvolumes >>> when rmdir(2) would have been allowed if they were normal directories. >>> This patch allows deletion of empty subvolume by default. >> >> Instead of modifying the ioctl, what about allowing rmdir(2) to work for an _empty_ subvolume (and all the permission check are satisfied) ? > > I'm inclined to agree with Goffredo. user_subvol_rm_allowed flag really > looks like a hack ontop of the ioctl. I'd rather we modify the generic > behavior. Ok, I will send another patch which allows rmdir(2) to delete a subvolume. Thanks, Tomohiro Misono -- 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 111ee282b777..838406a7a7f5 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2366,36 +2366,43 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, dest = BTRFS_I(inode)->root; if (!capable(CAP_SYS_ADMIN)) { /* - * Regular user. Only allow this with a special mount - * option, when the user has write+exec access to the - * subvol root, and when rmdir(2) would have been - * allowed. + * By default, regular user is only allowed to delete + * empty subvols when rmdir(2) would have been allowed + * if they were normal directories. * - * Note that this is _not_ check that the subvol is - * empty or doesn't contain data that we wouldn't + * If the mount option 'user_subvol_rm_allowed' is set, + * it allows users to delete non-empty subvols when the + * user has write+exec access to the subvol root and when + * rmdir(2) would have been allowed (except the emptiness + * check). + * + * Note that this option does _not_ check that if the subvol + * is empty or doesn't contain data that the user wouldn't * otherwise be able to delete. * - * Users who want to delete empty subvols should try - * rmdir(2). + * Users who want to delete empty subvols created by + * snapshot (ino number == 2) can use rmdir(2). */ - err = -EPERM; - if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) - goto out_dput; + err = -ENOTEMPTY; + if (inode->i_size != BTRFS_EMPTY_DIR_SIZE) { + if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED)) + goto out_dput; - /* - * Do not allow deletion if the parent dir is the same - * as the dir to be deleted. That means the ioctl - * must be called on the dentry referencing the root - * of the subvol, not a random directory contained - * within it. - */ - err = -EINVAL; - if (root == dest) - goto out_dput; + /* + * Do not allow deletion if the parent dir is the same + * as the dir to be deleted. That means the ioctl + * must be called on the dentry referencing the root + * of the subvol, not a random directory contained + * within it. + */ + err = -EINVAL; + if (root == dest) + goto out_dput; - err = inode_permission(inode, MAY_WRITE | MAY_EXEC); - if (err) - goto out_dput; + err = inode_permission(inode, MAY_WRITE | MAY_EXEC); + if (err) + goto out_dput; + } } /* check if subvolume may be deleted by a user */
Deletion of subvolume by non-privileged user is completely restricted by default because we can delete a subvolume even if it is not empty and may cause data loss. In other words, when user_subvol_rm_allowed mount option is used, a user can delete a subvolume containing the directory which cannot be deleted directly by the user. However, there should be no harm to allow users to delete empty subvolumes when rmdir(2) would have been allowed if they were normal directories. This patch allows deletion of empty subvolume by default. Note that user_subvol_rm_allowed option requires write+exec permission of the subvolume to be deleted, but they are not required for empty subvolume. The comment in the code is also updated accordingly. Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com> --- fs/btrfs/ioctl.c | 55 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 24 deletions(-)