Re: [PATCH 2/2] exec: move core_pattern pipe helper into the crashing namespace

[Neil Horman <nhorman@tuxdriver.com>]

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@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.

>  /* 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