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@xxxxxxx> 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@xxxxxxxxxxxxxxx #4.14+
> > > Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> >
> > 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.
