Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Misono Tomohiro]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Goffredo Baroncelli]

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


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Austin S. Hemmelgarn]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Goffredo Baroncelli]

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

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Austin S. Hemmelgarn]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Nikolay Borisov]

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

Re: [PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Goffredo Baroncelli]

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 
> ---
>  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 */
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] btrfs: Allow non-privileged user to delete empty subvolume by default

[Misono, Tomohiro]

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 
---
 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 */
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html