Add extra sanity check for btrfs_ioctl_defrag_range_args::flags.
This is not really to enhance fuzzing tests, but as a preparation for future expansion on btrfs_ioctl_defrag_range_args.
In the future we're adding new members, allowing more fine tuning for btrfs defrag. Without the -ENONOTSUPP error, there would be no way to detect if the kernel supports those new defrag features.
cc: stable@vger.kernel.org #4.14+ Signed-off-by: Qu Wenruo wqu@suse.com --- fs/btrfs/ioctl.c | 4 ++++ include/uapi/linux/btrfs.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a1743904202b..3a846b983b28 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2608,6 +2608,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = -EFAULT; goto out; } + if (range.flags & ~BTRFS_DEFRAG_RANGE_FLAGS_SUPP) { + ret = -EOPNOTSUPP; + goto out; + } /* compression requires us to start the IO */ if ((range.flags & BTRFS_DEFRAG_RANGE_COMPRESS)) { range.flags |= BTRFS_DEFRAG_RANGE_START_IO; diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index 7c29d82db9ee..48e9b7ffecf1 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -614,6 +614,8 @@ struct btrfs_ioctl_clone_range_args { */ #define BTRFS_DEFRAG_RANGE_COMPRESS 1 #define BTRFS_DEFRAG_RANGE_START_IO 2 +#define BTRFS_DEFRAG_RANGE_FLAGS_SUPP (BTRFS_DEFRAG_RANGE_COMPRESS |\ + BTRFS_DEFRAG_RANGE_START_IO) struct btrfs_ioctl_defrag_range_args { /* start of the defrag operation */ __u64 start;
On Wed, Jan 10, 2024 at 08:58:26AM +1030, Qu Wenruo wrote:
Add extra sanity check for btrfs_ioctl_defrag_range_args::flags.
This is not really to enhance fuzzing tests, but as a preparation for future expansion on btrfs_ioctl_defrag_range_args.
In the future we're adding new members, allowing more fine tuning for btrfs defrag. Without the -ENONOTSUPP error, there would be no way to detect if the kernel supports those new defrag features.
cc: stable@vger.kernel.org #4.14+ Signed-off-by: Qu Wenruo wqu@suse.com
Added to misc-next, thanks.
fs/btrfs/ioctl.c | 4 ++++ include/uapi/linux/btrfs.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a1743904202b..3a846b983b28 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2608,6 +2608,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = -EFAULT; goto out; }
if (range.flags & ~BTRFS_DEFRAG_RANGE_FLAGS_SUPP) {ret = -EOPNOTSUPP;
This should be EINVAL, this is for invalid parameter values or combinations, EOPNOTSUPP would be for the whole ioctl as not supported.
On Wed, Jan 10, 2024 at 12:55 AM David Sterba dsterba@suse.cz wrote:
On Wed, Jan 10, 2024 at 08:58:26AM +1030, Qu Wenruo wrote:
Add extra sanity check for btrfs_ioctl_defrag_range_args::flags.
This is not really to enhance fuzzing tests, but as a preparation for future expansion on btrfs_ioctl_defrag_range_args.
In the future we're adding new members, allowing more fine tuning for btrfs defrag. Without the -ENONOTSUPP error, there would be no way to detect if the kernel supports those new defrag features.
cc: stable@vger.kernel.org #4.14+ Signed-off-by: Qu Wenruo wqu@suse.com
Added to misc-next, thanks.
fs/btrfs/ioctl.c | 4 ++++ include/uapi/linux/btrfs.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a1743904202b..3a846b983b28 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2608,6 +2608,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = -EFAULT; goto out; }
if (range.flags & ~BTRFS_DEFRAG_RANGE_FLAGS_SUPP) {ret = -EOPNOTSUPP;This should be EINVAL, this is for invalid parameter values or combinations, EOPNOTSUPP would be for the whole ioctl as not supported.
I'm confused now. We return EOPNOTSUPP for a lot of ioctls when they are given an unknown flag, for example at btrfs_ioctl_scrub():
if (sa->flags & ~BTRFS_SCRUB_SUPPORTED_FLAGS) { ret = -EOPNOTSUPP; goto out; }
Or at btrfs_ioctl_snap_create_v2():
if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_ARGS_MASK) { ret = -EOPNOTSUPP; goto free_args; }
We also do similar for fallocate, at btrfs_fallocate():
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) return -EOPNOTSUPP;
I was under the expectation that EOPNOTSUPP is the correct thing to do in this patch. So what's different in this patch from those existing examples to justify EINVAL instead?
Thanks.
On Wed, Jan 10, 2024 at 11:43:39AM +0000, Filipe Manana wrote:
On Wed, Jan 10, 2024 at 12:55 AM David Sterba dsterba@suse.cz wrote:
On Wed, Jan 10, 2024 at 08:58:26AM +1030, Qu Wenruo wrote:
Add extra sanity check for btrfs_ioctl_defrag_range_args::flags.
This is not really to enhance fuzzing tests, but as a preparation for future expansion on btrfs_ioctl_defrag_range_args.
In the future we're adding new members, allowing more fine tuning for btrfs defrag. Without the -ENONOTSUPP error, there would be no way to detect if the kernel supports those new defrag features.
cc: stable@vger.kernel.org #4.14+ Signed-off-by: Qu Wenruo wqu@suse.com
Added to misc-next, thanks.
fs/btrfs/ioctl.c | 4 ++++ include/uapi/linux/btrfs.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a1743904202b..3a846b983b28 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2608,6 +2608,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = -EFAULT; goto out; }
if (range.flags & ~BTRFS_DEFRAG_RANGE_FLAGS_SUPP) {ret = -EOPNOTSUPP;This should be EINVAL, this is for invalid parameter values or combinations, EOPNOTSUPP would be for the whole ioctl as not supported.
I'm confused now. We return EOPNOTSUPP for a lot of ioctls when they are given an unknown flag, for example at btrfs_ioctl_scrub():
if (sa->flags & ~BTRFS_SCRUB_SUPPORTED_FLAGS) { ret = -EOPNOTSUPP; goto out; }
Or at btrfs_ioctl_snap_create_v2():
if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_ARGS_MASK) { ret = -EOPNOTSUPP; goto free_args; }
We also do similar for fallocate, at btrfs_fallocate():
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) return -EOPNOTSUPP;
I was under the expectation that EOPNOTSUPP is the correct thing to do in this patch. So what's different in this patch from those existing examples to justify EINVAL instead?
Seems that we indeed do EOPNOTSUPP for unsupported flags while EINVAL is for invalid parameters, altough there's
btrfs_ioctl_send()
8113 if (arg->flags & ~BTRFS_SEND_FLAG_MASK) { 8114 ret = -EINVAL; 8115 goto out; 8116 } 8117
Either way it should be consistent, so the send flag check is a mistake. I'll update the patch from Qu back to EOPNOTSUPP. Thanks.
On Wed, Jan 10, 2024 at 2:44 PM David Sterba dsterba@suse.cz wrote:
On Wed, Jan 10, 2024 at 11:43:39AM +0000, Filipe Manana wrote:
On Wed, Jan 10, 2024 at 12:55 AM David Sterba dsterba@suse.cz wrote:
On Wed, Jan 10, 2024 at 08:58:26AM +1030, Qu Wenruo wrote:
Add extra sanity check for btrfs_ioctl_defrag_range_args::flags.
This is not really to enhance fuzzing tests, but as a preparation for future expansion on btrfs_ioctl_defrag_range_args.
In the future we're adding new members, allowing more fine tuning for btrfs defrag. Without the -ENONOTSUPP error, there would be no way to detect if the kernel supports those new defrag features.
cc: stable@vger.kernel.org #4.14+ Signed-off-by: Qu Wenruo wqu@suse.com
Added to misc-next, thanks.
fs/btrfs/ioctl.c | 4 ++++ include/uapi/linux/btrfs.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a1743904202b..3a846b983b28 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2608,6 +2608,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = -EFAULT; goto out; }
if (range.flags & ~BTRFS_DEFRAG_RANGE_FLAGS_SUPP) {ret = -EOPNOTSUPP;This should be EINVAL, this is for invalid parameter values or combinations, EOPNOTSUPP would be for the whole ioctl as not supported.
I'm confused now. We return EOPNOTSUPP for a lot of ioctls when they are given an unknown flag, for example at btrfs_ioctl_scrub():
if (sa->flags & ~BTRFS_SCRUB_SUPPORTED_FLAGS) { ret = -EOPNOTSUPP; goto out; }
Or at btrfs_ioctl_snap_create_v2():
if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_ARGS_MASK) { ret = -EOPNOTSUPP; goto free_args; }
We also do similar for fallocate, at btrfs_fallocate():
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) return -EOPNOTSUPP;
I was under the expectation that EOPNOTSUPP is the correct thing to do in this patch. So what's different in this patch from those existing examples to justify EINVAL instead?
Seems that we indeed do EOPNOTSUPP for unsupported flags while EINVAL is for invalid parameters, altough there's
btrfs_ioctl_send()
8113 if (arg->flags & ~BTRFS_SEND_FLAG_MASK) { 8114 ret = -EINVAL; 8115 goto out; 8116 } 8117
Either way it should be consistent, so the send flag check is a mistake. I'll update the patch from Qu back to EOPNOTSUPP. Thanks.
Ok, with that:
Reviewed-by: Filipe Manana fdmanana@suse.com
Thanks.
linux-stable-mirror@lists.linaro.org
-
David Sterba -
Filipe Manana -
Qu Wenruo