linux-kernel - Re: [RFC][PATCH 00/14] VFS: Make all filesystems implement ->show

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 05 Jul 2017 11:34:28 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     David Howells <dhowells@...hat.com>
Cc:     viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 00/14] VFS: Make all filesystems implement ->show_options()

David Howells <dhowells@...hat.com> writes:

> Here's a set of patches that:
>
>  (1) Removes calls to save/replace_mount_options() where the filesystem
>      then implements ->show_options() anyway, ignoring ->s_options.
>
>  (2) Makes all filesystems implement the ->show_options() superblock
>      operation rather than using generic_show_options().  If necessary,
>      extra information is stored in the superblock information.
>
>  (3) Deletes save_mount_options(), replace_mount_options(),
>      generic_show_options() and super_block::s_options.
>
> This makes it easier to implement a context-based mount where the options
> are passed individually over a file descriptor.  It also allows duplicate
> options, options that override each other and ignored options to be
> resolved rather than storing irrelevant data.

This makes me a little nervous but is probably fine.  But we do need
to be careful with remount.  Today the rule is all options that need
to be preserved need to be passed to remount.  Passing options in one by
one looks like it may make it easy to get that confused while you are
developing your patches.

> Further, a lot of the time, all the information we want to display is
> stored in the super block information anyway, so the option string is
> redundant.
>
> Some things I noted whilst doing this:
>
>  (1) A number of filesystems take uid/gid options.  Should these be
>      reported relative to the observer's user namespace rather than init's
>      user namespace?  After all, you can't then use those uid/gid options
>      if the numbers are interpreted incorrectly if you try and forge a
>      mount command from them.

Options should be relative to the mount call.  Which is almost always
&init_user_ns and in the general case would be sb->s_user_ns.

Which I believe becomes the creator of the mount context in the model
you are working towards.

>  (2) Should I provide a helper for displaying uid/gid options?
>
>  (3) How much do we need to worry about racing with remount?  Some
>      filesystems happily give out the contents of the super_block without
>      regard to the fact that remount might be changing it simultaneously -
>      ext4, for example.
>
>  (4) What string options actually need 'munging'?
>
> These patches can be found here:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=for-viro
>
> on top of three other minor patches.
>
> David
> ---
> David Howells (14):
>       VFS: Don't use save/replace_mount_options if not using generic_show_options
>       hugetlbfs: Implement show_options
>       omfs: Implement show_options
>       pstore: Implement show_options
>       ramfs: Implement show_options
>       bpf: Implement show_options
>       spufs: Implement show_options
>       befs: Implement show_options
>       affs: Implement show_options
>       afs: Implement show_options
>       isofs: Implement show_options
>       9p: Implement show_options
>       orangefs: Implement show_options
>       VFS: Kill off s_options and helpers
>
>
>  Documentation/filesystems/vfs.txt         |    6 --
>  arch/powerpc/platforms/cell/spufs/inode.c |   21 +++++++--
>  fs/9p/v9fs.c                              |   59 ++++++++++++++++++++++++
>  fs/9p/v9fs.h                              |    3 +
>  fs/9p/vfs_super.c                         |    6 +-
>  fs/affs/super.c                           |   42 +++++++++++++++--
>  fs/afs/super.c                            |   45 ++++++++++++++++++-
>  fs/befs/linuxvfs.c                        |   24 +++++++++-
>  fs/btrfs/super.c                          |    1 
>  fs/debugfs/inode.c                        |    2 -
>  fs/efivarfs/super.c                       |    1 
>  fs/hugetlbfs/inode.c                      |   70 +++++++++++++++++++++++------
>  fs/isofs/inode.c                          |   51 ++++++++++++++++++++-
>  fs/isofs/isofs.h                          |    3 +
>  fs/namespace.c                            |   59 ------------------------
>  fs/omfs/inode.c                           |   33 ++++++++++++--
>  fs/orangefs/super.c                       |   15 ++++++
>  fs/pstore/inode.c                         |   14 +++++-
>  fs/pstore/internal.h                      |    3 +
>  fs/pstore/platform.c                      |    2 -
>  fs/ramfs/inode.c                          |   32 +++++++++----
>  fs/reiserfs/super.c                       |    4 --
>  fs/super.c                                |    1 
>  fs/tracefs/inode.c                        |    2 -
>  include/linux/fs.h                        |    9 ----
>  include/linux/hugetlb.h                   |    3 +
>  include/net/9p/client.h                   |   13 +++++
>  include/net/9p/transport.h                |    1 
>  kernel/bpf/inode.c                        |   16 +++++--
>  net/9p/client.c                           |   25 ++++++++++
>  net/9p/trans_fd.c                         |   31 ++++++++++++-
>  net/9p/trans_rdma.c                       |   31 ++++++++++++-
>  32 files changed, 481 insertions(+), 147 deletions(-)