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(-)
diff --git a/fs/exec.c b/fs/exec.c
index a9f2b36..2093c47 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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) {
--
1.7.5.3
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(-)
diff --git a/fs/exec.c b/fs/exec.c
index 2093c47..1027025 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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;
--
1.7.5.3
On 06/07, Jiri Slaby wrote:
>
> @@ -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.
On 06/07/2011 08:16 PM, Oleg Nesterov wrote:
> On 06/07, Jiri Slaby wrote:
>>
>> @@ -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?
Ah, I see. Perhaps it should check '< 0' instead of '== -ENOMEM' and
print the error in that case?
> 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,
--
js
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(-)
--- ptrace/fs/exec.c~corename_errcode 2011-06-07 19:44:30.000000000 +0200
+++ ptrace/fs/exec.c 2011-06-07 20:20:48.000000000 +0200
@@ -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
On 06/07/2011 08:35 PM, Oleg Nesterov wrote:
> 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]>
> ---
>
> fs/exec.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> --- ptrace/fs/exec.c~corename_errcode 2011-06-07 19:44:30.000000000 +0200
> +++ ptrace/fs/exec.c 2011-06-07 20:20:48.000000000 +0200
> @@ -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
>
thanks,
--
js
suse labs
On Tue, Jun 07, 2011 at 08:35:42PM +0200, Oleg Nesterov wrote:
> 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(-)
>
> --- ptrace/fs/exec.c~corename_errcode 2011-06-07 19:44:30.000000000 +0200
> +++ ptrace/fs/exec.c 2011-06-07 20:20:48.000000000 +0200
> @@ -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
>
>
Looks good. Thanks!
Reviewed-by: Neil Horman <[email protected]>
On 06/07, Jiri Slaby wrote:
>
> On 06/07/2011 08:16 PM, Oleg Nesterov wrote:
>
> > 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
which I can't find ;)
but,
> 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.