On 2025-03-29 15:33:11, Amir Goldstein wrote: > We intend to add support for more xflags to selective filesystems and > We cannot rely on copy_struct_from_user() to detect this extention. > > 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> > --- > fs/inode.c | 4 +++- > fs/ioctl.c | 19 +++++++++++++------ > include/linux/fileattr.h | 22 +++++++++++++++++++++- > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 3cfcb1b9865ea..6c4d08bd53052 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -3049,7 +3049,9 @@ SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename, > if (error) > return error; > > - fsxattr_to_fileattr(&fsx, &fa); > + error = fsxattr_to_fileattr(&fsx, &fa); > + if (error) > + return error; > > name = getname_maybe_null(filename, at_flags); > if (!name) { > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 840283d8c4066..b19858db4c432 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -540,8 +540,10 @@ EXPORT_SYMBOL(vfs_fileattr_get); > > void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx) > { > + __u32 mask = FS_XFALGS_MASK; > + > memset(fsx, 0, sizeof(struct fsxattr)); > - fsx->fsx_xflags = fa->fsx_xflags; > + fsx->fsx_xflags = fa->fsx_xflags & mask; > fsx->fsx_extsize = fa->fsx_extsize; > fsx->fsx_nextents = fa->fsx_nextents; > fsx->fsx_projid = fa->fsx_projid; > @@ -568,13 +570,20 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa) > } > EXPORT_SYMBOL(copy_fsxattr_to_user); > > -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa) > +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa) > { > + __u32 mask = FS_XFALGS_MASK; > + > + if (fsx->fsx_xflags & ~mask) > + return -EINVAL; > + > fileattr_fill_xflags(fa, fsx->fsx_xflags); > + fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK; > fa->fsx_extsize = fsx->fsx_extsize; > - fa->fsx_nextents = fsx->fsx_nextents; > fa->fsx_projid = fsx->fsx_projid; > fa->fsx_cowextsize = fsx->fsx_cowextsize; > + > + return 0; > } > > static int copy_fsxattr_from_user(struct fileattr *fa, > @@ -585,9 +594,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa, > if (copy_from_user(&xfa, ufa, sizeof(xfa))) > return -EFAULT; > > - fsxattr_to_fileattr(&xfa, fa); > - > - return 0; > + return fsxattr_to_fileattr(&xfa, fa); > } > > /* > diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h > index 31888fa2edf10..f682bfc7749dd 100644 > --- a/include/linux/fileattr.h > +++ b/include/linux/fileattr.h > @@ -14,6 +14,26 @@ > FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \ > FS_XFLAG_PROJINHERIT) > > +/* Read-only inode flags */ Maybe it's only me, but this "read-only" is a bit confusing, as those are not settable get-only flags and not flags of read-only inode > +#define FS_XFLAG_RDONLY_MASK \ > + (FS_XFLAG_PREALLOC | FS_XFLAG_HASATTR) > + > +/* Flags to indicate valid value of fsx_ fields */ > +#define FS_XFLAG_VALUES_MASK \ > + (FS_XFLAG_EXTSIZE | FS_XFLAG_COWEXTSIZE) > + > +/* Flags for directories */ > +#define FS_XFLAG_DIRONLY_MASK \ > + (FS_XFLAG_RTINHERIT | FS_XFLAG_NOSYMLINKS | FS_XFLAG_EXTSZINHERIT) > + > +/* Misc settable flags */ > +#define FS_XFLAG_MISC_MASK \ > + (FS_XFLAG_REALTIME | FS_XFLAG_NODEFRAG | FS_XFLAG_FILESTREAM) > + > +#define FS_XFALGS_MASK \ > + (FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \ > + FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK) > + I like the splitting but do we want to split flags like this? I can imagine new flags just getting pushed into _MISK_MASK or these names just loosing any sense. > /* > * Merged interface for miscellaneous file attributes. 'flags' originates from > * ext* and 'fsx_flags' from xfs. There's some overlap between the two, which > @@ -35,7 +55,7 @@ struct fileattr { > > void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx); > int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa); > -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa); > +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa); > > void fileattr_fill_xflags(struct fileattr *fa, u32 xflags); > void fileattr_fill_flags(struct fileattr *fa, u32 flags); > -- > 2.34.1 > Otherwise, this patch looks fine for limiting the interface for now I will include it in next iteration -- - Andrey
