lkml.org 
[lkml]   [2008]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
DateThu, 6 Nov 2008 21:35:20 +0100
FromIngo Molnar <>
SubjectRe: [patch] add /proc/pid/stack to dump task's stack trace

* Ken Chen <kenchen@google.com> wrote:

> On Thu, Nov 6, 2008 at 11:51 AM, Alexey Dobriyan wrote:
> > Please, name handler proc_pid_stack following current convention.
> > And drop space before casts.
>
> OK. handler name changed. For the space between cast, it looks
> like there are different styles in the code base, either with or
> without. I dropped the space since I don't have strong opinion one
> way or the other.

best way is to run scripts/checkpatch.pl on your patch, that will
remind you of any potential style issues.

> Also wrap proc_pid_stack() inside CONFIG_STACKTRACE to fix compile
> time error when config option is not selected.

> +#ifdef CONFIG_STACKTRACE
> +#define MAX_STACK_TRACE_DEPTH 32

How about 64 instead? (it's such a nice round number)

> +static int proc_pid_stack(struct task_struct *task, char *buffer)
> +{
> + int i, len = 0;
> + unsigned long *entries;
> + struct stack_trace trace;
> +
> + entries = kmalloc(sizeof(*entries) * MAX_STACK_TRACE_DEPTH, GFP_KERNEL);
> + if (!entries)
> + goto out;
> +
> + trace.nr_entries = 0;
> + trace.max_entries = MAX_STACK_TRACE_DEPTH;
> + trace.entries = entries;
> + trace.skip = 0;
> +
> + read_lock(&tasklist_lock);
> + save_stack_trace_tsk(task, &trace);
> + read_unlock(&tasklist_lock);
> +
> + for (i = 0; i < trace.nr_entries; i++) {
> + len += sprintf(buffer + len, "[<%p>] %pS\n",
> + (void *)entries[i], (void *)entries[i]);

hm, this looks like a potential buffer overflow - isnt 'buffer' here
only valid up to the next PAGE_SIZE boundary?

> + }

> + kfree(entries);
> +out:
> + return len;

Not sure about the error path convention here: in the !entries kmalloc
failure path, shouldnt we return -ENOMEM? Otherwise userspace will get
zero length and would retry again and again?

Also, please rename 'out:' to 'error:' - to make it clear that it's an
error path.

Ingo


\
 
 \ /
  Last update: 2008-11-06 21:37    [from the cache]
©2003-2010