Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1635100 > unrolled thread

[RFC][PATCH 0/9] VFS: Introduce mount context

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/9] VFS: Introduce mount context David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
    [PATCH 1/9] Provide a function to create a NUL-terminated string  from unterminated data David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
      Re: [PATCH 1/9] Provide a function to create a NUL-terminated  string from unterminated data Jeff Layton <jlayton@poochiereds.net> - 2017-05-03 19:00 +0200
      Re: [PATCH 1/9] Provide a function to create a NUL-terminated string from unterminated data Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2017-05-03 21:30 +0200
        Re: [PATCH 1/9] Provide a function to create a NUL-terminated string from unterminated data David Howells <dhowells@redhat.com> - 2017-05-03 22:20 +0200
    [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
      Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Jeff Layton <jlayton@poochiereds.net> - 2017-05-03 20:40 +0200
        Re: [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-03 20:50 +0200
      Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2017-05-03 22:50 +0200
        Re: [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-04 15:00 +0200
        Re: [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-04 15:00 +0200
      Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Karel Zak <kzak@redhat.com> - 2017-05-04 12:50 +0200
        Re: [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-04 15:10 +0200
          Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Karel Zak <kzak@redhat.com> - 2017-05-04 15:40 +0200
            Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Jeff Layton <jlayton@redhat.com> - 2017-05-09 20:50 +0200
      Re: [PATCH 4/9] Implement fsopen() to prepare for a mount Miklos Szeredi <mszeredi@redhat.com> - 2017-05-08 17:20 +0200
        Re: [PATCH 4/9] Implement fsopen() to prepare for a mount David Howells <dhowells@redhat.com> - 2017-05-09 01:20 +0200
    [PATCH 5/9] Implement fsmount() to effect a pre-configured mount David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
    [PATCH 2/9] Clean up whitespace in fs/namespace.c David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
    [PATCH 8/9] proc: Support the mount context in procfs David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
    [PATCH 7/9] procfs: Move proc_fill_super() to fs/proc/root.c David Howells <dhowells@redhat.com> - 2017-05-03 18:10 +0200
    Re: [RFC][PATCH 0/9] VFS: Introduce mount context Jeff Layton <jlayton@poochiereds.net> - 2017-05-03 18:50 +0200
      Re: [RFC][PATCH 0/9] VFS: Introduce mount context David Howells <dhowells@redhat.com> - 2017-05-03 19:00 +0200
        Re: [RFC][PATCH 0/9] VFS: Introduce mount context Jeff Layton <jlayton@poochiereds.net> - 2017-05-03 19:30 +0200
    Re: [PATCH 3/9] VFS: Introduce a mount context Joe Perches <joe@perches.com> - 2017-05-03 20:30 +0200
      Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-03 20:40 +0200
        Re: [PATCH 3/9] VFS: Introduce a mount context Joe Perches <joe@perches.com> - 2017-05-03 20:50 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-03 22:20 +0200
      Re: [PATCH 3/9] VFS: Introduce a mount context Matthew Wilcox <willy@infradead.org> - 2017-05-03 22:40 +0200
        Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-03 23:20 +0200
        Re: [PATCH 3/9] VFS: Introduce a mount context Joe Perches <joe@perches.com> - 2017-05-03 23:40 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context Julia Lawall <julia.lawall@lip6.fr> - 2017-05-04 08:30 +0200
      Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-04 11:30 +0200
        Re: [PATCH 3/9] VFS: Introduce a mount context Joe Perches <joe@perches.com> - 2017-05-04 16:40 +0200
    Re: [PATCH 3/9] VFS: Introduce a mount context Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2017-05-03 23:50 +0200
      Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-04 12:30 +0200
    Re: [RFC][PATCH 0/9] VFS: Introduce mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-05 16:40 +0200
      Re: [RFC][PATCH 0/9] VFS: Introduce mount context David Howells <dhowells@redhat.com> - 2017-05-05 17:50 +0200
        Re: [RFC][PATCH 0/9] VFS: Introduce mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-08 10:30 +0200
        Re: [RFC][PATCH 0/9] VFS: Introduce mount context David Howells <dhowells@redhat.com> - 2017-05-08 10:40 +0200
          Re: [RFC][PATCH 0/9] VFS: Introduce mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-08 10:50 +0200
    Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-08 17:10 +0200
      Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-09 01:00 +0200
        Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-09 10:10 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-09 11:40 +0200
            Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-09 13:10 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-09 11:50 +0200
            Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-09 14:10 +0200
              Re: [PATCH 3/9] VFS: Introduce a mount context Jeff Layton <jlayton@redhat.com> - 2017-05-09 21:00 +0200
                Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-10 09:30 +0200
                  Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-10 10:10 +0200
                    Re: [PATCH 3/9] VFS: Introduce a mount context Jeff Layton <jlayton@redhat.com> - 2017-05-10 15:30 +0200
                      Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-10 15:40 +0200
                        Re: [PATCH 3/9] VFS: Introduce a mount context Jeff Layton <jlayton@redhat.com> - 2017-05-10 15:40 +0200
                      Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-10 15:40 +0200
                      Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-10 15:40 +0200
                        Re: [PATCH 3/9] VFS: Introduce a mount context Jeff Layton <jlayton@redhat.com> - 2017-05-10 15:50 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context David Howells <dhowells@redhat.com> - 2017-05-09 12:00 +0200
            Re: [PATCH 3/9] VFS: Introduce a mount context Miklos Szeredi <mszeredi@redhat.com> - 2017-05-09 14:40 +0200
          Re: [PATCH 3/9] VFS: Introduce a mount context Karel Zak <kzak@redhat.com> - 2017-05-10 14:50 +0200
    Re: [RFC][PATCH 0/9] VFS: Introduce mount context Djalal Harouni <tixxdz@gmail.com> - 2017-05-08 19:10 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1637309

On Mon, May 8, 2017 at 10:35 AM, David Howells <dhowells@redhat.com> wrote:

>> Further, once you've created a superblock, what are you going to do with it
>> other than mount it?  I suppose you could statfs it and we could add other
>> superblock manipulation functions, but this is normally done by opening the
>> device directly (at least for bdev-based superblocks).
>
> How about sb_context, sb_config, sb_parameters or something like that?

I'd vote for sb_config.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1637493 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, May 3, 2017 at 6:04 PM, David Howells <dhowells@redhat.com> wrote:
> Introduce a mount context concept.  This is allocated at the beginning of
> the mount procedure and into it is placed:
>
>  (1) Filesystem type.
>
>  (2) Namespaces.
>
>  (3) Device name.
>
>  (4) Superblock flags (MS_*) and mount flags (MNT_*).
>
>  (5) Security details.
>
>  (6) Filesystem-specific data, as set by the mount options.
>
> It also gives a place in which to hang an error message for later retrieval
> (see the mount-by-fd syscall later in this series).
>
> Rather than calling fs_type->mount(), a mount_context struct is created and
> fs_type->fsopen() is called to set it up.  fs_type->mc_size says how much
> should be added on to the mount context for the filesystem's use.
>
> A set of operations have to be set by ->fsopen() to provide freeing,
> duplication, option parsing, binary data parsing, validation, mounting and
> superblock filling.
>
> It should be noted that, whilst this patch adds a lot of lines of code,
> there is quite a bit of duplication with existing code that can be
> eliminated should all filesystems be converted over.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  Documentation/filesystems/mounting.txt |  445 ++++++++++++++++++++++++++++++++
>  fs/Makefile                            |    3
>  fs/internal.h                          |    2
>  fs/mount.h                             |    3
>  fs/mount_context.c                     |  343 +++++++++++++++++++++++++
>  fs/namespace.c                         |  270 +++++++++++++++++--
>  fs/super.c                             |   50 +++-
>  include/linux/fs.h                     |   11 +
>  include/linux/lsm_hooks.h              |   37 +++
>  include/linux/mount.h                  |   67 +++++
>  include/linux/security.h               |   29 ++
>  security/security.c                    |   32 ++
>  security/selinux/hooks.c               |  179 +++++++++++++
>  13 files changed, 1435 insertions(+), 36 deletions(-)
>  create mode 100644 Documentation/filesystems/mounting.txt
>  create mode 100644 fs/mount_context.c
>
> diff --git a/Documentation/filesystems/mounting.txt b/Documentation/filesystems/mounting.txt
> new file mode 100644
> index 000000000000..a942ccd08376
> --- /dev/null
> +++ b/Documentation/filesystems/mounting.txt
> @@ -0,0 +1,445 @@
> +                             ===================
> +                             FILESYSTEM MOUNTING
> +                             ===================
> +
> +CONTENTS
> +
> + (1) Overview.
> +
> + (2) The mount context.
> +
> + (3) The mount context operations.
> +
> + (4) Mount context security.
> +
> + (5) VFS mount context operations.
> +
> +
> +========
> +OVERVIEW
> +========
> +
> +The creation of new mounts is now to be done in a multistep process:
> +
> + (1) Create a mount context.
> +
> + (2) Parse the options and attach them to the mount context.  Options may be
> +     passed individually from userspace.
> +
> + (3) Validate and pre-process the mount context.

(3.5) Create super block

I think this need to be triggered by something like a "commit" command
from userspace.  Basically this is where the options are atomically
set on the new (create) or existing (reconfigure) superblock.

> +
> + (4) Perform the mount.
> +
> + (5) Return an error message attached to the mount context.

Swap the order of the above.  There's no fs specific actions performed
at fsmount() time, and normal errno reporting should be perfectly
fine.

> +
> + (6) Destroy the mount context.
> +
> +To support this, the file_system_type struct gains two new fields:
> +
> +       unsigned short mc_size;
> +
> +which indicates how much space the filesystem would like tacked onto the end of
> +the mount_context struct for its own purposes, and:
> +
> +       int (*fsopen)(struct mount_context *mc, struct super_block *src_sb);
> +
> +which is invoked to set up the filesystem-specific parts of a mount context,
> +including the additional space.  The src_sb parameter is used to convey the
> +superblock from which the filesystem may draw extra information (such as
> +namespaces), for submount (MS_SUBMOUNT) or remount (MS_REMOUNT) purposes or it
> +will be NULL.

I think reconfigure (don't call it remount, there's no "mounting"
going on there) should start out with a context populated with with
the current state of the superblock.  User can then reset and start
over or individually add/remove options.   This should be a good place
to allow querying the options as well, as Karel suggested.  Then when
the configuration is finished the changes are committed to the
superblock.

> +
> +Note that security initialisation is done *after* the filesystem is called so
> +that the namespaces may be adjusted first.
> +
> +And the super_operations struct gains one:
> +
> +       int (*remount_fs_mc) (struct super_block *, struct mount_context *);
> +
> +This shadows the ->remount_fs() operation and takes a prepared mount context
> +instead of the mount flags and data page.  It may modify the ms_flags in the
> +context for the caller to pick up.
> +
> +[NOTE] remount_fs_mc is intended as a replacement for remount_fs.
> +
> +
> +=================
> +THE MOUNT CONTEXT
> +=================
> +
> +The mount process is governed by a mount context.  This is represented by the
> +mount_context structure:
> +
> +       struct mount_context {
> +               const struct mount_context_operations *ops;
> +               struct file_system_type *fs;
> +               struct user_namespace   *user_ns;
> +               struct mnt_namespace    *mnt_ns;
> +               struct pid_namespace    *pid_ns;
> +               struct net              *net_ns;
> +               const struct cred       *cred;
> +               char                    *device;
> +               char                    *root_path;
> +               void                    *security;
> +               const char              *error;
> +               unsigned int            ms_flags;
> +               unsigned int            mnt_flags;
> +               bool                    mounted;
> +               bool                    sloppy;
> +               bool                    silent;
> +               enum mount_type         mount_type : 8;
> +       };
> +
> +When allocated, the mount_context struct is extended by ->mc_size bytes as
> +specified by the specified file_system_type struct.  This is for use by the
> +filesystem.  The filesystem should wrap the struct in its own, e.g.:
> +
> +       struct nfs_mount_context {
> +               struct mount_context mc;
> +               ...
> +       };
> +
> +placing the mount_context struct first.  container_of() can then be used.
> +
> +The mount_context fields are as follows:
> +
> + (*) const struct mount_context_operations *ops
> +
> +     These are operations that can be done on a mount context.  See below.
> +     This must be set by the ->fsopen() file_system_type operation.
> +
> + (*) struct file_system_type *fs
> +
> +     A pointer to the file_system_type of the filesystem that is being
> +     mounted.  This retains a ref on the type owner.
> +
> + (*) struct user_namespace *user_ns
> + (*) struct mnt_namespace *mnt_ns
> + (*) struct pid_namespace *pid_ns
> + (*) struct net *net_ns
> +
> +     This is a subset of the namespaces in use by the invoking process.  This
> +     retains a ref on each namespace.  The subscribed namespaces may be
> +     replaced by the filesystem to reflect other sources, such as the parent
> +     mount superblock on an automount.
> +
> + (*) struct cred *cred
> +
> +     The mounter's credentials.  This retains a ref on the credentials.
> +
> + (*) char *device
> +
> +     This is the device to be mounted.  It may be a block device
> +     (e.g. /dev/sda1) or something more exotic, such as the "host:/path" that
> +     NFS desires.
> +
> + (*) char *root_path
> +
> +     A path to the place inside the filesystem to actually mount.  This allows
> +     a mount and bind-mount to be combined.
> +
> +     [NOTE] This isn't implemented yet, but NFS has the code to do this which
> +     could be moved to the VFS.
> +
> + (*) void *security
> +
> +     A place for the LSMs to hang their security data for the mount.  The
> +     relevant security operations are described below.
> +
> + (*) const char *error
> +
> +     A place for the VFS and the filesystem to hang an error message.  This
> +     should be in the form of a static string that doesn't need deallocation
> +     and the pointer to which can just be overwritten.  Under some
> +     circumstances, this can be retrieved by userspace.
> +
> +     Note that the existence of the error string is expected to be guaranteed
> +     by the reference on the file_system_type object held by ->fs or any
> +     filesystem-specific reference held in the filesystem context until the
> +     ->free() operation is called.
> +
> + (*) unsigned int ms_flags
> + (*) unsigned int mnt_flags
> +
> +     These hold the mount flags.  ms_flags holds MS_* flags and mnt_flags holds
> +     MNT_* flags.
> +
> + (*) bool mounted
> +
> +     This is set to true once a mount attempt is made.  This causes an error to
> +     be given on subsequent mount attempts with the same context and prevents
> +     multiple mount attempts.

No point.  A context is mountable if the superblock is non-NULL.
Don't even need to have the context committed, if not, it would simply
mount the sb in the previous state.

I'd hope some simplifications would fall out from this model.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1637781 — Re: [PATCH 3/9] VFS: Introduce a mount context

Miklos Szeredi <mszeredi@redhat.com> wrote:

> > + (3) Validate and pre-process the mount context.
> 
> (3.5) Create super block
> 
> I think this need to be triggered by something like a "commit" command
> from userspace.  Basically this is where the options are atomically
> set on the new (create) or existing (reconfigure) superblock.

Why do you need to expose this step to userspace?  Assuming in the "new" case
you do, say:

	fd = fsopen("nfs");
	write(fd, "s foo.bar:/bar", ...);
	write(fd, "o intr", ...);
	write(fd, "o fsc", ...);
	...
	write(fd, "c", ...); /* commit operation to get a superblock */
	fsmount(fd, AT_FDCWD, "/mnt");  /* mount the superblock we just got */

Then the "commit" op is dissimilar to "mount -o remount" since remount may
alter the superblock parameters *and* the mountpoint parameters, but commit
can only affect the superblock.

On the other hand, I could see that you might want to do:

	fd = fsopen("nfs");
	...
	write(fd, "c", ...); /* commit operation to get a superblock */
	fstatfs(fd, &buf); /* get info about the superblock */
	fsmount(fd, AT_FDCWD, "/mnt");  /* mount the superblock we just got */

> > + (4) Perform the mount.
> > +
> > + (5) Return an error message attached to the mount context.
> 
> Swap the order of the above.  There's no fs specific actions performed
> at fsmount() time, and normal errno reporting should be perfectly
> fine.

There's no reason not to allow error messages to be attached by the actual
vfsmount creation and insertion - and reasons that one might want to do so.
Think LSMs, for instance.  We don't look up the mountpoint until this point,
and so we can't do the security checks on them till this point.  It could make
it easier to debug problems if we can return a more comprehensive message at
this point.

> I think reconfigure (don't call it remount, there's no "mounting"
> going on there)

There's adjustment of the vfsmount structure too; besides, it is called
MS_REMOUNT in the UAPI and "mount -o remount", so we're somewhat stuck with
the label whether we like it or not.

> should start out with a context populated with with the current state of the
> superblock.

Hence why ->fsopen() takes a super_block parameter.

> User can then reset and start over

No, not really.  You cannot reset all options - the source for example,
probably has to remain the same.  IP addresses on NFS mounts possibly should
remain the same - though I can see situations where it might be convenient to
change these.

> or individually add/remove options.

This is very per-filesystem-type dependent.

> This should be a good place to allow querying the options as well, as Karel
> suggested.

I'm not sure it's worth the code unless we allow opening extant mounts and
querying using this mechanism.

> Then when the configuration is finished the changes are committed to the
> superblock.

You're going a lot beyond remount here.  Remount can, in one go, change some
options which are superblock-only, some options which are mountpoint-only and
at least one which crosses both domains.

> > + (*) bool mounted
> > +
> > +     This is set to true once a mount attempt is made.  This causes an error to
> > +     be given on subsequent mount attempts with the same context and prevents
> > +     multiple mount attempts.
> 
> No point.  A context is mountable if the superblock is non-NULL.
> Don't even need to have the context committed,

Ummm...  Doesn't that render "commit" unnecessary?

> if not, it would simply mount the sb in the previous state.

You want to be able to open a filesystem fd, create or reference a superblock
and then mount it several times?

> I'd hope some simplifications would fall out from this model.

Not really.  It makes things slightly less simple, particularly with the
"commit" operation that you want.  I'm not sure that sys_mount() and
sys_fsmount() will be able to share as much code.

It also makes the remount process less similar to the mount process because
the "commit" operation doesn't seem useful in the former because remount also
alters the vfsmount.

David

[toc] | [prev] | [next] | [standalone]

#1637961 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 9, 2017 at 12:57 AM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> > + (3) Validate and pre-process the mount context.
>>
>> (3.5) Create super block
>>
>> I think this need to be triggered by something like a "commit" command
>> from userspace.  Basically this is where the options are atomically
>> set on the new (create) or existing (reconfigure) superblock.
>
> Why do you need to expose this step to userspace?  Assuming in the "new" case
> you do, say:
>
>         fd = fsopen("nfs");
>         write(fd, "s foo.bar:/bar", ...);
>         write(fd, "o intr", ...);
>         write(fd, "o fsc", ...);
>         ...
>         write(fd, "c", ...); /* commit operation to get a superblock */
>         fsmount(fd, AT_FDCWD, "/mnt");  /* mount the superblock we just got */
>
> Then the "commit" op is dissimilar to "mount -o remount" since remount may
> alter the superblock parameters *and* the mountpoint parameters, but commit
> can only affect the superblock.

Forget remount, it's a historical remnant.  We need fsreconfig(sb) and
setmntattr(mnt).  They are changing properties of different objects.
Remount is like fcntl(fd, F_SETFL) and fchmod(fd, ...) rolled into
one.   They have nothing in common except the fact that the old
mount(2) API included both in one single operation, and I'm sure that
was a "oh we don't want to introduce a new flag for this, so lets
reuse the old one" sort of design decision.

>
> On the other hand, I could see that you might want to do:
>
>         fd = fsopen("nfs");
>         ...
>         write(fd, "c", ...); /* commit operation to get a superblock */
>         fstatfs(fd, &buf); /* get info about the superblock */
>         fsmount(fd, AT_FDCWD, "/mnt");  /* mount the superblock we just got */
>
>> > + (4) Perform the mount.
>> > +
>> > + (5) Return an error message attached to the mount context.
>>
>> Swap the order of the above.  There's no fs specific actions performed
>> at fsmount() time, and normal errno reporting should be perfectly
>> fine.
>
> There's no reason not to allow error messages to be attached by the actual
> vfsmount creation and insertion - and reasons that one might want to do so.
> Think LSMs, for instance.  We don't look up the mountpoint until this point,
> and so we can't do the security checks on them till this point.  It could make
> it easier to debug problems if we can return a more comprehensive message at
> this point.

I think that's crazy.  We don't return detailed errors for any other
syscall for path lookup, so why would path lookup for mount be
special.

And why would

    fd = open("/foo/bar", O_PATH);
    fsmount(fsfd, fd, NULL);

behave differently from

    fsmount(fsfd, -1, "/foo/bar");

?

>
>> I think reconfigure (don't call it remount, there's no "mounting"
>> going on there)
>
> There's adjustment of the vfsmount structure too; besides, it is called
> MS_REMOUNT in the UAPI and "mount -o remount", so we're somewhat stuck with
> the label whether we like it or not.

Oh, uapi compatibility: they can use the old mount(2) API for that and
introduce saner utils for the new stuff.  I'm sure we don't need to be
100% feature compatible with old  mount(2).

What we need is mount(2) to stay 100% compatible with itself while the
kernel internal APIs are reshuffled.


>> should start out with a context populated with with the current state of the
>> superblock.
>
> Hence why ->fsopen() takes a super_block parameter.
>
>> User can then reset and start over
>
> No, not really.  You cannot reset all options - the source for example,
> probably has to remain the same.  IP addresses on NFS mounts possibly should
> remain the same - though I can see situations where it might be convenient to
> change these.

Well, the current remount API is like that: you give a new set of
options (i.e. reset and replace anything that can be changed and leave
the rest).  Obviously "reset options" wouldn't allow you to change
options that cannot be changed.

>
>> or individually add/remove options.
>
> This is very per-filesystem-type dependent.

So say we have commands like

"o+ foo"
"o- bar"

The generic option parser would just add or remove the option in the
current set of options, and commit would just call ->remount_fs() with
the new set of options.  It would probably not work for the NFS case,
but that's okay, NFS can implement its own option parsing.

>> This should be a good place to allow querying the options as well, as Karel
>> suggested.
>
> I'm not sure it's worth the code unless we allow opening extant mounts and
> querying using this mechanism.

I'm saying we should allow opening an existent superblock and allow
query and change of options.

>> Then when the configuration is finished the changes are committed to the
>> superblock.
>
> You're going a lot beyond remount here.  Remount can, in one go, change some
> options which are superblock-only, some options which are mountpoint-only and
> at least one which crosses both domains.

We'll have s_op->remount_fs() for some time yet, but that's a clean,
super-block only operation.  MS_REMOUNT is a flag from hell, leave
that for mount(2) compatibility and forget it for the new API.

>
>> > + (*) bool mounted
>> > +
>> > +     This is set to true once a mount attempt is made.  This causes an error to
>> > +     be given on subsequent mount attempts with the same context and prevents
>> > +     multiple mount attempts.
>>
>> No point.  A context is mountable if the superblock is non-NULL.
>> Don't even need to have the context committed,
>
> Ummm...  Doesn't that render "commit" unnecessary?

No.   "commit" is a superblock operation.  fsmount() is a mount
operation.    fsmount() should not do anything to the superblock and
"commit" should not do anything to any mount.

>> if not, it would simply mount the sb in the previous state.
>
> You want to be able to open a filesystem fd, create or reference a superblock
> and then mount it several times?

Maybe.  I'm looking at this from the point of view of what objects we
have and what operations we want to do on them.  In that view it makes
no sense that fsmount() changes the state of the fsfd, since it's an
operation done on the mount tree and not on the superblock
configuration context.  The fact that you can do any number of mounts
on the fsfd just falls out from this premise.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1638008 — Re: [PATCH 3/9] VFS: Introduce a mount context

Miklos Szeredi <mszeredi@redhat.com> wrote:

> Forget remount, it's a historical remnant.

I don't think it can't be set aside so lightly.  Within the kernel, the option
parsing should share as much code as possible between new superblock config,
old new mount and old remount.

The 'trickiest' function we need to support is MS_RDONLY flipping.  That one
affects both the mount and the superblock.  I think all the rest only affect
one side or the other.

Given that a superblock can be mounted in multiple places, do we need to count
the number of read-only mounts that are holding a particular superblock and
only flip the superblock when they're all read-only?

Or do you advocate replacing "mount -o remount,[ro|rw]" with a pair of
operations - one to flip the mount and the other to flip the superblock?

Further, "emergency remount r/o" needs to be supported - though it might make
sense to add a special op just for that.

David

[toc] | [prev] | [next] | [standalone]

#1638039 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 9, 2017 at 11:32 AM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> Forget remount, it's a historical remnant.
>
> I don't think it can't be set aside so lightly.  Within the kernel, the option
> parsing should share as much code as possible between new superblock config,
> old new mount and old remount.

Lets make things clear:  VFS didn't do any option parsing for
mount(2), it was all in filesystem's fstype->mount() and
s_op->remount_fs() operations.  What the VFS did do is filter out the
junk from MS_xxx options and pass only the relevant ones to the
filesystem creation functions, which was mount_fs() and
do_remount_sb().   Note how those functions are in super.c and don't
have a vfsmount argument.

So I propose introducing a third way of parsing arguments, which a
filesystem may implement via sb_config_ops (or whatever we want to
call it) that allows it to parse options into its internal structures
and have it be passed to superblock creation and superblock
reconfiguration ops (which also need to be new ones, that thake the
parsed options in the sb_config structure instead of as a comma
delimited string).  With the fsopen() API the generic code (possibly
via helpers called from fs code) would need to parse the "MS_xxx" type
options now, and the infrastructure for that is new, since previously
those options were parsed in userland instead of in the kernel.

There would be no duplication as filesystems would either implement
the old option parsing or the new one.

Also we could have various helpers that do most of the dirty work of
option parsing, allowing easy migration of filesystems.   In the end
the old method taking the unparsed options can go away.

And, as you say, the option parsing would be shared between old "new
mount", old "remount" and new sb config.  And it would be shared for
the unmigrated fs case as well as the migrated fs case.

And we are still only taking about sb config, not a word about mount
attributes; they should be irrelevant to any of this API shuffling and
new API additions.

> The 'trickiest' function we need to support is MS_RDONLY flipping.  That one
> affects both the mount and the superblock.  I think all the rest only affect
> one side or the other.
>
> Given that a superblock can be mounted in multiple places, do we need to count
> the number of read-only mounts that are holding a particular superblock and
> only flip the superblock when they're all read-only?

Nothing special going on here.  If sb is ro then adding a rw mount
should either fail or automatically go ro.  I think just erroring out
is the better of the two.


> Or do you advocate replacing "mount -o remount,[ro|rw]" with a pair of
> operations - one to flip the mount and the other to flip the superblock?

Yes, definitely.  That's exactly what users have been asking for
(there's even a bugzilla somewhere I don't remember)

Thanks
Miklos

[toc] | [prev] | [next] | [standalone]

#1638013 — Re: [PATCH 3/9] VFS: Introduce a mount context

Miklos Szeredi <mszeredi@redhat.com> wrote:

> I think that's crazy.  We don't return detailed errors for any other
> syscall for path lookup, so why would path lookup for mount be
> special.

Firstly, we don't return detailed errors for mount() at the moment either.

Secondly, path lookup might entail automounts, so perhaps we should do it for
path lookup too.  Particularly in light of the fact that NFS4 mount uses
pathwalk to get from server:/ to server:/the/dir/I/actually/wanted/ so I'm
currently losing that error:-/

Thirdly, the security operation I'm talking about is separate to path lookup -
though perhaps we should pass LOOKUP_MOUNT as an intent flag into pathwalk so
that the security check can be done there; perhaps combined with another one.

Fourthly, why shouldn't we consider extending the facility to other system
calls in future?  It would involve copying the string to task_struct and
providing a way to retrieve it, but that's not that hard to achieve.

> And why would
> 
>     fd = open("/foo/bar", O_PATH);
>     fsmount(fsfd, fd, NULL);
> 
> behave differently from
> 
>     fsmount(fsfd, -1, "/foo/bar");
> 
> ?

There's argument that the former should return EFAULT.  And that you should
set the path to "" and pass AT_EMPTY_PATH.  I should probably make sure it
does that - and add a flags field.  statx() was fixed to work this way.

Question for you: Should the MNT_* flags be passed to fsmount(), perhaps in
MS_* form?

David

[toc] | [prev] | [next] | [standalone]

#1638067 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 9, 2017 at 11:41 AM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> I think that's crazy.  We don't return detailed errors for any other
>> syscall for path lookup, so why would path lookup for mount be
>> special.
>
> Firstly, we don't return detailed errors for mount() at the moment either.
>
> Secondly, path lookup might entail automounts, so perhaps we should do it for
> path lookup too.  Particularly in light of the fact that NFS4 mount uses
> pathwalk to get from server:/ to server:/the/dir/I/actually/wanted/ so I'm
> currently losing that error:-/
>
> Thirdly, the security operation I'm talking about is separate to path lookup -
> though perhaps we should pass LOOKUP_MOUNT as an intent flag into pathwalk so
> that the security check can be done there; perhaps combined with another one.
>
> Fourthly, why shouldn't we consider extending the facility to other system
> calls in future?  It would involve copying the string to task_struct and
> providing a way to retrieve it, but that's not that hard to achieve.

Maybe we should.   In fact that sounds like a splendid idea.  IMO even
better, than having errors go via the fsfd descriptor.  Pretty cheap
on the kernel side, and completely optional on the userspace side.

>
>> And why would
>>
>>     fd = open("/foo/bar", O_PATH);
>>     fsmount(fsfd, fd, NULL);
>>
>> behave differently from
>>
>>     fsmount(fsfd, -1, "/foo/bar");
>>
>> ?
>
> There's argument that the former should return EFAULT.  And that you should
> set the path to "" and pass AT_EMPTY_PATH.  I should probably make sure it
> does that - and add a flags field.  statx() was fixed to work this way.
>
> Question for you: Should the MNT_* flags be passed to fsmount(), perhaps in
> MS_* form?

MS_* flags are a mess.  I don't think they should be used for any new
functionality.  MNT_* flags are much better, but there are some
internal flags there as well.

I think the struct file model is better, where we have the external
O_* flags and the internal FMODE_* flags.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1638348 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, 2017-05-09 at 14:02 +0200, Miklos Szeredi wrote:
> On Tue, May 9, 2017 at 11:41 AM, David Howells <dhowells@redhat.com> wrote:
> > Miklos Szeredi <mszeredi@redhat.com> wrote:
> > 
> > > I think that's crazy.  We don't return detailed errors for any other
> > > syscall for path lookup, so why would path lookup for mount be
> > > special.
> > 
> > Firstly, we don't return detailed errors for mount() at the moment either.
> > 
> > Secondly, path lookup might entail automounts, so perhaps we should do it for
> > path lookup too.  Particularly in light of the fact that NFS4 mount uses
> > pathwalk to get from server:/ to server:/the/dir/I/actually/wanted/ so I'm
> > currently losing that error:-/
> > 
> > Thirdly, the security operation I'm talking about is separate to path lookup -
> > though perhaps we should pass LOOKUP_MOUNT as an intent flag into pathwalk so
> > that the security check can be done there; perhaps combined with another one.
> > 
> > Fourthly, why shouldn't we consider extending the facility to other system
> > calls in future?  It would involve copying the string to task_struct and
> > providing a way to retrieve it, but that's not that hard to achieve.
> 
> Maybe we should.   In fact that sounds like a splendid idea.  IMO even
> better, than having errors go via the fsfd descriptor.  Pretty cheap
> on the kernel side, and completely optional on the userspace side.
> 

A question here: What should happen if you go to set an error here, and
one is already set? Should it just free the string and replace it with
the new one? IOW, just keep the latest error? Or is it better to keep
the earlier one?

If you want to put this in the task_struct then I think you'll want to
sort that out. You could easily end up in this situation if a lot of
different kernel subsystems started using it to pass back detailed
errors.

> > 
> > > And why would
> > > 
> > >     fd = open("/foo/bar", O_PATH);
> > >     fsmount(fsfd, fd, NULL);
> > > 
> > > behave differently from
> > > 
> > >     fsmount(fsfd, -1, "/foo/bar");
> > > 
> > > ?
> > 
> > There's argument that the former should return EFAULT.  And that you should
> > set the path to "" and pass AT_EMPTY_PATH.  I should probably make sure it
> > does that - and add a flags field.  statx() was fixed to work this way.
> > 
> > Question for you: Should the MNT_* flags be passed to fsmount(), perhaps in
> > MS_* form?
> 
> MS_* flags are a mess.  I don't think they should be used for any new
> functionality.  MNT_* flags are much better, but there are some
> internal flags there as well.
> 
> I think the struct file model is better, where we have the external
> O_* flags and the internal FMODE_* flags.
> 
> Thanks,
> Miklos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1638611 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 9, 2017 at 8:51 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 2017-05-09 at 14:02 +0200, Miklos Szeredi wrote:
>> On Tue, May 9, 2017 at 11:41 AM, David Howells <dhowells@redhat.com> wrote:
>> > Miklos Szeredi <mszeredi@redhat.com> wrote:
>> >
>> > > I think that's crazy.  We don't return detailed errors for any other
>> > > syscall for path lookup, so why would path lookup for mount be
>> > > special.
>> >
>> > Firstly, we don't return detailed errors for mount() at the moment either.
>> >
>> > Secondly, path lookup might entail automounts, so perhaps we should do it for
>> > path lookup too.  Particularly in light of the fact that NFS4 mount uses
>> > pathwalk to get from server:/ to server:/the/dir/I/actually/wanted/ so I'm
>> > currently losing that error:-/
>> >
>> > Thirdly, the security operation I'm talking about is separate to path lookup -
>> > though perhaps we should pass LOOKUP_MOUNT as an intent flag into pathwalk so
>> > that the security check can be done there; perhaps combined with another one.
>> >
>> > Fourthly, why shouldn't we consider extending the facility to other system
>> > calls in future?  It would involve copying the string to task_struct and
>> > providing a way to retrieve it, but that's not that hard to achieve.
>>
>> Maybe we should.   In fact that sounds like a splendid idea.  IMO even
>> better, than having errors go via the fsfd descriptor.  Pretty cheap
>> on the kernel side, and completely optional on the userspace side.
>>
>
> A question here: What should happen if you go to set an error here, and
> one is already set? Should it just free the string and replace it with
> the new one? IOW, just keep the latest error? Or is it better to keep
> the earlier one?
>
> If you want to put this in the task_struct then I think you'll want to
> sort that out. You could easily end up in this situation if a lot of
> different kernel subsystems started using it to pass back detailed
> errors.

Possible rule of thumb: use it only at the place where the error
originates and not where errors are just passed on.  This would result
in at most one report per syscall, normally.

And the static string thing that David implemented is also a very good
idea, IMO.

So it would look something like this (possibly needs better naming:

   error_detail("description of error");

or

   return error_detail(-EINVAL, "description of error");

Compiler could automatically include source file/line information as
well, although it may be enough if the string is uniquely greppable
(we could check uniqueness at compile time).

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1638635 — Re: [PATCH 3/9] VFS: Introduce a mount context

Miklos Szeredi <mszeredi@redhat.com> wrote:

> And the static string thing that David implemented is also a very good
> idea, IMO.

There is an issue with it: it's fine as long as you keep a ref on the module
that generated it or clear all strings as part of module removal (which the
mount context in this patchset does).  With the NFS mount context I did, I
have to keep a ref on the NFS protocol module as well as the NFS filesystem
module.

I'm tempted to make it conditionally copy the string using kvasprintf_const()
- which would also permit format substitution.

David

[toc] | [prev] | [next] | [standalone]

#1638821 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, 2017-05-10 at 09:05 +0100, David Howells wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> > Possible rule of thumb: use it only at the place where the error
> > originates and not where errors are just passed on.  This would result
> > in at most one report per syscall, normally.
> >

That might be hard to enforce in practice once you get into some
complicated layering. What if we have device_mapper setting this along
with filesystems too? We need clear rules here.

> > And the static string thing that David implemented is also a very good
> > idea, IMO.
> 
> There is an issue with it: it's fine as long as you keep a ref on the module
> that generated it or clear all strings as part of module removal (which the
> mount context in this patchset does).  With the NFS mount context I did, I
> have to keep a ref on the NFS protocol module as well as the NFS filesystem
> module.
> 
> I'm tempted to make it conditionally copy the string using kvasprintf_const()
> - which would also permit format substitution.
> 

On balance, I think this is a reasonable way to pass back detailed
errors. Up until now, we've mostly relied on just printk'ing them. Now
though, a lot of larger machines are running containerized setups. Good
luck scraping dmesg for _your_ error in that situation. There may be
tons of mounts failing all over the place.

That said, I have some concerns here:

What's the lifetime of these strings? Do they just hang around forever
until the process goes away or they're replaced? If this becomes common,
then you could easily end up with an extra string allocation per task in
some cases. That could add up.

One idea might be to always kfree it on syscall entry, and that might
mitigate the problem assuming that not everything is erroring out. Then
you could always do some trivial syscall to clear it manually.

There's also the problem of how these should be formatted. Is English ok
everywhere? Do we need a facility to allow translating these things?
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1638824 — Re: [PATCH 3/9] VFS: Introduce a mount context

Jeff Layton <jlayton@redhat.com> wrote:

> One idea might be to always kfree it on syscall entry

You can't do that otherwise there's no way to retrieve the strings.

David

[toc] | [prev] | [next] | [standalone]

#1638827 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, 2017-05-10 at 14:31 +0100, David Howells wrote:
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > One idea might be to always kfree it on syscall entry
> 
> You can't do that otherwise there's no way to retrieve the strings.
> 
> 

True...you'd have to exempt the syscall that does the retrieving.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1638825 — Re: [PATCH 3/9] VFS: Introduce a mount context

> That's why I liked the static string thing.  It's just one assignment
> and no worries about freeing.  Not sure what to do about modules,
> though.  Can we somehow move the cost of checking the validity to the
> place where the error is retrieved?

I'm thinking along the lines of not allowing module virtual addresses
to be recycled after module remove...

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1638826 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, May 10, 2017 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 2017-05-10 at 09:05 +0100, David Howells wrote:
>> Miklos Szeredi <mszeredi@redhat.com> wrote:
>>
>> > Possible rule of thumb: use it only at the place where the error
>> > originates and not where errors are just passed on.  This would result
>> > in at most one report per syscall, normally.
>> >
>
> That might be hard to enforce in practice once you get into some
> complicated layering. What if we have device_mapper setting this along
> with filesystems too? We need clear rules here.

If the error originates in the devicemapper, then why would the
filesystem set it?

There's always a root cause of an error and that should be where the
detailed error is set.

Am I missing something?

>
>> > And the static string thing that David implemented is also a very good
>> > idea, IMO.
>>
>> There is an issue with it: it's fine as long as you keep a ref on the module
>> that generated it or clear all strings as part of module removal (which the
>> mount context in this patchset does).  With the NFS mount context I did, I
>> have to keep a ref on the NFS protocol module as well as the NFS filesystem
>> module.
>>
>> I'm tempted to make it conditionally copy the string using kvasprintf_const()
>> - which would also permit format substitution.
>>
>
> On balance, I think this is a reasonable way to pass back detailed
> errors. Up until now, we've mostly relied on just printk'ing them. Now
> though, a lot of larger machines are running containerized setups. Good
> luck scraping dmesg for _your_ error in that situation. There may be
> tons of mounts failing all over the place.
>
> That said, I have some concerns here:
>
> What's the lifetime of these strings? Do they just hang around forever
> until the process goes away or they're replaced? If this becomes common,
> then you could easily end up with an extra string allocation per task in
> some cases. That could add up.

That's why I liked the static string thing.  It's just one assignment
and no worries about freeing.  Not sure what to do about modules,
though.  Can we somehow move the cost of checking the validity to the
place where the error is retrieved?

>
> One idea might be to always kfree it on syscall entry, and that might
> mitigate the problem assuming that not everything is erroring out. Then
> you could always do some trivial syscall to clear it manually.
>
> There's also the problem of how these should be formatted. Is English ok
> everywhere? Do we need a facility to allow translating these things?

Messages in dmesg are in English too.  If necessary userspace will do
the translation.  I don't think the kernel would need to worry about
that.

Thanks,
Miklos


> --
> Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1638846 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, 2017-05-10 at 15:30 +0200, Miklos Szeredi wrote:
> On Wed, May 10, 2017 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Wed, 2017-05-10 at 09:05 +0100, David Howells wrote:
> > > Miklos Szeredi <mszeredi@redhat.com> wrote:
> > > 
> > > > Possible rule of thumb: use it only at the place where the error
> > > > originates and not where errors are just passed on.  This would result
> > > > in at most one report per syscall, normally.
> > > > 
> > 
> > That might be hard to enforce in practice once you get into some
> > complicated layering. What if we have device_mapper setting this along
> > with filesystems too? We need clear rules here.
> 
> If the error originates in the devicemapper, then why would the
> filesystem set it?
> 
> There's always a root cause of an error and that should be where the
> detailed error is set.
> 
> Am I missing something?
> 

I was thinking that you'd need some well-defined way to tell whether the
string should be replaced. If the thing just hangs out across syscalls,
then you don't know when it got put there. Is it a leftover from a
previous syscall or did a lower layer just put it there?

But...maybe I'm making assumptions about how this would work and I
should just wait until there are patches in flight. Getting the lifetime
of these strings right will be crucial though.

> > 
> > > > And the static string thing that David implemented is also a very good
> > > > idea, IMO.
> > > 
> > > There is an issue with it: it's fine as long as you keep a ref on the module
> > > that generated it or clear all strings as part of module removal (which the
> > > mount context in this patchset does).  With the NFS mount context I did, I
> > > have to keep a ref on the NFS protocol module as well as the NFS filesystem
> > > module.
> > > 
> > > I'm tempted to make it conditionally copy the string using kvasprintf_const()
> > > - which would also permit format substitution.
> > > 
> > 
> > On balance, I think this is a reasonable way to pass back detailed
> > errors. Up until now, we've mostly relied on just printk'ing them. Now
> > though, a lot of larger machines are running containerized setups. Good
> > luck scraping dmesg for _your_ error in that situation. There may be
> > tons of mounts failing all over the place.
> > 
> > That said, I have some concerns here:
> > 
> > What's the lifetime of these strings? Do they just hang around forever
> > until the process goes away or they're replaced? If this becomes common,
> > then you could easily end up with an extra string allocation per task in
> > some cases. That could add up.
> 
> That's why I liked the static string thing.  It's just one assignment
> and no worries about freeing.  Not sure what to do about modules,
> though.  Can we somehow move the cost of checking the validity to the
> place where the error is retrieved?
> 

Seems a little dangerous, and could be limiting. Dynamically allocated
strings seem like they could be more useful.

> > 
> > One idea might be to always kfree it on syscall entry, and that might
> > mitigate the problem assuming that not everything is erroring out. Then
> > you could always do some trivial syscall to clear it manually.
> > 
> > There's also the problem of how these should be formatted. Is English ok
> > everywhere? Do we need a facility to allow translating these things?
> 
> Messages in dmesg are in English too.  If necessary userspace will do
> the translation.  I don't think the kernel would need to worry about
> that.

Fair enough. It _is_ still an improvement over dmesg, IMO.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1638022 — Re: [PATCH 3/9] VFS: Introduce a mount context

Miklos Szeredi <mszeredi@redhat.com> wrote:

> So say we have commands like
> 
> "o+ foo"
> "o- bar"

The convention seems to be to prepend "no" to things you want to disable, so
let's stick with that, e.g.:

	"o foo"
	"o nobar"

otherwise we will have to have separate parsers for old mount() and the new sb
config code - and not just for NFS, but at least for ext2/3/4 also.

Further, we can only publish one format in /proc/mounts - and we cannot change
that from the foo/nofoo standard we already use as it's part of the UAPI.

> The generic option parser would just add or remove the option in the
> current set of options,

It sounds like you want to build up a string of "opt1,opt2,opt3" then have the
VFS add and remove things from it and then parse it into the filesystem's
internal structures on "commit".

> and commit would just call ->remount_fs() with the new set of options.

You're defining "commit" to do different things depending on the situation.
You need a separation between "commit create" and "commit update".

> It would probably not work for the NFS case, but that's okay, NFS can
> implement its own option parsing.

If NFS has to implement its own option parsing, we've done it wrong.

David

[toc] | [prev] | [next] | [standalone]

#1638096 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 9, 2017 at 11:56 AM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> So say we have commands like
>>
>> "o+ foo"
>> "o- bar"
>
> The convention seems to be to prepend "no" to things you want to disable, so
> let's stick with that, e.g.:
>
>         "o foo"
>         "o nobar"
>
> otherwise we will have to have separate parsers for old mount() and the new sb
> config code - and not just for NFS, but at least for ext2/3/4 also.
>
> Further, we can only publish one format in /proc/mounts - and we cannot change
> that from the foo/nofoo standard we already use as it's part of the UAPI.

You're right, that this is a complicated issue and worth more
discussion.  And also you are right that we cannot change existing
UAPI, which is going to cause some headaches.

But that doesn't mean the new UAPI must follow the conventions of the
badly defined existing UAPI.

And the "no*" convention is anything but well defined, so we cannot
just stick it into generic code, because you'll find exceptions
everywhere.

And one more reason to have a new, unambiguous UAPI for retrieving
superblock options.

>
>> The generic option parser would just add or remove the option in the
>> current set of options,
>
> It sounds like you want to build up a string of "opt1,opt2,opt3" then have the
> VFS add and remove things from it and then parse it into the filesystem's
> internal structures on "commit".

That would be the default operation, if the filesystem doesn't define
its own parser.

>> and commit would just call ->remount_fs() with the new set of options.
>
> You're defining "commit" to do different things depending on the situation.
> You need a separation between "commit create" and "commit update".

It would be different, yes, at least until the superblock creation api
is completely transformed, at which point it may actually become the
same thing.  But lets not jump ahead.

>> It would probably not work for the NFS case, but that's okay, NFS can
>> implement its own option parsing.
>
> If NFS has to implement its own option parsing, we've done it wrong.

My above sentence was not clear.  What I meant to say that NFS needs
to implement the non-generic option parsing function in order to be
able to handle the case of "you can't change the server IP address".
Which it would want to do anyway, since it will result in cleaner
code.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1638790 — Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, May 09, 2017 at 10:03:43AM +0200, Miklos Szeredi wrote:
> On Tue, May 9, 2017 at 12:57 AM, David Howells <dhowells@redhat.com> wrote:
> > Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> >> > + (3) Validate and pre-process the mount context.
> >>
> >> (3.5) Create super block
> >>
> >> I think this need to be triggered by something like a "commit" command
> >> from userspace.  Basically this is where the options are atomically
> >> set on the new (create) or existing (reconfigure) superblock.
> >
> > Why do you need to expose this step to userspace?  Assuming in the "new" case
> > you do, say:
> >
> >         fd = fsopen("nfs");
> >         write(fd, "s foo.bar:/bar", ...);
> >         write(fd, "o intr", ...);
> >         write(fd, "o fsc", ...);
> >         ...
> >         write(fd, "c", ...); /* commit operation to get a superblock */
> >         fsmount(fd, AT_FDCWD, "/mnt");  /* mount the superblock we just got */
> >
> > Then the "commit" op is dissimilar to "mount -o remount" since remount may
> > alter the superblock parameters *and* the mountpoint parameters, but commit
> > can only affect the superblock.
> 
> Forget remount, it's a historical remnant.  We need fsreconfig(sb) and
> setmntattr(mnt).  They are changing properties of different objects.

I agree and I'd like to highlight another issue we have with the
current mount(2). The problem is non-atomic work with more
propagation flags if you want to mount a filesystem. For example:

  mount /dev/sda1 /A -o private,unbindable,ro

this is supported by mount(8), but it's implemented by three
independent mount(2) calls

    - 1st mounts /dev/sda1 with MS_RDONLY
    - 2nd sets MS_PRIVATE flag
    - 3rd sets MS_UNBINDABLE flag.

it would be nice set all the VFS flags and then as atomic operation attach
the context to the tree.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[toc] | [prev] | [next] | [standalone]

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web