[Devel] Re: [PATCH 2/2] exec: move core_pattern pipe helper into the crashing namespace
Will Drewry
wad at chromium.org
Fri Sep 17 19:34:54 PDT 2010
On Fri, Sep 17, 2010 at 1:15 PM, Neil Horman <nhorman at tuxdriver.com> wrote:
> On Fri, Sep 17, 2010 at 10:16:58AM -0500, Will Drewry wrote:
>> Presently, a core_pattern pipe endpoint will be run in the init
>> namespace. It will receive the virtual pid (task_tgid_vnr->%p) of the
>> core dumping process but will have no access to that processes /proc
>> without walking the init namespace /proc looking through all the global
>> pids until it finds the one it thinks matches.
>>
>> One option for implementing this I've already posed:
>> https://patchwork.kernel.org/patch/185912/
>> However, it's unclear if it is desirable for the core_pattern to run
>> outside the namespace. In particular, it can easily access the mounts
>> via /proc/[pid_nr]/root, but if there is a net namespace, it will not
>> have access. (Originally introduced in 2007 in commit
>> b488893a390edfe027bae7a46e9af8083e740668 )
>>
>> Instead, this change implements the more complex option two. It
>> migrates the ____call_usermodehelper() thread into the same namespaces
>> as the dumping process. It does not assign a pid in that namespace so
>> the collector will appear to be pid 0. (Using alloc_pid and change_pid
>> are options though. I avoided it because kernel_thread's returned pid
>> will then mismatch the actual threads pid.)
>>
>> Signed-off-by: Will Drewry <wad at chromium.org>
>> ---
>> fs/exec.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 828dd24..3b82607 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -51,6 +51,7 @@
>> #include <linux/audit.h>
>> #include <linux/tracehook.h>
>> #include <linux/kmod.h>
>> +#include <linux/nsproxy.h>
>> #include <linux/fsnotify.h>
>> #include <linux/fs_struct.h>
>> #include <linux/pipe_fs_i.h>
>> @@ -65,6 +66,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
>> unsigned int core_pipe_limit;
>> int suid_dumpable = 0;
>>
>> +struct coredump_pipe_params {
>> + struct coredump_params *params;
>> + struct nsproxy *nsproxy;
>> + struct fs_struct *fs;
>> +};
>> +
> Generally, I like this approach, but i think perhaps it would be better if you
> made the coredump_pipe_params struct a member of the coredump_params structure
> (perhaps within an anonymous union, so you can handle future core dump types, if
> those ever come along). That will save you needing the extra pointer to the
> coredump_params structure itself.
I'll rework it as suggested while also taking into account Oleg's
feedback (another mail coming).
Thanks!
>> /* The maximal length of core_pattern is also specified in sysctl.c */
>>
>> static LIST_HEAD(formats);
>> @@ -1819,8 +1826,34 @@ static int umh_pipe_setup(struct subprocess_info *info)
>> {
>> struct file *rp, *wp;
>> struct fdtable *fdt;
>> - struct coredump_params *cp = (struct coredump_params *)info->data;
>> - struct files_struct *cf = current->files;
>> + struct coredump_pipe_params *pipe_params =
>> + (struct coredump_pipe_params *)info->data;
> If you do the above, you can just reference this as cp->pipe_params, or
> what-not.
>
>> + struct coredump_params *cp = pipe_params->params;
>> + struct task_struct *tsk = current;
>> + struct files_struct *cf = tsk->files;
>> + struct fs_struct *cfs = tsk->fs;
>> +
>> + /* Migrate this thread into the crashing namespaces, but
>> + * don't change its pid struct to avoid breaking any other
>> + * dependencies. This will mean the process is pid=0 if it
>> + * was migrated into a pid namespace.
>> + */
>> + if (pipe_params->nsproxy && pipe_params->fs) {
>> + int kill;
>> + switch_task_namespaces(tsk, pipe_params->nsproxy);
>> + pipe_params->nsproxy = NULL;
>> +
>> + task_lock(tsk);
>> + tsk->fs = pipe_params->fs;
>> + task_unlock(tsk);
>> + pipe_params->fs = NULL;
>> + /* Clean up the previous fs struct */
>> + write_lock(&cfs->lock);
>> + kill = !--cfs->users;
>> + write_unlock(&cfs->lock);
>> + if (kill)
>> + free_fs_struct(cfs);
>> + }
>>
>> wp = create_write_pipe(0);
>> if (IS_ERR(wp))
>> @@ -1911,6 +1944,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>> if (ispipe) {
>> int dump_count;
>> char **helper_argv;
>> + struct coredump_pipe_params pipe_params = { .params = &cprm };
>>
> And you won't have to allocate this, it will just be part of the
> coredump_params.
>
>> if (cprm.limit == 1) {
>> /*
>> @@ -1950,10 +1984,26 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>> goto fail_dropcount;
>> }
>>
>> + /* Run the core_collector in the crashing namespaces */
>> + if (copy_namespaces_unattached(0, current,
>> + &pipe_params.nsproxy, &pipe_params.fs)) {
>> + printk(KERN_WARNING "%s failed to copy namespaces\n",
>> + __func__);
>> + argv_free(helper_argv);
>> + goto fail_dropcount;
>> + }
>> +
>> retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
>> NULL, UMH_WAIT_EXEC, umh_pipe_setup,
>> - NULL, &cprm);
>> + NULL, &pipe_params);
> And you can pass a pointer to coredump_params here, instead of a pointer to a
> pipe_specific coredumps, to a function that isn't specific to pipes
>
>> argv_free(helper_argv);
>> + /* nsproxy and fs will survive if call_usermodehelper_fns hits
>> + * ENOMEM prior to creating a new thread.
>> + */
>> + if (pipe_params.nsproxy)
>> + put_nsproxy(pipe_params.nsproxy);
>> + if (pipe_params.fs) /* not in use by anything else */
>> + free_fs_struct(pipe_params.fs);
>> if (retval) {
>> printk(KERN_INFO "Core dump to %s pipe failed\n",
>> corename);
>> --
>> 1.7.0.4
>>
>>
>
>
> Regards
> Neil
>
>
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
More information about the Devel
mailing list