| Message ID | 0d35011f8afe8bd55c1f0318b0d2515ea10eac7f.1704839283.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: defrag: reject unknown flags of btrfs_ioctl_defrag_range_args | expand |
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.
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;
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(+)