| Message ID | 87wo86qxcs.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] uml: Don't consult current to find the proc_mnt in mconsole_proc | expand |
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
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
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
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);
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(-)