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. > > 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. Thanks, Amir.
