Date:	Sat, 13 Mar 2010 12:33:48 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	andi@...stfloor.org
Subject: Re: [PATCH 1/2] exec: Rediff wrecked bits of usermodehelper
	overhaul

Neil, you confused the ordering ;)

This is "2/2", the second patch. And the next email is "1/2".


On 03/12, Neil Horman wrote:
>
> Rediff of the usermodehelper changes after all the wreckage.  This patch
> encompasses the patches that were formally in -mm as:
> 
> kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limitcleanup.patch
> 
> kmod: replace call_usermodehelper_pipe() with use of umh init function and resolve limit
> 
> The first patch in this series introduced an init function to the
> call_usermodehelper api so that processes could be customized by caller.
> This patch takes advantage of that fact, by customizing the helper in
> do_coredump to create the pipe and set its core limit to one (for our
> recusrsion check).  This lets us clean up the previous uglyness in the
> usermodehelper internals and factor call_usermodehelper out entirely.
> While I'm at it, we can also modify the helper setup to look for a core
> limit value of 1 rather than zero for our recursion check
> 
> Signed-off-by: Neil Horman <nhorman@...driver.com>
> CC: Oleg Nesterov <oleg@...hat.com>
> CC: Andi Kleen <andi@...stfloor.org>
> 
> 
>  fs/exec.c            |   63 ++++++++++++++++++++++++++++++++++-----
>  include/linux/kmod.h |    7 ----
>  kernel/kmod.c        |   82 ---------------------------------------------------
>  3 files changed, 56 insertions(+), 96 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index f351cdb..64a50b4 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1784,6 +1784,50 @@ static void wait_for_dump_helpers(struct file *file)
>  }
>  
>  
> +/*
> + * uhm_pipe_setup
> + * helper function to customize the process used
> + * to collect the core in userspace.  Specifically
> + * it sets up a pipe and installs it as fd 0 (stdin)
> + * for the process.  Returns 0 on success, or
> + * PTR_ERR on failure.
> + * Note that it also sets the core limit to 1.  This
> + * is a special value that we use to trap recursive
> + * core dumps
> + */
> +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;
> +
> +	wp = create_write_pipe(0);
> +	if (IS_ERR(wp))
> +		return PTR_ERR(wp);
> +
> +	rp = create_read_pipe(wp, 0);
> +	if (IS_ERR(rp)) {
> +		free_write_pipe(wp);
> +		return PTR_ERR(rp);
> +	}
> +
> +	cp->file = wp;
> +
> +	sys_close(0);
> +	fd_install(0, rp);
> +	spin_lock(&cf->file_lock);
> +	fdt = files_fdtable(cf);
> +	FD_SET(0, fdt->open_fds);
> +	FD_CLR(0, fdt->close_on_exec);
> +	spin_unlock(&cf->file_lock);
> +
> +	/* and disallow core files too */
> +	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
> +
> +	return 0;
> +}
> +
>  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  {
>  	struct core_state core_state;
> @@ -1871,15 +1915,15 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  		goto fail_unlock;
>  
>   	if (ispipe) {
> -		if (cprm.limit == 0) {
> +		if (cprm.limit == 1) {
>  			/*
>  			 * Normally core limits are irrelevant to pipes, since
>  			 * we're not writing to the file system, but we use
> -			 * cprm.limit of 0 here as a speacial value. Any
> -			 * non-zero limit gets set to RLIM_INFINITY below, but
> +			 * cprm.limit of 1 here as a speacial value. Any
> +			 * non-1 limit gets set to RLIM_INFINITY below, but
>  			 * a limit of 0 skips the dump.  This is a consistent
>  			 * way to catch recursive crashes.  We can still crash
> -			 * if the core_pattern binary sets RLIM_CORE =  !0
> +			 * if the core_pattern binary sets RLIM_CORE =  !1
>  			 * but it runs as root, and can do lots of stupid things
>  			 * Note that we use task_tgid_vnr here to grab the pid
>  			 * of the process group leader.  That way we get the
> @@ -1887,7 +1931,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  			 * core_pattern process dies.
>  			 */
>  			printk(KERN_WARNING
> -				"Process %d(%s) has RLIMIT_CORE set to 0\n",
> +				"Process %d(%s) has RLIMIT_CORE set to 1\n",
>  				task_tgid_vnr(current), current->comm);
>  			printk(KERN_WARNING "Aborting core\n");
>  			goto fail_unlock;
> @@ -1911,8 +1955,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  		cprm.limit = RLIM_INFINITY;
>  
>  		/* SIGPIPE can happen, but it's just never processed */
> -		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
> -				&cprm.file)) {
> +		cprm.file = NULL;
> +		if (call_usermodehelper_fns(helper_argv[0], helper_argv, NULL,
> +					    UMH_WAIT_EXEC, umh_pipe_setup,
> +					    NULL, &cprm)) {
> +			if (cprm.file)
> +				filp_close(cprm.file, NULL);
> +
>   			printk(KERN_INFO "Core dump to %s pipe failed\n",
>  			       corename);
>  			goto fail_dropcount;
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index f9edf63..5c05877 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -62,7 +62,6 @@ struct subprocess_info {
>  	char **envp;
>  	enum umh_wait wait;
>  	int retval;
> -	struct file *stdin;
>  	int (*init)(struct subprocess_info *info);
>  	void (*cleanup)(struct subprocess_info *info);
>  	void *data;
> @@ -75,8 +74,6 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>  /* Set various pieces of state into the subprocess_info structure */
>  void call_usermodehelper_setkeys(struct subprocess_info *info,
>  				 struct key *session_keyring);
> -int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
> -				  struct file **filp);
>  void call_usermodehelper_setfns(struct subprocess_info *info,
>  		    int (*init)(struct subprocess_info *info),
>  		    void (*cleanup)(struct subprocess_info *info),
> @@ -132,10 +129,6 @@ call_usermodehelper_keys(char *path, char **argv, char **envp,
>  
>  extern void usermodehelper_init(void);
>  
> -struct file;
> -extern int call_usermodehelper_pipe(char *path, char *argv[], char *envp[],
> -				    struct file **filp);
> -
>  extern int usermodehelper_disable(void);
>  extern void usermodehelper_enable(void);
>  
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 531ef62..d154454 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -147,23 +147,6 @@ static int ____call_usermodehelper(void *data)
>  	commit_creds(sub_info->cred);
>  	sub_info->cred = NULL;
>  
> -	/* Install input pipe when needed */
> -	if (sub_info->stdin) {
> -		struct files_struct *f = current->files;
> -		struct fdtable *fdt;
> -		/* no races because files should be private here */
> -		sys_close(0);
> -		fd_install(0, sub_info->stdin);
> -		spin_lock(&f->file_lock);
> -		fdt = files_fdtable(f);
> -		FD_SET(0, fdt->open_fds);
> -		FD_CLR(0, fdt->close_on_exec);
> -		spin_unlock(&f->file_lock);
> -
> -		/* and disallow core files too */
> -		current->signal->rlim[RLIMIT_CORE] = (struct rlimit){0, 0};
> -	}
> -
>  	/* We can run anywhere, unlike our parent keventd(). */
>  	set_cpus_allowed_ptr(current, cpu_all_mask);
>  
> @@ -429,35 +412,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
>  EXPORT_SYMBOL(call_usermodehelper_setfns);
>  
>  /**
> - * call_usermodehelper_stdinpipe - set up a pipe to be used for stdin
> - * @sub_info: a subprocess_info returned by call_usermodehelper_setup
> - * @filp: set to the write-end of a pipe
> - *
> - * This constructs a pipe, and sets the read end to be the stdin of the
> - * subprocess, and returns the write-end in *@...p.
> - */
> -int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
> -				  struct file **filp)
> -{
> -	struct file *f;
> -
> -	f = create_write_pipe(0);
> -	if (IS_ERR(f))
> -		return PTR_ERR(f);
> -	*filp = f;
> -
> -	f = create_read_pipe(f, 0);
> -	if (IS_ERR(f)) {
> -		free_write_pipe(*filp);
> -		return PTR_ERR(f);
> -	}
> -	sub_info->stdin = f;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(call_usermodehelper_stdinpipe);
> -
> -/**
>   * call_usermodehelper_exec - start a usermode application
>   * @sub_info: information about the subprocessa
>   * @wait: wait for the application to finish and return status.
> @@ -504,42 +458,6 @@ unlock:
>  }
>  EXPORT_SYMBOL(call_usermodehelper_exec);
>  
> -/**
> - * call_usermodehelper_pipe - call a usermode helper process with a pipe stdin
> - * @path: path to usermode executable
> - * @argv: arg vector for process
> - * @envp: environment for process
> - * @filp: set to the write-end of a pipe
> - *
> - * This is a simple wrapper which executes a usermode-helper function
> - * with a pipe as stdin.  It is implemented entirely in terms of
> - * lower-level call_usermodehelper_* functions.
> - */
> -int call_usermodehelper_pipe(char *path, char **argv, char **envp,
> -			     struct file **filp)
> -{
> -	struct subprocess_info *sub_info;
> -	int ret;
> -
> -	sub_info = call_usermodehelper_setup(path, argv, envp,
> -					     GFP_KERNEL);
> -	if (sub_info == NULL)
> -		return -ENOMEM;
> -
> -	ret = call_usermodehelper_stdinpipe(sub_info, filp);
> -	if (ret < 0) {
> -		call_usermodehelper_freeinfo(sub_info);
> -		return ret;
> -	}
> -
> -	ret = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
> -	if (ret < 0)	/* Failed to execute helper, close pipe */
> -		filp_close(*filp, NULL);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL(call_usermodehelper_pipe);
> -
>  void __init usermodehelper_init(void)
>  {
>  	khelper_wq = create_singlethread_workqueue("khelper");

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists