public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
@ 2007-07-26 17:40 Neil Horman
  2007-07-26 18:48 ` Andrew Morton
  2007-07-26 22:02 ` Jeremy Fitzhardinge
  0 siblings, 2 replies; 11+ messages in thread
From: Neil Horman @ 2007-07-26 17:40 UTC (permalink / raw)
  To: linux-kernel, akpm, wwoods

Hey-
	Currently, core dumps can be redirected to a pipe by placing the
following string template in /proc/sys/kernel/core_pattern:
|<path/to/application>
This patch extends this ability, allowing the core_pattern to contain arguments
to be passed as an argv array to the userspace helper application.  It also add
a format specifier, %c, which allows the RLIM_CORE value of the crashing
application to be passed on the command line, since RLIMIT_CORE is reduced to
zero when execing the userspace helper

Tested successfully by me on x86 & x86_64.

Regards
Neil

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


 exec.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 119 insertions(+), 3 deletions(-)



diff --git a/fs/exec.c b/fs/exec.c
index c0b5def..e862019 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -51,6 +51,7 @@
 #include <linux/cn_proc.h>
 #include <linux/audit.h>
 #include <linux/signalfd.h>
+#include <linux/string.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1448,6 +1449,109 @@ int set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static void free_argv_array(char **argv)
+{
+	int i;
+	if (argv != NULL) {
+		for (i = 0; argv[i] != NULL; i++)
+			kfree(argv[i]);
+		kfree(argv);
+	}
+
+}
+
+/*
+ * format_corename_argv will inspect the corename string, 	
+ * and for every option found after the binary name
+ * it will remove the option from the string, and place it
+ * in the argv array, that can then be passed to the 
+ * usermodehelper if core_pattern is a pipe
+ * Assumes that corename is declared on the stack of the caller
+ */
+static char **format_corename_argv(char *corename)
+{
+	char *nptr = corename;
+	char *fptr = NULL;
+	char **orig_argv;
+	char **argv_ptr;
+	int i = 2;
+
+	/*
+	 * Start by populating element 0 with the name of the binary
+	 */	
+	argv_ptr = kmalloc(sizeof(char **)*i, GFP_KERNEL);
+	if (!argv_ptr)
+		goto out;
+	orig_argv = argv_ptr;
+
+	argv_ptr[0] = NULL;
+	argv_ptr[1] = NULL;
+	fptr = strchr(nptr, ' ');
+
+	/* This trucates the string command line for use in exec */
+	if (fptr != NULL) 
+		*fptr = '\0';
+
+	nptr = nptr + 1; /*this removes the leading | */
+	argv_ptr[0] = kmalloc(strlen(nptr), GFP_KERNEL);
+	if (argv_ptr[0] == NULL)
+		goto out_free;
+
+	strcpy(argv_ptr[0], nptr);
+
+	/*
+	 * This handles the case where we have no options
+	 */
+	if (fptr == NULL)
+		goto out;
+	/*
+	 * Now walk through the rest of the corename with nptr and
+	 * fptr, delimiting by spaces and filling out the array
+	 */
+	nptr = fptr + 1;
+	for(;;) {
+		fptr = strchr(nptr, ' ');
+
+		/*
+		 * Found a new option, lets add to the array
+		 */
+		argv_ptr = krealloc(argv_ptr, (sizeof(char **)*(i+1)), GFP_KERNEL);
+		if (!argv_ptr)
+			goto out_free;
+
+		argv_ptr[i] = NULL; /* set the end of the array to NULL */
+		if (fptr != NULL)
+			*fptr = '\0';
+
+		argv_ptr[i-1] = kmalloc(strlen(nptr), GFP_KERNEL);
+		if (argv_ptr[i-1] == NULL)
+			goto out_free;
+
+		/*
+		 * copy the option and advance our pointers
+		 */
+		strcpy(argv_ptr[i-1],nptr);
+		if (fptr == NULL)
+			break;
+		i++;
+		nptr = fptr + 1;
+	}
+
+
+	/*
+	 * now we have our array built
+	 */
+	return argv_ptr;
+
+out_free:
+	for (i = 0;orig_argv[i] != NULL; i++)
+		kfree(orig_argv[i]);
+	kfree(orig_argv);
+	argv_ptr = NULL;
+out:
+	return argv_ptr;
+}
+
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
@@ -1543,6 +1647,14 @@ static int format_corename(char *corename, const char *pattern, long signr)
 					goto out;
 				out_ptr += rc;
 				break;
+			/* core limit size */
+			case 'c':
+				rc = snprintf(out_ptr, out_end - out_ptr,
+					      "%lu", current->signal->rlim[RLIMIT_CORE].rlim_cur); 
+				if (rc > out_end - out_ptr)
+					goto out;
+				out_ptr += rc;
+				break;
 			default:
 				break;
 			}
@@ -1727,6 +1839,7 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	int flag = 0;
 	int ispipe = 0;
 	unsigned long core_limit = current->signal->rlim[RLIMIT_CORE].rlim_cur;
+	char **helper_argv = NULL;
 
 	audit_core_dumps(signr);
 
@@ -1775,14 +1888,14 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	 * at which point file size limits and permissions will be imposed 
 	 * as it does with any other process
 	 */
-	if ((!ispipe) &&
-	   (core_limit < binfmt->min_coredump))
+	if ((!ispipe) && (core_limit < binfmt->min_coredump))
 		goto fail_unlock;
 
  	if (ispipe) {
 		core_limit = RLIM_INFINITY;
+		helper_argv = format_corename_argv(corename);
 		/* SIGPIPE can happen, but it's just never processed */
- 		if(call_usermodehelper_pipe(corename+1, NULL, NULL, &file)) {
+ 		if(call_usermodehelper_pipe(corename+1, helper_argv, NULL, &file)) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       corename);
  			goto fail_unlock;
@@ -1817,6 +1930,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 close_fail:
 	filp_close(file, NULL);
 fail_unlock:
+	if (ispipe)
+		free_argv_array(helper_argv);
+
 	current->fsuid = fsuid;
 	complete_all(&mm->core_done);
 fail:
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 17:40 [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe Neil Horman
@ 2007-07-26 18:48 ` Andrew Morton
  2007-07-26 19:31   ` Neil Horman
  2007-07-26 19:36   ` Will Woods
  2007-07-26 22:02 ` Jeremy Fitzhardinge
  1 sibling, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2007-07-26 18:48 UTC (permalink / raw)
  To: Neil Horman; +Cc: linux-kernel, wwoods

On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:

> 	Currently, core dumps can be redirected to a pipe by placing the
> following string template in /proc/sys/kernel/core_pattern:
> |<path/to/application>
> This patch extends this ability, allowing the core_pattern to contain arguments
> to be passed as an argv array to the userspace helper application.  It also add
> a format specifier, %c, which allows the RLIM_CORE value of the crashing
> application to be passed on the command line, since RLIMIT_CORE is reduced to
> zero when execing the userspace helper

This all seems to be getting a bit nutty.  Who needs this feature
and what will they do with it, etc?

You have a few open-coded kstrdup()s in there, btw.  And an open-coded
free_argv_array() on the error path.  And a lot of checkpatch warnings.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 18:48 ` Andrew Morton
@ 2007-07-26 19:31   ` Neil Horman
  2007-07-26 19:47     ` Andrew Morton
  2007-07-26 19:36   ` Will Woods
  1 sibling, 1 reply; 11+ messages in thread
From: Neil Horman @ 2007-07-26 19:31 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, wwoods

On Thu, Jul 26, 2007 at 11:48:32AM -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > 	Currently, core dumps can be redirected to a pipe by placing the
> > following string template in /proc/sys/kernel/core_pattern:
> > |<path/to/application>
> > This patch extends this ability, allowing the core_pattern to contain arguments
> > to be passed as an argv array to the userspace helper application.  It also add
> > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > zero when execing the userspace helper
> 
> This all seems to be getting a bit nutty.  Who needs this feature
> and what will they do with it, etc?
> 
Why nutty?  Ubuntu has had apport for a bit now, which monitors the system for
crashed process and attempts to help the user auto-file a bugreport with a
relevant distro based on its configuration.  Ubuntu has implemented lots of
their functionality with some patches that they never pushed upstream (and IMHO,
have some security issues).  This is my attempt to do what their doing sanely,
so the other distro's (primarily fedora) can take advantage of this technology.
Will Woods (who has been copied on this set of patches) can give you more detail
if you like.   As for this specific feature, I wanted to include it for the same
reason that I mentioned above.  core_pattern, when set to a pipe, reduces
RLIMIT_CORE to zero.  This patch lets you pass the real core limit value
directly to the application in the event it wants to save the core to disk, but
still wishes to respect the original ulimit value.

> You have a few open-coded kstrdup()s in there, btw.  And an open-coded
> free_argv_array() on the error path.  And a lot of checkpatch warnings.
I don't see what you're referring to.  free_argv_array is only called if it was
initially setup (keyed of the same ispipe variable), and all the memory for the
strcpy's in format_corename_argv is unwound and freed in the out_free label of
the same function on error.  If there is somethign I'm missing, please let me
know, I'm happy to fix it.

I know its been a large number of patches over the last few days, and I'm sorry
for that.  I've been trying to break this up into a sane amount of discrete
chunks that don't depend on one another.  If you would prefer that I roll them
up into a larger patch, I'm happy to do that as well.  Just let me know.

Thanks & Regards
Neil

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 18:48 ` Andrew Morton
  2007-07-26 19:31   ` Neil Horman
@ 2007-07-26 19:36   ` Will Woods
  2007-07-26 19:52     ` Andrew Morton
  1 sibling, 1 reply; 11+ messages in thread
From: Will Woods @ 2007-07-26 19:36 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Neil Horman, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

On Thu, 2007-07-26 at 11:48 -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > 	Currently, core dumps can be redirected to a pipe by placing the
> > following string template in /proc/sys/kernel/core_pattern:
> > |<path/to/application>
> > This patch extends this ability, allowing the core_pattern to contain arguments
> > to be passed as an argv array to the userspace helper application.  It also add
> > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > zero when execing the userspace helper
> 
> This all seems to be getting a bit nutty.  Who needs this feature
> and what will they do with it, etc?

We're using it for doing a system-wide crash dump handler. Currently
Ubuntu's using it with their Apport tool[1] for this purpose; I'm
adapting that for Fedora. 

The Ubuntu approach requires a kernel patch that adds a bunch of process
information (process pid, RLIMIT_CORE, etc) to the environment of the
crash handler[2]. Most of that information can instead be parsed out of
the ELF headers - which is what I wrote code to do[3]. The problem that
remains is determining the value of RLIMIT_CORE. (This is used to
determine whether the user wants a normal corefile, thus retaining
normal core dump behavior). 

As I understand it, getrlimit() won't return the correct values while
dumping to a pipe. So we need to pass the original RLIMIT_CORE to the
userspace helper somehow. It seems sensible to pass arguments to a
userspace program by using argv[]. So there we are.

There's probably many other uses for this stuff but that's the specific
one we're targeting. Does that make sense? If there's an easier way to
get the original RLIMIT_CORE from inside the crash handler, I'd love to
hear it.

-w

[1] https://wiki.ubuntu.com/Apport 
[2]
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-feisty.git;a=blob;hb=HEAD;f=fs/exec.c#l1557
[3] http://rdr.to/bX


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 19:31   ` Neil Horman
@ 2007-07-26 19:47     ` Andrew Morton
  2007-07-26 20:20       ` Neil Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2007-07-26 19:47 UTC (permalink / raw)
  To: Neil Horman; +Cc: linux-kernel, wwoods

On Thu, 26 Jul 2007 15:31:45 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:

> On Thu, Jul 26, 2007 at 11:48:32AM -0700, Andrew Morton wrote:
> > On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > 	Currently, core dumps can be redirected to a pipe by placing the
> > > following string template in /proc/sys/kernel/core_pattern:
> > > |<path/to/application>
> > > This patch extends this ability, allowing the core_pattern to contain arguments
> > > to be passed as an argv array to the userspace helper application.  It also add
> > > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > > zero when execing the userspace helper
> > 
> > This all seems to be getting a bit nutty.  Who needs this feature
> > and what will they do with it, etc?
> > 
> Why nutty?  Ubuntu has had apport for a bit now, which monitors the system for
> crashed process and attempts to help the user auto-file a bugreport with a
> relevant distro based on its configuration.  Ubuntu has implemented lots of
> their functionality with some patches that they never pushed upstream (and IMHO,
> have some security issues).  This is my attempt to do what their doing sanely,
> so the other distro's (primarily fedora) can take advantage of this technology.
> Will Woods (who has been copied on this set of patches) can give you more detail
> if you like.   As for this specific feature, I wanted to include it for the same
> reason that I mentioned above.  core_pattern, when set to a pipe, reduces
> RLIMIT_CORE to zero.  This patch lets you pass the real core limit value
> directly to the application in the event it wants to save the core to disk, but
> still wishes to respect the original ulimit value.

Somewhere in there is a changelog trying to get out.

It's quite important to explan things like this: what the feature is *for*,
why we want it in Linux, what it value is to our users, stuff like that. 
Things with which we can justify the additional overhead/maintenance cost,
etc.

So please, include a full changelog in v2?

> > You have a few open-coded kstrdup()s in there, btw.  And an open-coded
> > free_argv_array() on the error path.  And a lot of checkpatch warnings.
> I don't see what you're referring to.  free_argv_array is only called if it was
> initially setup (keyed of the same ispipe variable), and all the memory for the
> strcpy's in format_corename_argv is unwound and freed in the out_free label of
> the same function on error.  If there is somethign I'm missing, please let me
> know, I'm happy to fix it.

The cleanup code at out_free() could, I think, just be a call to
free_argv_array().

> I know its been a large number of patches over the last few days, and I'm sorry
> for that.

heh.  More than 100 patches is getting largeish.

>  I've been trying to break this up into a sane amount of discrete
> chunks that don't depend on one another.  If you would prefer that I roll them
> up into a larger patch, I'm happy to do that as well.  Just let me know.

Well if you have mutiple patches hitting on the same area it would be
better to present it as a sequence-number patch series which all tells a
nice story.  Start out describing the end-point and what value it all adds,
follow that up with the various bits of implementation.

Dribbling in various related patches at one-per-day makes it all a bit hard
to follow and manage.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 19:36   ` Will Woods
@ 2007-07-26 19:52     ` Andrew Morton
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2007-07-26 19:52 UTC (permalink / raw)
  To: Will Woods; +Cc: Neil Horman, linux-kernel

On Thu, 26 Jul 2007 15:36:24 -0400 Will Woods <wwoods@redhat.com> wrote:

> On Thu, 2007-07-26 at 11:48 -0700, Andrew Morton wrote:
> > On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > 	Currently, core dumps can be redirected to a pipe by placing the
> > > following string template in /proc/sys/kernel/core_pattern:
> > > |<path/to/application>
> > > This patch extends this ability, allowing the core_pattern to contain arguments
> > > to be passed as an argv array to the userspace helper application.  It also add
> > > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > > zero when execing the userspace helper
> > 
> > This all seems to be getting a bit nutty.  Who needs this feature
> > and what will they do with it, etc?
> 
> We're using it for doing a system-wide crash dump handler. Currently
> Ubuntu's using it with their Apport tool[1] for this purpose; I'm
> adapting that for Fedora. 

Can you please copy the relevant Ubuntu people on the patches then?  It
would be valuable to get their input on the proposal.

> The Ubuntu approach requires a kernel patch that adds a bunch of process
> information (process pid, RLIMIT_CORE, etc) to the environment of the
> crash handler[2]. Most of that information can instead be parsed out of
> the ELF headers - which is what I wrote code to do[3]. The problem that
> remains is determining the value of RLIMIT_CORE. (This is used to
> determine whether the user wants a normal corefile, thus retaining
> normal core dump behavior). 
> 
> As I understand it, getrlimit() won't return the correct values while
> dumping to a pipe. So we need to pass the original RLIMIT_CORE to the
> userspace helper somehow. It seems sensible to pass arguments to a
> userspace program by using argv[]. So there we are.

yup.  The changelog should clearly document this new kernel/userspace
interface, please.  Things like what the argv format will be, which args
are optional, etc.

> There's probably many other uses for this stuff but that's the specific
> one we're targeting. Does that make sense? If there's an easier way to
> get the original RLIMIT_CORE from inside the crash handler, I'd love to
> hear it.
> 

I'm still emerging from the "what the heck is this" state ;)

You need to tell us these things...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 19:47     ` Andrew Morton
@ 2007-07-26 20:20       ` Neil Horman
  2007-07-26 20:35         ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2007-07-26 20:20 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, wwoods

On Thu, Jul 26, 2007 at 12:47:31PM -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2007 15:31:45 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Thu, Jul 26, 2007 at 11:48:32AM -0700, Andrew Morton wrote:
> > > On Thu, 26 Jul 2007 13:40:19 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> > > 
> > > > 	Currently, core dumps can be redirected to a pipe by placing the
> > > > following string template in /proc/sys/kernel/core_pattern:
> > > > |<path/to/application>
> > > > This patch extends this ability, allowing the core_pattern to contain arguments
> > > > to be passed as an argv array to the userspace helper application.  It also add
> > > > a format specifier, %c, which allows the RLIM_CORE value of the crashing
> > > > application to be passed on the command line, since RLIMIT_CORE is reduced to
> > > > zero when execing the userspace helper
> > > 
> > > This all seems to be getting a bit nutty.  Who needs this feature
> > > and what will they do with it, etc?
> > > 
> > Why nutty?  Ubuntu has had apport for a bit now, which monitors the system for
> > crashed process and attempts to help the user auto-file a bugreport with a
> > relevant distro based on its configuration.  Ubuntu has implemented lots of
> > their functionality with some patches that they never pushed upstream (and IMHO,
> > have some security issues).  This is my attempt to do what their doing sanely,
> > so the other distro's (primarily fedora) can take advantage of this technology.
> > Will Woods (who has been copied on this set of patches) can give you more detail
> > if you like.   As for this specific feature, I wanted to include it for the same
> > reason that I mentioned above.  core_pattern, when set to a pipe, reduces
> > RLIMIT_CORE to zero.  This patch lets you pass the real core limit value
> > directly to the application in the event it wants to save the core to disk, but
> > still wishes to respect the original ulimit value.
> 
> Somewhere in there is a changelog trying to get out.
> 
> It's quite important to explan things like this: what the feature is *for*,
> why we want it in Linux, what it value is to our users, stuff like that. 
> Things with which we can justify the additional overhead/maintenance cost,
> etc.
> 
> So please, include a full changelog in v2?
> 
> > > You have a few open-coded kstrdup()s in there, btw.  And an open-coded
> > > free_argv_array() on the error path.  And a lot of checkpatch warnings.
> > I don't see what you're referring to.  free_argv_array is only called if it was
> > initially setup (keyed of the same ispipe variable), and all the memory for the
> > strcpy's in format_corename_argv is unwound and freed in the out_free label of
> > the same function on error.  If there is somethign I'm missing, please let me
> > know, I'm happy to fix it.
> 
> The cleanup code at out_free() could, I think, just be a call to
> free_argv_array().
> 
> > I know its been a large number of patches over the last few days, and I'm sorry
> > for that.
> 
> heh.  More than 100 patches is getting largeish.
> 
> >  I've been trying to break this up into a sane amount of discrete
> > chunks that don't depend on one another.  If you would prefer that I roll them
> > up into a larger patch, I'm happy to do that as well.  Just let me know.
> 
> Well if you have mutiple patches hitting on the same area it would be
> better to present it as a sequence-number patch series which all tells a
> nice story.  Start out describing the end-point and what value it all adds,
> follow that up with the various bits of implementation.
> 
> Dribbling in various related patches at one-per-day makes it all a bit hard
> to follow and manage.

All your points are well taken.  If you'd like to back them all out, I have them
all in a git tree here, and I can repost (say early next week when I finish
local testing) as a patch sequence, with all my screw-ups properly fixed,
and with an appropriate changelog describing the whole sequence.

Sorry for the confusion.
Neil

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 20:20       ` Neil Horman
@ 2007-07-26 20:35         ` Andrew Morton
  2007-07-26 21:46           ` Neil Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2007-07-26 20:35 UTC (permalink / raw)
  To: Neil Horman; +Cc: linux-kernel, wwoods

On Thu, 26 Jul 2007 16:20:18 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:

> If you'd like to back them all out, I have them
> all in a git tree here, and I can repost (say early next week when I finish
> local testing) as a patch sequence, with all my screw-ups properly fixed,
> and with an appropriate changelog describing the whole sequence.

hm, I have six fixes here against
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe.patch
and I always worry that some might get lost.

How many more patches have you after this one?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 20:35         ` Andrew Morton
@ 2007-07-26 21:46           ` Neil Horman
  0 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2007-07-26 21:46 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, wwoods

On Thu, Jul 26, 2007 at 01:35:36PM -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2007 16:20:18 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > If you'd like to back them all out, I have them
> > all in a git tree here, and I can repost (say early next week when I finish
> > local testing) as a patch sequence, with all my screw-ups properly fixed,
> > and with an appropriate changelog describing the whole sequence.
> 
> hm, I have six fixes here against
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe.patch
> and I always worry that some might get lost.
> 
> How many more patches have you after this one?

I think the best organization that I could give you would leave me with one
patch remaining, for a total of three that you would have to carry:

Patch 1) Removing the RLIMIT_CORE restriction on core_dumps in do_coredump and
the core_dump methods from fs/binfmt_*.c

Patch 2) Add the ability to parse arguments out of
/proc/sys/kernel/core_pattern, along with the ability to specify the origional
RLIMIT_CORE size as an option.

Patch 3) fix up some pre-existing bugs that I found while doing the other work.
Specifically I found that call_usermodehelper_pipe waits for the usermodehelper
to exit.  Since we are redirecting stdin for the helper to a pipe, any reader of
the pipe will cause a deadlock in the do_coredump path.  I want to fix up that
and a few simmilar bugs.

You have the first two as you know, and I plan to be done with the third on
friday or monday.  Just let me know if you would like me to repost the first two
with the third as a sequence set.

As far as the changelog for these three (or six, if you want to count the
interviening fixes separately), you can use this:

For some time now /proc/sys/core_pattern has allowed for the specification that
the output desitination of a core dump be a pipe to a user space process, rather
than a file system location.  This enables a class of applications that allows
for intellegent handling of application crashes.  While the pipe core_pattern
infrastrucutre is beneficial, it is lacking in certain points.  Specifically it
is unable to dump cores if a users core dump ulimit is too low, despite the fact
that no data need be written to a filesystem.  It is also lacking in the sense
that while an application can be specified as the reader end of the pipe,
arguments cannot be currently passed to this application.  This set of patches
seeks to improve this infrastructure by:

1) Ignoring RLIMIT_CORE restrictions when the core destination is a pipe,
relying instead on user space security to prevent oversubscription of resources

2) Allowing arguments to be specified and passed to applications specified in
core_pattern

3) Fixing a few pre-existing bugs discovered during the aforementioned two
development points.


Thanks and Regards
Neil

P.S. Will, please send me email addresses for relevant interested parties from
ubuntu, and I'll happily copy them on these patches.


-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 17:40 [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe Neil Horman
  2007-07-26 18:48 ` Andrew Morton
@ 2007-07-26 22:02 ` Jeremy Fitzhardinge
  2007-07-27 10:32   ` Neil Horman
  1 sibling, 1 reply; 11+ messages in thread
From: Jeremy Fitzhardinge @ 2007-07-26 22:02 UTC (permalink / raw)
  To: Neil Horman; +Cc: linux-kernel, akpm, wwoods

Neil Horman wrote:
> +static void free_argv_array(char **argv)
> +{
> +	int i;
> +	if (argv != NULL) {
> +		for (i = 0; argv[i] != NULL; i++)
> +			kfree(argv[i]);
> +		kfree(argv);
> +	}
> +
> +}
>   

I've helpfully already provided free_argv() in lib/argv_split.c.

> +
> +/*
> + * format_corename_argv will inspect the corename string, 	
> + * and for every option found after the binary name
> + * it will remove the option from the string, and place it
> + * in the argv array, that can then be passed to the 
> + * usermodehelper if core_pattern is a pipe
> + * Assumes that corename is declared on the stack of the caller
> + */
> +static char **format_corename_argv(char *corename)
> +{
>   

And it looks like this could be adapted to use argv_split() with a bit
of additional processing.

    J

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe
  2007-07-26 22:02 ` Jeremy Fitzhardinge
@ 2007-07-27 10:32   ` Neil Horman
  0 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2007-07-27 10:32 UTC (permalink / raw)
  To: Jeremy Fitzhardinge; +Cc: linux-kernel, akpm, wwoods

On Thu, Jul 26, 2007 at 03:02:19PM -0700, Jeremy Fitzhardinge wrote:
> Neil Horman wrote:
> > +static void free_argv_array(char **argv)
> > +{
> > +	int i;
> > +	if (argv != NULL) {
> > +		for (i = 0; argv[i] != NULL; i++)
> > +			kfree(argv[i]);
> > +		kfree(argv);
> > +	}
> > +
> > +}
> >   
> 
> I've helpfully already provided free_argv() in lib/argv_split.c.
> 
> > +
> > +/*
> > + * format_corename_argv will inspect the corename string, 	
> > + * and for every option found after the binary name
> > + * it will remove the option from the string, and place it
> > + * in the argv array, that can then be passed to the 
> > + * usermodehelper if core_pattern is a pipe
> > + * Assumes that corename is declared on the stack of the caller
> > + */
> > +static char **format_corename_argv(char *corename)
> > +{
> >   
> 
> And it looks like this could be adapted to use argv_split() with a bit
> of additional processing.
> 
>     J
I didn't see these, thanks.  I've got the aforementioned misc cleanup patch to
send for this work to Andrew still, I'll roll these changes in with it.

Regards
Neil


-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2007-07-27 10:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-07-26 17:40 [PATCH] core_pattern: Add ability for core_pattern to parse arguments when pattern is a pipe Neil Horman
2007-07-26 18:48 ` Andrew Morton
2007-07-26 19:31   ` Neil Horman
2007-07-26 19:47     ` Andrew Morton
2007-07-26 20:20       ` Neil Horman
2007-07-26 20:35         ` Andrew Morton
2007-07-26 21:46           ` Neil Horman
2007-07-26 19:36   ` Will Woods
2007-07-26 19:52     ` Andrew Morton
2007-07-26 22:02 ` Jeremy Fitzhardinge
2007-07-27 10:32   ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox