Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls

On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> > On Fri, Feb 21, 2025 at 08:15:24PM +0100, Amir Goldstein wrote:
> >> On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <[email protected]> wrote:
> 
> >> > > @@ -23,6 +23,9 @@
> >> > >  #include <linux/rw_hint.h>
> >> > >  #include <linux/seq_file.h>
> >> > >  #include <linux/debugfs.h>
> >> > > +#include <linux/syscalls.h>
> >> > > +#include <linux/fileattr.h>
> >> > > +#include <linux/namei.h>
> >> > >  #include <trace/events/writeback.h>
> >> > >  #define CREATE_TRACE_POINTS
> >> > >  #include <trace/events/timestamp.h>
> >> > > @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> >> > >       return mode & ~S_ISGID;
> >> > >  }
> >> > >  EXPORT_SYMBOL(mode_strip_sgid);
> >> > > +
> >> > > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
> >> > > +             struct fsxattr __user *, fsx, unsigned int, at_flags)
> >> >
> >> > Should the kernel require userspace to pass the size of the fsx buffer?
> >> > That way we avoid needing to rev the interface when we decide to grow
> >> > the structure.
> >
> > Please version the struct by size as we do for clone3(),
> > mount_setattr(), listmount()'s struct mnt_id_req, sched_setattr(), all
> > the new xattrat*() system calls and a host of others. So laying out the
> > struct 64bit and passing a size alongside it.
> >
> > This is all handled by copy_struct_from_user() and copy_struct_to_user()
> > so nothing to reinvent. And it's easy to copy from existing system
> > calls.
> 
> I don't think that works in this case, because 'struct fsxattr'
> is an existing structure that is defined with a fixed size of
> 28 bytes. If we ever need more than 8 extra bytes, then the
> existing ioctl commands are also broken.
> 
> Replacing fsxattr with an extensible structure of the same contents
> would work, but I feel that just adds more complication for little
> gain.
> 
> On the other hand, there is an open question about how unknown
> flags and fields in this structure. FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
> treats them as optional and just ignores anything it doesn't
> understand, while copy_struct_from_user() would treat any unknown
> but set bytes as -E2BIG.
> 
> The ioctl interface relies on the existing behavior, see
> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> CoW extent size hint") for how it was previously extended
> with an optional flag/word. I think that is fine for the syscall
> as well, but should be properly documented since it is different
> from how most syscalls work.

If we're doing a new system call I see no reason to limit us to a
pre-existing structure or structure layout.