3.3 and hidepid feature problem - options not always applied at mount

3.3 and hidepid feature problem - options not always applied at mount

[Arkadiusz Miśkiewicz]

Hi,

I'm trying to use hidepid feature in 3.3 kernel but I'm getting weird
things like options not being applied _sometimes_ at mount.

[@ ~]# cat /proc/mounts
sh: cat: /proc/mounts: No such file or directory
[@ ~]# strace -e mount -f -F -s 200 mount none /proc -t proc -o hidepid=2,gid=17                            
mount("none", "/proc", "proc", MS_MGC_VAL, "hidepid=2,gid=17") = 0
[@ ~]# cat /proc/mounts
rootfs / rootfs rw 0 0
/dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
run /run tmpfs rw,relatime 0 0
none /proc proc rw,relatime 0 0

No hidepid, no gid - huh?

[@ ~]# mount /proc -o remount,hidepid=2,gid=17
[@ ~]# cat /proc/mounts
rootfs / rootfs rw 0 0
/dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
run /run tmpfs rw,relatime 0 0
none /proc proc rw,relatime,gid=17,hidepid=2 0 0

remount and hidepid/gid is there

[@ ~]# umount /proc
[@ ~]# strace -e mount -f -F -s 200 mount none /proc -t proc -o hidepid=2,gid=17                            
mount("none", "/proc", "proc", MS_MGC_VAL, "hidepid=2,gid=17") = 0
[@ ~]# cat /proc/mounts
rootfs / rootfs rw 0 0
/dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
run /run tmpfs rw,relatime 0 0
none /proc proc rw,relatime,gid=17,hidepid=2 0 0

and now I'm lost - every new umount & mount gets hidepid/gid right.

Any ideas why initial mount fails to get hidepid/gid options applied?
The syscall seems correct.
-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

[PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

Hi Arkadiusz,

On Thu, Mar 22, 2012 at 09:03 +0100, Arkadiusz Miśkiewicz wrote:
> Hi,
> 
> I'm trying to use hidepid feature in 3.3 kernel but I'm getting weird
> things like options not being applied _sometimes_ at mount.
> 
> [@ ~]# cat /proc/mounts
> sh: cat: /proc/mounts: No such file or directory
> [@ ~]# strace -e mount -f -F -s 200 mount none /proc -t proc -o hidepid=2,gid=17                            
> mount("none", "/proc", "proc", MS_MGC_VAL, "hidepid=2,gid=17") = 0
> [@ ~]# cat /proc/mounts
> rootfs / rootfs rw 0 0
> /dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
> run /run tmpfs rw,relatime 0 0
> none /proc proc rw,relatime 0 0
> 
> No hidepid, no gid - huh?
> 
> [@ ~]# mount /proc -o remount,hidepid=2,gid=17
> [@ ~]# cat /proc/mounts
> rootfs / rootfs rw 0 0
> /dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
> run /run tmpfs rw,relatime 0 0
> none /proc proc rw,relatime,gid=17,hidepid=2 0 0
> 
> remount and hidepid/gid is there
> 
> [@ ~]# umount /proc
> [@ ~]# strace -e mount -f -F -s 200 mount none /proc -t proc -o hidepid=2,gid=17                            
> mount("none", "/proc", "proc", MS_MGC_VAL, "hidepid=2,gid=17") = 0
> [@ ~]# cat /proc/mounts
> rootfs / rootfs rw 0 0
> /dev/sda3 / xfs rw,relatime,attr2,noquota 0 0
> run /run tmpfs rw,relatime 0 0
> none /proc proc rw,relatime,gid=17,hidepid=2 0 0
> 
> and now I'm lost - every new umount & mount gets hidepid/gid right.
> 
> Any ideas why initial mount fails to get hidepid/gid options applied?
> The syscall seems correct.

Thanks for the report.  Please test the following patch.

--------------------------------------------------------------
From: Vasiliy Kulikov <segoon@openwall.com>
Date: Fri, 23 Mar 2012 20:56:42 +0400
Subject: [PATCH] proc: fix mount -t proc -o AAA

proc_parse_options() inside of proc_mount() runs only once at the boot
time without any given options.  So, following umount(2)+mount(2) ignore
mount options: proc_parse_options() is not called as ->s_root is already
initialized.  To fix that parse mount options unconditionally.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Reported-by: Arkadiusz Miśkiewicz <a.miskiewicz@gmail.com>
---
 fs/proc/root.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/proc/root.c b/fs/proc/root.c
index 46a15d8..eed44bf 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
 	if (IS_ERR(sb))
 		return ERR_CAST(sb);
 
+	if (!proc_parse_options(options, ns)) {
+		deactivate_locked_super(sb);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (!sb->s_root) {
 		sb->s_flags = flags;
-		if (!proc_parse_options(options, ns)) {
-			deactivate_locked_super(sb);
-			return ERR_PTR(-EINVAL);
-		}
 		err = proc_fill_super(sb);
 		if (err) {
 			deactivate_locked_super(sb);
-- 
1.7.0.4

Re: [PATCH] proc: fix mount -t proc -o AAA

[Arkadiusz Miśkiewicz]

On Friday 23 of March 2012, Vasiliy Kulikov wrote:

> Thanks for the report.  Please test the following patch.

Seems to be working fine, now - thanks!



> 
> --------------------------------------------------------------
> From: Vasiliy Kulikov <segoon@openwall.com>
> Date: Fri, 23 Mar 2012 20:56:42 +0400
> Subject: [PATCH] proc: fix mount -t proc -o AAA
> 
> proc_parse_options() inside of proc_mount() runs only once at the boot
> time without any given options.  So, following umount(2)+mount(2) ignore
> mount options: proc_parse_options() is not called as ->s_root is already
> initialized.  To fix that parse mount options unconditionally.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> Reported-by: Arkadiusz Miśkiewicz <a.miskiewicz@gmail.com>
> ---
>  fs/proc/root.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 46a15d8..eed44bf 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct
> file_system_type *fs_type, if (IS_ERR(sb))
>  		return ERR_CAST(sb);
> 
> +	if (!proc_parse_options(options, ns)) {
> +		deactivate_locked_super(sb);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (!sb->s_root) {
>  		sb->s_flags = flags;
> -		if (!proc_parse_options(options, ns)) {
> -			deactivate_locked_super(sb);
> -			return ERR_PTR(-EINVAL);
> -		}
>  		err = proc_fill_super(sb);
>  		if (err) {
>  			deactivate_locked_super(sb);


-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

Andrew,

Can you pick this patch, please?

On Fri, Mar 23, 2012 at 19:45 +0100, Arkadiusz Miśkiewicz wrote:
> On Friday 23 of March 2012, Vasiliy Kulikov wrote:
> 
> > Thanks for the report.  Please test the following patch.
> 
> Seems to be working fine, now - thanks!
> 
> 
> 
> > 
> > --------------------------------------------------------------
> > From: Vasiliy Kulikov <segoon@openwall.com>
> > Date: Fri, 23 Mar 2012 20:56:42 +0400
> > Subject: [PATCH] proc: fix mount -t proc -o AAA
> > 
> > proc_parse_options() inside of proc_mount() runs only once at the boot
> > time without any given options.  So, following umount(2)+mount(2) ignore
> > mount options: proc_parse_options() is not called as ->s_root is already
> > initialized.  To fix that parse mount options unconditionally.
> > 
> > Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> > Reported-by: Arkadiusz Miśkiewicz <a.miskiewicz@gmail.com>
> > ---
> >  fs/proc/root.c |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index 46a15d8..eed44bf 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct
> > file_system_type *fs_type, if (IS_ERR(sb))
> >  		return ERR_CAST(sb);
> > 
> > +	if (!proc_parse_options(options, ns)) {
> > +		deactivate_locked_super(sb);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >  	if (!sb->s_root) {
> >  		sb->s_flags = flags;
> > -		if (!proc_parse_options(options, ns)) {
> > -			deactivate_locked_super(sb);
> > -			return ERR_PTR(-EINVAL);
> > -		}
> >  		err = proc_fill_super(sb);
> >  		if (err) {
> >  			deactivate_locked_super(sb);
> 
> 
> -- 
> Arkadiusz Miśkiewicz        PLD/Linux Team
> arekm / maven.pl            http://ftp.pld-linux.org/

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: fix mount -t proc -o AAA

[Andrew Morton]

On Fri, 23 Mar 2012 21:10:58 +0400
Vasiliy Kulikov <segoon@openwall.com> wrote:

> proc_parse_options() inside of proc_mount() runs only once at the boot
> time without any given options.  So, following umount(2)+mount(2) ignore
> mount options: proc_parse_options() is not called as ->s_root is already
> initialized.  To fix that parse mount options unconditionally.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> Reported-by: Arkadiusz Mi__kiewicz <a.miskiewicz@gmail.com>
> ---
>  fs/proc/root.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 46a15d8..eed44bf 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
>  	if (IS_ERR(sb))
>  		return ERR_CAST(sb);
>  
> +	if (!proc_parse_options(options, ns)) {
> +		deactivate_locked_super(sb);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (!sb->s_root) {
>  		sb->s_flags = flags;
> -		if (!proc_parse_options(options, ns)) {
> -			deactivate_locked_super(sb);
> -			return ERR_PTR(-EINVAL);
> -		}
>  		err = proc_fill_super(sb);
>  		if (err) {
>  			deactivate_locked_super(sb);

I'm surprised.  "mount -o remount,<options>" doesn't work on a mounted
procfs, and nobody noticed until now?

The patch looks OK - has it been tested with both valid and invalid
mount options?


I redid the changelog:


From: Vasiliy Kulikov <segoon@openwall.com>
Subject: proc: fix mount -t proc -o AAA

The proc_parse_options() call from proc_mount() runs only once at boot
time.  So on any later mount attempt, any mount options are ignored
because ->s_root is already initialized.

As a consequence, "mount -o remount,<options>" will ignore the options.

To fix this, parse the mount options unconditionally.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/proc/root.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff -puN fs/proc/root.c~proc-fix-mount-t-proc-o-aaa fs/proc/root.c
--- a/fs/proc/root.c~proc-fix-mount-t-proc-o-aaa
+++ a/fs/proc/root.c
@@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct 
 	if (IS_ERR(sb))
 		return ERR_CAST(sb);
 
+	if (!proc_parse_options(options, ns)) {
+		deactivate_locked_super(sb);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (!sb->s_root) {
 		sb->s_flags = flags;
-		if (!proc_parse_options(options, ns)) {
-			deactivate_locked_super(sb);
-			return ERR_PTR(-EINVAL);
-		}
 		err = proc_fill_super(sb);
 		if (err) {
 			deactivate_locked_super(sb);
_

Re: [PATCH] proc: fix mount -t proc -o AAA

[Arkadiusz Miśkiewicz]

On Saturday 24 of March 2012, Andrew Morton wrote:
> On Fri, 23 Mar 2012 21:10:58 +0400
> 
> Vasiliy Kulikov <segoon@openwall.com> wrote:
> > proc_parse_options() inside of proc_mount() runs only once at the boot
> > time without any given options.  So, following umount(2)+mount(2) ignore
> > mount options: proc_parse_options() is not called as ->s_root is already
> > initialized.  To fix that parse mount options unconditionally.
> > 
> > Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> > Reported-by: Arkadiusz Mi__kiewicz <a.miskiewicz@gmail.com>
> > ---
> > 
> >  fs/proc/root.c |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index 46a15d8..eed44bf 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct
> > file_system_type *fs_type,
> > 
> >  	if (IS_ERR(sb))
> >  	
> >  		return ERR_CAST(sb);
> > 
> > +	if (!proc_parse_options(options, ns)) {
> > +		deactivate_locked_super(sb);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > 
> >  	if (!sb->s_root) {
> >  	
> >  		sb->s_flags = flags;
> > 
> > -		if (!proc_parse_options(options, ns)) {
> > -			deactivate_locked_super(sb);
> > -			return ERR_PTR(-EINVAL);
> > -		}
> > 
> >  		err = proc_fill_super(sb);
> >  		if (err) {
> >  		
> >  			deactivate_locked_super(sb);
> 
> I'm surprised.  "mount -o remount,<options>" doesn't work on a mounted
> procfs, and nobody noticed until now?

For me initial mount -o options didn't apply these options. Then mount -o 
remount,options applied these.

> The patch looks OK - has it been tested with both valid and invalid
> mount options?

Well, it fixes my initial case where initial mount failed to apply options.

Just tested with invalid options.

[   18.518529] proc: unrecognized mount option "crap" or missing value

but there is another problem - unmounting it and mounting without options 
causes old option to persist:

# mount none /proc -t proc -o hidepid=2
# umount /proc
# mount none /proc -t proc
# grep "/proc" /proc/mounts
none /proc proc rw,relatime,hidepid=2 0 0

There should be no hidepid=2 now.

> I redid the changelog:
> 
> 
> From: Vasiliy Kulikov <segoon@openwall.com>
> Subject: proc: fix mount -t proc -o AAA
> 
> The proc_parse_options() call from proc_mount() runs only once at boot
> time.  So on any later mount attempt, any mount options are ignored
> because ->s_root is already initialized.
> 
> As a consequence, "mount -o remount,<options>" will ignore the options.

So this changelog doesn't match what I saw.

> 
> To fix this, parse the mount options unconditionally.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/proc/root.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff -puN fs/proc/root.c~proc-fix-mount-t-proc-o-aaa fs/proc/root.c
> --- a/fs/proc/root.c~proc-fix-mount-t-proc-o-aaa
> +++ a/fs/proc/root.c
> @@ -115,12 +115,13 @@ static struct dentry *proc_mount(struct
>  	if (IS_ERR(sb))
>  		return ERR_CAST(sb);
> 
> +	if (!proc_parse_options(options, ns)) {
> +		deactivate_locked_super(sb);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (!sb->s_root) {
>  		sb->s_flags = flags;
> -		if (!proc_parse_options(options, ns)) {
> -			deactivate_locked_super(sb);
> -			return ERR_PTR(-EINVAL);
> -		}
>  		err = proc_fill_super(sb);
>  		if (err) {
>  			deactivate_locked_super(sb);
> _


-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

On Sun, Mar 25, 2012 at 09:24 +0200, Arkadiusz Miśkiewicz wrote:
> but there is another problem - unmounting it and mounting without options 
> causes old option to persist:
> 
> # mount none /proc -t proc -o hidepid=2
> # umount /proc
> # mount none /proc -t proc
> # grep "/proc" /proc/mounts
> none /proc proc rw,relatime,hidepid=2 0 0
> 
> There should be no hidepid=2 now.

No, that's an expected behaviour.

Procfs is a special filesystem.  Mount options are not reset on each
mount(2) as you can mount procfs multiple times at different mount points
(/proc/, /aaa/proc/, etc.).  Each time you add mount options they are
applied to _each_ mount point because there is no per-mount point sb, but
there is a per pid_ns superblock: pid_ns itself.  All options are stored
at pid_ns.

When you mount it another time without any option nothing should change
at the old mount points.  When you umount the last mount point all mount
options are still stored.  When you mount it again old options are used
(unless you override them).

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: fix mount -t proc -o AAA

[Arkadiusz Miśkiewicz]

On Sunday 25 of March 2012, Vasiliy Kulikov wrote:
> On Sun, Mar 25, 2012 at 09:24 +0200, Arkadiusz Miśkiewicz wrote:
> > but there is another problem - unmounting it and mounting without options
> > causes old option to persist:
> > 
> > # mount none /proc -t proc -o hidepid=2
> > # umount /proc
> > # mount none /proc -t proc
> > # grep "/proc" /proc/mounts
> > none /proc proc rw,relatime,hidepid=2 0 0
> > 
> > There should be no hidepid=2 now.
> 
> No, that's an expected behaviour.
> 
> Procfs is a special filesystem.  Mount options are not reset on each
> mount(2) as you can mount procfs multiple times at different mount points
> (/proc/, /aaa/proc/, etc.).  Each time you add mount options they are
> applied to _each_ mount point because there is no per-mount point sb, but
> there is a per pid_ns superblock: pid_ns itself.  All options are stored
> at pid_ns.
> 
> When you mount it another time without any option nothing should change
> at the old mount points.  When you umount the last mount point all mount
> options are still stored.  When you mount it again old options are used
> (unless you override them).

I wonder if it should support noxx options then (like nogid, nohidepid) ? 

-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

On Sun, Mar 25, 2012 at 19:40 +0200, Arkadiusz Miśkiewicz wrote:
> I wonder if it should support noxx options then (like nogid, nohidepid) ? 

hidepid=0 is "nohidepid".  It is described in Documentation/filesystems/proc.txt.

gid makes sense only when hidepid > 0, so there is no sense in nogid.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: fix mount -t proc -o AAA

[Valdis.Kletnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 930 bytes --]

On Sun, 25 Mar 2012 19:36:12 +0400, Vasiliy Kulikov said:
> On Sun, Mar 25, 2012 at 09:24 +0200, Arkadiusz Miśkiewicz wrote:
> > but there is another problem - unmounting it and mounting without options
> > causes old option to persist:
> >
> > # mount none /proc -t proc -o hidepid=2
> > # umount /proc
> > # mount none /proc -t proc
> > # grep "/proc" /proc/mounts
> > none /proc proc rw,relatime,hidepid=2 0 0
> >
> > There should be no hidepid=2 now.
>
> No, that's an expected behaviour.

"You keep using that word. I do not think it means what you think it means". ;)

> Procfs is a special filesystem.

And the fact it's "special" makes this *unexpected* behavior.  Are there
any other filesystems where -o values will persist across an unmount
and then take effect *even if no -o is given* on a subsequent mount?

Yes, it may be what the code actually *does*, but it certainly violates
the Principle of Least Surprise...

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: [PATCH] proc: fix mount -t proc -o AAA

[Andrew Morton]

On Sun, 25 Mar 2012 18:23:16 -0400
Valdis.Kletnieks@vt.edu wrote:

> On Sun, 25 Mar 2012 19:36:12 +0400, Vasiliy Kulikov said:
> > On Sun, Mar 25, 2012 at 09:24 +0200, Arkadiusz MiE^[kiewicz wrote:
> > > but there is another problem - unmounting it and mounting without options
> > > causes old option to persist:
> > >
> > > # mount none /proc -t proc -o hidepid=2
> > > # umount /proc
> > > # mount none /proc -t proc
> > > # grep "/proc" /proc/mounts
> > > none /proc proc rw,relatime,hidepid=2 0 0
> > >
> > > There should be no hidepid=2 now.
> >
> > No, that's an expected behaviour.
> 
> "You keep using that word. I do not think it means what you think it means". ;)
> 
> > Procfs is a special filesystem.
> 
> And the fact it's "special" makes this *unexpected* behavior.  Are there
> any other filesystems where -o values will persist across an unmount
> and then take effect *even if no -o is given* on a subsequent mount?
> 
> Yes, it may be what the code actually *does*, but it certainly violates
> the Principle of Least Surprise...

It surprises me ;)  I never noticed that before.

It does seem pretty insane.  I wonder how much downstream damage would
result from fixing it.

[PATCH] proc: reset mount options after the last procfs umount

[Vasiliy Kulikov]

On Mon, Mar 26, 2012 at 15:37 -0700, Andrew Morton wrote:
> On Sun, 25 Mar 2012 18:23:16 -0400
> Valdis.Kletnieks@vt.edu wrote:
> 
> > Yes, it may be what the code actually *does*, but it certainly violates
> > the Principle of Least Surprise...
> 
> It surprises me ;)  I never noticed that before.
> 
> It does seem pretty insane.  I wonder how much downstream damage would
> result from fixing it.

Resetting options on each mount is implemented in the following patch.
However, I'm stuck with searching a race-free way to learn current
mounts count of this procfs sb.  sget() + atomic_read() is racy:

	(A)				(B)
	mount -t proc			mount -t proc

	sget()				|
	|				sget()
	atomic_read(&sb->s_active)	|
	|				atomic_read(&sb->s_active)

So, it has a theoretical race of reading sb->s_active with 2+ parallel
mounts and both reads will get 3 instead of 2.  Consequently, neither of
mounts will reset mount options.

I wonder whether anybody will try to do such parallel type of things
in reality (IOW, is it OK to leave this race?)

--------------
From: Vasiliy Kulikov <segoon@openwall.com>
Subject: [PATCH] proc: reset mount options after the last procfs umount

The following command sequence leads to wrong mount options setting:

  mount -t proc -o hidepid=1 none /proc
  umount /proc
  mount -t proc none /proc

The second "mount" mounts procfs with old options from the first "mount",
but should use no options.

Fix that by resetting mount options in pid_namespace each time procfs
is mounted in a pid namespace without other procfs mount points.

Note that if one creates second procfs mount point, it should use old
mount options (IOW, reuse first mount point's options).

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>

--
 fs/proc/root.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

---
diff --git a/fs/proc/root.c b/fs/proc/root.c
index eed44bf..7067a5c 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -47,6 +47,12 @@ static const match_table_t tokens = {
 	{Opt_err, NULL},
 };
 
+static void proc_reset_options(struct pid_namespace *pid)
+{
+	pid->pid_gid = 0;
+	pid->hide_pid = 0;
+}
+
 static int proc_parse_options(char *options, struct pid_namespace *pid)
 {
 	char *p;
@@ -115,6 +121,9 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
 	if (IS_ERR(sb))
 		return ERR_CAST(sb);
 
+	if (atomic_read(&sb->s_active) <= 2)
+		proc_reset_options(ns);
+
 	if (!proc_parse_options(options, ns)) {
 		deactivate_locked_super(sb);
 		return ERR_PTR(-EINVAL);
--

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: reset mount options after the last procfs umount

[Arkadiusz Miśkiewicz]

On Saturday 31 of March 2012, Vasiliy Kulikov wrote:
> On Mon, Mar 26, 2012 at 15:37 -0700, Andrew Morton wrote:
> > On Sun, 25 Mar 2012 18:23:16 -0400
> > 
> > Valdis.Kletnieks@vt.edu wrote:
> > > Yes, it may be what the code actually *does*, but it certainly violates
> > > the Principle of Least Surprise...
> > 
> > It surprises me ;)  I never noticed that before.
> > 
> > It does seem pretty insane.  I wonder how much downstream damage would
> > result from fixing it.
> 
> Resetting options on each mount is implemented in the following patch.

"after all procs are umounted". For me such way is fine but still can suprise 
people.


Anyway - what's the problem with implementing support for separate options for 
each mount point?

> I wonder whether anybody will try to do such parallel type of things
> in reality (IOW, is it OK to leave this race?)

I mount multiple procs when using linux-vserver but these are currently not 
happening in parallel (but could be).

-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] proc: reset mount options after the last procfs umount

[Vasiliy Kulikov]

On Sat, Mar 31, 2012 at 16:19 +0200, Arkadiusz Miśkiewicz wrote:
> "after all procs are umounted". For me such way is fine but still can suprise 
> people.
> 
> Anyway - what's the problem with implementing support for separate options for 
> each mount point?

Well, IMHO multiple procs in one pid namespace is a very strange system
configuration.  I didn't see such installations.  Why does anybody need it?

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: reset mount options after the last procfs umount

[Arkadiusz Miśkiewicz]

On Saturday 31 of March 2012, Vasiliy Kulikov wrote:
> On Sat, Mar 31, 2012 at 16:19 +0200, Arkadiusz Miśkiewicz wrote:
> > "after all procs are umounted". For me such way is fine but still can
> > suprise people.
> > 
> > Anyway - what's the problem with implementing support for separate
> > options for each mount point?
> 
> Well, IMHO multiple procs in one pid namespace is a very strange system
> configuration.  I didn't see such installations. 

I have one real world case - linux-vserver.org guests where pid namespace is 
optional and usually not used.

> Why does anybody need it?

For me current "3.3 +  initial mount options fix" behaviour is fine anyway.

> Thanks,

-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Re: [PATCH] proc: reset mount options after the last procfs umount

[Vasiliy Kulikov]

On Sat, Mar 31, 2012 at 17:31 +0200, Arkadiusz Miśkiewicz wrote:
> On Saturday 31 of March 2012, Vasiliy Kulikov wrote:
> > On Sat, Mar 31, 2012 at 16:19 +0200, Arkadiusz Miśkiewicz wrote:
> > > "after all procs are umounted". For me such way is fine but still can
> > > suprise people.
> > > 
> > > Anyway - what's the problem with implementing support for separate
> > > options for each mount point?
> > 
> > Well, IMHO multiple procs in one pid namespace is a very strange system
> > configuration.  I didn't see such installations. 
> 
> I have one real world case - linux-vserver.org guests where pid namespace is 
> optional and usually not used.

I'm a vserver noob.  Does it allow using multiple containers sharing the same
pid namespace?  Having only a single init process?

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: fix mount -t proc -o AAA

[Andrew Morton]

On Sun, 25 Mar 2012 09:24:06 +0200
Arkadiusz Mi__kiewicz <a.miskiewicz@gmail.com> wrote:

> > I redid the changelog:
> > 
> > 
> > From: Vasiliy Kulikov <segoon@openwall.com>
> > Subject: proc: fix mount -t proc -o AAA
> > 
> > The proc_parse_options() call from proc_mount() runs only once at boot
> > time.  So on any later mount attempt, any mount options are ignored
> > because ->s_root is already initialized.
> > 
> > As a consequence, "mount -o remount,<options>" will ignore the options.
> 
> So this changelog doesn't match what I saw.

So could someone please send a changelog which *does* match what you
saw?  The original remains incomprehensible :(

Re: [PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

On Fri, Mar 23, 2012 at 16:15 -0700, Andrew Morton wrote:
> I'm surprised.  "mount -o remount,<options>" doesn't work on a mounted
> procfs, and nobody noticed until now?
> 
> The patch looks OK - has it been tested with both valid and invalid
> mount options?
> 
> 
> I redid the changelog:
> 
> 
> From: Vasiliy Kulikov <segoon@openwall.com>
> Subject: proc: fix mount -t proc -o AAA
> 
> The proc_parse_options() call from proc_mount() runs only once at boot
> time.  So on any later mount attempt, any mount options are ignored
> because ->s_root is already initialized.
> 
> As a consequence, "mount -o remount,<options>" will ignore the options.

No, remount works as it should.  _mount_ doesn't work.  Why it was not spotted:
Live case is:

1) upstart and systemd don't use /etc/fstab for /proc when mounting it at
the boot time.
2) dbus, etc. use /proc/ from the boot, so /proc cannot be umounted without
dbus stop.

So, to apply hidepid=X without system reboot procfs should be remounted
instead of umount+mount.

> To fix this, parse the mount options unconditionally.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] proc: fix mount -t proc -o AAA

[Vasiliy Kulikov]

On Fri, Mar 23, 2012 at 16:15 -0700, Andrew Morton wrote:
> From: Vasiliy Kulikov <segoon@openwall.com>
> Subject: proc: fix mount -t proc -o AAA
> 
> The proc_parse_options() call from proc_mount() runs only once at boot
> time.  So on any later mount attempt, any mount options are ignored
> because ->s_root is already initialized.
> 
> As a consequence, "mount -o remount,<options>" will ignore the options.

s/-o remount,<options>/-o <options>/

> To fix this, parse the mount options unconditionally.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>

So, it should look like:

---
The proc_parse_options() call from proc_mount() runs only once at boot
time.  So on any later mount attempt, any mount options are ignored
because ->s_root is already initialized.

As a consequence, "mount -o <options>" will ignore the options.  The only
way to change mount options is "mount -o remount,<options>".

To fix this, parse the mount options unconditionally.
---

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments