Re: Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer valid?

Re: Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer valid?

[kd6lvw]

--- On Thu, 5/24/12, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "D. Stussy" <spam+newsgroups@bde-arc.ampr.org> writes:
> > proc:  unrecognized mount option "uid=1" or missing value
> >
> > This works under 3.3.6 and earlier (I never tried 3.3.7).  It appears that the
> > "uid" mount option was removed.  WHY?  More importantly, where is this change
> > listed?  I didn't see it in the git log.
> >
> > This is really bad as I rebooted a remotely colocated server and since the
> > procfs is not mounted, I cannot log in via SSH to correct the problem.
> >
> > OK, so there's not that much point in changing the UID of the procfs files, but
> > I preferred to set their default ownership to a non-privileged user. 
> 
> There has never been a uid= option to /proc in Linus's tree.  I believe
> if you look you will find this feature was from a patch (perhaps from
> your distro) that added that support.

I compile both the util-linux-ng and the kernel from source.  There is no distribution patch involved.  As noted, kernel version 3.3.6 didn't bitch about the setting, while 3.4.0 does and won't mount procfs.

>From the man page (copy at "http://linux.die.net/man/8/mount"):

"Mount options for proc

     "uid=value and gid=value 

"These options are recognized, but have no effect as far as I can see."


Removal of this compatibility should have been noted.  WHERE was it noted?  Linux.die.net is a distribution independent documentation site.  It might not do anything, but per mount's documentation, it is acceptable.

Regression Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer ignored.

[Eric W. Biederman]

kd6lvw@yahoo.com writes:

> --- On Thu, 5/24/12, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "D. Stussy" <spam+newsgroups@bde-arc.ampr.org> writes:
>> > proc:  unrecognized mount option "uid=1" or missing value
>> >
>> > This works under 3.3.6 and earlier (I never tried 3.3.7).  It appears that the
>> > "uid" mount option was removed.  WHY?  More importantly, where is this change
>> > listed?  I didn't see it in the git log.
>> >
>> > This is really bad as I rebooted a remotely colocated server and since the
>> > procfs is not mounted, I cannot log in via SSH to correct the problem.
>> >
>> > OK, so there's not that much point in changing the UID of the procfs files, but
>> > I preferred to set their default ownership to a non-privileged user. 
>> 
>> There has never been a uid= option to /proc in Linus's tree.  I believe
>> if you look you will find this feature was from a patch (perhaps from
>> your distro) that added that support.
>
> I compile both the util-linux-ng and the kernel from source.  There is no distribution patch involved.  As noted, kernel version 3.3.6 didn't bitch about the setting, while 3.4.0 does and won't mount procfs.
>
> From the man page (copy at "http://linux.die.net/man/8/mount"):
>
> "Mount options for proc
>
>      "uid=value and gid=value 
>
> "These options are recognized, but have no effect as far as I can see."
>
>
> Removal of this compatibility should have been noted.  WHERE was it
> noted?  Linux.die.net is a distribution independent documentation
> site.  It might not do anything, but per mount's documentation, it is
> acceptable.

The only possible change to /proc that could have cause this behavior
between 3.3 and 3.4 is below.

Not ignoring options seems to be a regression that has affected your
setup on 3.4.

My gut feel is that we should revert all of the /proc option parsing
code as it is ugly code and apparently it could not have been used
before 3.4 so no one can be depending on it.  But I just want an excuse
to get rid of ugly code.  Accepting that /proc ignores all options
passed ot it as part of the ABI and not being able to add any options
to /proc ever is also a pretty horrible state to be in.

Shrug I will let you and the parties involved in creating this
regression figure this out.

Eric

ommit 99663be772c827b8f5f594fe87eb4807be1994e5
Author: Vasiliy Kulikov <segoon@openwall.com>
Date:   Thu Apr 5 14:25:04 2012 -0700

    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 <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.
    
    Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
    Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
    Tested-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
    Cc: Alexey Dobriyan <adobriyan@gmail.com>
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Cc: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

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

Re: Regression Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer ignored.

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]

Heh.

If it is *only* uid, maybe we should just expressly ignore it. Like
the (completely untested) attached patch.

Hmm?

                  Linus

On Fri, May 25, 2012 at 1:19 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> kd6lvw@yahoo.com writes:
>
>> --- On Thu, 5/24/12, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> "D. Stussy" <spam+newsgroups@bde-arc.ampr.org> writes:
>>> > proc:  unrecognized mount option "uid=1" or missing value
>>> >
>>> > This works under 3.3.6 and earlier (I never tried 3.3.7).  It appears that the
>>> > "uid" mount option was removed.  WHY?  More importantly, where is this change
>>> > listed?  I didn't see it in the git log.
>>> >
>>> > This is really bad as I rebooted a remotely colocated server and since the
>>> > procfs is not mounted, I cannot log in via SSH to correct the problem.
>>> >
>>> > OK, so there's not that much point in changing the UID of the procfs files, but
>>> > I preferred to set their default ownership to a non-privileged user.
>>>
>>> There has never been a uid= option to /proc in Linus's tree.  I believe
>>> if you look you will find this feature was from a patch (perhaps from
>>> your distro) that added that support.
>>
>> I compile both the util-linux-ng and the kernel from source.  There is no distribution patch involved.  As noted, kernel version 3.3.6 didn't bitch about the setting, while 3.4.0 does and won't mount procfs.
>>
>> From the man page (copy at "http://linux.die.net/man/8/mount"):
>>
>> "Mount options for proc
>>
>>      "uid=value and gid=value
>>
>> "These options are recognized, but have no effect as far as I can see."
>>
>>
>> Removal of this compatibility should have been noted.  WHERE was it
>> noted?  Linux.die.net is a distribution independent documentation
>> site.  It might not do anything, but per mount's documentation, it is
>> acceptable.
>
> The only possible change to /proc that could have cause this behavior
> between 3.3 and 3.4 is below.
>
> Not ignoring options seems to be a regression that has affected your
> setup on 3.4.
>
> My gut feel is that we should revert all of the /proc option parsing
> code as it is ugly code and apparently it could not have been used
> before 3.4 so no one can be depending on it.  But I just want an excuse
> to get rid of ugly code.  Accepting that /proc ignores all options
> passed ot it as part of the ABI and not being able to add any options
> to /proc ever is also a pretty horrible state to be in.
>
> Shrug I will let you and the parties involved in creating this
> regression figure this out.
>
> Eric
>
> ommit 99663be772c827b8f5f594fe87eb4807be1994e5
> Author: Vasiliy Kulikov <segoon@openwall.com>
> Date:   Thu Apr 5 14:25:04 2012 -0700
>
>    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 <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.
>
>    Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
>    Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
>    Tested-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
>    Cc: Alexey Dobriyan <adobriyan@gmail.com>
>    Cc: Al Viro <viro@zeniv.linux.org.uk>
>    Cc: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
>    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>
> 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) {
>

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 882 bytes --]

 fs/proc/root.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/proc/root.c b/fs/proc/root.c
index 7c30fce037c0..b1de04f6e41b 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -38,12 +38,13 @@ static int proc_set_super(struct super_block *sb, void *data)
 }
 
 enum {
-	Opt_gid, Opt_hidepid, Opt_err,
+	Opt_gid, Opt_hidepid, Opt_uid, Opt_err,
 };
 
 static const match_table_t tokens = {
 	{Opt_hidepid, "hidepid=%u"},
 	{Opt_gid, "gid=%u"},
+	{Opt_uid, "uid=%u"},	// ignored
 	{Opt_err, NULL},
 };
 
@@ -69,6 +70,11 @@ static int proc_parse_options(char *options, struct pid_namespace *pid)
 				return 0;
 			pid->pid_gid = make_kgid(current_user_ns(), option);
 			break;
+		case Opt_uid:
+			if (match_int(&args[0], &option))
+				return 0;
+			/* ignore it */
+			break;
 		case Opt_hidepid:
 			if (match_int(&args[0], &option))
 				return 0;

Re: Regression Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer ignored.

[kd6lvw]

Ignoring "uid=#" is the documented behavior from the "mount(8)" man page for procfs.  That implies that the proposed patch should be [temporarily] installed.

I personally don't care if the option is restored or not.  My issue was that this side effect was not documented in any way in the change that occurred -- which affected documented behavior.

I mount my /proc file system in a "mount -at nonfs" statement in an initialization script.  The "uid" optional parameter was in /etc/fstab.  I have removed "uid" in my "at home" copy of the configuration but haven't visited the remote server to fix it there (I can't log in from remote as a result of this issue).

--- On Fri, 5/25/12, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> If it is *only* uid, maybe we should just expressly ignore it. Like
> the (completely untested) attached patch.

Re: Regression Mounting /proc w/ kernel 3.4.0 - "uid" parameter no longer ignored.

[Linus Torvalds]

On Fri, May 25, 2012 at 5:26 PM,  <kd6lvw@yahoo.com> wrote:
>
> I personally don't care if the option is restored or not.  My issue was that this side effect was not documented in any way in the change that occurred -- which affected documented behavior.

Umm. The side effect wasn't documented, because nobody even *thought* of it.

If people had thought of it, it wouldn't have happened. So harping
about documentation is kind of pointless.

The real question is whether there might be some other option
(documented or not) that people might have used and just accidentally
relied on the fact that /proc never even bothered to look at them.

                      Linus