[PATCH 0/3] Clean up supported flags for ioctls

[PATCH 0/3] Clean up supported flags for ioctls

[David Sterba]

The subvolume args passed as the vol_args_v2 should be separated by the
ioctl using them, add that for existing ioctls. The upcoming subvolume
deletion-by-id will add new flags, so this should be considered
preparatory patchset.

David Sterba (3):
  btrfs: define support masks for ioctl volume args v2
  btrfs: use ioctl args support mask for subvolume create/delete
  btrfs: use ioctl args support mask for device delete

 fs/btrfs/ioctl.c           |  7 ++-----
 include/uapi/linux/btrfs.h | 16 +++++++++++++---
 2 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.25.0

[PATCH 1/3] btrfs: define support masks for ioctl volume args v2

[David Sterba]

The ioctl data for devices or subvolumes can be passed via
btrfs_ioctl_vol_args or btrfs_ioctl_vol_args_v2. The latter is more
versatile and needs some caution as some of the flags make sense only
for some ioctls.

As we're going to extend the flags, define support masks for each ioctl
class separately.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 include/uapi/linux/btrfs.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 7a8bc8b920f5..49ed71df5e94 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -97,16 +97,26 @@ struct btrfs_ioctl_qgroup_limit_args {
 };
 
 /*
- * flags for subvolumes
+ * Arguments for specification of subvolumes or devices, supporting by-name or
+ * by-id and flags
  *
- * Used by:
- * struct btrfs_ioctl_vol_args_v2.flags
+ * The set of supported flags depends on the ioctl
  *
  * BTRFS_SUBVOL_RDONLY is also provided/consumed by the following ioctls:
  * - BTRFS_IOC_SUBVOL_GETFLAGS
  * - BTRFS_IOC_SUBVOL_SETFLAGS
  */
 
+/* Supported flags for BTRFS_IOC_RM_DEV_V2 */
+#define BTRFS_DEVICE_REMOVE_ARGS_MASK					\
+	(BTRFS_DEVICE_SPEC_BY_ID)
+
+/* Supported flags for BTRFS_IOC_SNAP_CREATE_V2 and BTRFS_IOC_SUBVOL_CREATE_V2 */
+#define BTRFS_SUBVOL_CREATE_ARGS_MASK					\
+	(BTRFS_SUBVOL_CREATE_ASYNC |					\
+	 BTRFS_SUBVOL_RDONLY |						\
+	 BTRFS_SUBVOL_QGROUP_INHERIT)
+
 struct btrfs_ioctl_vol_args_v2 {
 	__s64 fd;
 	__u64 transid;
-- 
2.25.0

Re: [PATCH 1/3] btrfs: define support masks for ioctl volume args v2

[Marcos Paulo de Souza]

On Fri, 2020-02-21 at 14:02 +0100, David Sterba wrote:
> The ioctl data for devices or subvolumes can be passed via
> btrfs_ioctl_vol_args or btrfs_ioctl_vol_args_v2. The latter is more
> versatile and needs some caution as some of the flags make sense only
> for some ioctls.
> 
> As we're going to extend the flags, define support masks for each
> ioctl
> class separately.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  include/uapi/linux/btrfs.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 7a8bc8b920f5..49ed71df5e94 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -97,16 +97,26 @@ struct btrfs_ioctl_qgroup_limit_args {
>  };
>  
>  /*
> - * flags for subvolumes
> + * Arguments for specification of subvolumes or devices, supporting
> by-name or
> + * by-id and flags
>   *
> - * Used by:
> - * struct btrfs_ioctl_vol_args_v2.flags
> + * The set of supported flags depends on the ioctl
>   *
>   * BTRFS_SUBVOL_RDONLY is also provided/consumed by the following
> ioctls:
>   * - BTRFS_IOC_SUBVOL_GETFLAGS
>   * - BTRFS_IOC_SUBVOL_SETFLAGS
>   */
>  
> +/* Supported flags for BTRFS_IOC_RM_DEV_V2 */
> +#define BTRFS_DEVICE_REMOVE_ARGS_MASK				
> 	\
> +	(BTRFS_DEVICE_SPEC_BY_ID)
> +
> +/* Supported flags for BTRFS_IOC_SNAP_CREATE_V2 and
> BTRFS_IOC_SUBVOL_CREATE_V2 */
> +#define BTRFS_SUBVOL_CREATE_ARGS_MASK				
> 	\
> +	(BTRFS_SUBVOL_CREATE_ASYNC |					
> \
> +	 BTRFS_SUBVOL_RDONLY |						
> \
> +	 BTRFS_SUBVOL_QGROUP_INHERIT)
> +
>  struct btrfs_ioctl_vol_args_v2 {
>  	__s64 fd;
>  	__u64 transid;

Looks good to me,
Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>

[PATCH 2/3] btrfs: use ioctl args support mask for subvolume create/delete

[David Sterba]

Using the defined mask instead of flag enumeration in the ioctl handler
is preferred. No functional changes.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7305e6770157..a7872cacd0aa 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1838,9 +1838,7 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 		return PTR_ERR(vol_args);
 	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
 
-	if (vol_args->flags &
-	    ~(BTRFS_SUBVOL_CREATE_ASYNC | BTRFS_SUBVOL_RDONLY |
-	      BTRFS_SUBVOL_QGROUP_INHERIT)) {
+	if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_ARGS_MASK) {
 		ret = -EOPNOTSUPP;
 		goto free_args;
 	}
-- 
2.25.0

Re: [PATCH 2/3] btrfs: use ioctl args support mask for subvolume create/delete

[Marcos Paulo de Souza]

On Fri, 2020-02-21 at 14:02 +0100, David Sterba wrote:
> Using the defined mask instead of flag enumeration in the ioctl
> handler
> is preferred. No functional changes.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7305e6770157..a7872cacd0aa 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1838,9 +1838,7 @@ static noinline int
> btrfs_ioctl_snap_create_v2(struct file *file,
>  		return PTR_ERR(vol_args);
>  	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
>  
> -	if (vol_args->flags &
> -	    ~(BTRFS_SUBVOL_CREATE_ASYNC | BTRFS_SUBVOL_RDONLY |
> -	      BTRFS_SUBVOL_QGROUP_INHERIT)) {
> +	if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_ARGS_MASK) {
>  		ret = -EOPNOTSUPP;
>  		goto free_args;
>  	}

Looks good to me,
Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>

[PATCH 3/3] btrfs: use ioctl args support mask for device delete

[David Sterba]

When the device remove v2 ioctl was added, the full support mask was
added to sanity check the flags. However this would allow to let the
subvolume related flags to be accepted. This is not supposed to happen.

Use the correct support mask, which means that now any of
BTRFS_SUBVOL_CREATE_ASYNC, BTRFS_SUBVOL_RDONLY or
BTRFS_SUBVOL_QGROUP_INHERIT will be rejected as ENOTSUPP. Though this is
a user-visible change, specifying subvolume flags for device deletion
does not make sense and there are hopefully no applications doing that.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a7872cacd0aa..cd2d11dcd477 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3075,8 +3075,7 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 		goto err_drop;
 	}
 
-	/* Check for compatibility reject unknown flags */
-	if (vol_args->flags & ~BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED) {
+	if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.25.0

Re: [PATCH 3/3] btrfs: use ioctl args support mask for device delete

[Marcos Paulo de Souza]

On Fri, 2020-02-21 at 14:02 +0100, David Sterba wrote:
> When the device remove v2 ioctl was added, the full support mask was
> added to sanity check the flags. However this would allow to let the
> subvolume related flags to be accepted. This is not supposed to
> happen.
> 
> Use the correct support mask, which means that now any of
> BTRFS_SUBVOL_CREATE_ASYNC, BTRFS_SUBVOL_RDONLY or
> BTRFS_SUBVOL_QGROUP_INHERIT will be rejected as ENOTSUPP. Though this
> is
> a user-visible change, specifying subvolume flags for device deletion
> does not make sense and there are hopefully no applications doing
> that.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a7872cacd0aa..cd2d11dcd477 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3075,8 +3075,7 @@ static long btrfs_ioctl_rm_dev_v2(struct file
> *file, void __user *arg)
>  		goto err_drop;
>  	}
>  
> -	/* Check for compatibility reject unknown flags */
> -	if (vol_args->flags & ~BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED) {
> +	if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}

Looks good to me,
Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Re: [PATCH 0/3] Clean up supported flags for ioctls

[Nikolay Borisov]

On 21.02.20 г. 15:02 ч., David Sterba wrote:
> The subvolume args passed as the vol_args_v2 should be separated by the
> ioctl using them, add that for existing ioctls. The upcoming subvolume
> deletion-by-id will add new flags, so this should be considered
> preparatory patchset.
> 
> David Sterba (3):
>   btrfs: define support masks for ioctl volume args v2
>   btrfs: use ioctl args support mask for subvolume create/delete
>   btrfs: use ioctl args support mask for device delete
> 
>  fs/btrfs/ioctl.c           |  7 ++-----
>  include/uapi/linux/btrfs.h | 16 +++++++++++++---
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 


For the whole series:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


I was only worried about patch 3 but I see you have intentionally done
it this way and I'm fine with this.