linux-kernel - Re: [patch] sched: fix single-depth wchan output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081106074417.GB8459@elte.hu>
Date:	Thu, 6 Nov 2008 08:44:17 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Ken Chen <kenchen@...gle.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [patch] sched: fix single-depth wchan output

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

> On Wed, Nov 5, 2008 at 10:59 PM, Ingo Molnar <mingo@...e.hu> wrote:
> > If it does stack walking manually then please update it to use
> > save_stack_trace() instead - that is the standard API that will
> > utilize the best possible stack walking machinery on the architecture
> > level.
> 
> OK, I pulled the patch out of our code that export stack trace via 
> /proc/pid/trace.  I didn't write this patch, but I think a better 
> choice would be to override struct stacktrace_ops print_trace_ops 
> with a memory buffer pointer to dump the stack into.  If you have 
> any comments, please let me know.  I will polish this patch up and 
> rebase to git-head.

hm, instead of modifying the lowlevel arch dump code, why not just use 
the existing save_stack_trace(), and render the output yourself via a 
trivial sprintf, just like kernel/lockdep.c does?

See kernel/stacktrace.c's print_stack_trace() - that could be extended 
with a sprintf_stack_trace() method. Allocate a large enough buffer 
dynamically, with a max of 128 stacktrace entries or so. (the output 
buffer is limited to 4K-ish anyway, right?)

As a bonus this will work on every architecture, not just x86.

a few other details:

> -	char namebuf[KSYM_NAME_LEN];
> +	char namebuf[128];

... time machine back to old crappy code ;-)

>  	symname = kallsyms_lookup(address, &symsize, &offset,
> -					&modname, namebuf);
> +				  &modname, namebuf);

ditto. But none of this has to be modified so you can just drop these 
bits.

> +	read_lock(&tasklist_lock);
> +	buf_show_task(posp, end, task);
> +	read_unlock(&tasklist_lock);

to get a stable trace we'll need more locking than that (tasklist_lock 
does not exclude scheduling, etc.) - but it takes care of the most 
important detail: tasks exiting from under us. So this should be OK.

>  #endif
> +	INF("trace",  S_IFREG|S_IRUGO, pid_trace),

that needs to be r-------- instead of r--r--r--, for security reasons.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/