[2/3] uml: Create a private mount of proc for mconsole
diff mbox series

Message ID 87wo86qxcs.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • [1/3] uml: Don't consult current to find the proc_mnt in mconsole_proc
Related show

Commit Message

Eric W. Biederman Feb. 28, 2020, 8:18 p.m. UTC
The mconsole code only ever accesses proc for the initial pid
namespace.  Instead of depending upon the proc_mnt which is
for proc_flush_task have uml create it's own mount of proc
instead.

This allows proc_flush_task to evolve and remove the
need for having a proc_mnt to do it's job.

Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Christian Brauner Feb. 28, 2020, 8:30 p.m. UTC | #1
On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
> 
> The mconsole code only ever accesses proc for the initial pid
> namespace.  Instead of depending upon the proc_mnt which is
> for proc_flush_task have uml create it's own mount of proc
> instead.
> 
> This allows proc_flush_task to evolve and remove the
> need for having a proc_mnt to do it's job.
> 
> Cc: Jeff Dike <jdike@addtoit.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
> index e8f5c81c2c6c..30575bd92975 100644
> --- a/arch/um/drivers/mconsole_kern.c
> +++ b/arch/um/drivers/mconsole_kern.c
> @@ -36,6 +36,8 @@
>  #include "mconsole_kern.h"
>  #include <os.h>
>  
> +static struct vfsmount *proc_mnt = NULL;
> +
>  static int do_unlink_socket(struct notifier_block *notifier,
>  			    unsigned long what, void *data)
>  {
> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
>  
>  void mconsole_proc(struct mc_request *req)
>  {
> -	struct vfsmount *mnt = init_pid_ns.proc_mnt;
> +	struct vfsmount *mnt = proc_mnt;
>  	char *buf;
>  	int len;
>  	struct file *file;
> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
>  	ptr += strlen("proc");
>  	ptr = skip_spaces(ptr);
>  
> +	if (!mnt) {
> +		mconsole_reply(req, "Proc not available", 1, 0);
> +		goto out;
> +	}
>  	file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
>  	if (IS_ERR(file)) {
>  		mconsole_reply(req, "Failed to open file", 1, 0);
> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
>  	with_console(req, stack_proc, to);
>  }
>  
> +static int __init mount_proc(void)
> +{
> +	struct file_system_type *proc_fs_type;
> +	struct vfsmount *mnt;
> +
> +	proc_fs_type = get_fs_type("proc");
> +	if (!proc_fs_type)
> +		return -ENODEV;
> +
> +	mnt = kern_mount(proc_fs_type);
> +	put_filesystem(proc_fs_type);
> +	if (IS_ERR(mnt))
> +		return PTR_ERR(mnt);
> +
> +	proc_mnt = mnt;
> +	return 0;
> +}
> +
>  /*
>   * Changed by mconsole_setup, which is __setup, and called before SMP is
>   * active.
> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
>  	int err;
>  	char file[UNIX_PATH_MAX];
>  
> +	mount_proc();

Hm, either check the return value or make the mount_proc() void?
Probably worth logging something but moving on without proc.

I guess this is user visible in some scenarios but the patch series
seems worth it!

Christian
Eric W. Biederman Feb. 28, 2020, 9:28 p.m. UTC | #2
Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
>> 
>> The mconsole code only ever accesses proc for the initial pid
>> namespace.  Instead of depending upon the proc_mnt which is
>> for proc_flush_task have uml create it's own mount of proc
>> instead.
>> 
>> This allows proc_flush_task to evolve and remove the
>> need for having a proc_mnt to do it's job.
>> 
>> Cc: Jeff Dike <jdike@addtoit.com>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>> ---
>>  arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
>> index e8f5c81c2c6c..30575bd92975 100644
>> --- a/arch/um/drivers/mconsole_kern.c
>> +++ b/arch/um/drivers/mconsole_kern.c
>> @@ -36,6 +36,8 @@
>>  #include "mconsole_kern.h"
>>  #include <os.h>
>>  
>> +static struct vfsmount *proc_mnt = NULL;
>> +
>>  static int do_unlink_socket(struct notifier_block *notifier,
>>  			    unsigned long what, void *data)
>>  {
>> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
>>  
>>  void mconsole_proc(struct mc_request *req)
>>  {
>> -	struct vfsmount *mnt = init_pid_ns.proc_mnt;
>> +	struct vfsmount *mnt = proc_mnt;
>>  	char *buf;
>>  	int len;
>>  	struct file *file;
>> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
>>  	ptr += strlen("proc");
>>  	ptr = skip_spaces(ptr);
>>  
>> +	if (!mnt) {
>> +		mconsole_reply(req, "Proc not available", 1, 0);
>> +		goto out;
>> +	}
>>  	file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
>>  	if (IS_ERR(file)) {
>>  		mconsole_reply(req, "Failed to open file", 1, 0);
>> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
>>  	with_console(req, stack_proc, to);
>>  }
>>  
>> +static int __init mount_proc(void)
>> +{
>> +	struct file_system_type *proc_fs_type;
>> +	struct vfsmount *mnt;
>> +
>> +	proc_fs_type = get_fs_type("proc");
>> +	if (!proc_fs_type)
>> +		return -ENODEV;
>> +
>> +	mnt = kern_mount(proc_fs_type);
>> +	put_filesystem(proc_fs_type);
>> +	if (IS_ERR(mnt))
>> +		return PTR_ERR(mnt);
>> +
>> +	proc_mnt = mnt;
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Changed by mconsole_setup, which is __setup, and called before SMP is
>>   * active.
>> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
>>  	int err;
>>  	char file[UNIX_PATH_MAX];
>>  
>> +	mount_proc();
>
> Hm, either check the return value or make the mount_proc() void?
> Probably worth logging something but moving on without proc.

I modified mconsole_proc (the only place that cares to see if
it has a valid proc_mnt).

So the code already does the moving on without mounting proc
and continues to work.

Further the code logs something when it tries to use the mount
of proc and proc is not available.

I think this can happen if someone is strange enough to compile
the kernel without proc.  So at least in some scenarios I believe
it is expected that it will fail.

So while I think it is good form to generate good error codes in
the incredibly unlikely case that proc_mount() fails during boot
I don't see the point of doing anything with them.

> I guess this is user visible in some scenarios but the patch series
> seems worth it!

What scenarios do you think this would be user visible?

The set of calls to mount proc are slightly different, but the options
to proc when mounting (none) remain the same.

For the series as a whole the only place where it should be user visible
is when the proc mount options start getting honored.  AKA when
hidepid=N starts working as designed again.

Eric
Christian Brauner Feb. 28, 2020, 9:59 p.m. UTC | #3
On Fri, Feb 28, 2020 at 03:28:54PM -0600, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
> > On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote:
> >> 
> >> The mconsole code only ever accesses proc for the initial pid
> >> namespace.  Instead of depending upon the proc_mnt which is
> >> for proc_flush_task have uml create it's own mount of proc
> >> instead.
> >> 
> >> This allows proc_flush_task to evolve and remove the
> >> need for having a proc_mnt to do it's job.
> >> 
> >> Cc: Jeff Dike <jdike@addtoit.com>
> >> Cc: Richard Weinberger <richard@nod.at>
> >> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> >> ---
> >>  arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++-
> >>  1 file changed, 27 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
> >> index e8f5c81c2c6c..30575bd92975 100644
> >> --- a/arch/um/drivers/mconsole_kern.c
> >> +++ b/arch/um/drivers/mconsole_kern.c
> >> @@ -36,6 +36,8 @@
> >>  #include "mconsole_kern.h"
> >>  #include <os.h>
> >>  
> >> +static struct vfsmount *proc_mnt = NULL;
> >> +
> >>  static int do_unlink_socket(struct notifier_block *notifier,
> >>  			    unsigned long what, void *data)
> >>  {
> >> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req)
> >>  
> >>  void mconsole_proc(struct mc_request *req)
> >>  {
> >> -	struct vfsmount *mnt = init_pid_ns.proc_mnt;
> >> +	struct vfsmount *mnt = proc_mnt;
> >>  	char *buf;
> >>  	int len;
> >>  	struct file *file;
> >> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req)
> >>  	ptr += strlen("proc");
> >>  	ptr = skip_spaces(ptr);
> >>  
> >> +	if (!mnt) {
> >> +		mconsole_reply(req, "Proc not available", 1, 0);
> >> +		goto out;
> >> +	}
> >>  	file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
> >>  	if (IS_ERR(file)) {
> >>  		mconsole_reply(req, "Failed to open file", 1, 0);
> >> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req)
> >>  	with_console(req, stack_proc, to);
> >>  }
> >>  
> >> +static int __init mount_proc(void)
> >> +{
> >> +	struct file_system_type *proc_fs_type;
> >> +	struct vfsmount *mnt;
> >> +
> >> +	proc_fs_type = get_fs_type("proc");
> >> +	if (!proc_fs_type)
> >> +		return -ENODEV;
> >> +
> >> +	mnt = kern_mount(proc_fs_type);
> >> +	put_filesystem(proc_fs_type);
> >> +	if (IS_ERR(mnt))
> >> +		return PTR_ERR(mnt);
> >> +
> >> +	proc_mnt = mnt;
> >> +	return 0;
> >> +}
> >> +
> >>  /*
> >>   * Changed by mconsole_setup, which is __setup, and called before SMP is
> >>   * active.
> >> @@ -696,6 +720,8 @@ static int __init mconsole_init(void)
> >>  	int err;
> >>  	char file[UNIX_PATH_MAX];
> >>  
> >> +	mount_proc();
> >
> > Hm, either check the return value or make the mount_proc() void?
> > Probably worth logging something but moving on without proc.
> 
> I modified mconsole_proc (the only place that cares to see if
> it has a valid proc_mnt).
> 
> So the code already does the moving on without mounting proc
> and continues to work.

Ok, but then make mount_proc()

static void __init mount_proc(void)

and not

static int __init mount_proc(void)

like you have now. That was what I was getting it. Unless there's
another reason for this.

Christian

Patch
diff mbox series

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index e8f5c81c2c6c..30575bd92975 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -36,6 +36,8 @@ 
 #include "mconsole_kern.h"
 #include <os.h>
 
+static struct vfsmount *proc_mnt = NULL;
+
 static int do_unlink_socket(struct notifier_block *notifier,
 			    unsigned long what, void *data)
 {
@@ -123,7 +125,7 @@  void mconsole_log(struct mc_request *req)
 
 void mconsole_proc(struct mc_request *req)
 {
-	struct vfsmount *mnt = init_pid_ns.proc_mnt;
+	struct vfsmount *mnt = proc_mnt;
 	char *buf;
 	int len;
 	struct file *file;
@@ -134,6 +136,10 @@  void mconsole_proc(struct mc_request *req)
 	ptr += strlen("proc");
 	ptr = skip_spaces(ptr);
 
+	if (!mnt) {
+		mconsole_reply(req, "Proc not available", 1, 0);
+		goto out;
+	}
 	file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0);
 	if (IS_ERR(file)) {
 		mconsole_reply(req, "Failed to open file", 1, 0);
@@ -683,6 +689,24 @@  void mconsole_stack(struct mc_request *req)
 	with_console(req, stack_proc, to);
 }
 
+static int __init mount_proc(void)
+{
+	struct file_system_type *proc_fs_type;
+	struct vfsmount *mnt;
+
+	proc_fs_type = get_fs_type("proc");
+	if (!proc_fs_type)
+		return -ENODEV;
+
+	mnt = kern_mount(proc_fs_type);
+	put_filesystem(proc_fs_type);
+	if (IS_ERR(mnt))
+		return PTR_ERR(mnt);
+
+	proc_mnt = mnt;
+	return 0;
+}
+
 /*
  * Changed by mconsole_setup, which is __setup, and called before SMP is
  * active.
@@ -696,6 +720,8 @@  static int __init mconsole_init(void)
 	int err;
 	char file[UNIX_PATH_MAX];
 
+	mount_proc();
+
 	if (umid_file_name("mconsole", file, sizeof(file)))
 		return -1;
 	snprintf(mconsole_socket_name, sizeof(file), "%s", file);