Re: [PATCH 2/3] uml: Create a private mount of proc for mconsole

[ebiederm@xmission.com (Eric W. Biederman)]

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