Messages in this thread |  | | | Date | Thu, 6 Nov 2008 21:35:20 +0100 | | From | Ingo Molnar <> | | Subject | Re: [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
|  |