[PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0

[PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0

[Neil Horman]

Hey all-
	So, about 6 months ago, I made a set of changes to how the
core-dump-to-a-pipe feature in the kernel works.  We had reports of several
races, including some reports of apps bypassing our recursion check so that a
process that was forked as part of a core_pattern setup could infinitely crash
and refork until the system crashed.

	We fixes those by improving our recursion checks.  The new check
basically refuses to fork a process if its core limit is zero, which works well.

	Unfortunately, I've been getting grief from maintainer of user space
programs that are inserted as the forked process of core_pattern.  They contend
that in order for their programs (such as abrt and apport) to work, all the
running processes in a system must have their core limits set to a non-zero
value, to which I say 'yes'.  I did this by design, and think thats the right
way to do things.

	But I've been asked to ease this burden on user space enough times that
I thought I would take a look at it.  The first suggestion was to make the
recursion check fail on a non-zero 'special' number, like one.  That way the
core collector process could set its core size ulimit to 1, and enable the
kernel's recursion detection.  This isn't a bad idea on the surface, but I don't
like it since its opt-in, in that if a program like abrt or apport has a bug and
fails to set such a core limit, we're left with a recursively crashing system
again.

	So I've come up with this.  What I've done is modify the
call_usermodehelper api such that an extra parameter is added, a function
pointer which will be called by the user helper task, after it forks, but before
it exec's the required process.  This will give the caller the opportunity to
get a call back in the processes context, allowing it to do whatever it needs to
to the process in the kernel prior to exec-ing the user space code.  In the case
of do_coredump, this callback is ues to set the core ulimit of the helper
process to 1.  This elimnates the opt-in problem that I had above, as it allows
the ulimit for core sizes to be set to the value of 1, which is what the
recursion check looks for in do_coredump.

	This patch has been tested successfully by some of the Abrt maintainers
in fedora, with good results.  Patch applies to the latest -mm tree as of this
AM.

Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Jiri Moskovcak <jmoskovc@redhat.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: drbd-dev@lists.linbit.com
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Thomas Sailer <t.sailer@alumni.ethz.ch>
CC: Adam Belay <abelay@mit.edu>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Michal Januszewski <spock@gentoo.org>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Neil Brown <neilb@suse.de>
CC: Mark Fasheh <mfasheh@suse.com>
CC: Paul Menage <menage@google.com>
CC: Stephen Hemminger <shemminger@linux-foundation.org>
CC: Kentaro Takeda <takedakn@nttdata.co.jp>

 arch/x86/kernel/cpu/mcheck/mce.c       |    2 +-
 drivers/block/drbd/drbd_nl.c           |    2 +-
 drivers/macintosh/therm_pm72.c         |    2 +-
 drivers/macintosh/windfarm_core.c      |    2 +-
 drivers/net/hamradio/baycom_epp.c      |    2 +-
 drivers/pnp/pnpbios/core.c             |    2 +-
 drivers/staging/rtl8187se/r8180_core.c |    2 +-
 drivers/video/uvesafb.c                |    2 +-
 fs/exec.c                              |   18 +++++++++++++++---
 fs/nfs/cache_lib.c                     |    2 +-
 fs/ocfs2/stackglue.c                   |    2 +-
 include/linux/kmod.h                   |   16 ++++++++++------
 kernel/cgroup.c                        |    2 +-
 kernel/kmod.c                          |   17 +++++++++++++----
 kernel/sys.c                           |    2 +-
 lib/kobject_uevent.c                   |    2 +-
 net/bridge/br_stp_if.c                 |    4 ++--
 security/keys/request_key.c            |    2 +-
 security/tomoyo/common.c               |    2 +-
 19 files changed, 55 insertions(+), 30 deletions(-)


diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a8aacd4..1f59e0d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1164,7 +1164,7 @@ static void mce_start_timer(unsigned long data)
 
 static void mce_do_trigger(struct work_struct *work)
 {
-	call_usermodehelper(mce_helper, mce_helper_argv, NULL, UMH_NO_WAIT);
+	call_usermodehelper(mce_helper, mce_helper_argv, NULL, NULL, UMH_NO_WAIT);
 }
 
 static DECLARE_WORK(mce_trigger_work, mce_do_trigger);
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 4e0726a..ae2719b 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -172,7 +172,7 @@ int drbd_khelper(struct drbd_conf *mdev, char *cmd)
 	dev_info(DEV, "helper command: %s %s %s\n", usermode_helper, cmd, mb);
 
 	drbd_bcast_ev_helper(mdev, cmd);
-	ret = call_usermodehelper(usermode_helper, argv, envp, 1);
+	ret = call_usermodehelper(usermode_helper, argv, envp, NULL, 1);
 	if (ret)
 		dev_warn(DEV, "helper command: %s %s %s exit code %u (0x%x)\n",
 				usermode_helper, cmd, mb,
diff --git a/drivers/macintosh/therm_pm72.c b/drivers/macintosh/therm_pm72.c
index 454bc50..899f895 100644
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -1763,7 +1763,7 @@ static int call_critical_overtemp(void)
 				NULL };
 
 	return call_usermodehelper(critical_overtemp_path,
-				   argv, envp, UMH_WAIT_EXEC);
+				   argv, envp, NULL, UMH_WAIT_EXEC);
 }
 
 
diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
index 075b4d9..640c856 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -81,7 +81,7 @@ int wf_critical_overtemp(void)
 				NULL };
 
 	return call_usermodehelper(critical_overtemp_path,
-				   argv, envp, UMH_WAIT_EXEC);
+				   argv, envp, NULL, UMH_WAIT_EXEC);
 }
 EXPORT_SYMBOL_GPL(wf_critical_overtemp);
 
diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c
index a3c0dc9..daa2097 100644
--- a/drivers/net/hamradio/baycom_epp.c
+++ b/drivers/net/hamradio/baycom_epp.c
@@ -320,7 +320,7 @@ static int eppconfig(struct baycom_state *bc)
 	sprintf(portarg, "%ld", bc->pdev->port->base);
 	printk(KERN_DEBUG "%s: %s -s -p %s -m %s\n", bc_drvname, eppconfig_path, portarg, modearg);
 
-	return call_usermodehelper(eppconfig_path, argv, envp, UMH_WAIT_PROC);
+	return call_usermodehelper(eppconfig_path, argv, envp, NULL, UMH_WAIT_PROC);
 }
 
 /* ---------------------------------------------------------------------- */
diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
index cfe8685..00bb172 100644
--- a/drivers/pnp/pnpbios/core.c
+++ b/drivers/pnp/pnpbios/core.c
@@ -142,7 +142,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
 			   info->location_id, info->serial, info->capabilities);
 	envp[i] = NULL;
 
-	value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
+	value = call_usermodehelper(argv [0], argv, envp, NULL, UMH_WAIT_EXEC);
 	kfree(buf);
 	kfree(envp);
 	return 0;
diff --git a/drivers/staging/rtl8187se/r8180_core.c b/drivers/staging/rtl8187se/r8180_core.c
index e0f13ef..aaafc00 100644
--- a/drivers/staging/rtl8187se/r8180_core.c
+++ b/drivers/staging/rtl8187se/r8180_core.c
@@ -4287,7 +4287,7 @@ void GPIOChangeRFWorkItemCallBack(struct work_struct *work)
                                 argv[0] = RadioPowerPath;
                                 argv[2] = NULL;
 
-                                call_usermodehelper(RadioPowerPath,argv,envp,1);
+                                call_usermodehelper(RadioPowerPath,argv,envp,NULL,1);
 			}
 		}
 }
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 54fbb29..188aea3 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -120,7 +120,7 @@ static int uvesafb_helper_start(void)
 		NULL,
 	};
 
-	return call_usermodehelper(v86d_path, argv, envp, 1);
+	return call_usermodehelper(v86d_path, argv, envp, NULL, 1);
 }
 
 /*
diff --git a/fs/exec.c b/fs/exec.c
index 632b02e..3f2f829 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1755,6 +1755,18 @@ static void wait_for_dump_helpers(struct file *file)
 
 }
 
+/*
+ * This is used as a helper to set up the task that execs
+ * our user space core collector application
+ * Its called in the context of the task thats going to 
+ * exec itself to be the helper, so we can modify current here
+ */
+void core_pipe_setup(void)
+{
+	task_lock(current->group_leader);
+	current->signal->rlim[RLIMIT_CORE].rlim_cur = 1;
+	task_unlock(current->group_leader);
+}
 
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
@@ -1836,7 +1848,7 @@ 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
@@ -1852,7 +1864,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;
@@ -1877,7 +1889,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 
 		/* SIGPIPE can happen, but it's just never processed */
 		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
-				&cprm.file)) {
+				&cprm.file, core_pipe_setup)) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       corename);
 			goto fail_dropcount;
diff --git a/fs/nfs/cache_lib.c b/fs/nfs/cache_lib.c
index b4ffd01..73d79e9 100644
--- a/fs/nfs/cache_lib.c
+++ b/fs/nfs/cache_lib.c
@@ -46,7 +46,7 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
 
 	if (nfs_cache_getent_prog[0] == '\0')
 		goto out;
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+	ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_EXEC);
 	/*
 	 * Disable the upcall mechanism if we're getting an ENOENT or
 	 * EACCES error. The admin can re-enable it on the fly by using
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index f3df0ba..dddf780 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -407,7 +407,7 @@ static void ocfs2_leave_group(const char *group)
 	envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
 	envp[2] = NULL;
 
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_PROC);
 	if (ret < 0) {
 		printk(KERN_ERR
 		       "ocfs2: Error %d running user helper "
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 384ca8b..ca5e531 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -48,7 +48,9 @@ struct subprocess_info;
 
 /* Allocate a subprocess_info structure */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask);
+						  char **envp, 
+						  void (*finit)(void),
+						  gfp_t gfp_mask);
 
 /* Set various pieces of state into the subprocess_info structure */
 void call_usermodehelper_setkeys(struct subprocess_info *info,
@@ -72,12 +74,13 @@ int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
 void call_usermodehelper_freeinfo(struct subprocess_info *info);
 
 static inline int
-call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
+call_usermodehelper(char *path, char **argv, char **envp, 
+		    void (*finit)(void), enum umh_wait wait)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+	info = call_usermodehelper_setup(path, argv, envp, finit, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
 	return call_usermodehelper_exec(info, wait);
@@ -85,12 +88,13 @@ call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 
 static inline int
 call_usermodehelper_keys(char *path, char **argv, char **envp,
-			 struct key *session_keyring, enum umh_wait wait)
+			 struct key *session_keyring, 
+			 void (*finit)(void), enum umh_wait wait)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+	info = call_usermodehelper_setup(path, argv, envp, finit, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
 
@@ -102,7 +106,7 @@ extern void usermodehelper_init(void);
 
 struct file;
 extern int call_usermodehelper_pipe(char *path, char *argv[], char *envp[],
-				    struct file **filp);
+				    struct file **filp, void (*finit)(void));
 
 extern int usermodehelper_disable(void);
 extern void usermodehelper_enable(void);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 1fbcc74..2dcbd51 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3780,7 +3780,7 @@ static void cgroup_release_agent(struct work_struct *work)
 		 * since the exec could involve hitting disk and hence
 		 * be a slow process */
 		mutex_unlock(&cgroup_mutex);
-		call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+		call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_EXEC);
 		mutex_lock(&cgroup_mutex);
  continue_free:
 		kfree(pathbuf);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bf0e231..c80db3c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -35,6 +35,7 @@
 #include <linux/resource.h>
 #include <linux/notifier.h>
 #include <linux/suspend.h>
+#include <linux/gfp.h>
 #include <asm/uaccess.h>
 
 #include <trace/events/module.h>
@@ -117,7 +118,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	trace_module_request(module_name, wait, _RET_IP_);
 
 	ret = call_usermodehelper(modprobe_path, argv, envp,
-			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
+			NULL, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 	atomic_dec(&kmod_concurrent);
 	return ret;
 }
@@ -134,6 +135,7 @@ struct subprocess_info {
 	enum umh_wait wait;
 	int retval;
 	struct file *stdin;
+	void (*finit)(void);
 	void (*cleanup)(char **argv, char **envp);
 };
 
@@ -184,6 +186,9 @@ static int ____call_usermodehelper(void *data)
 	 */
 	set_user_nice(current, 0);
 
+	if (sub_info->finit)
+		sub_info->finit();
+
 	retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
 
 	/* Exec failed? */
@@ -365,7 +370,9 @@ static inline void helper_unlock(void) {}
  * exec the process and free the structure.
  */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask)
+						  char **envp, 
+						  void (*finit)(void),
+						  gfp_t gfp_mask)
 {
 	struct subprocess_info *sub_info;
 	sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
@@ -381,6 +388,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 		kfree(sub_info);
 		return NULL;
 	}
+	sub_info->finit = finit;
 
   out:
 	return sub_info;
@@ -510,12 +518,13 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
  * lower-level call_usermodehelper_* functions.
  */
 int call_usermodehelper_pipe(char *path, char **argv, char **envp,
-			     struct file **filp)
+			     struct file **filp,
+			     void (*finit)(void))
 {
 	struct subprocess_info *sub_info;
 	int ret;
 
-	sub_info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL);
+	sub_info = call_usermodehelper_setup(path, argv, envp, finit, GFP_KERNEL);
 	if (sub_info == NULL)
 		return -ENOMEM;
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 26a6b73..0ad1306 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1629,7 +1629,7 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	info = call_usermodehelper_setup(argv[0], argv, envp, GFP_ATOMIC);
+	info = call_usermodehelper_setup(argv[0], argv, envp, NULL, GFP_ATOMIC);
 	if (info == NULL) {
 		argv_free(argv);
 		goto out;
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 920a3ca..df55126 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -258,7 +258,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 			goto exit;
 
 		retval = call_usermodehelper(argv[0], argv,
-					     env->envp, UMH_WAIT_EXEC);
+					     env->envp, NULL, UMH_WAIT_EXEC);
 	}
 
 exit:
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 9a52ac5..9d75e7d 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -123,7 +123,7 @@ static void br_stp_start(struct net_bridge *br)
 	char *argv[] = { BR_STP_PROG, br->dev->name, "start", NULL };
 	char *envp[] = { NULL };
 
-	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+	r = call_usermodehelper(BR_STP_PROG, argv, envp, NULL, UMH_WAIT_PROC);
 	if (r == 0) {
 		br->stp_enabled = BR_USER_STP;
 		printk(KERN_INFO "%s: userspace STP started\n", br->dev->name);
@@ -146,7 +146,7 @@ static void br_stp_stop(struct net_bridge *br)
 	char *envp[] = { NULL };
 
 	if (br->stp_enabled == BR_USER_STP) {
-		r = call_usermodehelper(BR_STP_PROG, argv, envp, 1);
+		r = call_usermodehelper(BR_STP_PROG, argv, envp, NULL, 1);
 		printk(KERN_INFO "%s: userspace STP stopped, return code %d\n",
 			br->dev->name, r);
 
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 03fe63e..f929b69 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -139,7 +139,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 
 	/* do it */
 	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
-				       UMH_WAIT_PROC);
+				       NULL, UMH_WAIT_PROC);
 	kdebug("usermode -> 0x%x", ret);
 	if (ret >= 0) {
 		/* ret is the exit/wait code */
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index e0d0354..4aeefd7 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -1882,7 +1882,7 @@ void tomoyo_load_policy(const char *filename)
 	envp[0] = "HOME=/";
 	envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
 	envp[2] = NULL;
-	call_usermodehelper(argv[0], argv, envp, 1);
+	call_usermodehelper(argv[0], argv, envp, NULL, 1);
 
 	printk(KERN_INFO "TOMOYO: 2.2.0   2009/04/01\n");
 	printk(KERN_INFO "Mandatory Access Control activated.\n");

Re: [PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0

[Thomas Sailer]

Acked-by: Thomas Sailer <t.sailer@alumni.ethz.ch>

Re: [PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0

[Neil Horman]

On Thu, Jan 21, 2010 at 10:29:17PM +0100, Thomas Sailer wrote:
> Acked-by: Thomas Sailer <t.sailer@alumni.ethz.ch>
> 
Thank you, anyone else have thoughts on this?

Regards
Neil

Re: [PATCH] exec: allow core_pipe recursion check to look for a value of 1 rather than 0

[Andrew Morton]

On Thu, 21 Jan 2010 15:08:06 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

> Hey all-
> 	So, about 6 months ago, I made a set of changes to how the
> core-dump-to-a-pipe feature in the kernel works.  We had reports of several
> races, including some reports of apps bypassing our recursion check so that a
> process that was forked as part of a core_pattern setup could infinitely crash
> and refork until the system crashed.
> 
> 	We fixes those by improving our recursion checks.  The new check
> basically refuses to fork a process if its core limit is zero, which works well.
> 
> 	Unfortunately, I've been getting grief from maintainer of user space
> programs that are inserted as the forked process of core_pattern.  They contend
> that in order for their programs (such as abrt and apport) to work, all the
> running processes in a system must have their core limits set to a non-zero
> value, to which I say 'yes'.  I did this by design, and think thats the right
> way to do things.
> 
> 	But I've been asked to ease this burden on user space enough times that
> I thought I would take a look at it.  The first suggestion was to make the
> recursion check fail on a non-zero 'special' number, like one.  That way the
> core collector process could set its core size ulimit to 1, and enable the
> kernel's recursion detection.  This isn't a bad idea on the surface, but I don't
> like it since its opt-in, in that if a program like abrt or apport has a bug and
> fails to set such a core limit, we're left with a recursively crashing system
> again.
> 
> 	So I've come up with this.  What I've done is modify the
> call_usermodehelper api such that an extra parameter is added, a function
> pointer which will be called by the user helper task, after it forks, but before
> it exec's the required process.  This will give the caller the opportunity to
> get a call back in the processes context, allowing it to do whatever it needs to
> to the process in the kernel prior to exec-ing the user space code.  In the case
> of do_coredump, this callback is ues to set the core ulimit of the helper
> process to 1.  This elimnates the opt-in problem that I had above, as it allows
> the ulimit for core sizes to be set to the value of 1, which is what the
> recursion check looks for in do_coredump.
> 
> 	This patch has been tested successfully by some of the Abrt maintainers
> in fedora, with good results.  Patch applies to the latest -mm tree as of this
> AM.
> 

hrm.  Fair enough, I guess..

> +/*
> + * This is used as a helper to set up the task that execs
> + * our user space core collector application
> + * Its called in the context of the task thats going to 
> + * exec itself to be the helper, so we can modify current here
> + */

It's worth spending another 15 seconds on the comments.  That way, they
end up looking like they're written in English.

> +void core_pipe_setup(void)
> +{
> +	task_lock(current->group_leader);
> +	current->signal->rlim[RLIMIT_CORE].rlim_cur = 1;
> +	task_unlock(current->group_leader);
> +}

I'll make this static.

> --- a/fs/nfs/cache_lib.c
> +++ b/fs/nfs/cache_lib.c
> @@ -46,7 +46,7 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
>  
>  	if (nfs_cache_getent_prog[0] == '\0')
>  		goto out;
> -	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> +	ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_EXEC);
>  	/*
>  	 * Disable the upcall mechanism if we're getting an ENOENT or
>  	 * EACCES error. The admin can re-enable it on the fly by using
> diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> index f3df0ba..dddf780 100644
> --- a/fs/ocfs2/stackglue.c
> +++ b/fs/ocfs2/stackglue.c
> @@ -407,7 +407,7 @@ static void ocfs2_leave_group(const char *group)
>  	envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
>  	envp[2] = NULL;
>  
> -	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> +	ret = call_usermodehelper(argv[0], argv, envp, NULL, UMH_WAIT_PROC);
>  	if (ret < 0) {
>  		printk(KERN_ERR
>  		       "ocfs2: Error %d running user helper "
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 384ca8b..ca5e531 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -48,7 +48,9 @@ struct subprocess_info;
>  
>  /* Allocate a subprocess_info structure */
>  struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> -						  char **envp, gfp_t gfp_mask);
> +						  char **envp, 
> +						  void (*finit)(void),
> +						  gfp_t gfp_mask);

What does "finit" mean?  Doesn't seem very intuitive.

>  /* Set various pieces of state into the subprocess_info structure */
>  void call_usermodehelper_setkeys(struct subprocess_info *info,
> @@ -72,12 +74,13 @@ int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
>  void call_usermodehelper_freeinfo(struct subprocess_info *info);
>  
>  static inline int
> -call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
> +call_usermodehelper(char *path, char **argv, char **envp, 
> +		    void (*finit)(void), enum umh_wait wait)
>  {
>  	struct subprocess_info *info;
>  	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>  
> -	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
> +	info = call_usermodehelper_setup(path, argv, envp, finit, gfp_mask);
>  	if (info == NULL)
>  		return -ENOMEM;
>  	return call_usermodehelper_exec(info, wait);

The semantics of the `finit' callback should be documented here.  It's
a kernel-wide interface and others might want to use it.


You're not a big fan of checkpatch, it seems.

Re: [PATCH 0/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

Ok, version two of this patch.  I've cleaned up the comments, checkpatched,
rediffed to the latest -mm, etc.  More interestingly, I've taken Olegs changes
into account in this version of the patch.  By modifying Andi's work slightly,
I've introduced a new init fptr to the usermodehelper api, which lets a caller
customize the process that will be doing the helping.  In the case of the
do_coredump path its now used to create the read/write pipes.  This allows us to
remove the stdin specifics from the usermodehelper internals and factor our
call_usermodehelper_pipe entirely.  I've tested this myself using abrt on
Fedora, with good results

Summary:
	So, about 6 months ago, I made a set of changes to how the
core-dump-to-a-pipe feature in the kernel works.  We had reports of several
races, including some reports of apps bypassing our recursion check so that a
process that was forked as part of a core_pattern setup could infinitely crash
and refork until the system crashed.

	We fixes those by improving our recursion checks.  The new check
basically refuses to fork a process if its core limit is zero, which works well.

	Unfortunately, I've been getting grief from maintainer of user space
programs that are inserted as the forked process of core_pattern.  They contend
that in order for their programs (such as abrt and apport) to work, all the
running processes in a system must have their core limits set to a non-zero
value, to which I say 'yes'.  I did this by design, and think thats the right
way to do things.

	But I've been asked to ease this burden on user space enough times that
I thought I would take a look at it.  The first suggestion was to make the
recursion check fail on a non-zero 'special' number, like one.  That way the
core collector process could set its core size ulimit to 1, and enable the
kernel's recursion detection.  This isn't a bad idea on the surface, but I don't
like it since its opt-in, in that if a program like abrt or apport has a bug and
fails to set such a core limit, we're left with a recursively crashing system
again.

	So I've come up with this.  What I've done is modify the
call_usermodehelper api such that an extra parameter is added, a function
pointer which will be called by the user helper task, after it forks, but before
it exec's the required process.  This will give the caller the opportunity to
get a call back in the processes context, allowing it to do whatever it needs to
to the process in the kernel prior to exec-ing the user space code.  In the case
of do_coredump, this callback is ues to set the core ulimit of the helper
process to 1.  This elimnates the opt-in problem that I had above, as it allows
the ulimit for core sizes to be set to the value of 1, which is what the
recursion check looks for in do_coredump.

	This patch has been tested successfully by some of the Abrt maintainers
in fedora, with good results.  Patch applies to the latest -mm tree as of this
AM.

Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Neil Horman <nhorman@redhat.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: drbd-dev@lists.linbit.com
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Thomas Sailer <t.sailer@alumni.ethz.ch>
CC: Adam Belay <abelay@mit.edu>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Michal Januszewski <spock@gentoo.org>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Neil Brown <neilb@suse.de>
CC: Mark Fasheh <mfasheh@suse.com>
CC: Paul Menage <menage@google.com>
CC: Stephen Hemminger <shemminger@linux-foundation.org>
CC: Kentaro Takeda <takedakn@nttdata.co.jp>
CC: Oleg Nesterov <oleg@redhat.com

Re: [PATCH 1/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

Add init function to usermodehelper

Convert call_usermodehelper_cleanup to usermodehelper_fns and allow it to assign
both an init function and a cleanup function.  The init function is called from
the context of the forked process and allows for customization of the helper
process prior to calling exec.  Its return code gates the continuation of the
process, or causes its exit.  Also add an arbitrary data pointer to the
subprocess_info struct allowing for data to be passed from the caller to the new
process, and the subsequent cleanup process

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 include/linux/kmod.h |   21 +++++++++++++-------
 kernel/kmod.c        |   53 ++++++++++++++++++++++++++++++++++++++++++---------
 kernel/sys.c         |    4 +--
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 25d227c..54db3ff 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -48,7 +48,8 @@ struct subprocess_info;
 
 /* Allocate a subprocess_info structure */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask);
+						  char **envp, void *data,
+						  gfp_t gfp_mask);
 
 /* Set various pieces of state into the subprocess_info structure */
 void call_usermodehelper_setkeys(struct subprocess_info *info,
@@ -56,7 +57,9 @@ void call_usermodehelper_setkeys(struct subprocess_info *info,
 int call_usermodehelper_stdinpipe(struct subprocess_info *sub_info,
 				  struct file **filp);
 void call_usermodehelper_setcleanup(struct subprocess_info *info,
-				    void (*cleanup)(char **argv, char **envp));
+		    void (*cleanup)(char **argv, char **envp, void *data));
+void call_usermodehelper_setinit(struct subprocess_info *info,
+				 int (*init)(void *data));
 
 enum umh_wait {
 	UMH_NO_WAIT = -1,	/* don't wait at all */
@@ -72,15 +75,18 @@ int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
 void call_usermodehelper_freeinfo(struct subprocess_info *info);
 
 static inline int
-call_usermodehelper_cleanup(char *path, char **argv, char **envp, enum umh_wait wait,
-			    void (*cleanup)(char **, char **))
+call_usermodehelper_fns(char *path, char **argv, char **envp,
+		    enum umh_wait wait, int (*init)(void *data),
+		    void (*cleanup)(char **, char **, void *data), void *data)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+	info = call_usermodehelper_setup(path, argv, envp, data, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
+	if (init)
+		call_usermodehelper_setinit(info, init);
 	if (cleanup)
 		call_usermodehelper_setcleanup(info, cleanup);
 	return call_usermodehelper_exec(info, wait);
@@ -89,7 +95,8 @@ call_usermodehelper_cleanup(char *path, char **argv, char **envp, enum umh_wait
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 {
-	return call_usermodehelper_cleanup(path, argv, envp, wait, NULL);
+	return call_usermodehelper_fns(path, argv, envp,
+				       wait, NULL, NULL, NULL);
 }
 
 static inline int
@@ -99,7 +106,7 @@ call_usermodehelper_keys(char *path, char **argv, char **envp,
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+	info = call_usermodehelper_setup(path, argv, envp, NULL, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2db0689..cab23a8 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -51,7 +51,7 @@ static struct workqueue_struct *khelper_wq;
 */
 char *modprobe_path = "/sbin/modprobe";
 
-static void free_arg(char **argv, char **env)
+static void free_arg(char **argv, char **env, void *unused)
 {
 	kfree(argv[0]);
 }
@@ -133,8 +133,9 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	trace_module_request(module_name, wait, _RET_IP_);
 
-	ret = call_usermodehelper_cleanup(mp_copy, argv, envp,
-			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC, free_arg);
+	ret = call_usermodehelper_fns(mp_copy, argv, envp,
+			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC,
+			NULL, free_arg, NULL);
 	mp_copy = NULL; /* free_arg frees */
 	atomic_dec(&kmod_concurrent);
 error:
@@ -154,7 +155,9 @@ struct subprocess_info {
 	enum umh_wait wait;
 	int retval;
 	struct file *stdin;
-	void (*cleanup)(char **argv, char **envp);
+	int (*init)(void *data);
+	void (*cleanup)(char **argv, char **envp, void *data);
+	void *data;
 };
 
 /*
@@ -198,6 +201,12 @@ static int ____call_usermodehelper(void *data)
 	/* We can run anywhere, unlike our parent keventd(). */
 	set_cpus_allowed_ptr(current, cpu_all_mask);
 
+	if (sub_info->init) {
+		retval = sub_info->init(sub_info->data);
+		if (retval)
+			goto fail;
+	}
+
 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
 	 * Avoid propagating that into the userspace child.
@@ -207,6 +216,7 @@ static int ____call_usermodehelper(void *data)
 	retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
 
 	/* Exec failed? */
+fail:
 	sub_info->retval = retval;
 	do_exit(0);
 }
@@ -214,7 +224,7 @@ static int ____call_usermodehelper(void *data)
 void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
-		(*info->cleanup)(info->argv, info->envp);
+		(*info->cleanup)(info->argv, info->envp, info->data);
 	if (info->cred)
 		put_cred(info->cred);
 	kfree(info);
@@ -385,7 +395,8 @@ static inline void helper_unlock(void) {}
  * exec the process and free the structure.
  */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask)
+						  char **envp, void *data,
+						  gfp_t gfp_mask)
 {
 	struct subprocess_info *sub_info;
 	sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
@@ -396,6 +407,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	sub_info->path = path;
 	sub_info->argv = argv;
 	sub_info->envp = envp;
+	sub_info->data = data;
 	sub_info->cred = prepare_usermodehelper_creds();
 	if (!sub_info->cred) {
 		kfree(sub_info);
@@ -430,19 +442,41 @@ EXPORT_SYMBOL(call_usermodehelper_setkeys);
  * @info: a subprocess_info returned by call_usermodehelper_setup
  * @cleanup: a cleanup function
  *
- * The cleanup function is just befor ethe subprocess_info is about to
+ * The cleanup function is just before ethe subprocess_info is about to
  * be freed.  This can be used for freeing the argv and envp.  The
  * Function must be runnable in either a process context or the
  * context in which call_usermodehelper_exec is called.
  */
 void call_usermodehelper_setcleanup(struct subprocess_info *info,
-				    void (*cleanup)(char **argv, char **envp))
+		    void (*cleanup)(char **argv, char **envp, void *data))
 {
 	info->cleanup = cleanup;
 }
 EXPORT_SYMBOL(call_usermodehelper_setcleanup);
 
 /**
+ * call_usermodehelper_setinit - set a init function
+ * @info - a subprocess_info returned by a call_usermodehelper_setup
+ * @init - an init function
+ *
+ * The init function is called from the context of the process that
+ * the call to call_usermodehelper forks, just prior to it calling
+ * exec.  Its purpose is to customize the process in whatever way
+ * the caller feels is needed just prior to execing and entering
+ * user space.  the init function pointer returns an int.  A return
+ * of 0 from the init function is treated as a success case, and
+ * allows the usermodehelper process to complete its task.  A return
+ * of anything non-zero, causes the usermodehelper to set that return
+ * code in @info->retval, and exits the process immediately
+ */
+void call_usermodehelper_setinit(struct subprocess_info *info,
+				 int (*init)(void *data))
+{
+	info->init = init;
+}
+EXPORT_SYMBOL(call_usermodehelper_setinit);
+
+/**
  * 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
@@ -535,7 +569,8 @@ int call_usermodehelper_pipe(char *path, char **argv, char **envp,
 	struct subprocess_info *sub_info;
 	int ret;
 
-	sub_info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL);
+	sub_info = call_usermodehelper_setup(path, argv, envp,
+					     NULL, GFP_KERNEL);
 	if (sub_info == NULL)
 		return -ENOMEM;
 
diff --git a/kernel/sys.c b/kernel/sys.c
index ef286ab..207cf36 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1728,7 +1728,7 @@ SYSCALL_DEFINE3(getcpu, unsigned __user *, cpup, unsigned __user *, nodep,
 
 char *poweroff_cmd = "/sbin/poweroff";
 
-static void argv_cleanup(char **argv, char **envp)
+static void argv_cleanup(char **argv, char **envp, void *unused)
 {
 	argv_free(argv);
 }
@@ -1762,7 +1762,7 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	info = call_usermodehelper_setup(argv[0], argv, envp, GFP_ATOMIC);
+	info = call_usermodehelper_setup(argv[0], argv, envp, NULL, GFP_ATOMIC);
 	if (info == NULL) {
 		argv_free(argv);
 		goto out;

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

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.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 fs/exec.c            |   63 ++++++++++++++++++++++++++++++++++----
 include/linux/kmod.h |    4 --
 kernel/kmod.c        |   83 ---------------------------------------------------
 3 files changed, 56 insertions(+), 94 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 1a0b921..428ea6f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1762,6 +1762,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(void *data)
+{
+	struct file *rp, *wp;
+	struct fdtable *fdt;
+	struct coredump_params *cp = (struct coredump_params *)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;
@@ -1848,15 +1892,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
@@ -1864,7 +1908,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;
@@ -1888,8 +1932,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 54db3ff..4a2957d 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -116,10 +116,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 cab23a8..85e2cc6 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -154,7 +154,6 @@ struct subprocess_info {
 	char **envp;
 	enum umh_wait wait;
 	int retval;
-	struct file *stdin;
 	int (*init)(void *data);
 	void (*cleanup)(char **argv, char **envp, void *data);
 	void *data;
@@ -181,23 +180,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);
 
@@ -477,35 +459,6 @@ void call_usermodehelper_setinit(struct subprocess_info *info,
 EXPORT_SYMBOL(call_usermodehelper_setinit);
 
 /**
- * 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 *@filp.
- */
-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.
@@ -552,42 +505,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,
-					     NULL, 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");

Re: [PATCH 1/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Oleg Nesterov]

On 01/29, Neil Horman wrote:
>
> Add init function to usermodehelper
>
> Convert call_usermodehelper_cleanup to usermodehelper_fns and allow it to assign
> both an init function and a cleanup function.

Can't apply this patch, I guess -mm tree has other changes which
this patch depends on. However afaics this series is fine, just
a couple of nits.

> @@ -154,7 +155,9 @@ struct subprocess_info {
>  	enum umh_wait wait;
>  	int retval;
>  	struct file *stdin;
> -	void (*cleanup)(char **argv, char **envp);
> +	int (*init)(void *data);
> +	void (*cleanup)(char **argv, char **envp, void *data);
> +	void *data;

OK, we add *data. But why this patch changes the prototype of ->cleanup() ?
OTOH, I completely agree, it should be changed, and it should lose the
ugly argv/envp arguments.

Since we add subprocess_info->data ptr, I think both ->init and ->cleanup
funcs should have the single arg, "subprocess_info *info". argv, envp, data
they all can be accessed via *info.

Also. It is not clear why this patch changes call_usermodehelper_setup()
to set info->data. Unless the caller uses call_usermodehelper_setinit()
or call_usermodehelper_setcleanup() info->data is not used. Perhaps
it is better to have a single helper which takes (init, cleanup, data)
args.

What do you think?

In any case, I believe you should fix the subjects ;)

Oleg.

Re: [PATCH 1/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

On Sun, Jan 31, 2010 at 03:46:06PM +0100, Oleg Nesterov wrote:
> On 01/29, Neil Horman wrote:
> >
> > Add init function to usermodehelper
> >
> > Convert call_usermodehelper_cleanup to usermodehelper_fns and allow it to assign
> > both an init function and a cleanup function.
> 
> Can't apply this patch, I guess -mm tree has other changes which
> this patch depends on. However afaics this series is fine, just
> a couple of nits.
> 
Yeah, this will only apply to latest -mm

> > @@ -154,7 +155,9 @@ struct subprocess_info {
> >  	enum umh_wait wait;
> >  	int retval;
> >  	struct file *stdin;
> > -	void (*cleanup)(char **argv, char **envp);
> > +	int (*init)(void *data);
> > +	void (*cleanup)(char **argv, char **envp, void *data);
> > +	void *data;
> 
> OK, we add *data. But why this patch changes the prototype of ->cleanup() ?
> OTOH, I completely agree, it should be changed, and it should lose the
> ugly argv/envp arguments.
> 
> Since we add subprocess_info->data ptr, I think both ->init and ->cleanup
> funcs should have the single arg, "subprocess_info *info". argv, envp, data
> they all can be accessed via *info.
> 
Yeah, I can do that.

> Also. It is not clear why this patch changes call_usermodehelper_setup()
> to set info->data. Unless the caller uses call_usermodehelper_setinit()
> or call_usermodehelper_setcleanup() info->data is not used. Perhaps
> it is better to have a single helper which takes (init, cleanup, data)
> args.
> 
> What do you think?
> 
Yeah, that seems reasonable, Honestly, I'm a bit confused as to why there are
set* helpers in the first place, we could just eliminate them entirely, since
callers can set all three independently with call_usermodehelper_fns.  Anywho,
I'll clean that up some more.
 
> In any case, I believe you should fix the subjects ;)
> 
Not sure whats wrong with the subjects, although I guess I am doing a good bit
more than just fixing that at this point :).  I'll expand them.

Give me a few days, and I'll repost.
Neil

> Oleg.
> 
> 

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Oleg Nesterov]

On 01/29, Neil Horman wrote:
>
>  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  {
> ...
> -		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
> -				&cprm.file)) {
> +		cprm.file = NULL;

it is already 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);

Hmm. Looks like this change fixes the bug by accident.

Before this patch, I think we leak info->stdin if kernel_thread() fails
in __call_usermodehelper() pathes.



Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is
buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo().
Also, UMH_WAIT_EXEC should set ->retval in this case.

Cough. And why call_usermodehelper_exec() has this strange ->path[0] == '\0'
check?

Oleg.

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

On Sun, Jan 31, 2010 at 04:50:01PM +0100, Oleg Nesterov wrote:
> On 01/29, Neil Horman wrote:
> >
> >  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> >  {
> > ...
> > -		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
> > -				&cprm.file)) {
> > +		cprm.file = NULL;
> 
> it is already NULL,
> 
Are we sure, it was declared on the stack.  I think its safer to ensure that its
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);
> 
> Hmm. Looks like this change fixes the bug by accident.
> 
> Before this patch, I think we leak info->stdin if kernel_thread() fails
> in __call_usermodehelper() pathes.
> 
I think we did that in call_usermodehelper_pipe.

> 
> 
> Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is
> buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo().
> Also, UMH_WAIT_EXEC should set ->retval in this case.
> 
I went down that path last time I changed this code, Andrew and I decided that
yes it was buggy, but someone (can't recall how) smacked me around a bit and
explained how it worked (some odd artifact behavior of the scheduler).  Its in
the lkml archives if you want to get the whole story.

> Cough. And why call_usermodehelper_exec() has this strange ->path[0] == '\0'
> check?
> 
That I can't explain.  I figured I'd let that sleeping dog lie until this got
striaghtened out and fix it separately if it needed it
Neil

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

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Oleg Nesterov]

On 01/31, Neil Horman wrote:
>
> On Sun, Jan 31, 2010 at 04:50:01PM +0100, Oleg Nesterov wrote:
> > On 01/29, Neil Horman wrote:
> > >
> > >  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> > >  {
> > > ...
> > > -		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
> > > -				&cprm.file)) {
> > > +		cprm.file = NULL;
> >
> > it is already NULL,
> >
> Are we sure, it was declared on the stack.

it must be NULL, or compiler is buggy. it was declared as "= { ... }".

> I think its safer to ensure that its
> NULL.

OK, agreed. I mentioned this just in case.

> > > +		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);
> >
> > Hmm. Looks like this change fixes the bug by accident.
> >
> > Before this patch, I think we leak info->stdin if kernel_thread() fails
> > in __call_usermodehelper() pathes.
> >
> I think we did that in call_usermodehelper_pipe.

Afaics, no. Well yes, call_usermodehelper_pipe() closes write_pipe,
but I meant nobody closes read_pipe, info->stdin, if we fail before
____call_usermodehelper() is called.

> > Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is
> > buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo().
> > Also, UMH_WAIT_EXEC should set ->retval in this case.
> >
> I went down that path last time I changed this code, Andrew and I decided that
> yes it was buggy, but someone (can't recall how) smacked me around a bit and
> explained how it worked (some odd artifact behavior of the scheduler).  Its in
> the lkml archives if you want to get the whole story.

Hmm. I strongly believe this is buggy, and the scheduler can't help in any
way. Fortunately, kernel_thread() must "never" fail...

Oh. And in theory, it is better to change wait_for_helper(). It should
do allow_signal(SIGCHLD) after kernel_thread(). Otherwise, kernel_thread()
can fail if user-space sends SIGCHLD to the forking thread.

> > Cough. And why call_usermodehelper_exec() has this strange ->path[0] == '\0'
> > check?
> >
> That I can't explain.  I figured I'd let that sleeping dog lie until this got
> striaghtened out and fix it separately if it needed it
> Neil

Yes, yes, agreed. As I said, this has nothing to do with this series,
even if I am right these (minor) bugs should be fixed separately.

Oleg.

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Oleg Nesterov]

On 02/01, Oleg Nesterov wrote:
>
> Oh. And in theory, it is better to change wait_for_helper(). It should
> do allow_signal(SIGCHLD) after kernel_thread(). Otherwise, kernel_thread()
> can fail if user-space sends SIGCHLD to the forking thread.

Well ;) And since allow_signal(SIGCHLD) was called, in theory we should
call sys_wait4() + clear_thread_flag(TIF_SIGPENDING) in a loop to protect
against the false SIGCHLD.

> > > Cough. And why call_usermodehelper_exec() has this strange ->path[0] == '\0'
> > > check?
> > >
> > That I can't explain.  I figured I'd let that sleeping dog lie until this got
> > striaghtened out and fix it separately if it needed it
> > Neil
>
> Yes, yes, agreed. As I said, this has nothing to do with this series,
> even if I am right these (minor) bugs should be fixed separately.

Yes.

Oleg.

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Neil Horman]

On Mon, Feb 01, 2010 at 11:29:36AM +0100, Oleg Nesterov wrote:
> On 01/31, Neil Horman wrote:
> >
> > On Sun, Jan 31, 2010 at 04:50:01PM +0100, Oleg Nesterov wrote:
> > > On 01/29, Neil Horman wrote:
> > > >
> > > >  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> > > >  {
> > > > ...
> > > > -		if (call_usermodehelper_pipe(helper_argv[0], helper_argv, NULL,
> > > > -				&cprm.file)) {
> > > > +		cprm.file = NULL;
> > >
> > > it is already NULL,
> > >
> > Are we sure, it was declared on the stack.
> 
> it must be NULL, or compiler is buggy. it was declared as "= { ... }".
> 
> > I think its safer to ensure that its
> > NULL.
> 
> OK, agreed. I mentioned this just in case.
> 
> > > > +		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);
> > >
> > > Hmm. Looks like this change fixes the bug by accident.
> > >
> > > Before this patch, I think we leak info->stdin if kernel_thread() fails
> > > in __call_usermodehelper() pathes.
> > >
> > I think we did that in call_usermodehelper_pipe.
> 
> Afaics, no. Well yes, call_usermodehelper_pipe() closes write_pipe,
> but I meant nobody closes read_pipe, info->stdin, if we fail before
> ____call_usermodehelper() is called.
> 
> > > Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is
> > > buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo().
> > > Also, UMH_WAIT_EXEC should set ->retval in this case.
> > >
> > I went down that path last time I changed this code, Andrew and I decided that
> > yes it was buggy, but someone (can't recall how) smacked me around a bit and
> > explained how it worked (some odd artifact behavior of the scheduler).  Its in
> > the lkml archives if you want to get the whole story.
> 
> Hmm. I strongly believe this is buggy, and the scheduler can't help in any
> way. Fortunately, kernel_thread() must "never" fail...
> 
> Oh. And in theory, it is better to change wait_for_helper(). It should
> do allow_signal(SIGCHLD) after kernel_thread(). Otherwise, kernel_thread()
> can fail if user-space sends SIGCHLD to the forking thread.
> 

Commit 95e0d86badc410d525ea7218fd32df7bfbf9c837 has the discussion from the
previous time that I messed with this code.  Not sure how closely it relates,
but...

Neil

> 

Re: [PATCH 2/2] exec: allow core_pipe recursion check to look for a value of 1 rather than 0 (v2)

[Oleg Nesterov]

On 02/01, Neil Horman wrote:
>
> On Mon, Feb 01, 2010 at 11:29:36AM +0100, Oleg Nesterov wrote:
> >
> > > > Completely off-topic, but I think __call_usermodehelper(UMH_NO_WAIT) is
> > > > buggy. if kernel_thread() failes it should do call_usermodehelper_freeinfo().
> > > > Also, UMH_WAIT_EXEC should set ->retval in this case.
> > > >
> > > I went down that path last time I changed this code, Andrew and I decided that
> > > yes it was buggy, but someone (can't recall how) smacked me around a bit and
> > > explained how it worked (some odd artifact behavior of the scheduler).  Its in
> > > the lkml archives if you want to get the whole story.
> >
> > Hmm. I strongly believe this is buggy, and the scheduler can't help in any
> > way. Fortunately, kernel_thread() must "never" fail...
>
> Commit 95e0d86badc410d525ea7218fd32df7bfbf9c837 has the discussion from the
> previous time that I messed with this code.  Not sure how closely it relates,
> but...

The changelog correctly explains why it is OK to do complete() from
__call_usermodehelper() in UMH_WAIT_EXEC case: CLONE_VFORK guarantees
kernel_thread(CLONE_VFORK) won't return (see do_fork()) until
____call_usermodehelper() thread does exec or exits.

I meant a much more simple problem, I think we need something like this
patch:

	--- kernel/kmod.c
	+++ kernel/kmod.c
	@@ -266,15 +266,18 @@ static void __call_usermodehelper(struct
	 
		switch (wait) {
		case UMH_NO_WAIT:
	+		if (pid < 0)
	+			call_usermodehelper_freeinfo(sub_info);
			break;
	 
		case UMH_WAIT_PROC:
			if (pid > 0)
				break;
	-		sub_info->retval = pid;
			/* FALLTHROUGH */
	 
		case UMH_WAIT_EXEC:
	+		if (pid < 0)
	+			sub_info->retval = pid;
			complete(sub_info->complete);
		}
	 }

to fix 2 problems if kernel_thread() fails in __call_usermodehelper()

	- UMH_NO_WAIT should do call_usermodehelper_freeinfo()

	- UMH_WAIT_EXEC should set sub_info->retval to indicate
	  the error

> > Oh. And in theory, it is better to change wait_for_helper(). It should
> > do allow_signal(SIGCHLD) after kernel_thread().

I was wrong, of course we can't do this, the child can exit and reap
itself before we do sys_wait4().

> Otherwise, kernel_thread()
> > can fail if user-space sends SIGCHLD to the forking thread.

but this is still true. Fortunately this is very minor problem.

Oleg.

Re: [PATCH 0/2] exec: refactor how call_usermodehelper works, and update the sense of the core_pipe recursion check (v3)

[Neil Horman]

Ok, version 3, taking olegs next set of notes into account.  Really just made
the subprocess_info struct visible from the header file (not opaque), and
modified the cleanup and init routines such that they accept a subprocess_info
parameter.


Hey all-
	So, about 6 months ago, I made a set of changes to how the
core-dump-to-a-pipe feature in the kernel works.  We had reports of several
races, including some reports of apps bypassing our recursion check so that a
process that was forked as part of a core_pattern setup could infinitely crash
and refork until the system crashed.

	We fixes those by improving our recursion checks.  The new check
basically refuses to fork a process if its core limit is zero, which works well.

	Unfortunately, I've been getting grief from maintainer of user space
programs that are inserted as the forked process of core_pattern.  They contend
that in order for their programs (such as abrt and apport) to work, all the
running processes in a system must have their core limits set to a non-zero
value, to which I say 'yes'.  I did this by design, and think thats the right
way to do things.

	But I've been asked to ease this burden on user space enough times that
I thought I would take a look at it.  The first suggestion was to make the
recursion check fail on a non-zero 'special' number, like one.  That way the
core collector process could set its core size ulimit to 1, and enable the
kernel's recursion detection.  This isn't a bad idea on the surface, but I don't
like it since its opt-in, in that if a program like abrt or apport has a bug and
fails to set such a core limit, we're left with a recursively crashing system
again.

	So I've come up with this.  What I've done is modify the
call_usermodehelper api such that an extra parameter is added, a function
pointer which will be called by the user helper task, after it forks, but before
it exec's the required process.  This will give the caller the opportunity to
get a call back in the processes context, allowing it to do whatever it needs to
to the process in the kernel prior to exec-ing the user space code.  In the case
of do_coredump, this callback is ues to set the core ulimit of the helper
process to 1.  This elimnates the opt-in problem that I had above, as it allows
the ulimit for core sizes to be set to the value of 1, which is what the
recursion check looks for in do_coredump.

	This patch has been tested successfully by some of the Abrt maintainers
in fedora, with good results.  Patch applies to the latest -mm tree as of this
AM.

Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Jiri Moskovcak <jmoskovc@redhat.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: drbd-dev@lists.linbit.com
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Thomas Sailer <t.sailer@alumni.ethz.ch>
CC: Adam Belay <abelay@mit.edu>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Michal Januszewski <spock@gentoo.org>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Neil Brown <neilb@suse.de>
CC: Mark Fasheh <mfasheh@suse.com>
CC: Paul Menage <menage@google.com>
CC: Stephen Hemminger <shemminger@linux-foundation.org>
CC: Kentaro Takeda <takedakn@nttdata.co.jp>

Re: [PATCH 1/2] exec: refactor how call_usermodehelper works, and update the sense of the core_pipe recursion check (v3)

[Neil Horman]

Add init function to usermodehelper

Convert call_usermodehelper_cleanup to usermodehelper_fns and allow it to assign
both an init function and a cleanup function.  The init function is called from
the context of the forked process and allows for customization of the helper
process prior to calling exec.  Its return code gates the continuation of the
process, or causes its exit.  Also add an arbitrary data pointer to the
subprocess_info struct allowing for data to be passed from the caller to the new
process, and the subsequent cleanup process

Also, use this patch to cleanup the cleanup function.  It currently takes an
argp and envp pointer for freeing, which is ugly.  Lets instead just make the
subprocess_info structure public, and pass that to the cleanup and init routines

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 25d227c..2f3efdc 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -44,7 +44,27 @@ static inline int request_module_nowait(const char *name, ...) { return -ENOSYS;
 
 struct key;
 struct file;
-struct subprocess_info;
+
+enum umh_wait {
+	UMH_NO_WAIT = -1,	/* don't wait at all */
+	UMH_WAIT_EXEC = 0,	/* wait for the exec, but not the process */
+	UMH_WAIT_PROC = 1,	/* wait for the process to complete */
+};
+
+struct subprocess_info {
+	struct work_struct work;
+	struct completion *complete;
+	struct cred *cred;
+	char *path;
+	char **argv;
+	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;
+};
 
 /* Allocate a subprocess_info structure */
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
@@ -55,14 +75,10 @@ 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_setcleanup(struct subprocess_info *info,
-				    void (*cleanup)(char **argv, char **envp));
-
-enum umh_wait {
-	UMH_NO_WAIT = -1,	/* don't wait at all */
-	UMH_WAIT_EXEC = 0,	/* wait for the exec, but not the process */
-	UMH_WAIT_PROC = 1,	/* wait for the process to complete */
-};
+void call_usermodehelper_setfns(struct subprocess_info *info,
+		    int (*init)(struct subprocess_info *info),
+		    void (*cleanup)(struct subprocess_info *info),
+		    void *data);
 
 /* Actually execute the sub-process */
 int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
@@ -72,8 +88,10 @@ int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
 void call_usermodehelper_freeinfo(struct subprocess_info *info);
 
 static inline int
-call_usermodehelper_cleanup(char *path, char **argv, char **envp, enum umh_wait wait,
-			    void (*cleanup)(char **, char **))
+call_usermodehelper_fns(char *path, char **argv, char **envp,
+		    enum umh_wait wait,
+		    int (*init)(struct subprocess_info *info),
+		    void (*cleanup)(struct subprocess_info *), void *data)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
@@ -81,15 +99,15 @@ call_usermodehelper_cleanup(char *path, char **argv, char **envp, enum umh_wait
 	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
-	if (cleanup)
-		call_usermodehelper_setcleanup(info, cleanup);
+	call_usermodehelper_setfns(info, init, cleanup, data);
 	return call_usermodehelper_exec(info, wait);
 }
 
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 {
-	return call_usermodehelper_cleanup(path, argv, envp, wait, NULL);
+	return call_usermodehelper_fns(path, argv, envp,
+				       wait, NULL, NULL, NULL);
 }
 
 static inline int
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2db0689..1094b41 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -51,9 +51,9 @@ static struct workqueue_struct *khelper_wq;
 */
 char *modprobe_path = "/sbin/modprobe";
 
-static void free_arg(char **argv, char **env)
+static void free_arg(struct subprocess_info *info)
 {
-	kfree(argv[0]);
+	kfree(info->argv[0]);
 }
 
 /**
@@ -133,8 +133,9 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	trace_module_request(module_name, wait, _RET_IP_);
 
-	ret = call_usermodehelper_cleanup(mp_copy, argv, envp,
-			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC, free_arg);
+	ret = call_usermodehelper_fns(mp_copy, argv, envp,
+			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC,
+			NULL, free_arg, NULL);
 	mp_copy = NULL; /* free_arg frees */
 	atomic_dec(&kmod_concurrent);
 error:
@@ -144,19 +145,6 @@ error:
 EXPORT_SYMBOL(__request_module);
 #endif /* CONFIG_MODULES */
 
-struct subprocess_info {
-	struct work_struct work;
-	struct completion *complete;
-	struct cred *cred;
-	char *path;
-	char **argv;
-	char **envp;
-	enum umh_wait wait;
-	int retval;
-	struct file *stdin;
-	void (*cleanup)(char **argv, char **envp);
-};
-
 /*
  * This is the task which runs the usermode application
  */
@@ -198,6 +186,12 @@ static int ____call_usermodehelper(void *data)
 	/* We can run anywhere, unlike our parent keventd(). */
 	set_cpus_allowed_ptr(current, cpu_all_mask);
 
+	if (sub_info->init) {
+		retval = sub_info->init(sub_info);
+		if (retval)
+			goto fail;
+	}
+
 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
 	 * Avoid propagating that into the userspace child.
@@ -207,6 +201,7 @@ static int ____call_usermodehelper(void *data)
 	retval = kernel_execve(sub_info->path, sub_info->argv, sub_info->envp);
 
 	/* Exec failed? */
+fail:
 	sub_info->retval = retval;
 	do_exit(0);
 }
@@ -214,7 +209,7 @@ static int ____call_usermodehelper(void *data)
 void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
-		(*info->cleanup)(info->argv, info->envp);
+		(*info->cleanup)(info);
 	if (info->cred)
 		put_cred(info->cred);
 	kfree(info);
@@ -426,21 +421,31 @@ void call_usermodehelper_setkeys(struct subprocess_info *info,
 EXPORT_SYMBOL(call_usermodehelper_setkeys);
 
 /**
- * call_usermodehelper_setcleanup - set a cleanup function
+ * call_usermodehelper_setfns - set a cleanup/init function
  * @info: a subprocess_info returned by call_usermodehelper_setup
  * @cleanup: a cleanup function
+ * @init: an init function
+ * @data: arbitrary context sensitive data
+ *
+ * The init function is used to customize the helper process prior to
+ * exec.  A non-zero return code causes the process to error out, exit,
+ * and return the failure to the calling process
  *
- * The cleanup function is just befor ethe subprocess_info is about to
+ * The cleanup function is just before ethe subprocess_info is about to
  * be freed.  This can be used for freeing the argv and envp.  The
  * Function must be runnable in either a process context or the
  * context in which call_usermodehelper_exec is called.
  */
-void call_usermodehelper_setcleanup(struct subprocess_info *info,
-				    void (*cleanup)(char **argv, char **envp))
+void call_usermodehelper_setfns(struct subprocess_info *info,
+		    int (*init)(struct subprocess_info *info),
+		    void (*cleanup)(struct subprocess_info *info),
+		    void *data)
 {
 	info->cleanup = cleanup;
+	info->init = init;
+	info->data = data;
 }
-EXPORT_SYMBOL(call_usermodehelper_setcleanup);
+EXPORT_SYMBOL(call_usermodehelper_setfns);
 
 /**
  * call_usermodehelper_stdinpipe - set up a pipe to be used for stdin
@@ -535,7 +540,8 @@ int call_usermodehelper_pipe(char *path, char **argv, char **envp,
 	struct subprocess_info *sub_info;
 	int ret;
 
-	sub_info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL);
+	sub_info = call_usermodehelper_setup(path, argv, envp,
+					     GFP_KERNEL);
 	if (sub_info == NULL)
 		return -ENOMEM;
 
diff --git a/kernel/sys.c b/kernel/sys.c
index ef286ab..71005f1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1728,9 +1728,9 @@ SYSCALL_DEFINE3(getcpu, unsigned __user *, cpup, unsigned __user *, nodep,
 
 char *poweroff_cmd = "/sbin/poweroff";
 
-static void argv_cleanup(char **argv, char **envp)
+static void argv_cleanup(struct subprocess_info *info)
 {
-	argv_free(argv);
+	argv_free(info->argv);
 }
 
 /**
@@ -1768,7 +1768,7 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	call_usermodehelper_setcleanup(info, argv_cleanup);
+	call_usermodehelper_setfns(info, NULL, argv_cleanup, NULL);
 
 	ret = call_usermodehelper_exec(info, UMH_NO_WAIT);
 

Re: [PATCH 2/2] exec: refactor how call_usermodehelper works, and update the sense of the core_pipe recursion check (v3)

[Neil Horman]

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

diff --git a/fs/exec.c b/fs/exec.c
index 1a0b921..4e77085 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1762,6 +1762,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;
@@ -1848,15 +1892,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
@@ -1864,7 +1908,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;
@@ -1888,8 +1932,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 2f3efdc..1b95e2f 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -60,7 +60,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;
@@ -127,10 +126,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 1094b41..ac8e1a5 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -166,23 +166,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);
 
@@ -448,35 +431,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 *@filp.
- */
-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.
@@ -523,42 +477,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");

Re: [PATCH 1/2] exec: refactor how call_usermodehelper works, and update the sense of the core_pipe recursion check (v3)

[Oleg Nesterov]

On 02/02, Neil Horman wrote:
>
> +void call_usermodehelper_setfns(struct subprocess_info *info,
> +		    int (*init)(struct subprocess_info *info),
> +		    void (*cleanup)(struct subprocess_info *info),
> +		    void *data);
> ...
> +call_usermodehelper_fns(char *path, char **argv, char **envp,
> +		    enum umh_wait wait,
> +		    int (*init)(struct subprocess_info *info),
> +		    void (*cleanup)(struct subprocess_info *), void *data)
> ...
> +	call_usermodehelper_setfns(info, init, cleanup, data);
>  	return call_usermodehelper_exec(info, wait);
>  }

Unless I misread the patch, this is the only caller of _setfns(), and
this helper is really trivial and probably deserves to be inline. But
this is very minor.

Personally I think these patches are nice. Not only this series adds
the new functionality, in my opinion it also cleanups and simplifies
the code.

Oleg.