Re: [patch] add /proc/pid/stack to dump task's stack trace

[Ingo Molnar <mingo@elte.hu>]

* 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