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

PATCH In review Arkadiusz Miśkiewicz 4 participants 19 messages Mar 22, 2012 – Mar 31, 2012 Proc filesystem

Share & tools

Arkadiusz Miśkiewicz

author

2012-03-22 08:03 UTC #0 lore raw

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.

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-23 17:11 UTC #1 lore raw

PATCH 1/1 proc: fix mount -t proc -o AAA

Hi Arkadiusz,

Quoted 35 lines · 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

Quoted 4 lines

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 <[email protected]>
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 <[email protected]>

Reported-by Arkadiusz Miśkiewicz <[email protected]>

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

fs/proc/root.c +5 -4

index 46a15d8..eed44bf 100644
@@ -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);

Signature · 1 line

1.7.0.4

Arkadiusz Miśkiewicz

author

2012-03-23 18:45 UTC #2 lore raw

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

Quoted 1 line · Vasiliy Kulikov

Thanks for the report. Please test the following patch.

Seems to be working fine, now - thanks!

Quoted 39 lines

--------------------------------------------------------------
From: Vasiliy Kulikov <[email protected]>
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 <[email protected]>
Reported-by Arkadiusz Miśkiewicz <[email protected]>
---
 fs/proc/root.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
fs/proc/root.c +5 -4
index 46a15d8..eed44bf 100644
@@ -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);

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-23 19:19 UTC #3 lore raw

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

Andrew,

Can you pick this patch, please?

Quoted 52 lines · Arkadiusz Miśkiewicz

Quoted 1 line · Vasiliy Kulikov
Thanks for the report. Please test the following patch.
Seems to be working fine, now - thanks!
Quoted 39 lines
--------------------------------------------------------------
From: Vasiliy Kulikov <[email protected]>
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 <[email protected]>
Reported-by Arkadiusz Miśkiewicz <[email protected]>
---
 fs/proc/root.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
fs/proc/root.c +5 -4
index 46a15d8..eed44bf 100644
@@ -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);
Signature · 2 lines
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/

Signature · 2 lines

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

Andrew Morton

2012-03-23 23:15 UTC #4 lore raw

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

In reply to Vasiliy Kulikov

Quoted 23 lines · On Fri, 23 Mar 2012 21:10:58 +0400 Vasiliy Kulikov <[email protected]> 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 <[email protected]>
Reported-by Arkadiusz Mi__kiewicz <[email protected]>
---
 fs/proc/root.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
fs/proc/root.c +4 -0
index 46a15d8..eed44bf 100644
@@ -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);
+	}

fs/proc/root.c static struct dentry *proc_mount(struct file_system_type *fs_type, · line 127

Quoted 10 lines

fs/proc/root.c +1 -4

+
 	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 <[email protected]>
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 <[email protected]>

Reported-by Arkadiusz Miskiewicz <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Al Viro <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

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

fs/proc/root.c renamed from fs/proc/root.c~proc-fix-mount-t-proc-o-aaa +5 -4

diff -puN fs/proc/root.c~proc-fix-mount-t-proc-o-aaa 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

author

2012-03-25 07:24 UTC #5 lore raw

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

Quoted 41 lines · Andrew Morton

On Fri, 23 Mar 2012 21:10:58 +0400
Quoted 38 lines · Vasiliy Kulikov
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 <[email protected]>
Reported-by Arkadiusz Mi__kiewicz <[email protected]>
---
 fs/proc/root.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
fs/proc/root.c +5 -4
index 46a15d8..eed44bf 100644
@@ -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);
-		}

Quoted 7 lines

fs/proc/root.c static struct dentry *proc_mount(struct · line 127
Quoted 4 lines
fs/proc/root.c +0 -0
 		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.

Quoted 2 lines

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.

Quoted 11 lines

I redid the changelog:
From: Vasiliy Kulikov <[email protected]>
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.

Quoted 35 lines

To fix this, parse the mount options unconditionally.
Signed-off-by Vasiliy Kulikov <[email protected]>
Reported-by Arkadiusz Miskiewicz <[email protected]>
Cc Alexey Dobriyan <[email protected]>
Cc Al Viro <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 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
fs/proc/root.c renamed from fs/proc/root.c~proc-fix-mount-t-proc-o-aaa +5 -4
@@ -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);
_

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-25 15:27 UTC #17 lore raw

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

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

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:

Quoted 8 lines

From: Vasiliy Kulikov <[email protected]>
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.

Quoted 1 line

To fix this, parse the mount options unconditionally.

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-25 15:36 UTC #6 lore raw

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

In reply to Arkadiusz Miśkiewicz

Quoted 10 lines · Arkadiusz Miśkiewicz

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,

Signature · 2 lines

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

Arkadiusz Miśkiewicz

author

2012-03-25 17:41 UTC #7 lore raw

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

Quoted 21 lines · Vasiliy Kulikov

Quoted 10 lines · Arkadiusz Miśkiewicz
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.

Quoted 4 lines

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) ?

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-25 17:49 UTC #8 lore raw

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

Quoted 1 line · Arkadiusz Miśkiewicz

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.

Signature · 2 lines

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

[email protected]

2012-03-25 22:23 UTC #9 lore raw

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

In reply to Vasiliy Kulikov

Quoted 10 lines · Vasiliy Kulikov

Quoted 9 lines · Arkadiusz MiÅ›kiewicz
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

Quoted 3 lines

Quoted 1 line
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". ;)

Quoted 1 line

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...

Andrew Morton

2012-03-26 22:35 UTC #16 lore raw

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

In reply to Arkadiusz Miśkiewicz

Quoted 6 lines · On Sun, 25 Mar 2012 09:24:06 +0200 Arkadiusz Mi__kiewicz…

Quoted 6 lines
I redid the changelog:
From: Vasiliy Kulikov <[email protected]>
Subject: proc: fix mount -t proc -o AAA

Quoted 7 lines

Quoted 5 lines
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 :(

Andrew Morton

2012-03-26 22:37 UTC #10 lore raw

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

In reply to [email protected]

Quoted 17 lines · On Sun, 25 Mar 2012 18:23:16 -0400 [email protected] wrote:

Quoted 10 lines · Vasiliy Kulikov
Quoted 9 lines · Arkadiusz MiEkiewicz
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
Quoted 3 lines
Quoted 1 line
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". ;)

Quoted 8 lines

Quoted 1 line
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.

Vasiliy Kulikov

2012-03-31 13:51 UTC #18 lore raw

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

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

From: Vasiliy Kulikov <[email protected]>
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>/

Quoted 3 lines

To fix this, parse the mount options unconditionally.
Signed-off-by Vasiliy Kulikov <[email protected]>

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,

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-31 13:55 UTC #11 lore raw

PATCH 1/1 proc: reset mount options after the last procfs umount

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

Quoted 2 lines · On Sun, 25 Mar 2012 18:23:16 -0400 [email protected] 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 <[email protected]>
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 <[email protected]>

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

---

fs/proc/root.c +9 -0

index eed44bf..7067a5c 100644
@@ -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);

Signature · 2 lines

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

Arkadiusz Miśkiewicz

author

2012-03-31 14:19 UTC #12 lore raw

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

Quoted 7 lines · Vasiliy Kulikov

Quoted 6 lines · Andrew Morton
On Sun, 25 Mar 2012 18:23:16 -0400
Quoted 2 lines · [email protected] wrote:
Yes, it may be what the code actually *does*, but it certainly violates the Principle of Least Surprise...

Quoted 6 lines

Quoted 4 lines
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?

Quoted 2 lines

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).

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-31 15:20 UTC #13 lore raw

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

Quoted 5 lines · Arkadiusz Miśkiewicz

"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,

Signature · 2 lines

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

Arkadiusz Miśkiewicz

author

2012-03-31 15:31 UTC #14 lore raw

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

Quoted 9 lines · Vasiliy Kulikov

Quoted 5 lines · Arkadiusz Miśkiewicz
"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.

Quoted 1 line

Why does anybody need it?

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

Quoted 1 line

Thanks,

Signature · 2 lines

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

Vasiliy Kulikov

2012-03-31 15:46 UTC #15 lore raw

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

Quoted 5 lines · Arkadiusz Miśkiewicz

Quoted 4 lines · Vasiliy Kulikov
Quoted 3 lines · Arkadiusz Miśkiewicz
"after all procs are umounted". For me such way is fine but still can suprise people.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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,

Signature · 2 lines

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