| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOQ4uxiOaYusURMrjozD_s24Swih2g0x5_nTTYXO=+=EPCYtjw@mail.gmail.com>
Date: Wed, 21 May 2025 12:44:30 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Andrey Albershteyn <aalbersh@...hat.com>
Cc: pali@...nel.org, Dave Chinner <david@...morbit.com>,
Christian Brauner <brauner@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Richard Henderson <richard.henderson@...aro.org>, Matt Turner <mattst88@...il.com>,
Russell King <linux@...linux.org.uk>, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>, Michal Simek <monstr@...str.eu>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
"James E . J . Bottomley" <James.Bottomley@...senpartnership.com>, Helge Deller <deller@....de>,
Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>,
Naveen N Rao <naveen@...nel.org>, Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>, Christian Borntraeger <borntraeger@...ux.ibm.com>,
Sven Schnelle <svens@...ux.ibm.com>, Rich Felker <dalias@...c.org>,
John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>, "David S . Miller" <davem@...emloft.net>,
Andreas Larsson <andreas@...sler.com>, Andy Lutomirski <luto@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, Chris Zankel <chris@...kel.net>, Max Filippov <jcmvbkbc@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
Mickaël Salaün <mic@...ikod.net>,
Günther Noack <gnoack@...gle.com>,
Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>, Stephen Smalley <stephen.smalley.work@...il.com>,
Ondrej Mosnacek <omosnace@...hat.com>, Tyler Hicks <code@...icks.com>,
Miklos Szeredi <miklos@...redi.hu>, linux-alpha@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org,
linux-parisc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
sparclinux@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-api@...r.kernel.org,
Linux-Arch <linux-arch@...r.kernel.org>, selinux@...r.kernel.org,
ecryptfs@...r.kernel.org, linux-unionfs@...r.kernel.org,
linux-xfs@...r.kernel.org, Andrey Albershteyn <aalbersh@...nel.org>
Subject: Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls
On Wed, May 21, 2025 at 12:06 PM Andrey Albershteyn <aalbersh@...hat.com> wrote:
>
> On 2025-05-21 11:36:31, Amir Goldstein wrote:
> > On Wed, May 21, 2025 at 10:48 AM Andrey Albershteyn <aalbersh@...hat.com> wrote:
> > >
> > > 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@...nel.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@...morbit.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.
> >
> > Not sure which two solutions you are referring to.
>
> Suggested by Christian and Dave
>
IIUC, those are suggestions of how we could cope with changing
struct fsxattr in the future, but it is easier not to have to do that.
> >
> > I proposed fsx_fileattr as what I think is the path of least resistance.
> > There are opinions that we may be able to avoid defining
> > this struct, but I don't think there was any objection to adding it.
> >
> > So unless I am missing an objection that I did not understand
> > define it and get over this hurdle?
>
> I see, sure, I misinterpreted the communication :) no problems, I
> will create 'struct fsx_fileattr' then.
>
> Pali, ah sorry, I forgot that you will extend fsxattr right away
>
Much less problems could be caused if fsxattr remain frozen in
time along with the ioctls as we continue to extend the syscalls.
Thanks,
Amir.
P.S. your CC list is a bit much.
I wouldn't trust get_maintainer.pl output when it provides such a huge list
it has some emails that bounce - not nice.
When you are at v5 you should be able to have figured out who is
participating in the review and for the rest, the public lists
linux-fsdevel, linux-api and linux-xfs should be enough.
Powered by blists - more mailing lists