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

Neil Horman nhorman at tuxdriver.com
Thu Jan 21 21:08:06 CET 2010


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


More information about the drbd-dev mailing list