linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: David Howells <dhowells@redhat.com>
Cc: viro@zeniv.linux.org.uk, linux-api@vger.kernel.org,
	torvalds@linux-foundation.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 28/33] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #11]
Date: Thu, 09 Aug 2018 10:32:37 -0500	[thread overview]
Message-ID: <87in4jwo6i.fsf@xmission.com> (raw)
In-Reply-To: <27374.1533824694@warthog.procyon.org.uk> (David Howells's message of "Thu, 09 Aug 2018 15:24:54 +0100")

David Howells <dhowells@redhat.com> writes:

> Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> First let me thank you for adding both FSCONFIG_CMD_CREATE and
>> FSCONFIG_CMD_RECONFIGURE.  Unfortunately the implementation is currently
>> broken.  So this patch gets my:
>> 
>> This is broken in two specific ways.
>> ...
>> 2) FSCONFIG_CMD_CREATE will succeed even if the superblock already
>>    exists and it can not use all of the superblock parameters.
>> 
>>    This happens because vfs_get_super will only call fill_super
>>    if the super block is created.  Which is reasonable on the face
>>    of it.  But it in practice this introduces security problems.
>> 
>>    a) Either through reconfiguring a shared super block you did not
>>       realize was shared (as we saw with devpts).
>> 
>>    b) Mounting a super block and not honoring it's mount options
>>       because something has already mounted it.  As we see today
>>       with proc.  Leaving userspace to think the filesystem will behave
>>       one way when in fact it behaves another.
>> 
>> I have already explained this several times, and apparently I have been
>> ignored.  This fundamental usability issue that leads to security
>> problems.
>
> I've also explained why you're wrong or at least only partially right.  I *do*
> *not* want to implement sget() in userspace with the ability for userspace to
> lock out other mount requests - which is what it appears that you've been
> asking for.

All I really care about is that when you ask for a set of paramaters
that you get a filesystem with that set of parameters.  Not the same
filsystem mounted a different way.

That has gone wrong twice badly.  There is no common case I know of that
requires returning the same filesystem twice.  AKA the pain of the
existing semantics seems much much worse than any benefit.  So I am
asking that we not propagate the existing semantics into the new API.
You are cleaning up dealing with mount options and this is one of the
places where they need cleaning up.

> However, as I have said, I *am* willing to add one of more flags to help with
> this, but I can't make any "legacy" fs honour them as this requires the
> fs_context to be passed down to sget_fc() and the filesystem - which is why I
> was considering leaving it for later.
>
>  (1) An FSOPEN_EXCL flag to tell sget_fc() to fail if the superblock already
>      exists at all.
>
>  (2) An FSOPEN_FAIL_ON_MISMATCH flag to explicitly say that we *don't* want a
>      superblock with different parameters.
>
> The implication of providing neither flag is that we are happy to accept a
> superblock from the same source but with different parameters.
>
> But it doesn't seem to be an absolute imperative to roll this out immediately,
> since what I have exactly mirrors what the kernel currently does - and forcing
> a change in that behaviour risks breaking userspace.  If it keeps you happy,
> however, I can try and work one up.

What I am asking is that the default behavior for the new API when using
FSCONFIG_CMD_CREATE is to call sget_fc with either FSOPEN_EXCL or
FSOPEN_FAIL_ON_MISMATCH.  I know FSOPEN_EXCL is trivial to implement.  I
don't know if there are any hidden gotcha's with
FSOPEN_FAIL_ON_MISMATCH.

This change in default behavior for your patch needs to be implemented
before this hits a released kernel.  Returning a filesystem with
different than the requested parameters has resulted in at least two
major issues, that are very hard to clean up after the fact.  A chroot
system changing the parameters on /dev/pts resulting in some
distributions keeping the suid pt_chown binary long past it's best buy
date, and other distributions instead choosing to break userspace.  Then
there is the current issue where in practice proc does not any of it's
mount paramaters which breaks the android security model.

The fact that these things happen silently and you have to be on your
toes to catch them is fundamentally a bug in the API.  If the mount
request had simply failed people would have noticed the issues much
sooner and silently b0rkend configuration would not have propagated.  As
such I do not believe we should propagate this misfeature from the old
API into the new API.

Conceptually I like FSOPEN_FAIL_ON_MISMATH as it looks like it is
sufficient to the needs, and with a little luck we could even change
the old API to those semantics.


Ultimately I want to close a giant mental model mismatch.

User:  I am creating the data structures to read filesystem X
       with parameters Y.

Kernel: He wants filesystem X.  If it is a slow day use parameters Y.

Given that historically the reuse of a superblock did not exist, and
that in practice it almost never happens.  It is quite reqsonable for
users to not expect the kernel to completely ignore the mount parameters
they pass to the kernel.

So please let's fix that now when it is easy.

Eric

  parent reply	other threads:[~2018-08-09 15:32 UTC|newest]

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-01 15:23 [PATCH 00/33] VFS: Introduce filesystem context [ver #11] David Howells
2018-08-01 15:24 ` [PATCH 01/33] vfs: syscall: Add open_tree(2) to reference or clone a mount " David Howells
2018-08-02 17:31   ` Alan Jenkins
2018-08-02 21:29     ` Al Viro
2018-08-02 21:51   ` David Howells
2018-08-02 23:46     ` Alan Jenkins
2018-08-01 15:24 ` [PATCH 02/33] vfs: syscall: Add move_mount(2) to move mounts around " David Howells
2018-08-01 15:24 ` [PATCH 03/33] teach move_mount(2) to work with OPEN_TREE_CLONE " David Howells
2018-10-12 14:25   ` Alan Jenkins
2018-08-01 15:24 ` [PATCH 04/33] vfs: Suppress MS_* flag defs within the kernel unless explicitly enabled " David Howells
2018-08-01 15:24 ` [PATCH 05/33] vfs: Introduce the basic header for the new mount API's filesystem context " David Howells
2018-08-01 15:24 ` [PATCH 06/33] vfs: Introduce logging functions " David Howells
2018-08-01 15:24 ` [PATCH 07/33] vfs: Add configuration parser helpers " David Howells
2018-08-01 15:24 ` [PATCH 08/33] vfs: Add LSM hooks for the new mount API " David Howells
2018-08-01 20:50   ` James Morris
2018-08-01 22:53   ` David Howells
2018-08-01 15:25 ` [PATCH 09/33] selinux: Implement the new mount API LSM hooks " David Howells
2018-08-01 15:25 ` [PATCH 10/33] smack: Implement filesystem context security " David Howells
2018-08-01 15:25 ` [PATCH 11/33] apparmor: Implement security hooks for the new mount API " David Howells
2018-08-01 15:25 ` [PATCH 12/33] tomoyo: " David Howells
2018-08-01 15:25 ` [PATCH 13/33] vfs: Separate changing mount flags full remount " David Howells
2018-08-01 15:25 ` [PATCH 14/33] vfs: Implement a filesystem superblock creation/configuration context " David Howells
2018-09-11 17:46   ` Guenter Roeck
2018-09-11 21:52   ` David Howells
2018-09-11 22:07     ` Guenter Roeck
2018-09-11 23:17     ` David Howells
2018-09-11 23:54       ` Guenter Roeck
2018-09-18  9:07         ` Sergey Senozhatsky
2018-09-18  9:40           ` Sergey Senozhatsky
2018-09-18 14:06           ` Guenter Roeck
2018-09-19  1:12             ` Sergey Senozhatsky
2018-09-19  1:26               ` Sergey Senozhatsky
2018-09-18 15:34         ` David Howells
2018-09-18 16:39         ` David Howells
2018-09-19  1:15           ` Sergey Senozhatsky
2018-09-18 17:43         ` David Howells
2018-09-18  9:54   ` Sergey Senozhatsky
2018-09-18 15:28   ` David Howells
2018-08-01 15:25 ` [PATCH 15/33] vfs: Remove unused code after filesystem context changes " David Howells
2018-08-01 15:25 ` [PATCH 16/33] procfs: Move proc_fill_super() to fs/proc/root.c " David Howells
2018-08-01 15:26 ` [PATCH 17/33] proc: Add fs_context support to procfs " David Howells
2018-08-01 15:26 ` [PATCH 18/33] ipc: Convert mqueue fs to fs_context " David Howells
2018-08-01 15:26 ` [PATCH 19/33] cpuset: Use " David Howells
2018-08-01 15:26 ` [PATCH 20/33] kernfs, sysfs, cgroup, intel_rdt: Support " David Howells
2018-08-01 15:26 ` [PATCH 21/33] hugetlbfs: Convert to " David Howells
2018-08-01 15:26 ` [PATCH 22/33] vfs: Remove kern_mount_data() " David Howells
2018-08-01 15:26 ` [PATCH 23/33] vfs: Provide documentation for new mount API " David Howells
2018-08-01 15:26 ` [PATCH 24/33] Make anon_inodes unconditional " David Howells
2018-08-01 15:26 ` [PATCH 25/33] vfs: syscall: Add fsopen() to prepare for superblock creation " David Howells
2018-08-01 15:27 ` [PATCH 26/33] vfs: Implement logging through fs_context " David Howells
2018-08-01 15:27 ` [PATCH 27/33] vfs: Add some logging to the core users of the fs_context log " David Howells
2018-08-01 15:27 ` [PATCH 28/33] vfs: syscall: Add fsconfig() for configuring and managing a context " David Howells
2018-08-06 17:28   ` Eric W. Biederman
2018-08-09 14:14   ` David Howells
2018-08-09 14:24   ` David Howells
2018-08-09 14:35     ` Miklos Szeredi
2018-08-09 15:32     ` Eric W. Biederman [this message]
2018-08-09 16:33     ` David Howells
2018-08-11 20:20     ` David Howells
2018-08-11 23:26       ` Andy Lutomirski
2018-08-01 15:27 ` [PATCH 29/33] vfs: syscall: Add fsmount() to create a mount for a superblock " David Howells
2018-08-01 15:27 ` [PATCH 30/33] vfs: syscall: Add fspick() to select a superblock for reconfiguration " David Howells
2018-08-24 14:51   ` Miklos Szeredi
2018-08-24 14:54     ` Andy Lutomirski
2018-08-01 15:27 ` [PATCH 31/33] afs: Add fs_context support " David Howells
2018-08-01 15:27 ` [PATCH 32/33] afs: Use fs_context to pass parameters over automount " David Howells
2018-08-01 15:27 ` [PATCH 33/33] vfs: Add a sample program for the new mount API " David Howells
2018-08-10 14:05 ` BUG: Mount ignores mount options Eric W. Biederman
2018-08-10 14:36   ` Andy Lutomirski
2018-08-10 15:17     ` Eric W. Biederman
2018-08-10 15:24     ` Al Viro
2018-08-10 15:11   ` Tetsuo Handa
2018-08-10 15:13   ` David Howells
2018-08-10 15:16   ` Al Viro
2018-08-11  1:05     ` Eric W. Biederman
2018-08-11  1:46       ` Theodore Y. Ts'o
2018-08-11  4:48         ` Eric W. Biederman
2018-08-11 17:47           ` Casey Schaufler
2018-08-15  4:03             ` Eric W. Biederman
2018-08-11  1:58       ` Al Viro
2018-08-11  2:17         ` Al Viro
2018-08-11  4:43           ` Eric W. Biederman
2018-08-13 12:54         ` Miklos Szeredi
2018-08-10 15:11 ` David Howells
2018-08-10 15:39   ` Theodore Y. Ts'o
2018-08-10 15:55     ` Casey Schaufler
2018-08-10 16:11     ` David Howells
2018-08-10 18:00     ` Eric W. Biederman
2018-08-10 15:53   ` David Howells
2018-08-10 16:14     ` Theodore Y. Ts'o
2018-08-10 20:06       ` Andy Lutomirski
2018-08-10 20:46         ` Theodore Y. Ts'o
2018-08-10 22:12           ` Darrick J. Wong
2018-08-10 23:54             ` Theodore Y. Ts'o
2018-08-11  0:38               ` Darrick J. Wong
2018-08-11  1:32                 ` Eric W. Biederman
2018-08-13 16:35         ` Alan Cox
2018-08-13 16:48           ` Andy Lutomirski
2018-08-13 17:29             ` Al Viro
2018-08-13 19:00               ` James Morris
2018-08-13 19:20                 ` Casey Schaufler
2018-08-15 23:29                 ` Serge E. Hallyn
2018-08-11  0:28       ` Eric W. Biederman
2018-08-11  1:19   ` Eric W. Biederman
2018-08-11  7:29   ` David Howells
2018-08-11 16:31     ` Andy Lutomirski
2018-08-11 16:51       ` Al Viro
2018-08-15 16:31 ` Should we split the network filesystem setup into two phases? David Howells
2018-08-15 16:51   ` Andy Lutomirski
2018-08-16  3:51   ` Steve French
2018-08-16  5:06   ` Eric W. Biederman
2018-08-16 16:24     ` Steve French
2018-08-16 17:21       ` Eric W. Biederman
2018-08-16 17:23       ` Aurélien Aptel
2018-08-16 18:36         ` Steve French
2018-08-17 23:11     ` Al Viro

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=87in4jwo6i.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=dhowells@redhat.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /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 NNTP newsgroup(s).