| Message ID | 20250329143312.1350603-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | etfsxattrat() followup patches | expand |
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@kernel.org/ > Cc: Pali Rohár <pali@kernel.org> > Cc: Andrey Albershteyn <aalbersh@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > 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
On Mon, Mar 31, 2025 at 4:43 PM Andrey Albershteyn <aalbersh@redhat.com> 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@kernel.org/ > > Cc: Pali Rohár <pali@kernel.org> > > Cc: Andrey Albershteyn <aalbersh@redhat.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > 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.
On Sat, Mar 29, 2025 at 03:33:11PM +0100, 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. ..... > + > +#define FS_XFALGS_MASK \ > + (FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \ > + FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK) You might want to fix the obvious typo.... -Dave.
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 */ +#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) + /* * 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);
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@kernel.org/ Cc: Pali Rohár <pali@kernel.org> Cc: Andrey Albershteyn <aalbersh@redhat.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/inode.c | 4 +++- fs/ioctl.c | 19 +++++++++++++------ include/linux/fileattr.h | 22 +++++++++++++++++++++- 3 files changed, 37 insertions(+), 8 deletions(-)