On Thu, May 15, 2025 at 12:33:31PM +0200, Amir Goldstein wrote:
> On Thu, May 15, 2025 at 11:02 AM Christian Brauner <brau...@kernel.org> wrote:
> >
> > On Tue, May 13, 2025 at 11:53:23AM +0200, Arnd Bergmann 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
> >
> > We're not going to add new interfaces that are fixed size unless for the
> > very basic cases. I don't care if we're doing that somewhere else in the
> > kernel but we're not doing that for vfs apis.
> >
> > > 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.
> >
> > Would that really be a problem? Just along the syscall simply add
> > something like:
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index c91fd2b46a77..d3943805c4be 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -868,12 +868,6 @@ static int do_vfs_ioctl(struct file *filp, unsigned
> > int fd,
> > case FS_IOC_SETFLAGS:
> > return ioctl_setflags(filp, argp);
> >
> > - case FS_IOC_FSGETXATTR:
> > - return ioctl_fsgetxattr(filp, argp);
> > -
> > - case FS_IOC_FSSETXATTR:
> > - return ioctl_fssetxattr(filp, argp);
> > -
> > case FS_IOC_GETFSUUID:
> > return ioctl_getfsuuid(filp, argp);
> >
> > @@ -886,6 +880,20 @@ static int do_vfs_ioctl(struct file *filp, unsigned
> > int fd,
> > break;
> > }
> >
> > + switch (_IOC_NR(cmd)) {
> > + case _IOC_NR(FS_IOC_FSGETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) !=
> > _IOC_TYPE(FS_IOC_FSGETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fsgetxattr(filp, argp);
> > +
> > + case _IOC_NR(FFS_IOC_FSSETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) !=
> > _IOC_TYPE(FFS_IOC_FSSETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fssetxattr(filp, argp);
> > + }
> > +
>
> I think what Arnd means is that we will not be able to change struct
> sfxattr in uapi
> going forward, because we are not going to deprecate the ioctls and
There's no need to deprecate anything to rev an ioctl API. We have
had to solve this "changing struct size" problem previously in XFS
ioctls. See XFS_IOC_FSGEOMETRY and the older XFS_IOC_FSGEOMETRY_V4
and XFS_IOC_FSGEOMETRY_V1 versions of the API/ABI.
If we need to increase the structure size, we can rename the existing
ioctl and struct to fix the version in the API, then use the
original name for the new ioctl and structure definition.
The only thing we have to make sure of is that the old and new
structures have exactly the same overlapping structure. i.e.
extension must always be done by appending new varibles, they can't
be put in the middle of the structure.
This way applications being rebuild will pick up the new definition
automatically when the system asserts that it is suppored, whilst
existing binaries will always still be supported by the kernel.
If the application wants/needs to support all possible kernels, then
if XFS_IOC_FSGEOMETRY is not supported, call XFS_IOC_FSGEOMETRY_V4,
and if that fails (only on really old irix!) or you only need
something in that original subset, call XFS_IOC_FSGEOMETRY_V1 which
will always succeed....
> Should we will need to depart from this struct definition and we might
> as well do it for the initial release of the syscall rather than later on,
> e.g.:
>
> --- 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)*/
> +};
> +
> +#define FSXATTR_SIZE_VER0 20
> +#define FSXATTR_SIZE_LATEST FSXATTR_SIZE_VER0
If all the structures overlap the same, all that is needed in the
code is to define the structure size that should be copied in and
parsed. i.e:
case FSXATTR..._V1:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr_v1));
case FSXATTR..._V2:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr_v2));
case FSXATTR...:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr));
-Dave.
--
Dave Chinner
da...@fromorbit.com
