Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753208AbYKFUfp (ORCPT ); Thu, 6 Nov 2008 15:35:45 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751258AbYKFUfh (ORCPT ); Thu, 6 Nov 2008 15:35:37 -0500 Received: from mx3.mail.elte.hu ([157.181.1.138]:50513 "EHLO mx3.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751219AbYKFUfg (ORCPT ); Thu, 6 Nov 2008 15:35:36 -0500 Date: Thu, 6 Nov 2008 21:35:20 +0100 From: Ingo Molnar To: Ken Chen Cc: Alexey Dobriyan , linux-kernel@vger.kernel.org Subject: Re: [patch] add /proc/pid/stack to dump task's stack trace Message-ID: <20081106203520.GD3578@elte.hu> References: <20081106195107.GA17545@x200.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-ELTE-VirusStatus: clean X-ELTE-SpamScore: -1.5 X-ELTE-SpamLevel: X-ELTE-SpamCheck: no X-ELTE-SpamVersion: ELTE 2.0 X-ELTE-SpamCheck-Details: score=-1.5 required=5.9 tests=BAYES_00,DNS_FROM_SECURITYSAGE autolearn=no SpamAssassin version=3.2.3 -1.5 BAYES_00 BODY: Bayesian spam probability is 0 to 1% [score: 0.0000] 0.0 DNS_FROM_SECURITYSAGE RBL: Envelope sender in blackholes.securitysage.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2125 Lines: 68 * Ken Chen 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/