Message-ID: <3183d8b21e78dce2c1d5cbc8a1304f2937110621.camel@kernel.org>
Date:   Mon, 18 Sep 2023 10:29:46 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Christian Brauner <brauner@...nel.org>,
        Miklos Szeredi <miklos@...redi.hu>
Cc:     Miklos Szeredi <mszeredi@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, linux-man@...r.kernel.org,
        linux-security-module@...r.kernel.org, Karel Zak <kzak@...hat.com>,
        Ian Kent <raven@...maw.net>,
        David Howells <dhowells@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <christian@...uner.io>,
        Amir Goldstein <amir73il@...il.com>
Subject: Re: [RFC PATCH 2/3] add statmnt(2) syscall

On Thu, 2023-09-14 at 17:26 +0200, Christian Brauner wrote:
> On Thu, Sep 14, 2023 at 12:13:54PM +0200, Miklos Szeredi wrote:
> > On Thu, 14 Sept 2023 at 11:28, Christian Brauner <brauner@...nel.org> wrote:
> > > 
> > > On Wed, Sep 13, 2023 at 05:22:35PM +0200, Miklos Szeredi wrote:
> > > > Add a way to query attributes of a single mount instead of having to parse
> > > > the complete /proc/$PID/mountinfo, which might be huge.
> > > > 
> > > > Lookup the mount by the old (32bit) or new (64bit) mount ID.  If a mount
> > > > needs to be queried based on path, then statx(2) can be used to first query
> > > > the mount ID belonging to the path.
> > > > 
> > > > Design is based on a suggestion by Linus:
> > > > 
> > > >   "So I'd suggest something that is very much like "statfsat()", which gets
> > > >    a buffer and a length, and returns an extended "struct statfs" *AND*
> > > >    just a string description at the end."
> > > 
> > > So what we agreed to at LSFMM was that we split filesystem option
> > > retrieval into a separate system call and just have a very focused
> > > statx() for mounts with just binary and non-variable sized information.
> > > We even gave David a hard time about this. :) I would really love if we
> > > could stick to that.
> > > 
> > > Linus, I realize this was your suggestion a long time ago but I would
> > > really like us to avoid structs with variable sized fields at the end of
> > > a struct. That's just so painful for userspace and universally disliked.
> > > If you care I can even find the LSFMM video where we have users of that
> > > api requesting that we please don't do this. So it'd be great if you
> > > wouldn't insist on it.
> > 
> > I completely missed that.
> 
> No worries, I think the discussion touching on this starts at:
> https://youtu.be/j3fp2MtRr2I?si=f-YBg6uWq80dV3VC&t=1603
> (with David talking quietly without a microphone for some parts
> unfortunately...)
> 
> > What I'm thinking is making it even simpler for userspace:
> > 
> > struct statmnt {
> >   ...
> >   char *mnt_root;
> >   char *mountpoint;
> >   char *fs_type;
> >   u32 num_opts;
> >   char *opts;
> > };
> > 
> > I'd still just keep options nul delimited.
> > 
> > Is there a good reason not to return pointers (pointing to within the
> > supplied buffer obviously) to userspace?
> 
> It's really unpleasant to program with. Yes, I think you pointed out
> before that it often doesn't matter much as long as the system call is
> really only relevant to some special purpose userspace.
> 
> But statmount() will be used pretty extensively pretty quickly for the
> purpose of finding out mount options on a mount (Querying a whole
> sequences of mounts via repeated listmount() + statmount() calls on the
> other hand will be rarer.).
> 
> And there's just so many tools that need this: libmount, systemd, all
> kinds of container runtimes, path lookup libraries such as libpathrs,
> languages like go and rust that expose and wrap these calls and so on.
> 
> Most of these tools don't need to know about filesystem mount options
> and if they do they can just query that through an extra system call. No
> harm in doing that.
> 
> The agreement we came to to split out listing submounts into a separate
> system call was exactly to avoid having to have a variable sized pointer
> at the end of the struct statmnt (That's also part of the video above
> btw.) and to make it as simple as possible.
> 
> Plus, the format for how to return arbitrary filesystem mount options
> warrants a separate discussion imho as that's not really vfs level
> information.
> 
> > > This will also allow us to turn statmnt() into an extensible argument
> > > system call versioned by size just like we do any new system calls with
> > > struct arguments (e.g., mount_setattr(), clone3(), openat2() and so on).
> > > Which is how we should do things like that.
> > 
> > The mask mechanism also allow versioning of the struct.
> 
> Yes, but this is done with reserved space which just pushes away the
> problem and bloats the struct for the sake of an unknown future. If we
> were to use an extensible argument struct we would just version by size.
> The only requirement is that you extend by 64 bit (see struct
> clone_args) which had been extended.
> 
> 

Fixed size structs are much nicer to deal with, and most of the fields
we're talking about don't change ofetn enough to make trying to strive
for perfect atomicity worthwhile.

What sort of interface are you thinking for fetching variable-length
string info? It sounds a lot like getxattr that uses a mnt_id in place
of a pathname. getmntattr() ?


> > > 
> > > Other than that I really think this is on track for what we ultimately
> > > want.
> > > 
> > > > +struct stmt_str {
> > > > +     __u32 off;
> > > > +     __u32 len;
> > > > +};
> > > > +
> > > > +struct statmnt {
> > > > +     __u64 mask;             /* What results were written [uncond] */
> > > > +     __u32 sb_dev_major;     /* Device ID */
> > > > +     __u32 sb_dev_minor;
> > > > +     __u64 sb_magic;         /* ..._SUPER_MAGIC */
> > > > +     __u32 sb_flags;         /* MS_{RDONLY,SYNCHRONOUS,DIRSYNC,LAZYTIME} */
> > > > +     __u32 __spare1;
> > > > +     __u64 mnt_id;           /* Unique ID of mount */
> > > > +     __u64 mnt_parent_id;    /* Unique ID of parent (for root == mnt_id) */
> > > > +     __u32 mnt_id_old;       /* Reused IDs used in proc/.../mountinfo */
> > > > +     __u32 mnt_parent_id_old;
> > > > +     __u64 mnt_attr;         /* MOUNT_ATTR_... */
> > > > +     __u64 mnt_propagation;  /* MS_{SHARED,SLAVE,PRIVATE,UNBINDABLE} */
> > > > +     __u64 mnt_peer_group;   /* ID of shared peer group */
> > > > +     __u64 mnt_master;       /* Mount receives propagation from this ID */
> > > > +     __u64 propagate_from;   /* Propagation from in current namespace */
> > > > +     __u64 __spare[20];
> > > > +     struct stmt_str mnt_root;       /* Root of mount relative to root of fs */
> > > > +     struct stmt_str mountpoint;     /* Mountpoint relative to root of process */
> > > > +     struct stmt_str fs_type;        /* Filesystem type[.subtype] */
> > > 

A bit tangential to this discussion, but one thing we could consider is
adding something like a mnt_change_cookie field that increments on any
significant changes on the mount: i.e. remounts with new options,
changes to parentage or propagation, etc.

That might make it more palatable to do something with separate syscalls
for the string-based fields. You could do:

statmnt(...);
getmntattr(mnt, "mnt.fstype", ...);
statmnt(...);

...and then if the mnt_change_cookie hasn't changed, you know that the
string option was stable during that window.


> > > I think if we want to do this here we should add:
> > > 
> > > __u64 fs_type
> > > __u64 fs_subtype
> > > 
> > > fs_type can just be our filesystem magic number and we introduce magic
> > 
> > It's already there: sb_magic.
> > 
> > However it's not a 1:1 mapping (ext* only has one magic).
> 
> That's a very odd choice but probably fixable by giving it a subtype.
> 
> > 
> > > numbers for sub types as well. So we don't need to use strings here.
> > 
> > Ugh.
> 
> Hm, idk. It's not that bad imho. We'll have to make some ugly tradeoffs.

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists