Message-ID: <CALCETrWan_2AVds=LfneJFDH66swPasirGBiYU+LDK15M_fBew@mail.gmail.com>
Date:   Sun, 7 May 2017 22:35:28 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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 Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> 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 think that, if the code were sufficiently robust, it would be handy
if the unwinder displayed function arguments.  DWARF can do that to a
limited extent.

That being said, having a simpler table format would probably cover
most of the use cases.

Powered by blists - more mailing lists