Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-12 Thread Eric W. Biederman
Alistair Strachan  writes:

> On Mon, Jun 11, 2018 at 6:22 PM Eric W. Biederman  
> wrote:
>>
>> Alistair Strachan  writes:
>>
>> > In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
>> > the parsing of mount parameters for the proc filesystem was broken.
>> >
>> > The SB_KERNMOUNT for procfs happens via:
>> >
>> >   start_kernel()
>> > rest_init()
>> >   kernel_thread()
>> > _do_fork()
>> >copy_process()
>> >  alloc_pid()
>> >pid_ns_prepare_proc()
>> >  kern_mount_data()
>> >proc_mount()
>> >  mount_ns()
>> >
>> > In mount_ns(), the kernel calls proc_fill_super() only if the superblock
>> > has not previously been set up (i.e. the first mount reference),
>> > regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
>> > been moved inside here, and the SB_KERNMOUNT uses no mount options, the
>> > option parser became a no-op.
>> >
>> > When userspace later mounted procfs with e.g. hidepid=2, the options
>> > would be ignored.
>> >
>> > This change backs out a part of the original cleanup and parses the
>> > procfs mount options at every mount call. Because the options currently
>> > only update the pid_ns for the mount, they are applied for all mounts of
>> > proc by that pid or childen of that pid, instantaneously. This is the
>> > same behavior as the original code.
>>
>> Two years for a regression to be reported is a litte long.  I think that
>> gets out of the kneejerk immediate fix or revert phase and into thinking
>> a little bout about what makes sense in this code.
>
> Android has been using hidepid=2 for a while, but most shipping
> products were on 3.18 or 4.4 kernels. To us, it's a new problem.

All that says is that no one from Android has looked at or tested a
kernel (not even a stable one) since 4.4.  That does not work for
justifying changes/fixes to the kernel.  People working together does
not work to well when some of the people don't show up.

As an engineer I sympathize with your position.   Whoever made the
decision that Android won't care for anything except for the best
effort long term stable kernels has made your job a challenge.  I do
appreciate that Android at least periodically updates their kernels.

The bottom line though is that if this had been caught within the
release or so of it's breaking restoring the old userspace behavior
would not have been a question.  Now after two years there is the
question of if other people have come to depend on the new behavior.

>> As we say with devpts there is a very real danger of someone mounting
>> a second instance of proc in a chroot and causing problems by either
>> strengthening or weakening the hid pid protections for the entire pid
>> namespace.  If we go with your proposed change in behavior.
>
> I guess my change does change the behavior, but it's just back to the
> behavior which the kernel had for a good while (~v3.3 thru v4.7).
>
>> Ordinary block device filesystems (like ext4) avoid this problem by
>> allowing a second mount and by not parsing the mount options except
>> on remount.  What proc currently does.
>
> IMHO, they're not really comparable. You'll only get kernmounts of an
> ext4 filesystem when finding rootfs, and in that case the user knows
> about the mount and can see it in /proc/mounts, so they know to use -o
> remount,.
>
> Since the first mount (where the options might have been respected) is
> *always* the kernmount done before init, with your change these mount
> options for procfs will never be respected. As userspace didn't yet
> mount /proc, it can't know /proc was already mounted, in order to know
> to use a remount to re-parse the options. The behavior was changed in
> a non-obvious way.

Please not it is fundamentally required if the in kernel superblocks are
going to be shared.  

>> So I think it can be reasonably argued that the change in behavior is
>> was an unintentional fix.
>>
>> I can see an argument for failing the mount of proc if mount options
>> are specified or if those mount options differ from the existing mount
>> options.
>>
>> proc_remount's call of proc_parse_options is definitely buggy as it can
>> partially succeed and change the pid namespace and return an error code.
>> That is bad error handling.
>>
>> There may be an argument for making these options available in something
>> other than a mount of proc.  As they are pid namespace wide.
>>
>> There may be an argument for multiple instances of proc so that it makes
>> sense to process these options during an ordinary mount.
>>
>>
>> Ultimately what I see is that this is a difficult area of semantics that
>> there is at least a little room for improvement on, but it is not
>> as simple as this proposed change.
>
> An alternative fix might be to ignore the super setup if done from a
> kernmount of procfs. IMO, this initial mount shouldn't be considered
> the first reference, because it 

Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-12 Thread Eric W. Biederman
Alistair Strachan  writes:

> On Mon, Jun 11, 2018 at 6:22 PM Eric W. Biederman  
> wrote:
>>
>> Alistair Strachan  writes:
>>
>> > In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
>> > the parsing of mount parameters for the proc filesystem was broken.
>> >
>> > The SB_KERNMOUNT for procfs happens via:
>> >
>> >   start_kernel()
>> > rest_init()
>> >   kernel_thread()
>> > _do_fork()
>> >copy_process()
>> >  alloc_pid()
>> >pid_ns_prepare_proc()
>> >  kern_mount_data()
>> >proc_mount()
>> >  mount_ns()
>> >
>> > In mount_ns(), the kernel calls proc_fill_super() only if the superblock
>> > has not previously been set up (i.e. the first mount reference),
>> > regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
>> > been moved inside here, and the SB_KERNMOUNT uses no mount options, the
>> > option parser became a no-op.
>> >
>> > When userspace later mounted procfs with e.g. hidepid=2, the options
>> > would be ignored.
>> >
>> > This change backs out a part of the original cleanup and parses the
>> > procfs mount options at every mount call. Because the options currently
>> > only update the pid_ns for the mount, they are applied for all mounts of
>> > proc by that pid or childen of that pid, instantaneously. This is the
>> > same behavior as the original code.
>>
>> Two years for a regression to be reported is a litte long.  I think that
>> gets out of the kneejerk immediate fix or revert phase and into thinking
>> a little bout about what makes sense in this code.
>
> Android has been using hidepid=2 for a while, but most shipping
> products were on 3.18 or 4.4 kernels. To us, it's a new problem.

All that says is that no one from Android has looked at or tested a
kernel (not even a stable one) since 4.4.  That does not work for
justifying changes/fixes to the kernel.  People working together does
not work to well when some of the people don't show up.

As an engineer I sympathize with your position.   Whoever made the
decision that Android won't care for anything except for the best
effort long term stable kernels has made your job a challenge.  I do
appreciate that Android at least periodically updates their kernels.

The bottom line though is that if this had been caught within the
release or so of it's breaking restoring the old userspace behavior
would not have been a question.  Now after two years there is the
question of if other people have come to depend on the new behavior.

>> As we say with devpts there is a very real danger of someone mounting
>> a second instance of proc in a chroot and causing problems by either
>> strengthening or weakening the hid pid protections for the entire pid
>> namespace.  If we go with your proposed change in behavior.
>
> I guess my change does change the behavior, but it's just back to the
> behavior which the kernel had for a good while (~v3.3 thru v4.7).
>
>> Ordinary block device filesystems (like ext4) avoid this problem by
>> allowing a second mount and by not parsing the mount options except
>> on remount.  What proc currently does.
>
> IMHO, they're not really comparable. You'll only get kernmounts of an
> ext4 filesystem when finding rootfs, and in that case the user knows
> about the mount and can see it in /proc/mounts, so they know to use -o
> remount,.
>
> Since the first mount (where the options might have been respected) is
> *always* the kernmount done before init, with your change these mount
> options for procfs will never be respected. As userspace didn't yet
> mount /proc, it can't know /proc was already mounted, in order to know
> to use a remount to re-parse the options. The behavior was changed in
> a non-obvious way.

Please not it is fundamentally required if the in kernel superblocks are
going to be shared.  

>> So I think it can be reasonably argued that the change in behavior is
>> was an unintentional fix.
>>
>> I can see an argument for failing the mount of proc if mount options
>> are specified or if those mount options differ from the existing mount
>> options.
>>
>> proc_remount's call of proc_parse_options is definitely buggy as it can
>> partially succeed and change the pid namespace and return an error code.
>> That is bad error handling.
>>
>> There may be an argument for making these options available in something
>> other than a mount of proc.  As they are pid namespace wide.
>>
>> There may be an argument for multiple instances of proc so that it makes
>> sense to process these options during an ordinary mount.
>>
>>
>> Ultimately what I see is that this is a difficult area of semantics that
>> there is at least a little room for improvement on, but it is not
>> as simple as this proposed change.
>
> An alternative fix might be to ignore the super setup if done from a
> kernmount of procfs. IMO, this initial mount shouldn't be considered
> the first reference, because it 

Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-12 Thread Alistair Strachan
On Mon, Jun 11, 2018 at 6:22 PM Eric W. Biederman  wrote:
>
> Alistair Strachan  writes:
>
> > In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
> > the parsing of mount parameters for the proc filesystem was broken.
> >
> > The SB_KERNMOUNT for procfs happens via:
> >
> >   start_kernel()
> > rest_init()
> >   kernel_thread()
> > _do_fork()
> >copy_process()
> >  alloc_pid()
> >pid_ns_prepare_proc()
> >  kern_mount_data()
> >proc_mount()
> >  mount_ns()
> >
> > In mount_ns(), the kernel calls proc_fill_super() only if the superblock
> > has not previously been set up (i.e. the first mount reference),
> > regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
> > been moved inside here, and the SB_KERNMOUNT uses no mount options, the
> > option parser became a no-op.
> >
> > When userspace later mounted procfs with e.g. hidepid=2, the options
> > would be ignored.
> >
> > This change backs out a part of the original cleanup and parses the
> > procfs mount options at every mount call. Because the options currently
> > only update the pid_ns for the mount, they are applied for all mounts of
> > proc by that pid or childen of that pid, instantaneously. This is the
> > same behavior as the original code.
>
> Two years for a regression to be reported is a litte long.  I think that
> gets out of the kneejerk immediate fix or revert phase and into thinking
> a little bout about what makes sense in this code.

Android has been using hidepid=2 for a while, but most shipping
products were on 3.18 or 4.4 kernels. To us, it's a new problem.

> As we say with devpts there is a very real danger of someone mounting
> a second instance of proc in a chroot and causing problems by either
> strengthening or weakening the hid pid protections for the entire pid
> namespace.  If we go with your proposed change in behavior.

I guess my change does change the behavior, but it's just back to the
behavior which the kernel had for a good while (~v3.3 thru v4.7).

> Ordinary block device filesystems (like ext4) avoid this problem by
> allowing a second mount and by not parsing the mount options except
> on remount.  What proc currently does.

IMHO, they're not really comparable. You'll only get kernmounts of an
ext4 filesystem when finding rootfs, and in that case the user knows
about the mount and can see it in /proc/mounts, so they know to use -o
remount,.

Since the first mount (where the options might have been respected) is
*always* the kernmount done before init, with your change these mount
options for procfs will never be respected. As userspace didn't yet
mount /proc, it can't know /proc was already mounted, in order to know
to use a remount to re-parse the options. The behavior was changed in
a non-obvious way.

> So I think it can be reasonably argued that the change in behavior is
> was an unintentional fix.
>
> I can see an argument for failing the mount of proc if mount options
> are specified or if those mount options differ from the existing mount
> options.
>
> proc_remount's call of proc_parse_options is definitely buggy as it can
> partially succeed and change the pid namespace and return an error code.
> That is bad error handling.
>
> There may be an argument for making these options available in something
> other than a mount of proc.  As they are pid namespace wide.
>
> There may be an argument for multiple instances of proc so that it makes
> sense to process these options during an ordinary mount.
>
>
> Ultimately what I see is that this is a difficult area of semantics that
> there is at least a little room for improvement on, but it is not
> as simple as this proposed change.

An alternative fix might be to ignore the super setup if done from a
kernmount of procfs. IMO, this initial mount shouldn't be considered
the first reference, because it will not pass the mount options and
cannot be observed by userspace. Such a change looks complicated,
though, and it would only be relevant to procfs. It might be better to
roll back the cleanup and implement these semantics directly in the
procfs code.

> > Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns")
> > Signed-off-by: Alistair Strachan 
> > Cc: Seth Forshee 
> > Cc: Djalal Harouni 
> > Cc: "Eric W. Biederman" 
> > Cc: kernel-t...@android.com
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  fs/proc/inode.c| 4 
> >  fs/proc/internal.h | 1 -
> >  fs/proc/root.c | 5 -
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> > index 2cf3b74391ca..f348be0a 100644
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -492,13 +492,9 @@ struct inode *proc_get_inode(struct super_block *sb, 
> > struct proc_dir_entry *de)
> >
> >  int proc_fill_super(struct super_block *s, void *data, int silent)
> >  {
> > - struct pid_namespace 

Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-12 Thread Alistair Strachan
On Mon, Jun 11, 2018 at 6:22 PM Eric W. Biederman  wrote:
>
> Alistair Strachan  writes:
>
> > In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
> > the parsing of mount parameters for the proc filesystem was broken.
> >
> > The SB_KERNMOUNT for procfs happens via:
> >
> >   start_kernel()
> > rest_init()
> >   kernel_thread()
> > _do_fork()
> >copy_process()
> >  alloc_pid()
> >pid_ns_prepare_proc()
> >  kern_mount_data()
> >proc_mount()
> >  mount_ns()
> >
> > In mount_ns(), the kernel calls proc_fill_super() only if the superblock
> > has not previously been set up (i.e. the first mount reference),
> > regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
> > been moved inside here, and the SB_KERNMOUNT uses no mount options, the
> > option parser became a no-op.
> >
> > When userspace later mounted procfs with e.g. hidepid=2, the options
> > would be ignored.
> >
> > This change backs out a part of the original cleanup and parses the
> > procfs mount options at every mount call. Because the options currently
> > only update the pid_ns for the mount, they are applied for all mounts of
> > proc by that pid or childen of that pid, instantaneously. This is the
> > same behavior as the original code.
>
> Two years for a regression to be reported is a litte long.  I think that
> gets out of the kneejerk immediate fix or revert phase and into thinking
> a little bout about what makes sense in this code.

Android has been using hidepid=2 for a while, but most shipping
products were on 3.18 or 4.4 kernels. To us, it's a new problem.

> As we say with devpts there is a very real danger of someone mounting
> a second instance of proc in a chroot and causing problems by either
> strengthening or weakening the hid pid protections for the entire pid
> namespace.  If we go with your proposed change in behavior.

I guess my change does change the behavior, but it's just back to the
behavior which the kernel had for a good while (~v3.3 thru v4.7).

> Ordinary block device filesystems (like ext4) avoid this problem by
> allowing a second mount and by not parsing the mount options except
> on remount.  What proc currently does.

IMHO, they're not really comparable. You'll only get kernmounts of an
ext4 filesystem when finding rootfs, and in that case the user knows
about the mount and can see it in /proc/mounts, so they know to use -o
remount,.

Since the first mount (where the options might have been respected) is
*always* the kernmount done before init, with your change these mount
options for procfs will never be respected. As userspace didn't yet
mount /proc, it can't know /proc was already mounted, in order to know
to use a remount to re-parse the options. The behavior was changed in
a non-obvious way.

> So I think it can be reasonably argued that the change in behavior is
> was an unintentional fix.
>
> I can see an argument for failing the mount of proc if mount options
> are specified or if those mount options differ from the existing mount
> options.
>
> proc_remount's call of proc_parse_options is definitely buggy as it can
> partially succeed and change the pid namespace and return an error code.
> That is bad error handling.
>
> There may be an argument for making these options available in something
> other than a mount of proc.  As they are pid namespace wide.
>
> There may be an argument for multiple instances of proc so that it makes
> sense to process these options during an ordinary mount.
>
>
> Ultimately what I see is that this is a difficult area of semantics that
> there is at least a little room for improvement on, but it is not
> as simple as this proposed change.

An alternative fix might be to ignore the super setup if done from a
kernmount of procfs. IMO, this initial mount shouldn't be considered
the first reference, because it will not pass the mount options and
cannot be observed by userspace. Such a change looks complicated,
though, and it would only be relevant to procfs. It might be better to
roll back the cleanup and implement these semantics directly in the
procfs code.

> > Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns")
> > Signed-off-by: Alistair Strachan 
> > Cc: Seth Forshee 
> > Cc: Djalal Harouni 
> > Cc: "Eric W. Biederman" 
> > Cc: kernel-t...@android.com
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  fs/proc/inode.c| 4 
> >  fs/proc/internal.h | 1 -
> >  fs/proc/root.c | 5 -
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> > index 2cf3b74391ca..f348be0a 100644
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -492,13 +492,9 @@ struct inode *proc_get_inode(struct super_block *sb, 
> > struct proc_dir_entry *de)
> >
> >  int proc_fill_super(struct super_block *s, void *data, int silent)
> >  {
> > - struct pid_namespace 

Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-11 Thread Eric W. Biederman
Alistair Strachan  writes:

> In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
> the parsing of mount parameters for the proc filesystem was broken.
>
> The SB_KERNMOUNT for procfs happens via:
>
>   start_kernel()
> rest_init()
>   kernel_thread()
> _do_fork()
>copy_process()
>  alloc_pid()
>pid_ns_prepare_proc()
>  kern_mount_data()
>proc_mount()
>  mount_ns()
>
> In mount_ns(), the kernel calls proc_fill_super() only if the superblock
> has not previously been set up (i.e. the first mount reference),
> regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
> been moved inside here, and the SB_KERNMOUNT uses no mount options, the
> option parser became a no-op.
>
> When userspace later mounted procfs with e.g. hidepid=2, the options
> would be ignored.
>
> This change backs out a part of the original cleanup and parses the
> procfs mount options at every mount call. Because the options currently
> only update the pid_ns for the mount, they are applied for all mounts of
> proc by that pid or childen of that pid, instantaneously. This is the
> same behavior as the original code.

Two years for a regression to be reported is a litte long.  I think that
gets out of the kneejerk immediate fix or revert phase and into thinking
a little bout about what makes sense in this code.

As we say with devpts there is a very real danger of someone mounting
a second instance of proc in a chroot and causing problems by either
strengthening or weakening the hid pid protections for the entire pid
namespace.  If we go with your proposed change in behavior.

Ordinary block device filesystems (like ext4) avoid this problem by
allowing a second mount and by not parsing the mount options except
on remount.  What proc currently does.

So I think it can be reasonably argued that the change in behavior is
was an unintentional fix.

I can see an argument for failing the mount of proc if mount options
are specified or if those mount options differ from the existing mount
options.  

proc_remount's call of proc_parse_options is definitely buggy as it can
partially succeed and change the pid namespace and return an error code.
That is bad error handling.

There may be an argument for making these options available in something
other than a mount of proc.  As they are pid namespace wide.

There may be an argument for multiple instances of proc so that it makes
sense to process these options during an ordinary mount.


Ultimately what I see is that this is a difficult area of semantics that
there is at least a little room for improvement on, but it is not
as simple as this proposed change.


> Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns")
> Signed-off-by: Alistair Strachan 
> Cc: Seth Forshee 
> Cc: Djalal Harouni 
> Cc: "Eric W. Biederman" 
> Cc: kernel-t...@android.com
> Cc: linux-kernel@vger.kernel.org
> ---
>  fs/proc/inode.c| 4 
>  fs/proc/internal.h | 1 -
>  fs/proc/root.c | 5 -
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 2cf3b74391ca..f348be0a 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -492,13 +492,9 @@ struct inode *proc_get_inode(struct super_block *sb, 
> struct proc_dir_entry *de)
>  
>  int proc_fill_super(struct super_block *s, void *data, int silent)
>  {
> - struct pid_namespace *ns = get_pid_ns(s->s_fs_info);
>   struct inode *root_inode;
>   int ret;
>  
> - if (!proc_parse_options(data, ns))
> - return -EINVAL;
> -
>   /* User space would break if executables or devices appear on proc */
>   s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
>   s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 50cb22a08c2f..89b7e845b000 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -264,7 +264,6 @@ static inline void proc_tty_init(void) {}
>   * root.c
>   */
>  extern struct proc_dir_entry proc_root;
> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
>  
>  extern void proc_self_init(void);
>  extern int proc_remount(struct super_block *, int *, char *);
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 61b7340b357a..d40676a5dd6c 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -36,7 +36,7 @@ static const match_table_t tokens = {
>   {Opt_err, NULL},
>  };
>  
> -int proc_parse_options(char *options, struct pid_namespace *pid)
> +static int proc_parse_options(char *options, struct pid_namespace *pid)
>  {
>   char *p;
>   substring_t args[MAX_OPT_ARGS];
> @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type 
> *fs_type,
>   ns = task_active_pid_ns(current);
>   }
>  
> + if (!proc_parse_options(data, ns))
> + return 

Re: [PATCH] proc: Fix parsing of mount parameters.

2018-06-11 Thread Eric W. Biederman
Alistair Strachan  writes:

> In commit e94591d0d90c "proc: Convert proc_mount to use mount_ns"
> the parsing of mount parameters for the proc filesystem was broken.
>
> The SB_KERNMOUNT for procfs happens via:
>
>   start_kernel()
> rest_init()
>   kernel_thread()
> _do_fork()
>copy_process()
>  alloc_pid()
>pid_ns_prepare_proc()
>  kern_mount_data()
>proc_mount()
>  mount_ns()
>
> In mount_ns(), the kernel calls proc_fill_super() only if the superblock
> has not previously been set up (i.e. the first mount reference),
> regardless of SB_KERNMOUNT. Because the call to proc_parse_options() had
> been moved inside here, and the SB_KERNMOUNT uses no mount options, the
> option parser became a no-op.
>
> When userspace later mounted procfs with e.g. hidepid=2, the options
> would be ignored.
>
> This change backs out a part of the original cleanup and parses the
> procfs mount options at every mount call. Because the options currently
> only update the pid_ns for the mount, they are applied for all mounts of
> proc by that pid or childen of that pid, instantaneously. This is the
> same behavior as the original code.

Two years for a regression to be reported is a litte long.  I think that
gets out of the kneejerk immediate fix or revert phase and into thinking
a little bout about what makes sense in this code.

As we say with devpts there is a very real danger of someone mounting
a second instance of proc in a chroot and causing problems by either
strengthening or weakening the hid pid protections for the entire pid
namespace.  If we go with your proposed change in behavior.

Ordinary block device filesystems (like ext4) avoid this problem by
allowing a second mount and by not parsing the mount options except
on remount.  What proc currently does.

So I think it can be reasonably argued that the change in behavior is
was an unintentional fix.

I can see an argument for failing the mount of proc if mount options
are specified or if those mount options differ from the existing mount
options.  

proc_remount's call of proc_parse_options is definitely buggy as it can
partially succeed and change the pid namespace and return an error code.
That is bad error handling.

There may be an argument for making these options available in something
other than a mount of proc.  As they are pid namespace wide.

There may be an argument for multiple instances of proc so that it makes
sense to process these options during an ordinary mount.


Ultimately what I see is that this is a difficult area of semantics that
there is at least a little room for improvement on, but it is not
as simple as this proposed change.


> Fixes: e94591d0d90c ("proc: Convert proc_mount to use mount_ns")
> Signed-off-by: Alistair Strachan 
> Cc: Seth Forshee 
> Cc: Djalal Harouni 
> Cc: "Eric W. Biederman" 
> Cc: kernel-t...@android.com
> Cc: linux-kernel@vger.kernel.org
> ---
>  fs/proc/inode.c| 4 
>  fs/proc/internal.h | 1 -
>  fs/proc/root.c | 5 -
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 2cf3b74391ca..f348be0a 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -492,13 +492,9 @@ struct inode *proc_get_inode(struct super_block *sb, 
> struct proc_dir_entry *de)
>  
>  int proc_fill_super(struct super_block *s, void *data, int silent)
>  {
> - struct pid_namespace *ns = get_pid_ns(s->s_fs_info);
>   struct inode *root_inode;
>   int ret;
>  
> - if (!proc_parse_options(data, ns))
> - return -EINVAL;
> -
>   /* User space would break if executables or devices appear on proc */
>   s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
>   s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 50cb22a08c2f..89b7e845b000 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -264,7 +264,6 @@ static inline void proc_tty_init(void) {}
>   * root.c
>   */
>  extern struct proc_dir_entry proc_root;
> -extern int proc_parse_options(char *options, struct pid_namespace *pid);
>  
>  extern void proc_self_init(void);
>  extern int proc_remount(struct super_block *, int *, char *);
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 61b7340b357a..d40676a5dd6c 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -36,7 +36,7 @@ static const match_table_t tokens = {
>   {Opt_err, NULL},
>  };
>  
> -int proc_parse_options(char *options, struct pid_namespace *pid)
> +static int proc_parse_options(char *options, struct pid_namespace *pid)
>  {
>   char *p;
>   substring_t args[MAX_OPT_ARGS];
> @@ -98,6 +98,9 @@ static struct dentry *proc_mount(struct file_system_type 
> *fs_type,
>   ns = task_active_pid_ns(current);
>   }
>  
> + if (!proc_parse_options(data, ns))
> + return