Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1212292 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2015-08-24 17:00 +0200 |
| Last post | 2015-08-28 22:00 +0200 |
| Articles | 12 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v11 00/20] Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-24 17:00 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Ingo Molnar <mingo@kernel.org> - 2015-08-25 10:20 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-25 17:30 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Ingo Molnar <mingo@kernel.org> - 2015-08-26 09:10 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Jiri Kosina <jikos@kernel.org> - 2015-08-26 10:50 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-27 15:20 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Ingo Molnar <mingo@kernel.org> - 2015-08-28 10:30 +0200
Re: [PATCH v11 00/20] Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-28 16:00 +0200
Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation Andi Kleen <andi@firstfloor.org> - 2015-08-26 16:30 +0200
Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-27 16:40 +0200
Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation Andi Kleen <andi@firstfloor.org> - 2015-08-28 19:30 +0200
Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-28 22:00 +0200
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-24 17:00 +0200 |
| Subject | [PATCH v11 00/20] Compile-time stack validation |
| Message-ID | <q11ep-2f3-3@gated-at.bofh.it> |
This is v11 of the compile-time stack validation patch set, along with
proposed fixes for many of the warnings it found. It's based on the
tip/master branch.
The only real change since v10 is some improvements in patch 3 to the
documentation and changelog which attempt to better describe why stack
validation is needed.
v10 can be found here:
https://lkml.kernel.org/r/cover.1439521412.git.jpoimboe@redhat.com
For more information about the motivation behind this patch set, and
more details about what it does, please see the changelog in patch 3.
Patch 3 also has Documentation/stack-validation.txt which has further
details.
Patches 1-5 are the stackvalidate tool and some related macros.
Patches 6-20 are some proposed fixes for several of the warnings
reported by stackvalidate. They've been compile-tested and boot tested
in a VM, but I haven't attempted any meaningful testing for many of
them.
v11:
- attempt to answer the "why" question better in the documentation and
commit message
- s/FP_SAVE/FRAME_BEGIN/ in documentation
v10:
- add scripts/mod to directory ignores
- remove circular dependencies for ignored objects which are built
before stackvalidate
- fix CONFIG_MODVERSIONS incompatibility
v9:
- rename FRAME/ENDFRAME -> FRAME_BEGIN/FRAME_END
- fix jump table issue for when the original instruction is a jump
- drop paravirt thunk alignment patch
- add maintainers to CC for proposed warning fixes
v8:
- add proposed fixes for warnings
- fix all memory leaks
- process ignores earlier and add more ignore checks
- always assume POPCNT alternative is enabled
- drop hweight inline asm fix
- drop __schedule() ignore patch
- change .Ltemp_\@ to .Lstackvalidate_ignore_\@ in asm macro
- fix CONFIG_* checks in asm macros
- add C versions of ignore macros and frame macros
- change ";" to "\n" in C macros
- add ifdef CONFIG_STACK_VALIDATION checks in C ignore macros
- use numbered label in C ignore macro
- add missing break in switch case statement in arch-x86.c
v7:
- sibling call support
- document proposed solution for inline asm() frame pointer issues
- say "kernel entry/exit" instead of "context switch"
- clarify the checking of switch statement jump tables
- discard __stackvalidate_ignore_* sections in linker script
- use .Ltemp_\@ to get a unique label instead of static 3-digit number
- change STACKVALIDATE_IGNORE_FUNC variable to a static
- move STACKVALIDATE_IGNORE_INSN to arch-specific .h file
v6:
- rename asmvalidate -> stackvalidate (again)
- gcc-generated object file support
- recursive branch state analysis
- external jump support
- fixup/exception table support
- jump label support
- switch statement jump table support
- added documentation
- detection of "noreturn" dead end functions
- added a Kbuild mechanism for skipping files and dirs
- moved frame pointer macros to arch/x86/include/asm/frame.h
- moved ignore macros to include/linux/stackvalidate.h
v5:
- stackvalidate -> asmvalidate
- frame pointers only required for non-leaf functions
- check for the use of the FP_SAVE/RESTORE macros instead of manually
analyzing code to detect frame pointer usage
- additional checks to ensure each function doesn't leave its boundaries
- make the macros simpler and more flexible
- support for analyzing ALTERNATIVE macros
- simplified the arch interfaces in scripts/asmvalidate/arch.h
- fixed some asmvalidate warnings
- rebased onto latest tip asm cleanups
- many more small changes
v4:
- Changed the default to CONFIG_STACK_VALIDATION=n, until all the asm
code can get cleaned up.
- Fixed a stackvalidate error path exit code issue found by Michal
Marek.
v3:
- Added a patch to make the push/pop CFI macros arch-independent, as
suggested by H. Peter Anvin
v2:
- Fixed memory leaks reported by Petr Mladek
Cc: linux-kernel@vger.kernel.org
Cc: live-patching@vger.kernel.org
Cc: Michal Marek <mmarek@suse.cz>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Pedro Alves <palves@redhat.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Bernd Petrovitsch <bernd@petrovitsch.priv.at>
Cc: "Chris J Arges" <chris.j.arges@canonical.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Josh Poimboeuf (20):
x86/asm: Frame pointer macro cleanup
x86/asm: Add C versions of frame pointer macros
x86/stackvalidate: Compile-time stack validation
x86/stackvalidate: Add file and directory ignores
x86/stackvalidate: Add ignore macros
x86/xen: Add stack frame dependency to hypercall inline asm calls
x86/paravirt: Add stack frame dependency to PVOP inline asm calls
x86/paravirt: Create a stack frame in PV_CALLEE_SAVE_REGS_THUNK
x86/amd: Set ELF function type for vide()
x86/reboot: Add ljmp instructions to stackvalidate whitelist
x86/xen: Add xen_cpuid() and xen_setup_gdt() to stackvalidate
whitelists
x86/asm/crypto: Create stack frames in aesni-intel_asm.S
x86/asm/crypto: Move .Lbswap_mask data to .rodata section
x86/asm/crypto: Move jump_table to .rodata section
x86/asm/crypto: Create stack frames in clmul_ghash_mul/update()
x86/asm/entry: Create stack frames in thunk functions
x86/asm/acpi: Create a stack frame in do_suspend_lowlevel()
x86/asm: Create stack frames in rwsem functions
x86/asm/efi: Create a stack frame in efi_call()
x86/asm/power: Create stack frames in hibernate_asm_64.S
Documentation/stack-validation.txt | 338 +++++++++++
MAINTAINERS | 8 +
arch/Kconfig | 6 +
arch/x86/Kconfig | 1 +
arch/x86/Makefile | 6 +-
arch/x86/boot/Makefile | 3 +-
arch/x86/boot/compressed/Makefile | 3 +-
arch/x86/crypto/aesni-intel_asm.S | 21 +
arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 8 +-
arch/x86/crypto/ghash-clmulni-intel_asm.S | 5 +
arch/x86/entry/thunk_64.S | 4 +
arch/x86/entry/vdso/Makefile | 5 +-
arch/x86/include/asm/frame.h | 51 +-
arch/x86/include/asm/paravirt.h | 9 +-
arch/x86/include/asm/paravirt_types.h | 18 +-
arch/x86/include/asm/stackvalidate.h | 45 ++
arch/x86/include/asm/xen/hypercall.h | 5 +-
arch/x86/kernel/acpi/wakeup_64.S | 3 +
arch/x86/kernel/cpu/amd.c | 5 +-
arch/x86/kernel/reboot.c | 7 +-
arch/x86/kernel/vmlinux.lds.S | 5 +-
arch/x86/lib/rwsem.S | 11 +-
arch/x86/platform/efi/efi_stub_64.S | 3 +
arch/x86/power/hibernate_asm_64.S | 7 +
arch/x86/purgatory/Makefile | 2 +
arch/x86/realmode/Makefile | 4 +-
arch/x86/realmode/rm/Makefile | 3 +-
arch/x86/xen/enlighten.c | 4 +-
drivers/firmware/efi/libstub/Makefile | 1 +
include/linux/stackvalidate.h | 28 +
lib/Kconfig.debug | 11 +
scripts/Makefile | 1 +
scripts/Makefile.build | 37 +-
scripts/mod/Makefile | 2 +
scripts/stackvalidate/Makefile | 24 +
scripts/stackvalidate/arch-x86.c | 160 +++++
scripts/stackvalidate/arch.h | 44 ++
scripts/stackvalidate/elf.c | 427 +++++++++++++
scripts/stackvalidate/elf.h | 92 +++
scripts/stackvalidate/list.h | 217 +++++++
scripts/stackvalidate/special.c | 199 ++++++
scripts/stackvalidate/special.h | 42 ++
scripts/stackvalidate/stackvalidate.c | 976 ++++++++++++++++++++++++++++++
43 files changed, 2803 insertions(+), 48 deletions(-)
create mode 100644 Documentation/stack-validation.txt
create mode 100644 arch/x86/include/asm/stackvalidate.h
create mode 100644 include/linux/stackvalidate.h
create mode 100644 scripts/stackvalidate/Makefile
create mode 100644 scripts/stackvalidate/arch-x86.c
create mode 100644 scripts/stackvalidate/arch.h
create mode 100644 scripts/stackvalidate/elf.c
create mode 100644 scripts/stackvalidate/elf.h
create mode 100644 scripts/stackvalidate/list.h
create mode 100644 scripts/stackvalidate/special.c
create mode 100644 scripts/stackvalidate/special.h
create mode 100644 scripts/stackvalidate/stackvalidate.c
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-08-25 10:20 +0200 |
| Message-ID | <q1hCA-P0-49@gated-at.bofh.it> |
| In reply to | #1212292 |
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > lib/Kconfig.debug | 11 + > scripts/Makefile | 1 + > scripts/Makefile.build | 37 +- > scripts/mod/Makefile | 2 + > scripts/stackvalidate/Makefile | 24 + > scripts/stackvalidate/arch-x86.c | 160 +++++ > scripts/stackvalidate/arch.h | 44 ++ > scripts/stackvalidate/elf.c | 427 +++++++++++++ > scripts/stackvalidate/elf.h | 92 +++ > scripts/stackvalidate/list.h | 217 +++++++ > scripts/stackvalidate/special.c | 199 ++++++ > scripts/stackvalidate/special.h | 42 ++ > scripts/stackvalidate/stackvalidate.c | 976 ++++++++++++++++++++++++++++++ > 43 files changed, 2803 insertions(+), 48 deletions(-) > create mode 100644 Documentation/stack-validation.txt > create mode 100644 arch/x86/include/asm/stackvalidate.h > create mode 100644 include/linux/stackvalidate.h > create mode 100644 scripts/stackvalidate/Makefile > create mode 100644 scripts/stackvalidate/arch-x86.c > create mode 100644 scripts/stackvalidate/arch.h > create mode 100644 scripts/stackvalidate/elf.c > create mode 100644 scripts/stackvalidate/elf.h > create mode 100644 scripts/stackvalidate/list.h > create mode 100644 scripts/stackvalidate/special.c > create mode 100644 scripts/stackvalidate/special.h > create mode 100644 scripts/stackvalidate/stackvalidate.c Btw., would you be against putting scripts/stackvalidate/ into tools/stackvalidate/ ? That way it would be more generic, and could be used in a standalone fashion as well I suspect. It would not change any of your current usage of it. (The other Kbuild glue changes still need to be in scripts/.) Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-25 17:30 +0200 |
| Message-ID | <q1okG-213-15@gated-at.bofh.it> |
| In reply to | #1212808 |
On Tue, Aug 25, 2015 at 10:05:00AM +0200, Ingo Molnar wrote: > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > lib/Kconfig.debug | 11 + > > scripts/Makefile | 1 + > > scripts/Makefile.build | 37 +- > > scripts/mod/Makefile | 2 + > > scripts/stackvalidate/Makefile | 24 + > > scripts/stackvalidate/arch-x86.c | 160 +++++ > > scripts/stackvalidate/arch.h | 44 ++ > > scripts/stackvalidate/elf.c | 427 +++++++++++++ > > scripts/stackvalidate/elf.h | 92 +++ > > scripts/stackvalidate/list.h | 217 +++++++ > > scripts/stackvalidate/special.c | 199 ++++++ > > scripts/stackvalidate/special.h | 42 ++ > > scripts/stackvalidate/stackvalidate.c | 976 ++++++++++++++++++++++++++++++ > > 43 files changed, 2803 insertions(+), 48 deletions(-) > > create mode 100644 Documentation/stack-validation.txt > > create mode 100644 arch/x86/include/asm/stackvalidate.h > > create mode 100644 include/linux/stackvalidate.h > > create mode 100644 scripts/stackvalidate/Makefile > > create mode 100644 scripts/stackvalidate/arch-x86.c > > create mode 100644 scripts/stackvalidate/arch.h > > create mode 100644 scripts/stackvalidate/elf.c > > create mode 100644 scripts/stackvalidate/elf.h > > create mode 100644 scripts/stackvalidate/list.h > > create mode 100644 scripts/stackvalidate/special.c > > create mode 100644 scripts/stackvalidate/special.h > > create mode 100644 scripts/stackvalidate/stackvalidate.c > > Btw., would you be against putting scripts/stackvalidate/ into > tools/stackvalidate/ ? > > That way it would be more generic, and could be used in a standalone fashion as > well I suspect. It would not change any of your current usage of it. > > (The other Kbuild glue changes still need to be in scripts/.) I like that idea. It could indeed be useful for any other projects which have asm code. In fact the musl project has been trying to solve similar problems with their CFI generation awk script. It does have some kernel-isms related to the reading of special sections (.altinstructions, __jump_table, etc). But that should be harmless for non-kernel code and could easily be factored out with a compile-time or cmdline flag if needed. BTW, since I'm planning on adding a CFI generation option to this tool, I wonder if "stackvalidate" is a misnomer. I can't think of anything better. Any naming ideas are welcome :-) -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-08-26 09:10 +0200 |
| Message-ID | <q1D0l-71b-1@gated-at.bofh.it> |
| In reply to | #1213110 |
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder > if "stackvalidate" is a misnomer. I can't think of anything better. Any naming > ideas are welcome :-) I'd suggest 'debuginfo' and a Git-alike set of subcommands: debuginfo help debuginfo check debuginfo check framepointers debuginfo check cfi Such a command scheme keeps open various future commands, such as: debuginfo show debuginfo size debuginfo compress debuginfo strip etc. I also checked that there does not appear to be such a Linux tool with such a name at the moment. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jiri Kosina <jikos@kernel.org> |
|---|---|
| Date | 2015-08-26 10:50 +0200 |
| Message-ID | <q1Ez8-Dv-5@gated-at.bofh.it> |
| In reply to | #1213587 |
On Wed, 26 Aug 2015, Ingo Molnar wrote: > > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder > > if "stackvalidate" is a misnomer. I can't think of anything better. Any naming > > ideas are welcome :-) > > I'd suggest 'debuginfo' and a Git-alike set of subcommands: IMO every name which is not googlable (like this one, because it already has other meaning) is bad. I always hated how 'crash' utlity is called, exactly because of this. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-27 15:20 +0200 |
| Message-ID | <q25fX-5vZ-9@gated-at.bofh.it> |
| In reply to | #1213587 |
On Wed, Aug 26, 2015 at 09:07:23AM +0200, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder > > if "stackvalidate" is a misnomer. I can't think of anything better. Any naming > > ideas are welcome :-) > > I'd suggest 'debuginfo' and a Git-alike set of subcommands: > > debuginfo help > debuginfo check > debuginfo check framepointers > debuginfo check cfi > > Such a command scheme keeps open various future commands, such as: > > debuginfo show > debuginfo size > debuginfo compress > debuginfo strip > > etc. > > I also checked that there does not appear to be such a Linux tool with such a name > at the moment. My feeling is that the subcommand model wouldn't fit this tool very well. Its core functionality is to analyze code paths -- which it does in a single pass, regardless of whether it's checking frame pointers, checking CFI, generating CFI, or some combination. Splitting it up into subcommands would mean having to repeat the same code analysis pass multiple times unnecessarily. Also, since it's really focused on stack-related code path analysis, I wouldn't expect it to be branching out into other debuginfo-related tasks. As far as naming goes, "debuginfo" usually refers to DWARF metadata. But this tool isn't DWARF-specific, so that would be confusing IMO. I also agree with Jiri that re-using a generic name which already has another meaning would add to the confusion. The goal of this tool is to fix stack traces, so how about "stackfix"? usage: stackfix [options] <file.o> -f, --frame-pointers Validate frame pointer usage -c, --cfi Validate CFI if it exists -g, --cfi-gen Generate CFI if it doesn't already exist The options could all be combinable. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-08-28 10:30 +0200 |
| Message-ID | <q2ncR-6b8-5@gated-at.bofh.it> |
| In reply to | #1214562 |
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Aug 26, 2015 at 09:07:23AM +0200, Ingo Molnar wrote: > > > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder > > > if "stackvalidate" is a misnomer. I can't think of anything better. Any naming > > > ideas are welcome :-) > > > > I'd suggest 'debuginfo' and a Git-alike set of subcommands: > > > > debuginfo help > > debuginfo check > > debuginfo check framepointers > > debuginfo check cfi > > > > Such a command scheme keeps open various future commands, such as: > > > > debuginfo show > > debuginfo size > > debuginfo compress > > debuginfo strip > > > > etc. > > > > I also checked that there does not appear to be such a Linux tool with such a name > > at the moment. > > My feeling is that the subcommand model wouldn't fit this tool very well. Its > core functionality is to analyze code paths -- which it does in a single pass, > regardless of whether it's checking frame pointers, checking CFI, generating > CFI, or some combination. Splitting it up into subcommands would mean having to > repeat the same code analysis pass multiple times unnecessarily. Huh? The subcommand approach is a user UI that does not limit the tool in any way: you are free to provide subcommands that combine more atomic functionality - similarly to how Git provides a 'git pull' subcommand that is a combination of 'fetch' and 'merge' steps. In this case it would be a simple: debuginfo check all to check everything. You can also make the selection of debuginfo components to check a regular option, not a subcommand. The important step is to not limit the tool to 'checking' only, if there's reasonable other tasks it could perform: - For example in the future we might want to sanity check the dwarf debuginfo whether it's all well suited for kernel probing. - And if the tool is doing such a good job analyzing stacks, why not extend it trivially to print max stack backtrace estimations? etc. By limiting the name at inception unreasonably you make all these things less obvious to add. The tool should obviously not be named 'kitchensink' just for future extensability, but at least don't limit it to stacks and checking only ... > Also, since it's really focused on stack-related code path analysis, I wouldn't > expect it to be branching out into other debuginfo-related tasks. > > As far as naming goes, "debuginfo" usually refers to DWARF metadata. But this > tool isn't DWARF-specific, so that would be confusing IMO. I also agree with > Jiri that re-using a generic name which already has another meaning would add to > the confusion. > > The goal of this tool is to fix stack traces, so how about "stackfix"? Sigh, please don't limit the tool's purpose with its name ... and choose a better name! :-) The tool does not 'fix' anything, and the normal case would be for it to produce no warnings. I.e. it's not a fixing, it's a checking tool. So something like 'stackcheck' or 'stackinfo' - but I really think 'debuginfo' is better, because for more complex kernel image operations like live patching the kernel has to double check the untrusted debuginfo that compilers barf out ... Frame pointers are simply a legacy debuginfo variant, one that the kernel happens to use as its primary debuginfo. I don't think it's confusing at all, especially if any dwarf logic is added ... Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-28 16:00 +0200 |
| Message-ID | <q2sme-545-11@gated-at.bofh.it> |
| In reply to | #1215217 |
On Fri, Aug 28, 2015 at 10:21:16AM +0200, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Wed, Aug 26, 2015 at 09:07:23AM +0200, Ingo Molnar wrote: > > > > > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder > > > > if "stackvalidate" is a misnomer. I can't think of anything better. Any naming > > > > ideas are welcome :-) > > > > > > I'd suggest 'debuginfo' and a Git-alike set of subcommands: > > > > > > debuginfo help > > > debuginfo check > > > debuginfo check framepointers > > > debuginfo check cfi > > > > > > Such a command scheme keeps open various future commands, such as: > > > > > > debuginfo show > > > debuginfo size > > > debuginfo compress > > > debuginfo strip > > > > > > etc. > > > > > > I also checked that there does not appear to be such a Linux tool with such a name > > > at the moment. > > > > My feeling is that the subcommand model wouldn't fit this tool very well. Its > > core functionality is to analyze code paths -- which it does in a single pass, > > regardless of whether it's checking frame pointers, checking CFI, generating > > CFI, or some combination. Splitting it up into subcommands would mean having to > > repeat the same code analysis pass multiple times unnecessarily. > > Huh? > > The subcommand approach is a user UI that does not limit the tool in any way: you > are free to provide subcommands that combine more atomic functionality - similarly > to how Git provides a 'git pull' subcommand that is a combination of 'fetch' and > 'merge' steps. Sure, but it doesn't scale if *all* the subcommands are combinable. For n subcommands which can be combined, you'd need (2^n - 1) total subcommands to cover all possible combinations. In that case, subcommands would be much more unwieldy than just having n flags that can be easily combined. This is an important point because I think any hypothetical future options would be likely to be combinable if they take advantage of the tool's main functionality, which is walking all the code paths. If they don't take advantage of that, they should probably be in a separate tool anyway. > In this case it would be a simple: > > debuginfo check all > > to check everything. You can also make the selection of debuginfo components to > check a regular option, not a subcommand. The reason I proposed a name change is that it will soon do *more* than just checking. It will also do CFI generation by modifying the object file. What subcommand would you suggest for the following? - do frame pointer validation; and - if CFI exists, do CFI validation, else do CFI generation. > The important step is to not limit the tool to 'checking' only, if there's > reasonable other tasks it could perform: > - For example in the future we might want to sanity check the dwarf debuginfo > whether it's all well suited for kernel probing. Yes, and this is planned soon. > - And if the tool is doing such a good job analyzing stacks, why not extend it > trivially to print max stack backtrace estimations? Sure, we could do that. > etc. By limiting the name at inception unreasonably you make all these things less > obvious to add. But note these examples are still related to stacks, so having "stack" in the name of the tool wouldn't be limiting (for these examples at least). > The tool should obviously not be named 'kitchensink' just for future > extensability, but at least don't limit it to stacks and checking only ... > > > Also, since it's really focused on stack-related code path analysis, I wouldn't > > expect it to be branching out into other debuginfo-related tasks. > > > > As far as naming goes, "debuginfo" usually refers to DWARF metadata. But this > > tool isn't DWARF-specific, so that would be confusing IMO. I also agree with > > Jiri that re-using a generic name which already has another meaning would add to > > the confusion. > > > > The goal of this tool is to fix stack traces, so how about "stackfix"? > > Sigh, please don't limit the tool's purpose with its name ... > > and choose a better name! :-) The tool does not 'fix' anything, and the normal > case would be for it to produce no warnings. I.e. it's not a fixing, it's a > checking tool. > > So something like 'stackcheck' or 'stackinfo' - but I really think 'debuginfo' is > better, because for more complex kernel image operations like live patching the > kernel has to double check the untrusted debuginfo that compilers barf out ... > > Frame pointers are simply a legacy debuginfo variant, one that the kernel happens > to use as its primary debuginfo. I don't think it's confusing at all, especially > if any dwarf logic is added ... I proposed the "fix" in "stackfix" because it will do more than just checking: it will also be able to modify the object file (as I describe above). And "stack" because thus far the proposed scope of the tool is strictly related to stacks. I think "debuginfo" is limiting in its own way. The core functionality of the tool is to analyze all possible code paths, which isn't directly related to debuginfo. We might want to do other kinds of code path analysis which are unrelated to debuginfo. For example, the tool could have a replacement for "make checkstack", which generates a list of functions which are stack hogs. That has nothing to do with debuginfo. (And note this is a further example of why subcommands are not a good fit. We would want to be able to combine this option with the others without needing an exponential growth in the number of subcommands.) -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <andi@firstfloor.org> |
|---|---|
| Date | 2015-08-26 16:30 +0200 |
| Subject | Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation |
| Message-ID | <q1JSa-8kg-35@gated-at.bofh.it> |
| In reply to | #1212292 |
> b) 100% reliable stack traces for DWARF enabled kernels > > This is not yet implemented. See Documentation/stack-validation.txt > for more details about what is planned. The automatic CFI generation tool seems like a bad idea to me. There's not that much assembler code in Linux, and often when new assembler code is added it is something tricky. In this case you may end up spending more time fixing the tool than just fixing the assembler. It would be also quite bad to require people who want to add some new assembler code to learn how to fix your tool to make their assembler work. It also wouldn't surprise me if there are some possible assembler tricks that are very hard/impossible to handle for a tool. For example how do you have alternative() style patching? (that's a generic problem with your approach BTW) Doing some kind of CFI verifier would seem more feasible, but it would need a black/white list to override it to handle the above cases. BTW how do handle the increasing number of JITs in the kernel? -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-27 16:40 +0200 |
| Subject | Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation |
| Message-ID | <q26vo-7el-35@gated-at.bofh.it> |
| In reply to | #1213920 |
On Wed, Aug 26, 2015 at 04:26:28PM +0200, Andi Kleen wrote: > > b) 100% reliable stack traces for DWARF enabled kernels > > > > This is not yet implemented. See Documentation/stack-validation.txt > > for more details about what is planned. > > The automatic CFI generation tool seems like a bad idea to me. There's not > that much assembler code in Linux, and often when new assembler code is added > it is something tricky. In this case you may end up spending more time > fixing the tool than just fixing the assembler. > > It would be also quite bad to require people who want to add some > new assembler code to learn how to fix your tool to make their > assembler work. Really I don't see that being much of a problem. The enforced rules were probably too stringent in earlier versions of the patch set. But I relaxed the rules quite a bit and now they allow things like sibling calls, jumps to outside the function, alternatives, etc. Generally it's easy and straightforward to follow the rules and make the tool happy. If you're trying to do something too weird, then yes the tool will complain, but IMO that's a good thing as it really makes you consider whether the weirdness (and associated complexity) is justified. I tried to document everything an asm coder would need to know. Also I have an invested interest in keeping the tool working and useful, and I'm listed in the MAINTAINERS file. So any frustrated people will know who to yell at. > It also wouldn't surprise me if there are some possible assembler tricks > that are very hard/impossible to handle for a tool. For example how do you > have alternative() style patching? (that's a generic problem with > your approach BTW) Yeah, there are definitely a lot of these tricks, but the tool can already handle them today: alternatives, jump labels, exception tables, gcc switch jump tables. It's usually a matter of parsing a special section and then treating these alternative code paths as conditional branches which need to be recursively followed and analyzed. When such conditional branches converge, it ensures that they have the same debug state at the convergence point. > Doing some kind of CFI verifier would seem more feasible, but it would > need a black/white list to override it to handle the above cases. CFI generation isn't any harder than validation. Either way the tool needs to do the code analysis which involves recursively following all branches and associating a debug state with each instruction. At the end it either compares those debug states with existing CFI, or writes them as new CFI. Also there are already some whitelist mechanisms for ignoring files, functions, or instructions. > BTW how do handle the increasing number of JITs in the kernel? Yeah, compile-time CFI wouldn't be applicable for code which is generated at runtime. Maybe we will need a mechanism to allow eBPF to quickly create minimal CFI-like metadata corresponding to the JIT code it generates, which can be used by stack dumping code to identify the JIT code and find the previous stack pointer on the stack. We can also add a debug NMI handler which validates stacks periodically to ensure that stacks are always sane. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <andi@firstfloor.org> |
|---|---|
| Date | 2015-08-28 19:30 +0200 |
| Subject | Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation |
| Message-ID | <q2vDt-1sU-25@gated-at.bofh.it> |
| In reply to | #1214615 |
> I tried to document everything an asm coder would need to know. Also I > have an invested interest in keeping the tool working and useful, and > I'm listed in the MAINTAINERS file. So any frustrated people will know > who to yell at. I find it somewhat ironic that you're proposing to simplifying write assembler code by making people hack some tool instead. > > BTW how do handle the increasing number of JITs in the kernel? > > Yeah, compile-time CFI wouldn't be applicable for code which is > generated at runtime. Maybe we will need a mechanism to allow eBPF to > quickly create minimal CFI-like metadata corresponding to the JIT code > it generates, which can be used by stack dumping code to identify the > JIT code and find the previous stack pointer on the stack. Perhaps I'm missing something, but for the hot patching you need some solution for this, as you rely on 100% accuracy. Right? I guess for now it could be some kind of big reader/writer lock for JIT code and reject hot patching if something is active there. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2015-08-28 22:00 +0200 |
| Subject | Re: [PATCH v11 03/20] x86/stackvalidate: Compile-time stack validation |
| Message-ID | <q2xYC-4IA-13@gated-at.bofh.it> |
| In reply to | #1215467 |
On Fri, Aug 28, 2015 at 07:26:23PM +0200, Andi Kleen wrote: > > > BTW how do handle the increasing number of JITs in the kernel? > > > > Yeah, compile-time CFI wouldn't be applicable for code which is > > generated at runtime. Maybe we will need a mechanism to allow eBPF to > > quickly create minimal CFI-like metadata corresponding to the JIT code > > it generates, which can be used by stack dumping code to identify the > > JIT code and find the previous stack pointer on the stack. > > Perhaps I'm missing something, but for the hot patching you need > some solution for this, as you rely on 100% accuracy. Right? We'll probably need something like that eventually, once we have an in-kernel CFI unwinder. Until then, for live patching we would just need to make sure the JIT generated code honors CONFIG_FRAME_POINTER. > I guess for now it could be some kind of big reader/writer lock > for JIT code and reject hot patching if something is active there. Yeah, maybe. Another easy way to handle it would be to bail if we can't find the CFI for a given address on a task's stack. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web