Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 30, 2025 at 06:20:15PM +0200, Andrey Albershteyn wrote:
> > > From: Amir Goldstein <amir73il@xxxxxxxxx>
> > >
> > > We intend to add support for more xflags to selective filesystems and
> > > We cannot rely on copy_struct_from_user() to detect this extension.
> > >
> > > In preparation of extending the API, do not allow setting xflags unknown
> > > by this kernel version.
> > >
> > > Also do not pass the read-only flags and read-only field fsx_nextents to
> > > filesystem.
> > >
> > > These changes should not affect existing chattr programs that use the
> > > ioctl to get fsxattr before setting the new values.
> > >
> > > Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@xxxxxxxxxx/
> > > Cc: Pali Rohár <pali@xxxxxxxxxx>
> > > Cc: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> > > ---
> > >  fs/file_attr.c           |  8 +++++++-
> > >  include/linux/fileattr.h | 20 ++++++++++++++++++++
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/file_attr.c b/fs/file_attr.c
> > > index 4e85fa00c092..62f08872d4ad 100644
> > > --- a/fs/file_attr.c
> > > +++ b/fs/file_attr.c
> > > @@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> > >  int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> > >  {
> > >       struct fsxattr xfa;
> > > +     __u32 mask = FS_XFLAGS_MASK;
> > >
> > >       memset(&xfa, 0, sizeof(xfa));
> > > -     xfa.fsx_xflags = fa->fsx_xflags;
> > > +     xfa.fsx_xflags = fa->fsx_xflags & mask;
> >
> > I wonder, should it be an error if a filesystem sets an fsx_xflags bit
> > outside of FS_XFLAGS_MASK?  I guess that's one way to prevent
> > filesystems from overriding the VFS bits. ;)
> 
> I think Pali has a plan on how to ensure that later
> when the mask is provided via the API.
> 
> >
> > Though couldn't that be:
> >
> >         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> >
> > instead?  And same below?
> >
> 
> Indeed. There is a reason for the var, because the next series
> by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> so I left it this way.
> 
> I don't see a problem with it keeping as is, but if it bothers you
> I guess we can re-add the var later.

Nah, it doesn't bother me that much.

> > >       xfa.fsx_extsize = fa->fsx_extsize;
> > >       xfa.fsx_nextents = fa->fsx_nextents;
> > >       xfa.fsx_projid = fa->fsx_projid;
> > > @@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
> > >                                 struct fsxattr __user *ufa)
> > >  {
> > >       struct fsxattr xfa;
> > > +     __u32 mask = FS_XFLAGS_MASK;
> > >
> > >       if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> > >               return -EFAULT;
> > >
> > > +     if (xfa.fsx_xflags & ~mask)
> > > +             return -EINVAL;
> >
> > I wonder if you want EOPNOTSUPP here?  We don't know how to support
> > unknown xflags.  OTOH if you all have beaten this to death while I was
> > out then don't start another round just for me. :P
> 
> We have beaten this API almost to death for sure ;)
> I don't remember if we discussed this specific aspect,
> but I am personally in favor of
> EOPNOTSUPP := the fs does not support the set/get operation
> EINVAL := some flags provided as value is invalid
> 
> For example, if the get API provides you with a mask of the
> valid flags that you can set, if you try to set flags outside of
> that mask you get EINVAL.
> 
> That's my interpretation, but I agree that EOPNOTSUPP can also
> make sense in this situation.

<nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
doesn't recognize" and EINVAL (or maybe something else like
EPROTONOSUPPORT) for "fs driver will not let you change this bit".
At least for the syscall interface; we probably have to flatten that to
EOPNOTSUPP for both legacy ioctls.

--D

> Thanks,
> Amir.
>