On Mon, Mar 31, 2025 at 4:43 PM Andrey Albershteyn <aalbersh@xxxxxxxxxx> wrote: > > 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 > I am also not crazy about this name. I am fine with GETONLY_MASK. > > +#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. > I mostly did this for my own sake of order, but I do not mind if you decide to include this grouping or not. Up to you. Just don't carry the typo FS_XFALGS_MASK ;) > > /* > > * 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 Thanks! Amir.
