linux-kernel - Re: [RFC][PATCH 0/3] ftrace: Add dynamically allocated trampolines

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 22 Jul 2014 18:47:07 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Slaby <jslaby@...e.cz>
Subject: Re: [RFC][PATCH 0/3] ftrace: Add dynamically allocated trampolines

On 07/03, Steven Rostedt wrote:
>
> [ NOT READY FOR INCLUSION! ]
>
> Note, this is based off of my remove ftrace_start/stop() patch set.

So I simply pulled your tree. I can't really comment these changes simply
because I do not understand this code. But I am hunting for RHEL bug in
(probably) this area, so I decided to take a look in a hope that may be
this can help me to understand the current code ;)

> The way the function callback mechanism works in ftrace is that if there's
> only one function callback registered, it will set the mcount/fentry
> trampoline to call that function directly. But as soon as you register
> another callback, the mcount trampoline calls a loop function that iterates
> over all the registered callbacks (ftrace_ops) checking their hash tables
> to see if the called function matches the ops before calling its callback.
> This happens even if the two registered functions are not even tracing
> the same function!
>
> This really sucks if you are tracing all functions, and then add a kprobe
> or perf event that traces a single function. That will cause all the
> other functions being traced to perform the loop test.

But this is even worse or I missed something? I mean, currently even
if you trace nothing and then add a KPROBE_FLAG_FTRACE kprobe, then
kprobe_ftrace_handler() is called by ftrace_ops_list_func() ?

After these changes it seems that kprobe will use a trampoline.

And I can't understand even the basic code. Say, __ftrace_hash_rec_update:

		if (inc) {
			rec->flags++;
			if (FTRACE_WARN_ON(ftrace_rec_count(rec) == FTRACE_REF_MAX))
				return;

			/*
			 * If there's only a single callback registered to a
			 * function, and the ops has a trampoline registered
			 * for it, then we can call it directly.
			 */
			if (ftrace_rec_count(rec) == 1 && ops->trampoline) {
				rec->flags |= FTRACE_FL_TRAMP;
				ops->trampolines++;
			} else {
				/*
				 * If we are adding another function callback
				 * to this function, and the previous had a
				 * trampoline used, then we need to go back to
				 * the default trampoline.
				 */
				rec->flags &= ~FTRACE_FL_TRAMP;

				/* remove trampolines from any ops for this rec */
				ftrace_clear_tramps(rec);
			}

It seems that "else if (ftrace_rec_count(rec) == 2)" can avoid the unnecessary
ftrace_clear_tramps() ? And not only unnecessary, ftrace_clear_tramps() decrements
->trampolines, can't this break the accounting?

		} else {
			if (FTRACE_WARN_ON(ftrace_rec_count(rec) == 0))
				return;
			rec->flags--;

			if (ops->trampoline && !ftrace_rec_count(rec))
				ftrace_remove_tramp(ops, rec);

I am wondering what should we do if ftrace_rec_count() becomes 1 again...

ftrace_save_ops_tramp_hash():

	do_for_each_ftrace_rec(pg, rec) {
		if (ftrace_rec_count(rec) == 1 &&
		    ftrace_ops_test(ops, rec->ip, rec)) {

			/* This record had better have a trampoline */
			if (FTRACE_WARN_ON(!(rec->flags & FTRACE_FL_TRAMP_EN)))
				return -1;

Yes, but I can't understand how this can work.

This ops can have  ->trampolines > 0, but FTRACE_FL_TRAMP_EN can be cleared
by another ftrace_ops?

Suppose there is a single tracer of this function, rec->flags = TRAMP | TRAMP_EN.
Suppose also that it traces more than 1 function, so ->trampolines > 1.

Another tracer comes, __ftrace_hash_rec_update() clears TRAMP. But it should
also do ftrace_check_record() and this should clear TRAMP_EN?

And yes, I can trigger this bug if I simply do "echo function > current_tracer"
and then add/remove a KPROBE_FLAG_FTRACE kprobe.


And you know, when I try to read this code I can't distinguish ->trampoline
from ->trampolines ;)

Oleg.

--
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/