Message-ID: <20170507165524.cdxfuwbd5alr7v6k@treble>
Date:   Sun, 7 May 2017 11:55:24 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jiri Slaby <jslaby@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        live-patching@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Jiri Kosina <jikos@...nel.org>
Subject: Re: [PATCH 7/7] DWARF: add the config option

On Fri, May 05, 2017 at 12:57:11PM -0700, Linus Torvalds wrote:
> On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@...e.cz> wrote:
> > The DWARF unwinder is in place and ready. So introduce the config option
> > to allow users to enable it. It is by default off due to missing
> > assembly annotations.
> 
> Who actually ends up using this?
> 
> Because from the last time we had fancy unwindoers, and all the
> problems it caused for oops handling with absolutely _zero_ upsides
> ever, I do not ever again want to see fancy unwinders with complex
> state machine handling used by the oopsing code.
> 
> The fact that it gets disabled for KASAN also makes me suspicious. It
> basically means that now all the accesses it does are not even
> validated.
> 
> The fact that the most of the code seems to be disabled for the first
> six patches, and then just enabled in the last patch, also seems to
> mean that the series also gets no bisection coverage or testing that
> the individual patches make any sense. (ie there's a lot of code
> inside "CONFIG_DWARF_UNWIND" in the early patches but that config
> option cannot even be enabled until the last patch).
> 
> We used to have nasty issues with not just missing dwarf info, but
> also actively *wrong* dwarf info. Compiler versions that generated
> subtly wrong info, because nobody actually really depended on it, and
> the people who had tested it seldom did the kinds of things we do in
> the kernel (eg inline asms etc).
> 
> So I'm personally still very suspicious of these things.
> 
> Last time I had huge issues with people also always blaming *anything*
> else than that unwinder. It was always "oh, somebody wrote asm without
> getting it right". Or "oh, the compiler generated bad tables, it's not
> *my* fault that now the kernel oopsing code no longer works".
> 
> When I asked for more stricter debug table validation to avoid issues,
> it was always "oh, we fixed it, no worries", and then two months later
> somebody hit another issue.
> 
> Put another way; the last time we did crazy stuff like this, it got
> reverted. For a damn good reason, despite some people being in denial
> about those reasons.

Here's another possible idea that's been rattling around in my head.
It's purely theoretical at this point, so I don't know for sure that it
would work.  But I haven't been able to find any major issues with it
yet.

DWARF is great for debuggers.  It helps you find all the registers on
the stack, so you can see function arguments and local variables.  All
expressed in a nice compact format.

But that's overkill for unwinders.  We don't need all those registers,
and the state machine is too complicated.  Unwinders basically only need
to know one thing: given an instruction address and a stack pointer,
where is the caller's stack frame?

I'm thinking/hoping that information can be expressed in a simple, easy
to parse, reasonably sized data structure.  Something like a sorted
array of this:

	struct undwarf {
		unsigned int ip;	/* instruction pointer (relative offset from base) */
		unsigned prev_frame:13;	/* offset to previous frame from current stack pointer */
		unsigned regs:1;	/* whether prev_frame contains entry regs (regs->ip) */
		unsigned align:2;	/* some details for dealing with gcc stack realignment */
	} __packed;

	extern struct undwarf undwarves[];

One instance of the structure would exist for each time the stack
pointer changes, e.g. for every function entry, push/pop, and rsp
add/subtract.  The data could be assembled and sorted offline, possibly
derived from DWARF, or more likely, generated by objtool.  After doing
some rough calculations, I think the section size would be comparable to
the sizes of the DWARF .eh_frame sections it would replace.

If it worked, the "undwarf" unwinder would be a lot simpler than a real
DWARF unwinder.  And validating the sanity of the data at runtime would
be a lot more straightforward.  It could ensure that each stack pointer
is within the bounds of the current stack, like our current unwinder
does.

It could also be easily plugged into the existing x86 unwinder
framework, and would "just work" with the oops dumping code
(show_trace_log_lvl), where if something goes wrong, all the remaining
found text addresses on the stack still get printed with the '?' prefix.

Modules could also have their own undwarf tables.

We could also add reliability checks and warnings to make it suitable
for live patching.  That combined with a debug feature to do periodic
unwinds from an NMI handler should flush out most issues over time.

Taking the idea further, this could even (eventually) support the
unwinding of generated code like bpf and dynamic ftrace trampolines.
When generating code, they could also register undwarf structs
associated with the code, which could be stored in a separate undwarf
list.

I'm not 100% sure it would work, but I can start prototyping it if there
aren't any objections.  The unwinder itself would be easy.  Most of the
work would be in tooling, though much of the needed tooling actually
already exists (in objtool).

-- 
Josh

Powered by blists - more mailing lists