On Wed, Jan 10, 2024 at 2:44 PM David Sterba <dsterba@xxxxxxx> 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@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.
Ok, with that:
Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
Thanks.
