Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls

On Tue, May 13, 2025 at 11:53 AM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, May 13, 2025, at 11:17, Andrey Albershteyn wrote:
>
> >
> >       long syscall(SYS_file_getattr, int dirfd, const char *pathname,
> >               struct fsxattr *fsx, size_t size, unsigned int at_flags);
> >       long syscall(SYS_file_setattr, int dirfd, const char *pathname,
> >               struct fsxattr *fsx, size_t size, unsigned int at_flags);
>
> I don't think we can have both the "struct fsxattr" from the uapi
> headers, and a variable size as an additional argument. I would
> still prefer not having the extensible structure at all and just
> use fsxattr, but if you want to make it extensible in this way,
> it should use a different structure (name). Otherwise adding
> fields after fsx_pad[] would break the ioctl interface.
>

Are you are suggesting that we need to define this variant?:

--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -148,6 +148,17 @@ struct fsxattr {
        unsigned char   fsx_pad[8];
 };

+/*
+ * Variable size structure for file_[sg]et_attr().
+ */
+struct fsx_fileattr {
+       __u32           fsx_xflags;     /* xflags field value (get/set) */
+       __u32           fsx_extsize;    /* extsize field value (get/set)*/
+       __u32           fsx_nextents;   /* nextents field value (get)   */
+       __u32           fsx_projid;     /* project identifier (get/set) */
+       __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
+};
+

> I also find the bit confusing where the argument contains both
> "ignored but assumed zero" flags, and "required to be zero"
> flags depending on whether it's in the fsx_pad[] field or
> after it. This would be fine if it was better documented.
>

I think that is an oversight.
The syscall should have required that fsx_pad is zero,
same as patch 6/7 requires that unknown xflags are zero.

If we change to:
       error = copy_struct_from_user(&fsx, sizeof(struct
fsx_fileattr), ufsx, usize);

It will take care of requiring zero fsx_pad even if user calls the syscall with
sizeof(struct fsxattr).

Thanks,
Amir.