Skip to contentSkip to conversation

coredump: use task comm instead of (unknown)

PATCH Superseded Jiri Slaby 5 participants 9 messages Jun 7, 2011 – Aug 30, 2011

Subsystems (2)
VFS exec and binfmt
lore
Share & tools
lore

Jiri Slaby

author
#0 lore raw

PATCH v2 1/2 coredump: use task comm instead of (unknown)

If we don't know the file corresponding to the binary (i.e. exe_file is unknown), use "task->comm (path unknown)" instead of simple "(unknown)" as suggested by ak.

The fallback is the same as %e except it will append "(path unknown)".

Signed-off-by Jiri Slaby <[email protected]>
Cc Alan Cox <[email protected]>
Cc Al Viro <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Oleg Nesterov <[email protected]>
---
 fs/exec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/exec.c +1 -1
index a9f2b36..2093c47 100644
@@ -1631,7 +1631,7 @@ static int cn_print_exe_file(struct core_name *cn)
 
 	exe_file = get_mm_exe_file(current->mm);
 	if (!exe_file)
-		return cn_printf(cn, "(unknown)");
+		return cn_printf(cn, "%s (path unknown)", current->comm);
 
 	pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
 	if (!pathbuf) {
Signature · 1 line
1.7.5.3
author
#1 lore raw

PATCH v2 2/2 coredump: escape / in hostname and comm

Change every occurence of / in comm and hostname to !. If the process changes its name to contain /, the core is not dumped (if the directory tree doesn't exist like that). The same with hostname being something like myhost/3. Fix this behaviour by using the escape loop used in %E. (We extract it to a separate function.)

Now both with comm == myprocess/1 and hostname == myhost/1, the core is dumped like (kernel.core_pattern='core.%p.%e.%h):
core.2349.myprocess!1.myhost!1

Signed-off-by Jiri Slaby <[email protected]>
Cc Alan Cox <[email protected]>
Cc Al Viro <[email protected]>
Cc Andi Kleen <[email protected]>
---
 fs/exec.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)
fs/exec.c +23 -8
index 2093c47..1027025 100644
@@ -1623,15 +1623,26 @@ expand_fail:
 	return ret;
 }
 
+static void cn_escape(char *str)
+{
+	for (; *str; str++)
+		if (*str == '/')
+			*str = '!';
+}
+
 static int cn_print_exe_file(struct core_name *cn)
 {
 	struct file *exe_file;
-	char *pathbuf, *path, *p;
+	char *pathbuf, *path;
 	int ret;
 
 	exe_file = get_mm_exe_file(current->mm);
-	if (!exe_file)
-		return cn_printf(cn, "%s (path unknown)", current->comm);
+	if (!exe_file) {
+		char *commstart = cn->corename + cn->used;
+		ret = cn_printf(cn, "%s (path unknown)", current->comm);
+		cn_escape(commstart);
+		return ret;
+	}
 
 	pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
 	if (!pathbuf) {

@@ -1645,9 +1656,7 @@ static int cn_print_exe_file(struct core_name *cn)
 		goto free_buf;
 	}
 
-	for (p = path; *p; p++)
-		if (*p == '/')
-			*p = '!';
+	cn_escape(path);
 
 	ret = cn_printf(cn, "%s", path);
 

@@ -1719,16 +1728,22 @@ static int format_corename(struct core_name *cn, long signr)
 				break;
 			}
 			/* hostname */
-			case 'h':
+			case 'h': {
+				char *namestart = cn->corename + cn->used;
 				down_read(&uts_sem);
 				err = cn_printf(cn, "%s",
 					      utsname()->nodename);
 				up_read(&uts_sem);
+				cn_escape(namestart);
 				break;
+			}
 			/* executable */
-			case 'e':
+			case 'e': {
+				char *commstart = cn->corename + cn->used;
 				err = cn_printf(cn, "%s", current->comm);
+				cn_escape(commstart);
 				break;
+			}
 			case 'E':
 				err = cn_print_exe_file(cn);
 				break;
Signature · 1 line
1.7.5.3
#2 lore raw

Re: [PATCH v2 2/2] coredump: escape / in hostname and comm

Jiri,

I would like to suggest that both '%e' and '%E' use current->group_leader->comm instead of just current->comm.

Using plain current->comm gets strange effects when threads within a process start naming themselves using PR_SET_NAME (or its pthread_setname_np() equivalent).

Earl

Acked-by on v2 1/2
#3 lore raw
In reply to Jiri Slaby

fs/exec.c static int cn_print_exe_file(struct core_name *cn) · line 1634

Quoted 7 lines · Jiri Slaby
fs/exec.c +1 -1
@@ -1631,7 +1631,7 @@ static int cn_print_exe_file(struct core_name *cn)
 
 	exe_file = get_mm_exe_file(current->mm);
 	if (!exe_file)
-		return cn_printf(cn, "(unknown)");
+		return cn_printf(cn, "%s (path unknown)", current->comm);

Hmm. The patch itself looks fine to me.

Acked-by Oleg Nesterov <[email protected]>

But the code looks wrong.

What if d_path() fails with, say, ENAMETOOLONG? do_coredump() doesn't expect an error code != ENOMEM. This is just ugly, I'll send the simple fix. Anyway, if we are changing cn_print_exe_file(), perhaps it makes sense to fallback if d_path fails too?

And, I am just noticed...

	for (p = path; *p; p++)
		if (*p == '/')
			*p = '!';

Why??? I am not arguing, just curious.

Oleg.

#4 lore raw
Quoted 15 lines · Oleg Nesterov

fs/exec.c static int cn_print_exe_file(struct core_name *cn) · line 1634

Quoted 7 lines · Jiri Slaby
fs/exec.c +1 -1
@@ -1631,7 +1631,7 @@ static int cn_print_exe_file(struct core_name *cn)
 
 	exe_file = get_mm_exe_file(current->mm);
 	if (!exe_file)
-		return cn_printf(cn, "(unknown)");
+		return cn_printf(cn, "%s (path unknown)", current->comm);

Hmm. The patch itself looks fine to me.

Acked-by Oleg Nesterov <[email protected]>
Quoted 6 lines

But the code looks wrong.

What if d_path() fails with, say, ENAMETOOLONG? do_coredump() doesn't expect an error code != ENOMEM. This is just ugly, I'll send the simple fix. Anyway, if we are changing cn_print_exe_file(), perhaps it makes sense to fallback if d_path fails too?

Ah, I see. Perhaps it should check '< 0' instead of '== -ENOMEM' and print the error in that case?

Quoted 7 lines

And, I am just noticed...

	for (p = path; *p; p++)
		if (*p == '/')
			*p = '!';

Why??? I am not arguing, just curious.

In fact the reason is in the patch 2/2:
coredump: escape / in hostname and comm

Change every occurence of / in comm and hostname to !. If the process changes its name to contain /, the core is not dumped (if the directory tree doesn't exist like that). The same with hostname being something like myhost/3. Fix this behaviour by using the escape loop used in %E. (We extract it to a separate function.)

Now both with comm == myprocess/1 and hostname == myhost/1, the core is dumped like (kernel.core_pattern='core.%p.%e.%h):
core.2349.myprocess!1.myhost!1

thanks,

Signature · 1 line
js
#5 lore raw
Quoted 5 lines · Jiri Slaby
Quoted 2 lines · Oleg Nesterov

And, I am just noticed...

Quoted 8 lines
Quoted 5 lines
	for (p = path; *p; p++)
		if (*p == '/')
			*p = '!';

Why??? I am not arguing, just curious.

In fact the reason is in the patch 2/2:
coredump: escape / in hostname and comm

which I can't find ;)

but,

Quoted 2 lines

Change every occurence of / in comm and hostname to !. If the process changes its name to contain /, the core is not dumped

Ah, indeed, somehow I forgot about !ispipe case.

Thanks.

Oleg.

#6 lore raw

PATCH 1/1 do_coredump: fix the "ispipe" error check

In reply to Oleg Nesterov

do_coredump() assumes that if format_corename() fails it should return -ENOMEM. This is not true, for example cn_print_exe_file() can propagate the error from d_path. Even if it was true, this is too fragile. Change the code to check "ispipe < 0".

Signed-off-by Oleg Nesterov <[email protected]>

---

 fs/exec.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
fs/exec.c renamed from fs/exec.c~corename_errcode +6 -6
@@ -2092,16 +2092,16 @@ void do_coredump(long signr, int exit_co
 
 	ispipe = format_corename(&cn, signr);
 
-	if (ispipe == -ENOMEM) {
-		printk(KERN_WARNING "format_corename failed\n");
-		printk(KERN_WARNING "Aborting core\n");
-		goto fail_corename;
-	}
-
  	if (ispipe) {
 		int dump_count;
 		char **helper_argv;
 
+		if (ispipe < 0) {
+			printk(KERN_WARNING "format_corename failed\n");
+			printk(KERN_WARNING "Aborting core\n");
+			goto fail_corename;
+		}
+
 		if (cprm.limit == 1) {
 			/*
 			 * Normally core limits are irrelevant to pipes, since
author Acked-by on v1 1/1
#7 lore raw

Re: [PATCH] do_coredump: fix the "ispipe" error check

Quoted 6 lines · Oleg Nesterov

do_coredump() assumes that if format_corename() fails it should return -ENOMEM. This is not true, for example cn_print_exe_file() can propagate the error from d_path. Even if it was true, this is too fragile. Change the code to check "ispipe < 0".

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Jiri Slaby <[email protected]>
Quoted 21 lines

---

 fs/exec.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
fs/exec.c renamed from fs/exec.c~corename_errcode +0 -6
@@ -2092,16 +2092,16 @@ void do_coredump(long signr, int exit_co
 
 	ispipe = format_corename(&cn, signr);
 
-	if (ispipe == -ENOMEM) {
-		printk(KERN_WARNING "format_corename failed\n");
-		printk(KERN_WARNING "Aborting core\n");
-		goto fail_corename;
-	}
-
  	if (ispipe) {
 		int dump_count;
 		char **helper_argv;

fs/exec.c void do_coredump(long signr, int exit_co · line 2107

Quoted 10 lines
fs/exec.c renamed from fs/exec.c~corename_errcode +6 -0
 
+		if (ispipe < 0) {
+			printk(KERN_WARNING "format_corename failed\n");
+			printk(KERN_WARNING "Aborting core\n");
+			goto fail_corename;
+		}
+
 		if (cprm.limit == 1) {
 			/*
 			 * Normally core limits are irrelevant to pipes, since

thanks,

Signature · 2 lines
js
suse labs
Reviewed-by on v1 1/1
#8 lore raw

Re: [PATCH] do_coredump: fix the "ispipe" error check

In reply to Oleg Nesterov
Quoted 28 lines · Oleg Nesterov

do_coredump() assumes that if format_corename() fails it should return -ENOMEM. This is not true, for example cn_print_exe_file() can propagate the error from d_path. Even if it was true, this is too fragile. Change the code to check "ispipe < 0".

Signed-off-by Oleg Nesterov <[email protected]>

---

 fs/exec.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
fs/exec.c renamed from fs/exec.c~corename_errcode +0 -6
@@ -2092,16 +2092,16 @@ void do_coredump(long signr, int exit_co
 
 	ispipe = format_corename(&cn, signr);
 
-	if (ispipe == -ENOMEM) {
-		printk(KERN_WARNING "format_corename failed\n");
-		printk(KERN_WARNING "Aborting core\n");
-		goto fail_corename;
-	}
-
  	if (ispipe) {
 		int dump_count;
 		char **helper_argv;

fs/exec.c void do_coredump(long signr, int exit_co · line 2107

Quoted 10 lines
fs/exec.c renamed from fs/exec.c~corename_errcode +6 -0
 
+		if (ispipe < 0) {
+			printk(KERN_WARNING "format_corename failed\n");
+			printk(KERN_WARNING "Aborting core\n");
+			goto fail_corename;
+		}
+
 		if (cprm.limit == 1) {
 			/*
 			 * Normally core limits are irrelevant to pipes, since

Looks good. Thanks!

Reviewed-by Neil Horman <[email protected]>