Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636343 > unrolled thread

[PATCH 7/7] DWARF: add the config option

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-05 14:30 +0200
    Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-05 22:00 +0200
      Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-06 09:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 09:50 +0200
      Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-06 16:30 +0200
      Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-07 23:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-07 23:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Vojtech Pavlik <vojtech@suse.com> - 2017-05-08 10:20 +0200
            Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 15:20 +0200
        Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-08 00:10 +0200
      Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-07 23:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@amacapital.net> - 2017-05-08 07:40 +0200
          Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-08 08:20 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 16:50 +0200
            Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-08 21:10 +0200
        Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-09 02:30 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 03:40 +0200
            Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-09 04:40 +0200
              Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 05:40 +0200
                Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-09 12:10 +0200
                  Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 17:00 +0200
                    Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-09 18:50 +0200
                  Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 10:20 +0200
                    Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 15:10 +0200
                      Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-10 18:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-09 20:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 21:30 +0200
            Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 10:40 +0200
              Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 15:20 +0200
      Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 09:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 14:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 14:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 20:20 +0200

Page 1 of 2  [1] 2  Next page →

#1636343 — [PATCH 7/7] DWARF: add the config option

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.

And we now allow turning off FRAME_POINTERS if DWARF unwinder is
selected.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/Kconfig  |  2 +-
 lib/Kconfig.debug | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index cd18994a9555..b37fa89c5f19 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -689,7 +689,7 @@ config X86_32_IRIS
 config SCHED_OMIT_FRAME_POINTER
 	def_bool y
 	prompt "Single-depth WCHAN output"
-	depends on X86
+	depends on X86 && !DWARF_UNWIND
 	---help---
 	  Calculate simpler /proc/<PID>/wchan values. If this option
 	  is disabled then wchan values will recurse back to the
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e90125b6498e..03297955ece0 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -55,6 +55,17 @@ config UNWIND_INFO
 	  If you don't debug the kernel, you can say N, but we may not be able
 	  to solve problems without frame unwind information or frame pointers.
 
+config DWARF_UNWIND
+	bool "DWARF stack unwind support"
+	depends on UNWIND_INFO && !KASAN
+	depends on X86
+	help
+	  This enables more precise stack traces, omitting all unrelated
+	  occurrences of pointers into kernel code from the dump.
+
+	  KASAN is too slow with this unwinder, so it is excluded from
+	  using in parallel.
+
 config BOOT_PRINTK_DELAY
 	bool "Delay each boot printk message by N milliseconds"
 	depends on DEBUG_KERNEL && PRINTK && GENERIC_CALIBRATE_DELAY
@@ -1690,7 +1701,8 @@ config FAULT_INJECTION_STACKTRACE_FILTER
 	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
 	depends on !X86_64
 	select STACKTRACE
-	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC && !SCORE
+	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !X86 && !ARM_UNWIND && !ARC && !SCORE
+	select UNWIND_INFO if X86 && !FRAME_POINTER
 	help
 	  Provide stacktrace filter for fault-injection capabilities
 
@@ -1699,7 +1711,8 @@ config LATENCYTOP
 	depends on DEBUG_KERNEL
 	depends on STACKTRACE_SUPPORT
 	depends on PROC_FS
-	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
+	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !X86 && !ARM_UNWIND && !ARC
+	select UNWIND_INFO if X86 && !FRAME_POINTER
 	select KALLSYMS
 	select KALLSYMS_ALL
 	select STACKTRACE
-- 
2.12.2

[toc] | [next] | [standalone]

#1636693

On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@suse.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.

                       Linus

[toc] | [prev] | [next] | [standalone]

#1636836

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@suse.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?

Also, why wasn't Josh Poimboeuf Cc:-ed, who de-facto maintains the x86 unwinding 
code?

AFAICS this series is just repeating the old mistakes of the old Dwarf unwinders 
of trusting GCC's unwinder data. So NAK for the time being on the whole approach:

NAcked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1638625

On 05/06/2017, 09:19 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@suse.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?
> 
> Also, why wasn't Josh Poimboeuf Cc:-ed, who de-facto maintains the x86 unwinding 
> code?

I explicitly CCed Josh on 5/7 and 6/7 which touches the code. Besides
that, I assumed he is implicitly CCed via live-patching@vger.kernel.org
which is carbon-copied on each of the patches.

> AFAICS this series is just repeating the old mistakes of the old Dwarf unwinders 
> of trusting GCC's unwinder data. So NAK for the time being on the whole approach:
> 
> NAcked-by: Ingo Molnar <mingo@kernel.org>

OK, as the series stands now, we indeed do. Noteworthy, we, in SUSE, had
no problems with this reliance for all the time we have been using the
unwinder.

Anyway, objtool is about to vaidate the DWARF data, generate it for
assembly and potentially fix it if problems occur. Could you elaborate
on what else would help you to change your stance?

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1636916

[ adding Josh to CC ]

On Fri, 5 May 2017, Linus Torvalds 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?

As a datapoint -- we've been using dwarf unwinder in SUSE kernels since 
stone age, because we do not enable frame pointer, as it adds up to 10% of 
runtime slowdown for certain workloads, but we want reliable stacktraces 
so that we can actually debug user reports.

Now that we have nicely reliable stacktraces thanks to Josh's objtool for 
FP-enabled builds, we'd like to bring comparably understandable traces 
also to non-FP enabled builds.

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

There had been personal issues involved previously, no questions about 
that :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1637012

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

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

That's something I've been thinking about as well: if objtool generates the 
unwinder data structures then the kernel is not directly exposed to tooling bugs 
anymore.

A fair chunk of the fragility of DWARF comes from the fact that it's generated by 
a tool chain that we cannot fix as part of the kernel project. If GCC generates 
crap debuginfo, and GDB happens to work with it but the kernel not, we'll have to 
work it around in the kernel. If GCC starts bloating debuginfo in the future we 
are screwed as well, etc.

If objtool generates debuginfo then it's _our_ responsibility to have sane 
unwinder info and we obviously manage its structure and size as well. Win-win.

The unwinder itself should still do sanity checks, etc. (like all good debugging 
infrastructure code) - but the nature of the kernel's exposure to tool chain 
details changes in a very fundamental way.

So yes, I think this is a very good idea, assuming it works in practice! ;-)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1637014

On Sun, May 07, 2017 at 11:08:19AM -0700, hpa@zytor.com wrote:
> On May 7, 2017 10:59:16 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> >> 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.
> >
> >That's something I've been thinking about as well: if objtool generates
> >the 
> >unwinder data structures then the kernel is not directly exposed to
> >tooling bugs 
> >anymore.
> >
> >A fair chunk of the fragility of DWARF comes from the fact that it's
> >generated by 
> >a tool chain that we cannot fix as part of the kernel project. If GCC
> >generates 
> >crap debuginfo, and GDB happens to work with it but the kernel not,
> >we'll have to 
> >work it around in the kernel. If GCC starts bloating debuginfo in the
> >future we 
> >are screwed as well, etc.
> >
> >If objtool generates debuginfo then it's _our_ responsibility to have
> >sane 
> >unwinder info and we obviously manage its structure and size as well.
> >Win-win.
> >
> >The unwinder itself should still do sanity checks, etc. (like all good
> >debugging 
> >infrastructure code) - but the nature of the kernel's exposure to tool
> >chain 
> >details changes in a very fundamental way.
> >
> >So yes, I think this is a very good idea, assuming it works in
> >practice! ;-)
> >
> >Thanks,
> >
> >	Ingo
> 
> Can objtool verify the unwinder at each address in the kernel, or is that an AI-complete problem?

It can't verify the *unwinder*, but it can verify the data which is fed
to the unwinder (either DWARF or the structs I proposed above).  For
each function, it follows every possible code path, and it can keep
track of the stack pointer while doing so.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637299

On Sun, May 07, 2017 at 04:48:36PM -0500, Josh Poimboeuf wrote:

> > Can objtool verify the unwinder at each address in the kernel, or is that an AI-complete problem?
> 
> It can't verify the *unwinder*, but it can verify the data which is fed
> to the unwinder (either DWARF or the structs I proposed above).  For
> each function, it follows every possible code path, and it can keep
> track of the stack pointer while doing so.

In that case, the kernel build process can verify the DWARF data and its
compatibility with the kernel unwinder by running the unwinder against
each kernel code address verifying the output and bail if there is a bug
in the toolchain that affects it.

-- 
Vojtech Pavlik
Director SuSE Labs

[toc] | [prev] | [next] | [standalone]

#1637439

On Mon, May 08, 2017 at 09:50:54AM +0200, Vojtech Pavlik wrote:
> On Sun, May 07, 2017 at 04:48:36PM -0500, Josh Poimboeuf wrote:
> 
> > > Can objtool verify the unwinder at each address in the kernel, or is that an AI-complete problem?
> > 
> > It can't verify the *unwinder*, but it can verify the data which is fed
> > to the unwinder (either DWARF or the structs I proposed above).  For
> > each function, it follows every possible code path, and it can keep
> > track of the stack pointer while doing so.
> 
> In that case, the kernel build process can verify the DWARF data and its
> compatibility with the kernel unwinder by running the unwinder against
> each kernel code address verifying the output

If I understand the idea correctly, we'd have to make the unwinder
dual-purpose such that it can run both in the kernel and in some kind of
user space objtool test harness.  The stack wouldn't be real, so
presumably each iteration of the test would only unwind a frame
associated with the current function.

It wouldn't be able to test edge cases like entry code and generated
code which aren't normal "functions", which objtool currently has no way
of understanding.  Also it wouldn't test how the unwinder deals with
corrupt DWARF data or corrupt stacks, unless we integrated some kind of
fuzzer in the harness.

And, at the end of the day, we'd still just be testing in an artificial
unit test environment.  So I'm not really crazy about the idea.

> and bail if there is a bug in the toolchain that affects it.

Objtool can already find _toolchain_ bugs without having to run the
unwinder in some kind of emulator.  It can't find _unwinder_ bugs, but I
really think such testing should be done at runtime in the unwinder's
native kernel environment.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637059

On May 7, 2017 10:59:16 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
>> 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.
>
>That's something I've been thinking about as well: if objtool generates
>the 
>unwinder data structures then the kernel is not directly exposed to
>tooling bugs 
>anymore.
>
>A fair chunk of the fragility of DWARF comes from the fact that it's
>generated by 
>a tool chain that we cannot fix as part of the kernel project. If GCC
>generates 
>crap debuginfo, and GDB happens to work with it but the kernel not,
>we'll have to 
>work it around in the kernel. If GCC starts bloating debuginfo in the
>future we 
>are screwed as well, etc.
>
>If objtool generates debuginfo then it's _our_ responsibility to have
>sane 
>unwinder info and we obviously manage its structure and size as well.
>Win-win.
>
>The unwinder itself should still do sanity checks, etc. (like all good
>debugging 
>infrastructure code) - but the nature of the kernel's exposure to tool
>chain 
>details changes in a very fundamental way.
>
>So yes, I think this is a very good idea, assuming it works in
>practice! ;-)
>
>Thanks,
>
>	Ingo

Can objtool verify the unwinder at each address in the kernel, or is that an AI-complete problem?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1637032

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@suse.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

[toc] | [prev] | [next] | [standalone]

#1637216

On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@redhat.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@suse.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.

[toc] | [prev] | [next] | [standalone]

#1637236

* Andy Lutomirski <luto@amacapital.net> wrote:

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

I'd say that if objtool generates the kernel's debuginfo, it all becomes a kernel 
internal matter to a large degree: we can add function argument display support as 
well and see what effect it has on data structure size and complexity - it would 
certainly be a nice improvement in oops output to see function arguments.

But it should all start from a minimum complexity step (which will be complex 
enough!) and the first step should be stack trace display, in a performance 
optimized data structure, to allow both kernel stack dumps and various tracing and 
other instrumentation facilities that make use of the kernel's dwarf-ish unwider 
as-is.

The goal of this first step would be to allow x86 to drop generating the RBP frame 
pointer:

 - This shrinks kernel text and instruction count (by 1-2% IIRC),
 - speeds up certain code paths measurably,
 - plus it also allows larger functions to use one more general purpose register.

... so it's a good thing to have, if and only if the unwinder's fragility and 
complexity does not kill us. And much of that fragility comes from who generates 
the debuginfo ...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1637482

On Sun, May 07, 2017 at 10:35:28PM -0700, Andy Lutomirski wrote:
> 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.

Honestly I get the feeling that displaying function arguments wouldn't
be realistic (DWARF or no DWARF).  On x86-64, arguments are passed in
registers, so tracking down their values is a lot more involved than
just looking at the stack.

The DWARF CFI only shows you the callee-saved registers.  To figure out
the other registers you'd have to dive into the other DWARF sections and
examine previous stack frames for clues.  I think it's not a
deterministic process, based on how often I see gdb complain with
'<value optimized out>'.  I'd bet it's a lot harder than a basic stack
dump.

Also, most kernel functions rely on pointer arguments, which are pretty
much useless without dumping the contents of the structs they point to.
But then doing that properly would be a whole new level of difficulty.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637653

On May 8, 2017 7:40:49 AM PDT, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>On Sun, May 07, 2017 at 10:35:28PM -0700, Andy Lutomirski wrote:
>> 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.
>
>Honestly I get the feeling that displaying function arguments wouldn't
>be realistic (DWARF or no DWARF).  On x86-64, arguments are passed in
>registers, so tracking down their values is a lot more involved than
>just looking at the stack.
>
>The DWARF CFI only shows you the callee-saved registers.  To figure out
>the other registers you'd have to dive into the other DWARF sections
>and
>examine previous stack frames for clues.  I think it's not a
>deterministic process, based on how often I see gdb complain with
>'<value optimized out>'.  I'd bet it's a lot harder than a basic stack
>dump.
>
>Also, most kernel functions rely on pointer arguments, which are pretty
>much useless without dumping the contents of the structs they point to.
>But then doing that properly would be a whole new level of difficulty.

At some point you are just reinventing k(g)db...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1637796

On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@redhat.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@suse.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[];

Some comments in case you're actually planning to do this:

'unsigned int ip' is the majority of the size of this thing.  It might
be worth trying to store a lot fewer bits.  You could split the
structure into:

struct undwarf_header {
  unsigned long start_ip;
  unsigned align:2;  /* i'm assuming this rarely changes */
  ...;
  unsigned int offset_to_details;
};

and

struct undwarf_details {
  unsigned short ip_offset;
  unsigned short prev_frame;
};

and you'd find the details by first looking up the last header before
the ip and then finding the details starting at (uintptr_t)header +
header->offset_to_details.

Also, don't you need some indication of which reg is the base from
which you find previous frame?  After all, sometimes GCC will emit a
frame pointer even in an otherwise frame-pointer-omitting kernel.

--Andy

[toc] | [prev] | [next] | [standalone]

#1637808

On Mon, May 08, 2017 at 05:21:24PM -0700, Andy Lutomirski wrote:
> On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >         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[];
> 
> Some comments in case you're actually planning to do this:
> 
> 'unsigned int ip' is the majority of the size of this thing.  It might
> be worth trying to store a lot fewer bits.  You could split the
> structure into:
> 
> struct undwarf_header {
>   unsigned long start_ip;
>   unsigned align:2;  /* i'm assuming this rarely changes */
>   ...;
>   unsigned int offset_to_details;
> };
> 
> and
> 
> struct undwarf_details {
>   unsigned short ip_offset;
>   unsigned short prev_frame;
> };
> 
> and you'd find the details by first looking up the last header before
> the ip and then finding the details starting at (uintptr_t)header +
> header->offset_to_details.

Good idea.  According to some back-of-a-napkin math, a scheme like this
could reduce the data size from 1.8M down to 1.2M with my kernel config,
a not-too-shabby 600k savings.

> Also, don't you need some indication of which reg is the base from
> which you find previous frame?  After all, sometimes GCC will emit a
> frame pointer even in an otherwise frame-pointer-omitting kernel.

I don't think we *need* to do that.  I believe the base reg can just
always[*] be the stack pointer, even with frame pointers.

That said, it might be beneficial to use the frame pointer as the base
reg where applicable, because it would shrink the data size, especially
in a kernel with frame pointers enabled.  And I think we would only need
an extra bit to store that info.

First I'll just start with the simplest possible scheme (i.e., what I
proposed above).  Even that calculates out to be smaller than the DWARF
.eh_frame stuff.  Then after that we can look at compression techniques
(and their associated tradeoffs).

[*] ignoring rare gcc stack realignments

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637833

On Mon, May 8, 2017 at 6:38 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, May 08, 2017 at 05:21:24PM -0700, Andy Lutomirski wrote:
>> On Sun, May 7, 2017 at 9:55 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >         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[];
>>
>> Some comments in case you're actually planning to do this:
>>
>> 'unsigned int ip' is the majority of the size of this thing.  It might
>> be worth trying to store a lot fewer bits.  You could split the
>> structure into:
>>
>> struct undwarf_header {
>>   unsigned long start_ip;
>>   unsigned align:2;  /* i'm assuming this rarely changes */
>>   ...;
>>   unsigned int offset_to_details;
>> };
>>
>> and
>>
>> struct undwarf_details {
>>   unsigned short ip_offset;
>>   unsigned short prev_frame;
>> };
>>
>> and you'd find the details by first looking up the last header before
>> the ip and then finding the details starting at (uintptr_t)header +
>> header->offset_to_details.
>
> Good idea.  According to some back-of-a-napkin math, a scheme like this
> could reduce the data size from 1.8M down to 1.2M with my kernel config,
> a not-too-shabby 600k savings.
>
>> Also, don't you need some indication of which reg is the base from
>> which you find previous frame?  After all, sometimes GCC will emit a
>> frame pointer even in an otherwise frame-pointer-omitting kernel.
>
> I don't think we *need* to do that.  I believe the base reg can just
> always[*] be the stack pointer, even with frame pointers.

What if there are functions that use alloca or variable length arrays
on the stack?  Functions using AHASH_REQUEST_ON_STACK come to mind.

--Andy

[toc] | [prev] | [next] | [standalone]

#1637850

On Mon, May 08, 2017 at 07:31:50PM -0700, Andy Lutomirski wrote:
> On Mon, May 8, 2017 at 6:38 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> Also, don't you need some indication of which reg is the base from
> >> which you find previous frame?  After all, sometimes GCC will emit a
> >> frame pointer even in an otherwise frame-pointer-omitting kernel.
> >
> > I don't think we *need* to do that.  I believe the base reg can just
> > always[*] be the stack pointer, even with frame pointers.
> 
> What if there are functions that use alloca or variable length arrays
> on the stack?  Functions using AHASH_REQUEST_ON_STACK come to mind.

Wow, mind blown.  This is why I added you to CC!

Ok, I guess we'll need to be able to use the frame pointer as a base
reg.  It should be easy anyway.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638023

On May 8, 2017 8:38:31 PM PDT, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>On Mon, May 08, 2017 at 07:31:50PM -0700, Andy Lutomirski wrote:
>> On Mon, May 8, 2017 at 6:38 PM, Josh Poimboeuf <jpoimboe@redhat.com>
>wrote:
>> >> Also, don't you need some indication of which reg is the base from
>> >> which you find previous frame?  After all, sometimes GCC will emit
>a
>> >> frame pointer even in an otherwise frame-pointer-omitting kernel.
>> >
>> > I don't think we *need* to do that.  I believe the base reg can
>just
>> > always[*] be the stack pointer, even with frame pointers.
>> 
>> What if there are functions that use alloca or variable length arrays
>> on the stack?  Functions using AHASH_REQUEST_ON_STACK come to mind.
>
>Wow, mind blown.  This is why I added you to CC!
>
>Ok, I guess we'll need to be able to use the frame pointer as a base
>reg.  It should be easy anyway.

[Adding H.J. Lu for obvious expertise - H.J., the issue at hand is doing stack unwinding to display the call stack on a kernel failure.  Right now the standard kernel configuration is using frame pointers even on x86-64 because we had very bad experiences with fragility of .eh_frame-based unwinding.  However, on some kernel workloads the cost of frame pointers  I is quite high.]

Yes is exactly why.  I believe gcc will still use a frame pointer when the relationship between sp and the incoming stack frame is indeterminate for some reason, but I have to admit that a) I'm not 100% sure and b) there are DWARF annotations for exactly this, so even if it is true for current gcc that a frame pointer is not needed it might not be in the future.

As far as I understand, the .eh_frame section is supposed to contain the subset of the DWARF bytecode needed to do a stack unwind when an exception is thrown, whereas the .debug* sections contain the full DWARF data a debugger might want.  Thus .eh_frame is mapped into the runtime process while .debug* [usually?] is not.  .debug* can easily be 10x larger than the executable text segments.

Since C doesn't *have* exceptions, the main user of this for C code is the case of calls C++ > C > C++ or equivalent; thus the vulnerability to toolchain filters failures – the case of an exception-caused unwind in the middle of a C function might not have been extensively tested.  [H.J.: is that correct?  Or could an asynchronous event like a signal cause an unwind of the .eh_frame from an arbitrary point?  If so, how is that tested?]

In the case of the kernel, size matters in a different way, because even though it will be cache cold this data takes up unreclaimable RAM.  However, frame pointer-related code eats up not just RAM but cache.

Assembly language routines become problematic no matter what you do unless you restrict the way the assembly can be written.  Experience has shown us that hand-maintaining annotations in assembly code is doomed to failure, and in many cases it isn't even clear to even experienced programmers how to do it.  [H.J.: is the .cfi_* operations set even documented anywhere in a way that non-compiler-writers can comprehend it?] Inline assembly becomes problematic in the case of non-frame-pointer builds if the assembly changes the stack pointer.  A static tool might be able to annotate simple cases like push/pop.

I'm, ahem, highly skeptical to creating our own unwinding data format unless there is *documented, supported, and tested* way to force the compiler to *automatically* fall back to frame pointers any time there may be complexity involved, which at a very minimum includes any kind of data-dependent manipulation of the stack pointer.  Otherwise you will have to fail the kernel build when your static tool runs into instruction sequences it can't deal with, but the compiler may generate them - boom.  Worse, your tool will not even recognize the problem and you're in a worse place than when you started.


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web