From: Andrey Albershteyn <aalbersh@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: "Amir Goldstein" <amir73il@gmail.com>,
"Christian Brauner" <brauner@kernel.org>,
"Arnd Bergmann" <arnd@arndb.de>,
"Richard Henderson" <richard.henderson@linaro.org>,
"Matt Turner" <mattst88@gmail.com>,
"Russell King" <linux@armlinux.org.uk>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Will Deacon" <will@kernel.org>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
"Michal Simek" <monstr@monstr.eu>,
"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
"James E . J . Bottomley" <James.Bottomley@hansenpartnership.com>,
"Helge Deller" <deller@gmx.de>,
"Madhavan Srinivasan" <maddy@linux.ibm.com>,
"Michael Ellerman" <mpe@ellerman.id.au>,
"Nicholas Piggin" <npiggin@gmail.com>,
"Christophe Leroy" <christophe.leroy@csgroup.eu>,
"Naveen N Rao" <naveen@kernel.org>,
"Heiko Carstens" <hca@linux.ibm.com>,
"Vasily Gorbik" <gor@linux.ibm.com>,
"Alexander Gordeev" <agordeev@linux.ibm.com>,
"Christian Borntraeger" <borntraeger@linux.ibm.com>,
"Sven Schnelle" <svens@linux.ibm.com>,
"Yoshinori Sato" <ysato@users.sourceforge.jp>,
"Rich Felker" <dalias@libc.org>,
"John Paul Adrian Glaubitz" <glaubitz@physik.fu-berlin.de>,
"David S . Miller" <davem@davemloft.net>,
"Andreas Larsson" <andreas@gaisler.com>,
"Andy Lutomirski" <luto@kernel.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Ingo Molnar" <mingo@redhat.com>,
"Borislav Petkov" <bp@alien8.de>,
"Dave Hansen" <dave.hansen@linux.intel.com>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
"Chris Zankel" <chris@zankel.net>,
"Max Filippov" <jcmvbkbc@gmail.com>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Jan Kara" <jack@suse.cz>, "Mickaël Salaün" <mic@digikod.net>,
"Günther Noack" <gnoack@google.com>,
"Pali Rohár" <pali@kernel.org>,
"Paul Moore" <paul@paul-moore.com>,
"James Morris" <jmorris@namei.org>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Stephen Smalley" <stephen.smalley.work@gmail.com>,
"Ondrej Mosnacek" <omosnace@redhat.com>,
"Tyler Hicks" <code@tyhicks.com>,
"Miklos Szeredi" <miklos@szeredi.hu>,
linux-alpha@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-m68k@lists.linux-m68k.org, linux-mips@vger.kernel.org,
linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, linux-sh@vger.kernel.org,
sparclinux@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org, linux-api@vger.kernel.org,
Linux-Arch <linux-arch@vger.kernel.org>,
selinux@vger.kernel.org, ecryptfs@vger.kernel.org,
linux-unionfs@vger.kernel.org, linux-xfs@vger.kernel.org,
"Andrey Albershteyn" <aalbersh@kernel.org>
Subject: Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls
Date: Wed, 21 May 2025 10:48:26 +0200 [thread overview]
Message-ID: <sfmrojifgnrpeilqxtixyqrdjj5uvvpbvirxmlju5yce7z72vi@ondnx7qbie4y> (raw)
In-Reply-To: <aCsX4LTpAnGfFjHg@dread.disaster.area>
On 2025-05-19 21:37:04, Dave Chinner wrote:
> On Thu, May 15, 2025 at 12:33:31PM +0200, Amir Goldstein wrote:
> > On Thu, May 15, 2025 at 11:02 AM Christian Brauner <brauner@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
> david@fromorbit.com
>
So, looks like there's at least two solutions to this concern.
Considering also that we have a bit of space in fsxattr,
'fsx_pad[8]', I think it's fine to stick with the current fsxattr
for now.
--
- Andrey
next prev parent reply other threads:[~2025-05-21 8:48 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-13 9:17 [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls Andrey Albershteyn
2025-05-13 9:17 ` [PATCH v5 1/7] fs: split fileattr related helpers into separate file Andrey Albershteyn
2025-05-13 9:17 ` [PATCH v5 2/7] lsm: introduce new hooks for setting/getting inode fsxattr Andrey Albershteyn
2025-05-22 22:26 ` Paul Moore
2025-05-13 9:17 ` [PATCH v5 3/7] selinux: implement inode_file_[g|s]etattr hooks Andrey Albershteyn
2025-05-22 22:26 ` Paul Moore
2025-05-13 9:17 ` [PATCH v5 4/7] fs: split fileattr/fsxattr converters into helpers Andrey Albershteyn
2025-05-13 9:17 ` [PATCH v5 5/7] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP Andrey Albershteyn
2025-05-13 9:17 ` [PATCH v5 6/7] fs: prepare for extending file_get/setattr() Andrey Albershteyn
2025-05-13 9:18 ` [PATCH v5 7/7] fs: introduce file_getattr and file_setattr syscalls Andrey Albershteyn
2025-05-13 9:53 ` [PATCH v5 0/7] " Arnd Bergmann
2025-05-13 12:53 ` Amir Goldstein
2025-05-14 15:10 ` H. Peter Anvin
2025-05-15 9:02 ` Christian Brauner
2025-05-15 10:33 ` Amir Goldstein
2025-05-19 10:12 ` Christian Brauner
2025-05-19 11:37 ` Dave Chinner
2025-05-21 8:48 ` Andrey Albershteyn [this message]
2025-05-21 8:57 ` Pali Rohár
2025-05-21 9:02 ` Arnd Bergmann
2025-05-21 9:36 ` Amir Goldstein
2025-05-21 10:06 ` Andrey Albershteyn
2025-05-21 10:44 ` Amir Goldstein
-- strict thread matches above, loose matches on Subject: below --
2025-05-12 13:25 Andrey Albershteyn
2025-05-12 13:27 ` Andrey Albershteyn
2025-05-12 13:18 Andrey Albershteyn
2025-05-12 13:27 ` Andrey Albershteyn
2025-05-13 8:24 ` Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sfmrojifgnrpeilqxtixyqrdjj5uvvpbvirxmlju5yce7z72vi@ondnx7qbie4y \
--to=aalbersh@redhat.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=aalbersh@kernel.org \
--cc=agordeev@linux.ibm.com \
--cc=amir73il@gmail.com \
--cc=andreas@gaisler.com \
--cc=arnd@arndb.de \
--cc=borntraeger@linux.ibm.com \
--cc=bp@alien8.de \
--cc=brauner@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=chris@zankel.net \
--cc=christophe.leroy@csgroup.eu \
--cc=code@tyhicks.com \
--cc=dalias@libc.org \
--cc=dave.hansen@linux.intel.com \
--cc=davem@davemloft.net \
--cc=david@fromorbit.com \
--cc=deller@gmx.de \
--cc=ecryptfs@vger.kernel.org \
--cc=geert@linux-m68k.org \
--cc=glaubitz@physik.fu-berlin.de \
--cc=gnoack@google.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=jack@suse.cz \
--cc=jcmvbkbc@gmail.com \
--cc=jmorris@namei.org \
--cc=linux-alpha@vger.kernel.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=luto@kernel.org \
--cc=maddy@linux.ibm.com \
--cc=mattst88@gmail.com \
--cc=mic@digikod.net \
--cc=miklos@szeredi.hu \
--cc=mingo@redhat.com \
--cc=monstr@monstr.eu \
--cc=mpe@ellerman.id.au \
--cc=naveen@kernel.org \
--cc=npiggin@gmail.com \
--cc=omosnace@redhat.com \
--cc=pali@kernel.org \
--cc=paul@paul-moore.com \
--cc=richard.henderson@linaro.org \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=sparclinux@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
--cc=svens@linux.ibm.com \
--cc=tglx@linutronix.de \
--cc=tsbogend@alpha.franken.de \
--cc=viro@zeniv.linux.org.uk \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).