linux-kernel - Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

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: <CA+55aFwF+qCGSKdGaEgW4p6N65GZ5_XTV=1NbtWDvxnd5yYLiw@mail.gmail.com>
Date:	Sun, 23 Nov 2014 14:51:57 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	stable <stable@...r.kernel.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Subject: Re: [GIT PULL] ftrace/x86: Add frames pointers to trampoline as necessary

On Sat, Nov 22, 2014 at 5:05 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> Honestly, I did it this way because it was the simplest way to do it.

So the thing is, I already dislike the whole setup, and your patch
just makes it worse.

And part of the problem is that there is *already* too many stupid
layers of indirection, and "setup" macros.

And yes, they really look pretty stupid.

There's MCOUNT_SAVE_FRAME, which is supposed to create a frame, but,
as you found out, doesn't actually do a good job of creating a real
frame, despite its name.

It's also in a really stupid place, excuse my french. Why is it in
that header file?

Then there's "ftrace_caller_setup", which uses MCOUNT_SAVE_FRAME, and
sets up %rsi/rdi to have the parent/rip stuff.

And that one does a particularly bad job *too*, since it's used in
only two of the four places that want this, with the other two doing
the whole thing by hand (look for things like

        movq RIP(%rsp), %rsi
        subq $MCOUNT_INSN_SIZE, %rsi

being duplicated..)

And it's also being particularly stupid about this all, because of the
horrible MCOUNT_SAVE_FRAME placement somewhere else. That
MCOUNT_SAVE_FRAME macro actually loads that %rip value into %rdx, but
because of the extra indirection and odd extra macro in a header file
(even though that one *.S file is the only apparent user),
"ftrace_caller_setup" doesn't actually realize that, and instead just
reloads it from where the previous macro just saved it. So the code it
generates just looks bad too.

And now your patch adds a *third* macro, to make up for the fact that
the first macro did a bad job.

You already have too many of these macros, and they are already ugly.
Why not just *fix* the existing macro, instead of adding yet another
layer of confusion and horror?

> If you want, I can add more comments to explain all this.

I'd really prefer the underlying problem to be fixed, not more
explanations for odd code choices.

Why can't MCOUNT_SAVE_FRAME just save the frame right? Maybe even do
it unconditionally - the cost of setting up a frame is not that huge,
and it might be better to have more understandable code and avoid yet
another ifdef for a config option.

And while at it, why not move it to where it's used, and maybe make it
not scream?

And maybe "ftrace_caller_setup" could be fixed so that the two cases
that now do that magic MCOUNT_INSN_SIZE stuff could use it?

Looking at that file, I just get the heebie-jeebies. It's like a
Halloween house of horrors of macros and #ifdefs. I'd really prefer
for a fix to not just be band-aid that makes it even worse.

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