[PATCH 00/33] Compile-time stack metadata validation

[PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

This is v16 of the compile-time stack metadata validation patch set,
along with proposed fixes for most of the warnings it found.  It's based
on the tip/master branch.

v15 can be found here:

  https://lkml.kernel.org/r/cover.1450442274.git.jpoimboe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

For more information about the motivation behind this patch set, and
more details about what it does, see the first patch changelog and
tools/stacktool/Documentation/stack-validation.txt.

Patches 1-4 add stacktool and integrate it into the kernel build.

Patches 5-28 are some proposed fixes for several of the warnings
reported by stacktool.  They've been compile-tested and boot-tested in a
VM, but I haven't attempted any meaningful testing for many of them.

Patches 29-33 add some directories, files, and functions to the
stacktool whitelist in order to silence false positive warnings.

v16:
- fix all allyesconfig warnings, except for staging
- get rid of STACKTOOL_IGNORE_INSN which is no longer needed
- remove several whitelists in favor of automatically whitelisting any
  function with a special instruction like ljmp, lret, or vmrun
- split up stacktool patch into 3 parts as suggested by Ingo
- update the global noreturn function list
- detect noreturn function fallthroughs
- skip weak functions in noreturn call detection logic
- add empty function check to noreturn logic
- allow non-section rela symbols for __ex_table sections
- support rare switch table case with jmpq *[addr](%rip)
- don't warn on frame pointer restore without save
- rearrange patch order a bit

v15:
- restructure code for a new cmdline interface "stacktool check" using
  the new subcommand framework in tools/lib/subcmd
- fix 32 bit build fail (put __sp at end) in paravirt_types.h patch 10
  which was reported by 0day

v14:
- make tools/include/linux/list.h self-sufficient
- create FRAME_OFFSET to allow 32-bit code to be able to access function
  arguments on the stack
- add FRAME_OFFSET usage in crypto patch 14/24: "Create stack frames in
  aesni-intel_asm.S"
- rename "index" -> "idx" to fix build with some compilers

v13:
- LDFLAGS order fix from Chris J Arges
- new warning fix patches from Chris J Arges
- "--frame-pointer" -> "--check-frame-pointer"

v12:
- rename "stackvalidate" -> "stacktool"
- move from scripts/ to tools/:
  - makefile rework
  - make a copy of the x86 insn code (and warn if the code diverges)
  - use tools/include/linux/list.h
- move warning macros to a new warn.h file
- change wording: "stack validation" -> "stack metadata validation"

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: live-patching-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Michal Marek <mmarek-AlSwsSmVLrQ@public.gmane.org>
Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
Cc: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Andi Kleen <andi-Vw/NltI1exuRpAAqCnN02g@public.gmane.org>
Cc: Pedro Alves <palves-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Namhyung Kim <namhyung-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Bernd Petrovitsch <bernd-JY8rjfDgnXcb9sJ47jD7nuTv7YV0F9Eg@public.gmane.org>
Cc: Chris J Arges <chris.j.arges-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
Cc: Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Chris J Arges (1):
  x86/uaccess: Add stack frame output operand in get_user inline asm

Josh Poimboeuf (32):
  x86/stacktool: Compile-time stack metadata validation
  kbuild/stacktool: Add CONFIG_STACK_VALIDATION option
  x86/stacktool: Enable stacktool on x86_64
  x86/stacktool: Add STACKTOOL_IGNORE_FUNC macro
  x86/xen: Add stack frame dependency to hypercall inline asm calls
  x86/asm/xen: Set ELF function type for xen_adjust_exception_frame()
  x86/asm/xen: Create stack frames in xen-asm.S
  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/asm/crypto: Move .Lbswap_mask data to .rodata section
  x86/asm/crypto: Move jump_table to .rodata section
  x86/asm/crypto: Simplify stack usage in sha-mb functions
  x86/asm/crypto: Don't use rbp as a scratch register
  x86/asm/crypto: Create stack frames in crypto functions
  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
  x86/asm/bpf: Annotate callable functions
  x86/asm/bpf: Create stack frames in bpf_jit.S
  x86/kprobes: Get rid of kretprobe_trampoline_holder()
  x86/kvm: Set ELF function type for fastop functions
  x86/kvm: Add stack frame dependency to test_cc() inline asm
  watchdog/hpwdt: Create stack frame in asminline_call()
  x86/locking: Create stack frame in PV unlock
  x86/stacktool: Add directory and file whitelists
  x86/xen: Add xen_cpuid() to stacktool whitelist
  bpf: Add __bpf_prog_run() to stacktool whitelist
  sched: Add __schedule() to stacktool whitelist
  x86/kprobes: Add kretprobe_trampoline() to stacktool whitelist

 MAINTAINERS                                        |   6 +
 Makefile                                           |   5 +-
 arch/Kconfig                                       |   6 +
 arch/x86/Kconfig                                   |   1 +
 arch/x86/boot/Makefile                             |   1 +
 arch/x86/boot/compressed/Makefile                  |   3 +-
 arch/x86/crypto/aesni-intel_asm.S                  |  75 +-
 arch/x86/crypto/camellia-aesni-avx-asm_64.S        |  15 +
 arch/x86/crypto/camellia-aesni-avx2-asm_64.S       |  15 +
 arch/x86/crypto/cast5-avx-x86_64-asm_64.S          |   9 +
 arch/x86/crypto/cast6-avx-x86_64-asm_64.S          |  13 +
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S          |   8 +-
 arch/x86/crypto/ghash-clmulni-intel_asm.S          |   5 +
 arch/x86/crypto/serpent-avx-x86_64-asm_64.S        |  13 +
 arch/x86/crypto/serpent-avx2-asm_64.S              |  13 +
 arch/x86/crypto/sha-mb/sha1_mb_mgr_flush_avx2.S    |  35 +-
 arch/x86/crypto/sha-mb/sha1_mb_mgr_submit_avx2.S   |  36 +-
 arch/x86/crypto/twofish-avx-x86_64-asm_64.S        |  13 +
 arch/x86/entry/Makefile                            |   4 +
 arch/x86/entry/thunk_64.S                          |   4 +
 arch/x86/entry/vdso/Makefile                       |   5 +-
 arch/x86/include/asm/paravirt.h                    |   9 +-
 arch/x86/include/asm/paravirt_types.h              |  18 +-
 arch/x86/include/asm/qspinlock_paravirt.h          |   4 +
 arch/x86/include/asm/uaccess.h                     |   5 +-
 arch/x86/include/asm/xen/hypercall.h               |   5 +-
 arch/x86/kernel/Makefile                           |   5 +
 arch/x86/kernel/acpi/wakeup_64.S                   |   3 +
 arch/x86/kernel/cpu/amd.c                          |   5 +-
 arch/x86/kernel/kprobes/core.c                     |  59 +-
 arch/x86/kernel/vmlinux.lds.S                      |   5 +-
 arch/x86/kvm/emulate.c                             |  33 +-
 arch/x86/lib/rwsem.S                               |  11 +-
 arch/x86/net/bpf_jit.S                             |  48 +-
 arch/x86/platform/efi/Makefile                     |   2 +
 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                           |   3 +-
 arch/x86/xen/xen-asm.S                             |  10 +-
 arch/x86/xen/xen-asm_64.S                          |   1 +
 drivers/firmware/efi/libstub/Makefile              |   1 +
 drivers/watchdog/hpwdt.c                           |   8 +-
 include/linux/stacktool.h                          |  23 +
 kernel/bpf/core.c                                  |   2 +
 kernel/sched/core.c                                |   2 +
 lib/Kconfig.debug                                  |  12 +
 scripts/Makefile.build                             |  38 +-
 scripts/mod/Makefile                               |   2 +
 tools/Makefile                                     |  14 +-
 tools/stacktool/.gitignore                         |   2 +
 tools/stacktool/Build                              |  13 +
 tools/stacktool/Documentation/stack-validation.txt | 333 +++++++
 tools/stacktool/Makefile                           |  60 ++
 tools/stacktool/arch.h                             |  44 +
 tools/stacktool/arch/x86/Build                     |  12 +
 tools/stacktool/arch/x86/decode.c                  | 172 ++++
 .../stacktool/arch/x86/insn/gen-insn-attr-x86.awk  | 387 ++++++++
 tools/stacktool/arch/x86/insn/inat.c               |  97 ++
 tools/stacktool/arch/x86/insn/inat.h               | 221 +++++
 tools/stacktool/arch/x86/insn/inat_types.h         |  29 +
 tools/stacktool/arch/x86/insn/insn.c               | 594 ++++++++++++
 tools/stacktool/arch/x86/insn/insn.h               | 201 +++++
 tools/stacktool/arch/x86/insn/x86-opcode-map.txt   | 984 ++++++++++++++++++++
 tools/stacktool/builtin-check.c                    | 991 +++++++++++++++++++++
 tools/stacktool/builtin.h                          |  22 +
 tools/stacktool/elf.c                              | 403 +++++++++
 tools/stacktool/elf.h                              |  79 ++
 tools/stacktool/special.c                          | 193 ++++
 tools/stacktool/special.h                          |  42 +
 tools/stacktool/stacktool.c                        | 134 +++
 tools/stacktool/warn.h                             |  60 ++
 74 files changed, 5516 insertions(+), 189 deletions(-)
 create mode 100644 include/linux/stacktool.h
 create mode 100644 tools/stacktool/.gitignore
 create mode 100644 tools/stacktool/Build
 create mode 100644 tools/stacktool/Documentation/stack-validation.txt
 create mode 100644 tools/stacktool/Makefile
 create mode 100644 tools/stacktool/arch.h
 create mode 100644 tools/stacktool/arch/x86/Build
 create mode 100644 tools/stacktool/arch/x86/decode.c
 create mode 100644 tools/stacktool/arch/x86/insn/gen-insn-attr-x86.awk
 create mode 100644 tools/stacktool/arch/x86/insn/inat.c
 create mode 100644 tools/stacktool/arch/x86/insn/inat.h
 create mode 100644 tools/stacktool/arch/x86/insn/inat_types.h
 create mode 100644 tools/stacktool/arch/x86/insn/insn.c
 create mode 100644 tools/stacktool/arch/x86/insn/insn.h
 create mode 100644 tools/stacktool/arch/x86/insn/x86-opcode-map.txt
 create mode 100644 tools/stacktool/builtin-check.c
 create mode 100644 tools/stacktool/builtin.h
 create mode 100644 tools/stacktool/elf.c
 create mode 100644 tools/stacktool/elf.h
 create mode 100644 tools/stacktool/special.c
 create mode 100644 tools/stacktool/special.h
 create mode 100644 tools/stacktool/stacktool.c
 create mode 100644 tools/stacktool/warn.h

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 22/33] x86/asm/bpf: Annotate callable functions

[Josh Poimboeuf]

bpf_jit.S has several functions which can be called from C code.  Give
them proper ELF annotations.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
 arch/x86/net/bpf_jit.S | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
index 4093216..eb4a3bd 100644
--- a/arch/x86/net/bpf_jit.S
+++ b/arch/x86/net/bpf_jit.S
@@ -22,15 +22,16 @@
 	32 /* space for rbx,r13,r14,r15 */ + \
 	8 /* space for skb_copy_bits */)
 
-sk_load_word:
-	.globl	sk_load_word
+#define FUNC(name) \
+	.globl name; \
+	.type name, @function; \
+	name:
 
+FUNC(sk_load_word)
 	test	%esi,%esi
 	js	bpf_slow_path_word_neg
 
-sk_load_word_positive_offset:
-	.globl	sk_load_word_positive_offset
-
+FUNC(sk_load_word_positive_offset)
 	mov	%r9d,%eax		# hlen
 	sub	%esi,%eax		# hlen - offset
 	cmp	$3,%eax
@@ -39,15 +40,11 @@ sk_load_word_positive_offset:
 	bswap   %eax  			/* ntohl() */
 	ret
 
-sk_load_half:
-	.globl	sk_load_half
-
+FUNC(sk_load_half)
 	test	%esi,%esi
 	js	bpf_slow_path_half_neg
 
-sk_load_half_positive_offset:
-	.globl	sk_load_half_positive_offset
-
+FUNC(sk_load_half_positive_offset)
 	mov	%r9d,%eax
 	sub	%esi,%eax		#	hlen - offset
 	cmp	$1,%eax
@@ -56,15 +53,11 @@ sk_load_half_positive_offset:
 	rol	$8,%ax			# ntohs()
 	ret
 
-sk_load_byte:
-	.globl	sk_load_byte
-
+FUNC(sk_load_byte)
 	test	%esi,%esi
 	js	bpf_slow_path_byte_neg
 
-sk_load_byte_positive_offset:
-	.globl	sk_load_byte_positive_offset
-
+FUNC(sk_load_byte_positive_offset)
 	cmp	%esi,%r9d   /* if (offset >= hlen) goto bpf_slow_path_byte */
 	jle	bpf_slow_path_byte
 	movzbl	(SKBDATA,%rsi),%eax
@@ -120,8 +113,8 @@ bpf_slow_path_byte:
 bpf_slow_path_word_neg:
 	cmp	SKF_MAX_NEG_OFF, %esi	/* test range */
 	jl	bpf_error	/* offset lower -> error  */
-sk_load_word_negative_offset:
-	.globl	sk_load_word_negative_offset
+
+FUNC(sk_load_word_negative_offset)
 	sk_negative_common(4)
 	mov	(%rax), %eax
 	bswap	%eax
@@ -130,8 +123,8 @@ sk_load_word_negative_offset:
 bpf_slow_path_half_neg:
 	cmp	SKF_MAX_NEG_OFF, %esi
 	jl	bpf_error
-sk_load_half_negative_offset:
-	.globl	sk_load_half_negative_offset
+
+FUNC(sk_load_half_negative_offset)
 	sk_negative_common(2)
 	mov	(%rax),%ax
 	rol	$8,%ax
@@ -141,8 +134,8 @@ sk_load_half_negative_offset:
 bpf_slow_path_byte_neg:
 	cmp	SKF_MAX_NEG_OFF, %esi
 	jl	bpf_error
-sk_load_byte_negative_offset:
-	.globl	sk_load_byte_negative_offset
+
+FUNC(sk_load_byte_negative_offset)
 	sk_negative_common(1)
 	movzbl	(%rax), %eax
 	ret
-- 
2.4.3

[PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

bpf_jit.S has several callable non-leaf functions which don't honor
CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create a stack frame before the call instructions when
CONFIG_FRAME_POINTER is enabled.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
 arch/x86/net/bpf_jit.S | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
index eb4a3bd..f2a7faf 100644
--- a/arch/x86/net/bpf_jit.S
+++ b/arch/x86/net/bpf_jit.S
@@ -8,6 +8,7 @@
  * of the License.
  */
 #include <linux/linkage.h>
+#include <asm/frame.h>
 
 /*
  * Calling convention :
@@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
 
 /* rsi contains offset and can be scratched */
 #define bpf_slow_path_common(LEN)		\
+	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
+	FRAME_BEGIN;				\
 	mov	%rbx, %rdi; /* arg1 == skb */	\
 	push	%r9;				\
 	push	SKBDATA;			\
 /* rsi already has offset */			\
 	mov	$LEN,%ecx;	/* len */	\
-	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
 	call	skb_copy_bits;			\
 	test    %eax,%eax;			\
 	pop	SKBDATA;			\
-	pop	%r9;
+	pop	%r9;				\
+	FRAME_END
 
 
 bpf_slow_path_word:
@@ -99,6 +102,7 @@ bpf_slow_path_byte:
 	ret
 
 #define sk_negative_common(SIZE)				\
+	FRAME_BEGIN;						\
 	mov	%rbx, %rdi; /* arg1 == skb */			\
 	push	%r9;						\
 	push	SKBDATA;					\
@@ -108,6 +112,7 @@ bpf_slow_path_byte:
 	test	%rax,%rax;					\
 	pop	SKBDATA;					\
 	pop	%r9;						\
+	FRAME_END;						\
 	jz	bpf_error
 
 bpf_slow_path_word_neg:
-- 
2.4.3

[PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Josh Poimboeuf]

stacktool reports the following false positive warnings:

  stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
  stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
  stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
  [...]

It's confused by the following dynamic jump instruction in
__bpf_prog_run()::

  jmp     *(%r12,%rax,8)

which corresponds to the following line in the C code:

  goto *jumptable[insn->code];

There's no way for stacktool to deterministically find all possible
branch targets for a dynamic jump, so it can't verify this code.

In this case the jumps all stay within the function, and there's nothing
unusual going on related to the stack, so we can whitelist the function.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: netdev@vger.kernel.org
---
 kernel/bpf/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 972d9a8..7108a96 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -27,6 +27,7 @@
 #include <linux/random.h>
 #include <linux/moduleloader.h>
 #include <linux/bpf.h>
+#include <linux/stacktool.h>
 
 #include <asm/unaligned.h>
 
@@ -649,6 +650,7 @@ load_byte:
 		WARN_RATELIMIT(1, "unknown opcode %02x\n", insn->code);
 		return 0;
 }
+STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
 
 bool bpf_prog_array_compatible(struct bpf_array *array,
 			       const struct bpf_prog *fp)
-- 
2.4.3

Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Daniel Borkmann]

On 01/21/2016 11:49 PM, Josh Poimboeuf wrote:
> stacktool reports the following false positive warnings:
>
>    stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
>    stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
>    stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
>    [...]
>
> It's confused by the following dynamic jump instruction in
> __bpf_prog_run()::
>
>    jmp     *(%r12,%rax,8)
>
> which corresponds to the following line in the C code:
>
>    goto *jumptable[insn->code];
>
> There's no way for stacktool to deterministically find all possible
> branch targets for a dynamic jump, so it can't verify this code.
>
> In this case the jumps all stay within the function, and there's nothing
> unusual going on related to the stack, so we can whitelist the function.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: netdev@vger.kernel.org

Fine by me:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> bpf_jit.S has several callable non-leaf functions which don't honor
> CONFIG_FRAME_POINTER, which can result in bad stack traces.
> 
> Create a stack frame before the call instructions when
> CONFIG_FRAME_POINTER is enabled.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: netdev@vger.kernel.org
> ---
>  arch/x86/net/bpf_jit.S | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> index eb4a3bd..f2a7faf 100644
> --- a/arch/x86/net/bpf_jit.S
> +++ b/arch/x86/net/bpf_jit.S
> @@ -8,6 +8,7 @@
>   * of the License.
>   */
>  #include <linux/linkage.h>
> +#include <asm/frame.h>
>  
>  /*
>   * Calling convention :
> @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
>  
>  /* rsi contains offset and can be scratched */
>  #define bpf_slow_path_common(LEN)		\
> +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> +	FRAME_BEGIN;				\
>  	mov	%rbx, %rdi; /* arg1 == skb */	\
>  	push	%r9;				\
>  	push	SKBDATA;			\
>  /* rsi already has offset */			\
>  	mov	$LEN,%ecx;	/* len */	\
> -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
>  	call	skb_copy_bits;			\
>  	test    %eax,%eax;			\
>  	pop	SKBDATA;			\
> -	pop	%r9;
> +	pop	%r9;				\
> +	FRAME_END

I'm not sure what above is doing.
There is already 'push rbp; mov rbp,rsp' at the beginning of generated
code and with above the stack trace will show two function at the same ip?
since there were no calls between them?
I think the stack walker will get even more confused?
Also the JIT of bpf_call insn will emit variable number of push/pop
around the call and I definitely don't want to add extra push rbp
there, since it's the critical path and callee will do its own
push rbp.
Also there are push/pops emitted around div/mod
and there is indirect goto emitted as well for bpf_tail_call
that jumps into different function body without touching
current stack.
Also none of the JITed function are dwarf annotated.
I could be missing something. I think either this patch
is not need or you need to teach the tool to ignore
all JITed stuff. I don't think it's practical to annotate
everything. Different JITs do their own magic.
s390 JIT is even more fancy.

Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Alexei Starovoitov]

On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> stacktool reports the following false positive warnings:
> 
>   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
>   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
>   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
>   [...]
> 
> It's confused by the following dynamic jump instruction in
> __bpf_prog_run()::
> 
>   jmp     *(%r12,%rax,8)
> 
> which corresponds to the following line in the C code:
> 
>   goto *jumptable[insn->code];
> 
> There's no way for stacktool to deterministically find all possible
> branch targets for a dynamic jump, so it can't verify this code.
> 
> In this case the jumps all stay within the function, and there's nothing
> unusual going on related to the stack, so we can whitelist the function.

well, few things are very unusual in this function.
did you see what JMP_CALL does? it's a call into a different function,
but not like typical indirect call. Will it be ok as well?

In general it's not possible for any tool to identify all possible
branch targets. bpf programs can be loaded on the fly and
jumping sequence will change.
So if this marking says 'don't bother analyzing this function because
it does sane stuff' that's probably not the case.
If this marking says 'don't bother analyzing, the stack may be crazy
from here on' then it's ok.

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > bpf_jit.S has several callable non-leaf functions which don't honor
> > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > 
> > Create a stack frame before the call instructions when
> > CONFIG_FRAME_POINTER is enabled.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: netdev@vger.kernel.org
> > ---
> >  arch/x86/net/bpf_jit.S | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > index eb4a3bd..f2a7faf 100644
> > --- a/arch/x86/net/bpf_jit.S
> > +++ b/arch/x86/net/bpf_jit.S
> > @@ -8,6 +8,7 @@
> >   * of the License.
> >   */
> >  #include <linux/linkage.h>
> > +#include <asm/frame.h>
> >  
> >  /*
> >   * Calling convention :
> > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> >  
> >  /* rsi contains offset and can be scratched */
> >  #define bpf_slow_path_common(LEN)		\
> > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > +	FRAME_BEGIN;				\
> >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> >  	push	%r9;				\
> >  	push	SKBDATA;			\
> >  /* rsi already has offset */			\
> >  	mov	$LEN,%ecx;	/* len */	\
> > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> >  	call	skb_copy_bits;			\
> >  	test    %eax,%eax;			\
> >  	pop	SKBDATA;			\
> > -	pop	%r9;
> > +	pop	%r9;				\
> > +	FRAME_END
> 
> I'm not sure what above is doing.
> There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> code and with above the stack trace will show two function at the same ip?
> since there were no calls between them?
> I think the stack walker will get even more confused?
> Also the JIT of bpf_call insn will emit variable number of push/pop
> around the call and I definitely don't want to add extra push rbp
> there, since it's the critical path and callee will do its own
> push rbp.
> Also there are push/pops emitted around div/mod
> and there is indirect goto emitted as well for bpf_tail_call
> that jumps into different function body without touching
> current stack.

Hm, I'm not sure I follow.  Let me try to explain my understanding.

As you mentioned, the generated code sets up the frame pointer.  From
emit_prologue():

        EMIT1(0x55); /* push rbp */
	EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */

And then later, do_jit() can generate a call into the functions in
bpf_jit.S.  For example:

	func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
	...
	EMIT1_off32(0xE8, jmp_offset); /* call */

So the functions in bpf_jit.S are being called by the generated code.
They're not part of the generated code itself.  So they're callees and
need to create their own stack frame before they call out to somewhere
else.

Or did I miss something?

> Also none of the JITed function are dwarf annotated.

But what does that have to do with frame pointers?

> I could be missing something. I think either this patch
> is not need or you need to teach the tool to ignore
> all JITed stuff. I don't think it's practical to annotate
> everything. Different JITs do their own magic.
> s390 JIT is even more fancy.

Well, but the point of these patches isn't to make the tool happy.  It's
really to make sure that runtime stack traces can be made reliable.
Maybe I'm missing something but I don't see why JIT code can't honor
CONFIG_FRAME_POINTER just like any other code.

-- 
Josh

Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Josh Poimboeuf]

On Thu, Jan 21, 2016 at 06:55:41PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> > stacktool reports the following false positive warnings:
> > 
> >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> >   [...]
> > 
> > It's confused by the following dynamic jump instruction in
> > __bpf_prog_run()::
> > 
> >   jmp     *(%r12,%rax,8)
> > 
> > which corresponds to the following line in the C code:
> > 
> >   goto *jumptable[insn->code];
> > 
> > There's no way for stacktool to deterministically find all possible
> > branch targets for a dynamic jump, so it can't verify this code.
> > 
> > In this case the jumps all stay within the function, and there's nothing
> > unusual going on related to the stack, so we can whitelist the function.
> 
> well, few things are very unusual in this function.
> did you see what JMP_CALL does? it's a call into a different function,
> but not like typical indirect call. Will it be ok as well?
> 
> In general it's not possible for any tool to identify all possible
> branch targets. bpf programs can be loaded on the fly and
> jumping sequence will change.
> So if this marking says 'don't bother analyzing this function because
> it does sane stuff' that's probably not the case.
> If this marking says 'don't bother analyzing, the stack may be crazy
> from here on' then it's ok.

So the tool doesn't need to follow all possible call targets.  Instead
it just verifies that all functions follow the frame pointer convention.
That way it doesn't matter *which* function is being called because they
all do the right thing.

But it *does* need to follow all jump targets, so that it can analyze
all possible code paths within the function itself.  With a dynamic
jump, it can't do that.

So the JMP_CALL is fine, but the goto *jumptable[insn->code] isn't.
(And BTW that's the only occurrence of such a dynamic jump table in the
entire kernel.)

-- 
Josh

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > 
> > > Create a stack frame before the call instructions when
> > > CONFIG_FRAME_POINTER is enabled.
> > > 
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: netdev@vger.kernel.org
> > > ---
> > >  arch/x86/net/bpf_jit.S | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > > index eb4a3bd..f2a7faf 100644
> > > --- a/arch/x86/net/bpf_jit.S
> > > +++ b/arch/x86/net/bpf_jit.S
> > > @@ -8,6 +8,7 @@
> > >   * of the License.
> > >   */
> > >  #include <linux/linkage.h>
> > > +#include <asm/frame.h>
> > >  
> > >  /*
> > >   * Calling convention :
> > > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> > >  
> > >  /* rsi contains offset and can be scratched */
> > >  #define bpf_slow_path_common(LEN)		\
> > > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > +	FRAME_BEGIN;				\
> > >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> > >  	push	%r9;				\
> > >  	push	SKBDATA;			\
> > >  /* rsi already has offset */			\
> > >  	mov	$LEN,%ecx;	/* len */	\
> > > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> > >  	call	skb_copy_bits;			\
> > >  	test    %eax,%eax;			\
> > >  	pop	SKBDATA;			\
> > > -	pop	%r9;
> > > +	pop	%r9;				\
> > > +	FRAME_END
> > 
> > I'm not sure what above is doing.
> > There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> > code and with above the stack trace will show two function at the same ip?
> > since there were no calls between them?
> > I think the stack walker will get even more confused?
> > Also the JIT of bpf_call insn will emit variable number of push/pop
> > around the call and I definitely don't want to add extra push rbp
> > there, since it's the critical path and callee will do its own
> > push rbp.
> > Also there are push/pops emitted around div/mod
> > and there is indirect goto emitted as well for bpf_tail_call
> > that jumps into different function body without touching
> > current stack.
> 
> Hm, I'm not sure I follow.  Let me try to explain my understanding.
> 
> As you mentioned, the generated code sets up the frame pointer.  From
> emit_prologue():
> 
>         EMIT1(0x55); /* push rbp */
> 	EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */
> 
> And then later, do_jit() can generate a call into the functions in
> bpf_jit.S.  For example:
> 
> 	func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> 	...
> 	EMIT1_off32(0xE8, jmp_offset); /* call */
> 
> So the functions in bpf_jit.S are being called by the generated code.
> They're not part of the generated code itself.  So they're callees and
> need to create their own stack frame before they call out to somewhere
> else.
> 
> Or did I miss something?

yes. all correct.
This particular patch is ok, since it adds to
bpf_slow_path_common and as the name says it's slow and rare,
but wanted to make sure the rest of it is understood.

> > Also none of the JITed function are dwarf annotated.
> 
> But what does that have to do with frame pointers?

nothing, but then why do you need
.type name, @function
annotations in another patch?

> > I could be missing something. I think either this patch
> > is not need or you need to teach the tool to ignore
> > all JITed stuff. I don't think it's practical to annotate
> > everything. Different JITs do their own magic.
> > s390 JIT is even more fancy.
> 
> Well, but the point of these patches isn't to make the tool happy.  It's
> really to make sure that runtime stack traces can be made reliable.
> Maybe I'm missing something but I don't see why JIT code can't honor
> CONFIG_FRAME_POINTER just like any other code.

It can if there is no performance cost added.
I can speak for x64 JIT, but the rest needs to be analyzed as well.
My point was that may be it's easier to ignore all JITed code and
just say that such call stacks may be unreliable?
live-patching is not applicable to JITed code anyway
or you want to livepatch the callees of it?

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Ingo Molnar]

* Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > > I could be missing something. I think either this patch is not need or you 
> > > need to teach the tool to ignore all JITed stuff. I don't think it's 
> > > practical to annotate everything. Different JITs do their own magic. s390 
> > > JIT is even more fancy.
> > 
> > Well, but the point of these patches isn't to make the tool happy.  It's 
> > really to make sure that runtime stack traces can be made reliable. Maybe I'm 
> > missing something but I don't see why JIT code can't honor 
> > CONFIG_FRAME_POINTER just like any other code.
> 
> It can if there is no performance cost added. I can speak for x64 JIT, but the 
> rest needs to be analyzed as well. My point was that may be it's easier to 
> ignore all JITed code and just say that such call stacks may be unreliable? 
> live-patching is not applicable to JITed code anyway or you want to livepatch 
> the callees of it?

So the rule is that if frame pointers are enabled all kernel code should have 
correct stack frames - in case an IRQ (or NMI) hits it or it crashes.

Thanks,

	Ingo

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > 
> > > > Create a stack frame before the call instructions when
> > > > CONFIG_FRAME_POINTER is enabled.
> > > > 
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > Cc: netdev@vger.kernel.org
> > > > ---
> > > >  arch/x86/net/bpf_jit.S | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
> > > > index eb4a3bd..f2a7faf 100644
> > > > --- a/arch/x86/net/bpf_jit.S
> > > > +++ b/arch/x86/net/bpf_jit.S
> > > > @@ -8,6 +8,7 @@
> > > >   * of the License.
> > > >   */
> > > >  #include <linux/linkage.h>
> > > > +#include <asm/frame.h>
> > > >  
> > > >  /*
> > > >   * Calling convention :
> > > > @@ -65,16 +66,18 @@ FUNC(sk_load_byte_positive_offset)
> > > >  
> > > >  /* rsi contains offset and can be scratched */
> > > >  #define bpf_slow_path_common(LEN)		\
> > > > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > +	FRAME_BEGIN;				\
> > > >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> > > >  	push	%r9;				\
> > > >  	push	SKBDATA;			\
> > > >  /* rsi already has offset */			\
> > > >  	mov	$LEN,%ecx;	/* len */	\
> > > > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> > > >  	call	skb_copy_bits;			\
> > > >  	test    %eax,%eax;			\
> > > >  	pop	SKBDATA;			\
> > > > -	pop	%r9;
> > > > +	pop	%r9;				\
> > > > +	FRAME_END
> > > 
> > > I'm not sure what above is doing.
> > > There is already 'push rbp; mov rbp,rsp' at the beginning of generated
> > > code and with above the stack trace will show two function at the same ip?
> > > since there were no calls between them?
> > > I think the stack walker will get even more confused?
> > > Also the JIT of bpf_call insn will emit variable number of push/pop
> > > around the call and I definitely don't want to add extra push rbp
> > > there, since it's the critical path and callee will do its own
> > > push rbp.
> > > Also there are push/pops emitted around div/mod
> > > and there is indirect goto emitted as well for bpf_tail_call
> > > that jumps into different function body without touching
> > > current stack.
> > 
> > Hm, I'm not sure I follow.  Let me try to explain my understanding.
> > 
> > As you mentioned, the generated code sets up the frame pointer.  From
> > emit_prologue():
> > 
> >         EMIT1(0x55); /* push rbp */
> > 	EMIT3(0x48, 0x89, 0xE5); /* mov rbp,rsp */
> > 
> > And then later, do_jit() can generate a call into the functions in
> > bpf_jit.S.  For example:
> > 
> > 	func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> > 	...
> > 	EMIT1_off32(0xE8, jmp_offset); /* call */
> > 
> > So the functions in bpf_jit.S are being called by the generated code.
> > They're not part of the generated code itself.  So they're callees and
> > need to create their own stack frame before they call out to somewhere
> > else.
> > 
> > Or did I miss something?
> 
> yes. all correct.
> This particular patch is ok, since it adds to
> bpf_slow_path_common and as the name says it's slow and rare,
> but wanted to make sure the rest of it is understood.
> 
> > > Also none of the JITed function are dwarf annotated.
> > 
> > But what does that have to do with frame pointers?
> 
> nothing, but then why do you need
> .type name, @function
> annotations in another patch?

Those are ELF function annotations which are needed so that stacktool
can find and analyze all callable functions (and they're a good idea
anyway for the sake of other tooling).

Obviously we can't annotate the JIT functions which have no name, but
that's ok.  As long as they're doing the right thing with respect to
frame pointers, the stack dump code will still be able to see their
frames (and that they're associated with generated code).

> > > I could be missing something. I think either this patch
> > > is not need or you need to teach the tool to ignore
> > > all JITed stuff. I don't think it's practical to annotate
> > > everything. Different JITs do their own magic.
> > > s390 JIT is even more fancy.
> > 
> > Well, but the point of these patches isn't to make the tool happy.  It's
> > really to make sure that runtime stack traces can be made reliable.
> > Maybe I'm missing something but I don't see why JIT code can't honor
> > CONFIG_FRAME_POINTER just like any other code.
> 
> It can if there is no performance cost added.

CONFIG_FRAME_POINTER always adds a small performance cost but as you
mentioned it only affects the slow path here.  And hopefully we'll soon
have an in-kernel DWARF unwinder on x86 so we can get rid of the need
for frame pointers.

> I can speak for x64 JIT, but the rest needs to be analyzed as well.
> My point was that may be it's easier to ignore all JITed code and
> just say that such call stacks may be unreliable?
> live-patching is not applicable to JITed code anyway
> or you want to livepatch the callees of it?

Right.  We can't patch JIT code, but we might need to patch other
functions on the call stack (including callees and callers).

-- 
Josh

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > > 
> > > > > Create a stack frame before the call instructions when
> > > > > CONFIG_FRAME_POINTER is enabled.
> > > > > 
> > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > Cc: netdev@vger.kernel.org
> > > > > ---
> > > > >  arch/x86/net/bpf_jit.S | 9 +++++++--
...
> > > > >  /* rsi contains offset and can be scratched */
> > > > >  #define bpf_slow_path_common(LEN)		\
> > > > > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > +	FRAME_BEGIN;				\
> > > > >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> > > > >  	push	%r9;				\
> > > > >  	push	SKBDATA;			\
> > > > >  /* rsi already has offset */			\
> > > > >  	mov	$LEN,%ecx;	/* len */	\
> > > > > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> > > > >  	call	skb_copy_bits;			\
> > > > >  	test    %eax,%eax;			\
> > > > >  	pop	SKBDATA;			\
> > > > > -	pop	%r9;
> > > > > +	pop	%r9;				\
> > > > > +	FRAME_END
...
> > > Well, but the point of these patches isn't to make the tool happy.  It's
> > > really to make sure that runtime stack traces can be made reliable.
> > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > CONFIG_FRAME_POINTER just like any other code.
> > 
> > It can if there is no performance cost added.
> 
> CONFIG_FRAME_POINTER always adds a small performance cost but as you
> mentioned it only affects the slow path here.  And hopefully we'll soon
> have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> for frame pointers.

ok. fair enough.
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Alexei Starovoitov]

On Thu, Jan 21, 2016 at 10:13:02PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 06:55:41PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> > > stacktool reports the following false positive warnings:
> > > 
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> > >   [...]
> > > 
> > > It's confused by the following dynamic jump instruction in
> > > __bpf_prog_run()::
> > > 
> > >   jmp     *(%r12,%rax,8)
> > > 
> > > which corresponds to the following line in the C code:
> > > 
> > >   goto *jumptable[insn->code];
> > > 
> > > There's no way for stacktool to deterministically find all possible
> > > branch targets for a dynamic jump, so it can't verify this code.
> > > 
> > > In this case the jumps all stay within the function, and there's nothing
> > > unusual going on related to the stack, so we can whitelist the function.
> > 
> > well, few things are very unusual in this function.
> > did you see what JMP_CALL does? it's a call into a different function,
> > but not like typical indirect call. Will it be ok as well?
> > 
> > In general it's not possible for any tool to identify all possible
> > branch targets. bpf programs can be loaded on the fly and
> > jumping sequence will change.
> > So if this marking says 'don't bother analyzing this function because
> > it does sane stuff' that's probably not the case.
> > If this marking says 'don't bother analyzing, the stack may be crazy
> > from here on' then it's ok.
> 
> So the tool doesn't need to follow all possible call targets.  Instead
> it just verifies that all functions follow the frame pointer convention.
> That way it doesn't matter *which* function is being called because they
> all do the right thing.
> 
> But it *does* need to follow all jump targets, so that it can analyze
> all possible code paths within the function itself.  With a dynamic
> jump, it can't do that.
> 
> So the JMP_CALL is fine, but the goto *jumptable[insn->code] isn't.
> (And BTW that's the only occurrence of such a dynamic jump table in the
> entire kernel.)

Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Josh Poimboeuf]

On Fri, Jan 22, 2016 at 09:18:23AM -0800, Alexei Starovoitov wrote:
> On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> > On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > > > 
> > > > > > Create a stack frame before the call instructions when
> > > > > > CONFIG_FRAME_POINTER is enabled.
> > > > > > 
> > > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > > Cc: netdev@vger.kernel.org
> > > > > > ---
> > > > > >  arch/x86/net/bpf_jit.S | 9 +++++++--
> ...
> > > > > >  /* rsi contains offset and can be scratched */
> > > > > >  #define bpf_slow_path_common(LEN)		\
> > > > > > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > > +	FRAME_BEGIN;				\
> > > > > >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> > > > > >  	push	%r9;				\
> > > > > >  	push	SKBDATA;			\
> > > > > >  /* rsi already has offset */			\
> > > > > >  	mov	$LEN,%ecx;	/* len */	\
> > > > > > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> > > > > >  	call	skb_copy_bits;			\
> > > > > >  	test    %eax,%eax;			\
> > > > > >  	pop	SKBDATA;			\
> > > > > > -	pop	%r9;
> > > > > > +	pop	%r9;				\
> > > > > > +	FRAME_END
> ...
> > > > Well, but the point of these patches isn't to make the tool happy.  It's
> > > > really to make sure that runtime stack traces can be made reliable.
> > > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > > CONFIG_FRAME_POINTER just like any other code.
> > > 
> > > It can if there is no performance cost added.
> > 
> > CONFIG_FRAME_POINTER always adds a small performance cost but as you
> > mentioned it only affects the slow path here.  And hopefully we'll soon
> > have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> > for frame pointers.
> 
> ok. fair enough.
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Thanks!

Can I assume your ack also applies to the previous patch which adds the
ELF annotations ("x86/asm/bpf: Annotate callable functions")?

-- 
Josh

Re: [PATCH 23/33] x86/asm/bpf: Create stack frames in bpf_jit.S

[Alexei Starovoitov]

On Fri, Jan 22, 2016 at 11:36:14AM -0600, Josh Poimboeuf wrote:
> On Fri, Jan 22, 2016 at 09:18:23AM -0800, Alexei Starovoitov wrote:
> > On Fri, Jan 22, 2016 at 09:58:04AM -0600, Josh Poimboeuf wrote:
> > > On Thu, Jan 21, 2016 at 08:18:46PM -0800, Alexei Starovoitov wrote:
> > > > On Thu, Jan 21, 2016 at 09:55:31PM -0600, Josh Poimboeuf wrote:
> > > > > On Thu, Jan 21, 2016 at 06:44:28PM -0800, Alexei Starovoitov wrote:
> > > > > > On Thu, Jan 21, 2016 at 04:49:27PM -0600, Josh Poimboeuf wrote:
> > > > > > > bpf_jit.S has several callable non-leaf functions which don't honor
> > > > > > > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > > > > > > 
> > > > > > > Create a stack frame before the call instructions when
> > > > > > > CONFIG_FRAME_POINTER is enabled.
> > > > > > > 
> > > > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > > > Cc: netdev@vger.kernel.org
> > > > > > > ---
> > > > > > >  arch/x86/net/bpf_jit.S | 9 +++++++--
> > ...
> > > > > > >  /* rsi contains offset and can be scratched */
> > > > > > >  #define bpf_slow_path_common(LEN)		\
> > > > > > > +	lea	-MAX_BPF_STACK + 32(%rbp), %rdx;\
> > > > > > > +	FRAME_BEGIN;				\
> > > > > > >  	mov	%rbx, %rdi; /* arg1 == skb */	\
> > > > > > >  	push	%r9;				\
> > > > > > >  	push	SKBDATA;			\
> > > > > > >  /* rsi already has offset */			\
> > > > > > >  	mov	$LEN,%ecx;	/* len */	\
> > > > > > > -	lea	- MAX_BPF_STACK + 32(%rbp),%rdx;			\
> > > > > > >  	call	skb_copy_bits;			\
> > > > > > >  	test    %eax,%eax;			\
> > > > > > >  	pop	SKBDATA;			\
> > > > > > > -	pop	%r9;
> > > > > > > +	pop	%r9;				\
> > > > > > > +	FRAME_END
> > ...
> > > > > Well, but the point of these patches isn't to make the tool happy.  It's
> > > > > really to make sure that runtime stack traces can be made reliable.
> > > > > Maybe I'm missing something but I don't see why JIT code can't honor
> > > > > CONFIG_FRAME_POINTER just like any other code.
> > > > 
> > > > It can if there is no performance cost added.
> > > 
> > > CONFIG_FRAME_POINTER always adds a small performance cost but as you
> > > mentioned it only affects the slow path here.  And hopefully we'll soon
> > > have an in-kernel DWARF unwinder on x86 so we can get rid of the need
> > > for frame pointers.
> > 
> > ok. fair enough.
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
> 
> Thanks!
> 
> Can I assume your ack also applies to the previous patch which adds the
> ELF annotations ("x86/asm/bpf: Annotate callable functions")?

Yes. Thanks.

Re: [PATCH 00/33] Compile-time stack metadata validation

[Chris J Arges]

On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found.  It's based
> on the tip/master branch.
>
Josh,

Looks good, with my config [1] I do still get a few warnings building
linux/linux-next.

Here are the warnings:
$ grep ^stacktool build.log | grep -v staging
stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without frame pointer save/setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without frame pointer restore
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame pointer save
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame pointer setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer state mismatch
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer state mismatch
stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section

For vmx_handle_external_intr, I'm wondering if ignoring this function is the
best option.

--

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6..d19dfb2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/tboot.h>
 #include <linux/hrtimer.h>
+#include <linux/stacktool.h>
 #include "kvm_cache_regs.h"
 #include "x86.h"
 
@@ -8398,6 +8399,7 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
        } else
                local_irq_enable();
 }
+STACKTOOL_IGNORE_FUNC(vmx_handle_external_intr);
 
 static bool vmx_has_high_real_mode_segbase(void)
 {

--chris

[1] http://paste.ubuntu.com/14599083/

 
> v15 can be found here:
> 
>   https://lkml.kernel.org/r/cover.1450442274.git.jpoimboe@redhat.com
> 
> For more information about the motivation behind this patch set, and
> more details about what it does, see the first patch changelog and
> tools/stacktool/Documentation/stack-validation.txt.
> 
> Patches 1-4 add stacktool and integrate it into the kernel build.
> 
> Patches 5-28 are some proposed fixes for several of the warnings
> reported by stacktool.  They've been compile-tested and boot-tested in a
> VM, but I haven't attempted any meaningful testing for many of them.
> 
> Patches 29-33 add some directories, files, and functions to the
> stacktool whitelist in order to silence false positive warnings.
> 
> v16:
> - fix all allyesconfig warnings, except for staging
> - get rid of STACKTOOL_IGNORE_INSN which is no longer needed
> - remove several whitelists in favor of automatically whitelisting any
>   function with a special instruction like ljmp, lret, or vmrun
> - split up stacktool patch into 3 parts as suggested by Ingo
> - update the global noreturn function list
> - detect noreturn function fallthroughs
> - skip weak functions in noreturn call detection logic
> - add empty function check to noreturn logic
> - allow non-section rela symbols for __ex_table sections
> - support rare switch table case with jmpq *[addr](%rip)
> - don't warn on frame pointer restore without save
> - rearrange patch order a bit
> 
> v15:
> - restructure code for a new cmdline interface "stacktool check" using
>   the new subcommand framework in tools/lib/subcmd
> - fix 32 bit build fail (put __sp at end) in paravirt_types.h patch 10
>   which was reported by 0day
> 
> v14:
> - make tools/include/linux/list.h self-sufficient
> - create FRAME_OFFSET to allow 32-bit code to be able to access function
>   arguments on the stack
> - add FRAME_OFFSET usage in crypto patch 14/24: "Create stack frames in
>   aesni-intel_asm.S"
> - rename "index" -> "idx" to fix build with some compilers
> 
> v13:
> - LDFLAGS order fix from Chris J Arges
> - new warning fix patches from Chris J Arges
> - "--frame-pointer" -> "--check-frame-pointer"
> 
> v12:
> - rename "stackvalidate" -> "stacktool"
> - move from scripts/ to tools/:
>   - makefile rework
>   - make a copy of the x86 insn code (and warn if the code diverges)
>   - use tools/include/linux/list.h
> - move warning macros to a new warn.h file
> - change wording: "stack validation" -> "stack metadata validation"
> 
> 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>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> 
> Chris J Arges (1):
>   x86/uaccess: Add stack frame output operand in get_user inline asm
> 
> Josh Poimboeuf (32):
>   x86/stacktool: Compile-time stack metadata validation
>   kbuild/stacktool: Add CONFIG_STACK_VALIDATION option
>   x86/stacktool: Enable stacktool on x86_64
>   x86/stacktool: Add STACKTOOL_IGNORE_FUNC macro
>   x86/xen: Add stack frame dependency to hypercall inline asm calls
>   x86/asm/xen: Set ELF function type for xen_adjust_exception_frame()
>   x86/asm/xen: Create stack frames in xen-asm.S
>   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/asm/crypto: Move .Lbswap_mask data to .rodata section
>   x86/asm/crypto: Move jump_table to .rodata section
>   x86/asm/crypto: Simplify stack usage in sha-mb functions
>   x86/asm/crypto: Don't use rbp as a scratch register
>   x86/asm/crypto: Create stack frames in crypto functions
>   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
>   x86/asm/bpf: Annotate callable functions
>   x86/asm/bpf: Create stack frames in bpf_jit.S
>   x86/kprobes: Get rid of kretprobe_trampoline_holder()
>   x86/kvm: Set ELF function type for fastop functions
>   x86/kvm: Add stack frame dependency to test_cc() inline asm
>   watchdog/hpwdt: Create stack frame in asminline_call()
>   x86/locking: Create stack frame in PV unlock
>   x86/stacktool: Add directory and file whitelists
>   x86/xen: Add xen_cpuid() to stacktool whitelist
>   bpf: Add __bpf_prog_run() to stacktool whitelist
>   sched: Add __schedule() to stacktool whitelist
>   x86/kprobes: Add kretprobe_trampoline() to stacktool whitelist
> 
>  MAINTAINERS                                        |   6 +
>  Makefile                                           |   5 +-
>  arch/Kconfig                                       |   6 +
>  arch/x86/Kconfig                                   |   1 +
>  arch/x86/boot/Makefile                             |   1 +
>  arch/x86/boot/compressed/Makefile                  |   3 +-
>  arch/x86/crypto/aesni-intel_asm.S                  |  75 +-
>  arch/x86/crypto/camellia-aesni-avx-asm_64.S        |  15 +
>  arch/x86/crypto/camellia-aesni-avx2-asm_64.S       |  15 +
>  arch/x86/crypto/cast5-avx-x86_64-asm_64.S          |   9 +
>  arch/x86/crypto/cast6-avx-x86_64-asm_64.S          |  13 +
>  arch/x86/crypto/crc32c-pcl-intel-asm_64.S          |   8 +-
>  arch/x86/crypto/ghash-clmulni-intel_asm.S          |   5 +
>  arch/x86/crypto/serpent-avx-x86_64-asm_64.S        |  13 +
>  arch/x86/crypto/serpent-avx2-asm_64.S              |  13 +
>  arch/x86/crypto/sha-mb/sha1_mb_mgr_flush_avx2.S    |  35 +-
>  arch/x86/crypto/sha-mb/sha1_mb_mgr_submit_avx2.S   |  36 +-
>  arch/x86/crypto/twofish-avx-x86_64-asm_64.S        |  13 +
>  arch/x86/entry/Makefile                            |   4 +
>  arch/x86/entry/thunk_64.S                          |   4 +
>  arch/x86/entry/vdso/Makefile                       |   5 +-
>  arch/x86/include/asm/paravirt.h                    |   9 +-
>  arch/x86/include/asm/paravirt_types.h              |  18 +-
>  arch/x86/include/asm/qspinlock_paravirt.h          |   4 +
>  arch/x86/include/asm/uaccess.h                     |   5 +-
>  arch/x86/include/asm/xen/hypercall.h               |   5 +-
>  arch/x86/kernel/Makefile                           |   5 +
>  arch/x86/kernel/acpi/wakeup_64.S                   |   3 +
>  arch/x86/kernel/cpu/amd.c                          |   5 +-
>  arch/x86/kernel/kprobes/core.c                     |  59 +-
>  arch/x86/kernel/vmlinux.lds.S                      |   5 +-
>  arch/x86/kvm/emulate.c                             |  33 +-
>  arch/x86/lib/rwsem.S                               |  11 +-
>  arch/x86/net/bpf_jit.S                             |  48 +-
>  arch/x86/platform/efi/Makefile                     |   2 +
>  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                           |   3 +-
>  arch/x86/xen/xen-asm.S                             |  10 +-
>  arch/x86/xen/xen-asm_64.S                          |   1 +
>  drivers/firmware/efi/libstub/Makefile              |   1 +
>  drivers/watchdog/hpwdt.c                           |   8 +-
>  include/linux/stacktool.h                          |  23 +
>  kernel/bpf/core.c                                  |   2 +
>  kernel/sched/core.c                                |   2 +
>  lib/Kconfig.debug                                  |  12 +
>  scripts/Makefile.build                             |  38 +-
>  scripts/mod/Makefile                               |   2 +
>  tools/Makefile                                     |  14 +-
>  tools/stacktool/.gitignore                         |   2 +
>  tools/stacktool/Build                              |  13 +
>  tools/stacktool/Documentation/stack-validation.txt | 333 +++++++
>  tools/stacktool/Makefile                           |  60 ++
>  tools/stacktool/arch.h                             |  44 +
>  tools/stacktool/arch/x86/Build                     |  12 +
>  tools/stacktool/arch/x86/decode.c                  | 172 ++++
>  .../stacktool/arch/x86/insn/gen-insn-attr-x86.awk  | 387 ++++++++
>  tools/stacktool/arch/x86/insn/inat.c               |  97 ++
>  tools/stacktool/arch/x86/insn/inat.h               | 221 +++++
>  tools/stacktool/arch/x86/insn/inat_types.h         |  29 +
>  tools/stacktool/arch/x86/insn/insn.c               | 594 ++++++++++++
>  tools/stacktool/arch/x86/insn/insn.h               | 201 +++++
>  tools/stacktool/arch/x86/insn/x86-opcode-map.txt   | 984 ++++++++++++++++++++
>  tools/stacktool/builtin-check.c                    | 991 +++++++++++++++++++++
>  tools/stacktool/builtin.h                          |  22 +
>  tools/stacktool/elf.c                              | 403 +++++++++
>  tools/stacktool/elf.h                              |  79 ++
>  tools/stacktool/special.c                          | 193 ++++
>  tools/stacktool/special.h                          |  42 +
>  tools/stacktool/stacktool.c                        | 134 +++
>  tools/stacktool/warn.h                             |  60 ++
>  74 files changed, 5516 insertions(+), 189 deletions(-)
>  create mode 100644 include/linux/stacktool.h
>  create mode 100644 tools/stacktool/.gitignore
>  create mode 100644 tools/stacktool/Build
>  create mode 100644 tools/stacktool/Documentation/stack-validation.txt
>  create mode 100644 tools/stacktool/Makefile
>  create mode 100644 tools/stacktool/arch.h
>  create mode 100644 tools/stacktool/arch/x86/Build
>  create mode 100644 tools/stacktool/arch/x86/decode.c
>  create mode 100644 tools/stacktool/arch/x86/insn/gen-insn-attr-x86.awk
>  create mode 100644 tools/stacktool/arch/x86/insn/inat.c
>  create mode 100644 tools/stacktool/arch/x86/insn/inat.h
>  create mode 100644 tools/stacktool/arch/x86/insn/inat_types.h
>  create mode 100644 tools/stacktool/arch/x86/insn/insn.c
>  create mode 100644 tools/stacktool/arch/x86/insn/insn.h
>  create mode 100644 tools/stacktool/arch/x86/insn/x86-opcode-map.txt
>  create mode 100644 tools/stacktool/builtin-check.c
>  create mode 100644 tools/stacktool/builtin.h
>  create mode 100644 tools/stacktool/elf.c
>  create mode 100644 tools/stacktool/elf.h
>  create mode 100644 tools/stacktool/special.c
>  create mode 100644 tools/stacktool/special.h
>  create mode 100644 tools/stacktool/stacktool.c
>  create mode 100644 tools/stacktool/warn.h
> 
> -- 
> 2.4.3
> 

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Jan 22, 2016 at 11:43:48AM -0600, Chris J Arges wrote:
> On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> > This is v16 of the compile-time stack metadata validation patch set,
> > along with proposed fixes for most of the warnings it found.  It's based
> > on the tip/master branch.
> >
> Josh,
> 
> Looks good, with my config [1] I do still get a few warnings building
> linux/linux-next.
> 
> Here are the warnings:
> $ grep ^stacktool build.log | grep -v staging

Thanks for reporting these!

> stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without frame pointer save/setup

This can be fixed by setting the stack pointer as an output operand for
the inline asm call in vmx_handle_external_intr().

Feel free to submit a patch, or I'll get around to it eventually.

> stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without frame pointer restore
> stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame pointer save
> stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame pointer setup
> stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer state mismatch
> stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer state mismatch

These are false positives.  Stacktool is confused by the use of a
"noreturn" function which it doesn't know about (__reiserfs_panic).

Unfortunately the only solution I currently have for dealing with global
noreturn functions is to just hard-code a list of them.  So the short
term fix would be to add "__reiserfs_panic" to the global_noreturns list
in tools/stacktool/builtin-check.c.

I'm still trying to figure out a better way to deal with this type of
issue, as it's a pain to have to keep a hard-coded list of noreturn
functions.  Unfortunately that info isn't available in the ELF.

> stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
> stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section

For some reason I'm not able to recreate these warnings...  Can you
share one of the .o files?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Chris J Arges]

On Fri, Jan 22, 2016 at 01:14:47PM -0600, Josh Poimboeuf wrote:
> On Fri, Jan 22, 2016 at 11:43:48AM -0600, Chris J Arges wrote:
> > On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> > > This is v16 of the compile-time stack metadata validation patch set,
> > > along with proposed fixes for most of the warnings it found.  It's based
> > > on the tip/master branch.
> > >
> > Josh,
> > 
> > Looks good, with my config [1] I do still get a few warnings building
> > linux/linux-next.
> > 
> > Here are the warnings:
> > $ grep ^stacktool build.log | grep -v staging
> 
> Thanks for reporting these!
> 
> > stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without frame pointer save/setup
> 
> This can be fixed by setting the stack pointer as an output operand for
> the inline asm call in vmx_handle_external_intr().
> 
> Feel free to submit a patch, or I'll get around to it eventually.
> 
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without frame pointer restore
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame pointer save
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame pointer setup
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer state mismatch
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer state mismatch
> 
> These are false positives.  Stacktool is confused by the use of a
> "noreturn" function which it doesn't know about (__reiserfs_panic).
> 
> Unfortunately the only solution I currently have for dealing with global
> noreturn functions is to just hard-code a list of them.  So the short
> term fix would be to add "__reiserfs_panic" to the global_noreturns list
> in tools/stacktool/builtin-check.c.
> 
> I'm still trying to figure out a better way to deal with this type of
> issue, as it's a pain to have to keep a hard-coded list of noreturn
> functions.  Unfortunately that info isn't available in the ELF.
> 

Josh,
Ok I'll hack on the patches above.

> > stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
> > stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section
> 
> For some reason I'm not able to recreate these warnings...  Can you
> share one of the .o files?
> 
> -- 
> Josh
> 

Binaries are here:
http://people.canonical.com/~arges/stacktool/

--chris
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Jan 22, 2016 at 02:40:35PM -0600, Chris J Arges wrote:
> On Fri, Jan 22, 2016 at 01:14:47PM -0600, Josh Poimboeuf wrote:
> > On Fri, Jan 22, 2016 at 11:43:48AM -0600, Chris J Arges wrote:
> > > On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> > > > This is v16 of the compile-time stack metadata validation patch set,
> > > > along with proposed fixes for most of the warnings it found.  It's based
> > > > on the tip/master branch.
> > > >
> > > Josh,
> > > 
> > > Looks good, with my config [1] I do still get a few warnings building
> > > linux/linux-next.
> > > 
> > > Here are the warnings:
> > > $ grep ^stacktool build.log | grep -v staging
> > 
> > Thanks for reporting these!
> > 
> > > stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without frame pointer save/setup
> > 
> > This can be fixed by setting the stack pointer as an output operand for
> > the inline asm call in vmx_handle_external_intr().
> > 
> > Feel free to submit a patch, or I'll get around to it eventually.
> > 
> > > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without frame pointer restore
> > > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame pointer save
> > > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame pointer setup
> > > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer state mismatch
> > > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer state mismatch
> > 
> > These are false positives.  Stacktool is confused by the use of a
> > "noreturn" function which it doesn't know about (__reiserfs_panic).
> > 
> > Unfortunately the only solution I currently have for dealing with global
> > noreturn functions is to just hard-code a list of them.  So the short
> > term fix would be to add "__reiserfs_panic" to the global_noreturns list
> > in tools/stacktool/builtin-check.c.
> > 
> > I'm still trying to figure out a better way to deal with this type of
> > issue, as it's a pain to have to keep a hard-coded list of noreturn
> > functions.  Unfortunately that info isn't available in the ELF.
> > 
> 
> Josh,
> Ok I'll hack on the patches above.
> 
> > > stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
> > > stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section
> > 
> > For some reason I'm not able to recreate these warnings...  Can you
> > share one of the .o files?
> 
> Binaries are here:
> http://people.canonical.com/~arges/stacktool/

Thanks, looks like the same __reiserfs_panic() noreturn fix for those.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Jiri Slaby]

On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found.  It's based
> on the tip/master branch.

Hi,

with this config:
https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla

I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:
stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup



For example do_profile_hits.isra.5:
0000000000003360 <hpsa_scsi_do_simple_cmd.constprop.106>:
    3360:       e8 00 00 00 00          callq  3365 <hpsa_scsi_do_simple_cmd.constprop.106+0x5>
                        3361: R_X86_64_PC32     __fentry__-0x4
    3365:       65 ff 05 00 00 00 00    incl   %gs:0x0(%rip)        # 336c <hpsa_scsi_do_simple_cmd.constprop.106+0xc>
                        3368: R_X86_64_PC32     __preempt_count-0x4
    336c:       65 8b 0d 00 00 00 00    mov    %gs:0x0(%rip),%ecx        # 3373 <hpsa_scsi_do_simple_cmd.constprop.106+0x13>
                        336f: R_X86_64_PC32     cpu_number-0x4
    3373:       48 63 c9                movslq %ecx,%rcx
    3376:       48 8b 87 b8 4b 00 00    mov    0x4bb8(%rdi),%rax
    337d:       48 8b 0c cd 00 00 00    mov    0x0(,%rcx,8),%rcx
    3384:       00 
                        3381: R_X86_64_32S      __per_cpu_offset
    3385:       8b 04 01                mov    (%rcx,%rax,1),%eax
    3388:       65 ff 0d 00 00 00 00    decl   %gs:0x0(%rip)        # 338f <hpsa_scsi_do_simple_cmd.constprop.106+0x2f>
                        338b: R_X86_64_PC32     __preempt_count-0x4
    338f:       74 48                   je     33d9 <hpsa_scsi_do_simple_cmd.constprop.106+0x79>
    3391:       85 c0                   test   %eax,%eax
    3393:       75 4d                   jne    33e2 <hpsa_scsi_do_simple_cmd.constprop.106+0x82>
    3395:       55                      push   %rbp
    3396:       48 89 e5                mov    %rsp,%rbp
    3399:       53                      push   %rbx
    339a:       48 8d 5d d8             lea    -0x28(%rbp),%rbx
    339e:       48 83 ec 20             sub    $0x20,%rsp
    33a2:       c7 45 d8 00 00 00 00    movl   $0x0,-0x28(%rbp)
    33a9:       c7 45 e0 00 00 00 00    movl   $0x0,-0x20(%rbp)
    33b0:       48 8d 43 10             lea    0x10(%rbx),%rax
    33b4:       48 89 9e 54 02 00 00    mov    %rbx,0x254(%rsi)
    33bb:       48 89 45 e8             mov    %rax,-0x18(%rbp)
    33bf:       48 89 45 f0             mov    %rax,-0x10(%rbp)
    33c3:       e8 f8 ce ff ff          callq  2c0 <__enqueue_cmd_and_start_io>
    33c8:       48 89 df                mov    %rbx,%rdi
    33cb:       e8 00 00 00 00          callq  33d0 <hpsa_scsi_do_simple_cmd.constprop.106+0x70>
                        33cc: R_X86_64_PC32     wait_for_completion_io-0x4
    33d0:       48 83 c4 20             add    $0x20,%rsp
    33d4:       31 c0                   xor    %eax,%eax
    33d6:       5b                      pop    %rbx
    33d7:       5d                      pop    %rbp
    33d8:       c3                      retq   
    33d9:       e8 00 00 00 00          callq  33de <hpsa_scsi_do_simple_cmd.constprop.106+0x7e>
                        33da: R_X86_64_PC32     ___preempt_schedule-0x4
    33de:       85 c0                   test   %eax,%eax
    33e0:       74 b3                   je     3395 <hpsa_scsi_do_simple_cmd.constprop.106+0x35>
    33e2:       48 8b 86 38 02 00 00    mov    0x238(%rsi),%rax
    33e9:       ba ff ff ff ff          mov    $0xffffffff,%edx
    33ee:       66 89 50 02             mov    %dx,0x2(%rax)
    33f2:       31 c0                   xor    %eax,%eax
    33f4:       c3                      retq   
    33f5:       90                      nop
    33f6:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
    33fd:       00 00 00 

It there some compilation flag missing? -f flags when compiling that file are:
-falign-jumps=1
-falign-loops=1
-fconserve-stack
-fno-asynchronous-unwind-tables
-fno-common
-fno-delete-null-pointer-checks
-fno-inline-functions-called-once
-fno-omit-frame-pointer
-fno-optimize-sibling-calls
-fno-strict-aliasing
-fno-strict-overflow
-fno-var-tracking-assignments
-fstack-protector
-funit-at-a-time

thanks,
-- 
js
suse labs

Re: [PATCH 00/33] Compile-time stack metadata validation

[Jiri Slaby]

On 02/12/2016, 11:36 AM, Jiri Slaby wrote:
> It there some compilation flag missing? -f flags when compiling that file are:
> -falign-jumps=1
> -falign-loops=1
> -fconserve-stack
> -fno-asynchronous-unwind-tables
> -fno-common
> -fno-delete-null-pointer-checks
> -fno-inline-functions-called-once
> -fno-omit-frame-pointer
> -fno-optimize-sibling-calls
> -fno-strict-aliasing
> -fno-strict-overflow
> -fno-var-tracking-assignments
> -fstack-protector
> -funit-at-a-time

Happens with:
gcc (SUSE Linux) 5.3.1 20151207 [gcc-5-branch revision 231355]
gcc-6 (SUSE Linux) 6.0.0 20160202 (experimental) [trunk revision 233076]

> thanks,
-- 
js
suse labs

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> > This is v16 of the compile-time stack metadata validation patch set,
> > along with proposed fixes for most of the warnings it found.  It's based
> > on the tip/master branch.
> 
> Hi,
> 
> with this config:
> https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla
> 
> I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:

Hi Jiri,

Thanks for testing.

> stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup

This seems like a real frame pointer bug caused by the following line in
arch/x86/include/asm/preempt.h:

  # define __preempt_schedule() asm ("call ___preempt_schedule")

The asm statement doesn't have the stack pointer as an output operand,
so gcc doesn't skips the frame pointer setup before calling.

However, I suspect the "bug" is intentional for optimization purposes.

> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section

These staging driver issues are caused by stacktool getting confused by
gcc optimizations related to noreturn functions.  I have it on the TODO
list to make the noreturn function detection more intelligent.

> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
> stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
> stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
> stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup

These are all the same "call ___preempt_schedule" issue from above.
I'll need to look into it to figure out if it's a real bug or if it's a
"feature" we should ignore.

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Peter Zijlstra]

On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> 
> This seems like a real frame pointer bug caused by the following line in
> arch/x86/include/asm/preempt.h:
> 
>   # define __preempt_schedule() asm ("call ___preempt_schedule")

The purpose there is that:

	preempt_enable();

turns into:

	decl	__percpu_prefix:__preempt_count
	jnz	1f:
	call	___preempt_schedule
1:

See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Feb 12, 2016 at 06:10:37PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> > On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> > 
> > This seems like a real frame pointer bug caused by the following line in
> > arch/x86/include/asm/preempt.h:
> > 
> >   # define __preempt_schedule() asm ("call ___preempt_schedule")
> 
> The purpose there is that:
> 
> 	preempt_enable();
> 
> turns into:
> 
> 	decl	__percpu_prefix:__preempt_count
> 	jnz	1f:
> 	call	___preempt_schedule
> 1:
> 
> See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()

Sorry, I'm kind of confused.  Do you mean that's what preempt_enable()
would turn into *without* the above define?

What I actually see in the listing is:

 	decl	__percpu_prefix:__preempt_count
 	je	1f:
	....
 1:
 	call	___preempt_schedule

So it puts the "call ___preempt_schedule" in the slow path.

I also don't see how that would be related to the use of the asm
statement in the __preempt_schedule() macro.  Doesn't the use of
unlikely() in preempt_enable() put the call in the slow path?

  #define preempt_enable() \
  do { \
	  barrier(); \
	  if (unlikely(preempt_count_dec_and_test())) \
		  preempt_schedule(); \
  } while (0)

Also, why is the thunk needed?  Any reason why preempt_enable() can't be
called directly from C?

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 06:10:37PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> > > On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> > > 
> > > This seems like a real frame pointer bug caused by the following line in
> > > arch/x86/include/asm/preempt.h:
> > > 
> > >   # define __preempt_schedule() asm ("call ___preempt_schedule")
> > 
> > The purpose there is that:
> > 
> > 	preempt_enable();
> > 
> > turns into:
> > 
> > 	decl	__percpu_prefix:__preempt_count
> > 	jnz	1f:
> > 	call	___preempt_schedule
> > 1:
> > 
> > See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
> 
> Sorry, I'm kind of confused.  Do you mean that's what preempt_enable()
> would turn into *without* the above define?
> 
> What I actually see in the listing is:
> 
>  	decl	__percpu_prefix:__preempt_count
>  	je	1f:
> 	....
>  1:
>  	call	___preempt_schedule
> 
> So it puts the "call ___preempt_schedule" in the slow path.
> 
> I also don't see how that would be related to the use of the asm
> statement in the __preempt_schedule() macro.  Doesn't the use of
> unlikely() in preempt_enable() put the call in the slow path?
> 
>   #define preempt_enable() \
>   do { \
> 	  barrier(); \
> 	  if (unlikely(preempt_count_dec_and_test())) \
> 		  preempt_schedule(); \
>   } while (0)
> 
> Also, why is the thunk needed?  Any reason why preempt_enable() can't be
> called directly from C?

Sorry, s/preempt_enable/preempt_schedule/ on that last sentence.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Peter Zijlstra]

On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> What I actually see in the listing is:
> 
>  	decl	__percpu_prefix:__preempt_count
>  	je	1f:
> 	....
>  1:
>  	call	___preempt_schedule
> 
> So it puts the "call ___preempt_schedule" in the slow path.

Ah yes indeed. Same difference though.

> I also don't see how that would be related to the use of the asm
> statement in the __preempt_schedule() macro.  Doesn't the use of
> unlikely() in preempt_enable() put the call in the slow path?

Sadly no, unlikely() and asm_goto don't work well together. But the slow
path or not isn't the reason we do the asm call thing.

>   #define preempt_enable() \
>   do { \
> 	  barrier(); \
> 	  if (unlikely(preempt_count_dec_and_test())) \
> 		  preempt_schedule(); \
>   } while (0)
> 
> Also, why is the thunk needed?  Any reason why preempt_enable() can't be
> called directly from C?

That would make the call-site save registers and increase the size of
every preempt_enable(). By using the thunk we can do callee saved
registers and avoid blowing up the call site.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Fri, Feb 12, 2016 at 09:10:11PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> > What I actually see in the listing is:
> > 
> >  	decl	__percpu_prefix:__preempt_count
> >  	je	1f:
> > 	....
> >  1:
> >  	call	___preempt_schedule
> > 
> > So it puts the "call ___preempt_schedule" in the slow path.
> 
> Ah yes indeed. Same difference though.
> 
> > I also don't see how that would be related to the use of the asm
> > statement in the __preempt_schedule() macro.  Doesn't the use of
> > unlikely() in preempt_enable() put the call in the slow path?
> 
> Sadly no, unlikely() and asm_goto don't work well together. But the slow
> path or not isn't the reason we do the asm call thing.
> 
> >   #define preempt_enable() \
> >   do { \
> > 	  barrier(); \
> > 	  if (unlikely(preempt_count_dec_and_test())) \
> > 		  preempt_schedule(); \
> >   } while (0)
> > 
> > Also, why is the thunk needed?  Any reason why preempt_enable() can't be
> > called directly from C?
> 
> That would make the call-site save registers and increase the size of
> every preempt_enable(). By using the thunk we can do callee saved
> registers and avoid blowing up the call site.

So is the goal to optimize for size?  If I replace the calls to
__preempt_schedule[_notrace]() with real C calls and remove the thunks,
it only adds about 2k to vmlinux.

There are two ways to fix the warnings:

1. get rid of the thunks and call the C functions directly; or

2. add the stack pointer to the asm() statement output operand list to
ensure a stack frame gets created in the caller function before the
call.  (Note this still allows the thunks to do callee saved registers.)

I like #1 better, but maybe I'm still missing the point of the thunks.

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Peter Zijlstra]

On Mon, Feb 15, 2016 at 10:31:34AM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 09:10:11PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> > > What I actually see in the listing is:
> > > 
> > >  	decl	__percpu_prefix:__preempt_count
> > >  	je	1f:
> > > 	....
> > >  1:
> > >  	call	___preempt_schedule
> > > 
> > > So it puts the "call ___preempt_schedule" in the slow path.
> > 
> > Ah yes indeed. Same difference though.
> > 
> > > I also don't see how that would be related to the use of the asm
> > > statement in the __preempt_schedule() macro.  Doesn't the use of
> > > unlikely() in preempt_enable() put the call in the slow path?
> > 
> > Sadly no, unlikely() and asm_goto don't work well together. But the slow
> > path or not isn't the reason we do the asm call thing.
> > 
> > >   #define preempt_enable() \
> > >   do { \
> > > 	  barrier(); \
> > > 	  if (unlikely(preempt_count_dec_and_test())) \
> > > 		  preempt_schedule(); \
> > >   } while (0)
> > > 
> > > Also, why is the thunk needed?  Any reason why preempt_enable() can't be
> > > called directly from C?
> > 
> > That would make the call-site save registers and increase the size of
> > every preempt_enable(). By using the thunk we can do callee saved
> > registers and avoid blowing up the call site.
> 
> So is the goal to optimize for size?  

General performance impact of preempt_enable().

> If I replace the calls to
> __preempt_schedule[_notrace]() with real C calls and remove the thunks,
> it only adds about 2k to vmlinux.

That's less than I had expected, but probably still worth it.

And is that added text purely in the slow path? We really want to avoid
putting any more register pressure on the preempt_enable() call sites.
The single memop and Jcc is about as fast we can get and we spend quite
a bit of effort getting there.

> There are two ways to fix the warnings:
> 
> 1. get rid of the thunks and call the C functions directly; or
> 
> 2. add the stack pointer to the asm() statement output operand list to
> ensure a stack frame gets created in the caller function before the
> call.  (Note this still allows the thunks to do callee saved registers.)
> 
> I like #1 better, but maybe I'm still missing the point of the thunks.

Ingo, Linus?
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Mon, Feb 15, 2016 at 08:56:21AM -0800, Linus Torvalds wrote:
> On Feb 15, 2016 8:31 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > So is the goal to optimize for size?  If I replace the calls to
> > __preempt_schedule[_notrace]() with real C calls and remove the thunks,
> > it only adds about 2k to vmlinux.
> 
> It adds nasty register pressure in some of the most critical parts of the
> kernel, and makes the asm code harder to read.
> 
> And yes, I still read the asm. For performance reasons, and when decoding
> oopses.
> 
> I realize that few other people care about generated code. That's sad.
> 
> > There are two ways to fix the warnings:
> >
> > 1. get rid of the thunks and call the C functions directly; or
> 
> No. Not until gcc learns about per-function callibg conventions (so that it
> can be marked as not clobbering registers).
> 
> > 2. add the stack pointer to the asm() statement output operand list to
> > ensure a stack frame gets created in the caller function before the
> > call.
> 
> That probably doesn't make things much worse. It probably makes least
> functions have a stack frame if they do preempt disable, but it might still
> be acceptable. Hard to say before I end up hurting this case again.

Oddly, this change (see patch below) actually seems to make things
faster in a lot of cases.  For many smaller functions it causes the
stack frame creation to get moved out of the common path and into the
unlikely path.

For example, here's the original cyc2ns_read_end():

  ffffffff8101f8c0 <cyc2ns_read_end>:
  ffffffff8101f8c0:	55                   	push   %rbp
  ffffffff8101f8c1:	48 89 e5             	mov    %rsp,%rbp
  ffffffff8101f8c4:	83 6f 10 01          	subl   $0x1,0x10(%rdi)
  ffffffff8101f8c8:	75 08                	jne    ffffffff8101f8d2 <cyc2ns_read_end+0x12>
  ffffffff8101f8ca:	65 48 89 3d e6 5a ff 	mov    %rdi,%gs:0x7eff5ae6(%rip)        # 153b8 <cyc2ns+0x38>
  ffffffff8101f8d1:	7e 
  ffffffff8101f8d2:	65 ff 0d 77 c4 fe 7e 	decl   %gs:0x7efec477(%rip)        # bd50 <__preempt_count>
  ffffffff8101f8d9:	74 02                	je     ffffffff8101f8dd <cyc2ns_read_end+0x1d>
  ffffffff8101f8db:	5d                   	pop    %rbp
  ffffffff8101f8dc:	c3                   	retq   
  ffffffff8101f8dd:	e8 1e 37 fe ff       	callq  ffffffff81003000 <___preempt_schedule>
  ffffffff8101f8e2:	5d                   	pop    %rbp
  ffffffff8101f8e3:	c3                   	retq   
  ffffffff8101f8e4:	66 66 66 2e 0f 1f 84 	data16 data16 nopw %cs:0x0(%rax,%rax,1)
  ffffffff8101f8eb:	00 00 00 00 00 

And here's the same function with the patch:

  ffffffff8101f8c0 <cyc2ns_read_end>:
  ffffffff8101f8c0:	83 6f 10 01          	subl   $0x1,0x10(%rdi)
  ffffffff8101f8c4:	75 08                	jne    ffffffff8101f8ce <cyc2ns_read_end+0xe>
  ffffffff8101f8c6:	65 48 89 3d ea 5a ff 	mov    %rdi,%gs:0x7eff5aea(%rip)        # 153b8 <cyc2ns+0x38>
  ffffffff8101f8cd:	7e 
  ffffffff8101f8ce:	65 ff 0d 7b c4 fe 7e 	decl   %gs:0x7efec47b(%rip)        # bd50 <__preempt_count>
  ffffffff8101f8d5:	74 01                	je     ffffffff8101f8d8 <cyc2ns_read_end+0x18>
  ffffffff8101f8d7:	c3                   	retq   
  ffffffff8101f8d8:	55                   	push   %rbp
  ffffffff8101f8d9:	48 89 e5             	mov    %rsp,%rbp
  ffffffff8101f8dc:	e8 1f 37 fe ff       	callq  ffffffff81003000 <___preempt_schedule>
  ffffffff8101f8e1:	5d                   	pop    %rbp
  ffffffff8101f8e2:	c3                   	retq   
  ffffffff8101f8e3:	66 66 66 66 2e 0f 1f 	data16 data16 data16 nopw %cs:0x0(%rax,%rax,1)
  ffffffff8101f8ea:	84 00 00 00 00 00 

Notice that it moved the frame pointer setup code to the unlikely
___preempt_schedule() call path.  Going through a sampling of the
differences in the asm, that's the most common change I see.

Otherwise it has no real effect on callers which already have stack
frames (though it does change the ordering of some 'mov's).

And it has the intended effect of fixing the following warnings by
ensuring these call sites have stack frames:

  stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
  stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
  stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
  stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
  stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
  stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
  stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
  stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
  stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
  stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup

So that only adds a stack frame to 15 call sites out of ~5000 calls to
___preempt_schedule[_notrace].  All the others already had stack frames.

Any idea for a good benchmark to run with the patch?

> The alternative is to just teach the tools about the magic issue of a few
> things like this.

I think that would be problematic for our goal of making stack traces of
sleeping tasks reliable.  If preempt_enable()'s caller doesn't first
create a stack frame, the caller of the caller will get skipped in the
stack trace.

---8<---

diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index 01bcde8..2989aa6 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -94,10 +94,19 @@ static __always_inline bool should_resched(int preempt_offset)
 
 #ifdef CONFIG_PREEMPT
   extern asmlinkage void ___preempt_schedule(void);
-# define __preempt_schedule() asm ("call ___preempt_schedule")
+# define __preempt_schedule() \
+({ \
+	register void *__sp asm(_ASM_SP); \
+	asm volatile ("call ___preempt_schedule" : "+r"(__sp)); \
+})
+
   extern asmlinkage void preempt_schedule(void);
   extern asmlinkage void ___preempt_schedule_notrace(void);
-# define __preempt_schedule_notrace() asm ("call ___preempt_schedule_notrace")
+# define __preempt_schedule_notrace() \
+({ \
+	register void *__sp asm(_ASM_SP); \
+	asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp)); \
+})
   extern asmlinkage void preempt_schedule_notrace(void);
 #endif
 

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Andi Kleen]

> > There are two ways to fix the warnings:
> >
> > 1. get rid of the thunks and call the C functions directly; or
> 
> No. Not until gcc learns about per-function callibg conventions (so that it can
> be marked as not clobbering registers).

It does already for static functions in 5.x (with -fipa-ra).
And with LTO it can be used even somewhat globally.

Even older version supported it, for only for x86->SSE on 32bit, which
is useless for the kernel. But the new IPA-RA propagates which registers
are clobbered.

That said it will probably be a long time until we can drop support for
older compilers.  So for now the manual method is still needed.

-Andi

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Ingo Molnar]

So I tried out this latest stacktool series and it looks mostly good for an 
upstream merge.

To help this effort move forward I've applied the preparatory/fix patches that are 
part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
propagated all the acks that the latest submission got into the changelogs.)

I have 5 (easy to address) observations that need to be addressed before we can 
move on with the rest of the merge:

1)

Due to recent changes to x86 exception handling, I get a lot of bogus warnings 
about exception table sizes:

  stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24

this is due to:

  548acf19234d x86/mm: Expand the exception table logic to allow new handling options

2)

The fact that 'stacktool' already checks about assembly details like __ex_table[] 
shows that my review feedback early iterations of this series, that the 
'stacktool' name is too specific, was correct.

We really need to rename it before it gets upstream and the situation gets worse. 
__ex_table[] has obviously nothing to do with the 'stack layout' of binaries.

Another suitable name would be 'asmtool' or 'objtool'. For example the following 
would naturally express what it does:

  objtool check kernel/built-in.o

the name expresses that the tool checks object files, independently of the 
existing toolchain. Its primary purpose right now is the checking of stack layout 
details, but the tool is not limited to that at all.

3)

There's quite a bit of overhead when running the tool on larger object files, most 
prominently in cmd_check():

    62.06%  stacktool        stacktool                              [.] cmd_check
     6.72%  stacktool        stacktool                              [.] find_rela_by_dest_range

I added -g to the CFLAGS, which made it visible to annotated output of perf top:

    0.00 :        40430d:       lea    0x4(%rdx,%rax,1),%r13
         :      find_instruction():
         :                                                  struct section *sec,
         :                                                  unsigned long offset)
         :      {
         :              struct instruction *insn;
         :
         :              list_for_each_entry(insn, &file->insns, list)
    0.03 :        404312:       mov    0x38(%rsp),%rax
    0.00 :        404317:       cmp    %rbp,%rax
    0.00 :        40431a:       jne    404334 <cmd_check+0x5b4>
    0.00 :        40431c:       jmpq   4045ba <cmd_check+0x83a>
    0.00 :        404321:       nopl   0x0(%rax)
    6.14 :        404328:       mov    (%rax),%rax
    0.00 :        40432b:       cmp    %rbp,%rax
    2.02 :        40432e:       je     4045ba <cmd_check+0x83a>
         :                      if (insn->sec == sec && insn->offset == offset)
    0.55 :        404334:       cmp    %r12,0x10(%rax)
   87.91 :        404338:       jne    404328 <cmd_check+0x5a8>
    0.00 :        40433a:       cmp    %r13,0x18(%rax)
    3.36 :        40433e:       jne    404328 <cmd_check+0x5a8>
         :      get_jump_destinations():
         :                               * later in validate_functions().
         :                               */
         :                              continue;
         :                      }
         :
         :                      insn->jump_dest = find_instruction(file, dest_sec, dest_off);
    0.00 :        404340:       mov    %rax,0x48(%rbx)
    0.00 :        404344:       jmpq   4042b0 <cmd_check+0x530>
    0.00 :        404349:       nopl   0x0(%rax)
         :      fprintf():
         :
         :      # ifdef __va_arg_pack
         :      __fortify_function int
         :      fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
         :      {
         :        return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,

that looks like a linear list search? That would suck with thousands of entries.

(If this is non-trivial to improve then we can delay this optimization to a later 
patch.)

4)

I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer 
- it's not the tool we want to name but the actual property of the code.

So instead of:

  STACKTOOL_IGNORE_FUNC(__bpf_prog_run);

we should do something like:

  STACK_FRAME_NON_STANDARD(__bpf_prog_run);

see how much more expressive it is? It becomes a function attribute independent of 
the tooling that makes use of it.

Similarly, for the highest level 'don't check these directories' makefile flags, 
I'd suggest, instead of using this rather opaque, tool dependent naming:

  STACKTOOL := n

something more specific, like:

  OBJECT_FILES_NON_STANDARD := y

which makes it clearer what's going on: these are special object files that are 
not the typical kernel object files.

Stacktool (or objtool) would be one consumer of this annotation.

I think Boris made similar observations in past reviews.

5)

Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
we do exception table checks as well - and it does not express all the other 
things we may check in object files in the future.

Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
would list all the things the tool is able to checks for at the moment.

-------------------

Please send followup iterations of the series against the tip:x86/debug tree (or 
tip:master), to keep the size of the series down.

Thanks,

	Ingo

Re: [PATCH 00/33] Compile-time stack metadata validation

[Arnaldo Carvalho de Melo]

Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> shows that my review feedback early iterations of this series, that the 
> 'stacktool' name is too specific, was correct.
> 
> We really need to rename it before it gets upstream and the situation gets worse. 
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> 
> Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> would naturally express what it does:
> 
>   objtool check kernel/built-in.o
> 
> the name expresses that the tool checks object files, independently of the 
> existing toolchain. Its primary purpose right now is the checking of stack layout 
> details, but the tool is not limited to that at all.

Removing 'tool' from the tool name would be nice too :-) Making it
easily googlable would be good too, lotsa people complain about 'perf'
being too vague, see for a quick laugher:

http://www.brendangregg.com/perf.html

``Searching for just "perf" finds sites on the police, petroleum, weed
control, and a T-shirt. This is not an official perf page, for either
perf_events or the T-shirt.''

The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg

Maybe we should ask Linus to come with some other nice, short,
searchable, funny name like 'git'?

'chob' as in 'check object'?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

Hi Ingo,

On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> So I tried out this latest stacktool series and it looks mostly good for an 
> upstream merge.
> 
> To help this effort move forward I've applied the preparatory/fix patches that are 
> part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> propagated all the acks that the latest submission got into the changelogs.)

Thanks very much for your review and for applying the fixes!

A few issues relating to the merge:

- The tip:x86/debug branch fails to build because it depends on
  ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
  is in tip:x86/asm.

- As Pavel mentioned, the tip-bot seems to be spitting out garbage
  emails from:
  =?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.

> I have 5 (easy to address) observations that need to be addressed before we can 
> move on with the rest of the merge:
> 
> 1)
> 
> Due to recent changes to x86 exception handling, I get a lot of bogus warnings 
> about exception table sizes:
> 
>   stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
>   stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
>   stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24
> 
> this is due to:
> 
>   548acf19234d x86/mm: Expand the exception table logic to allow new handling options

Ok, I'll fix it up.

> 2)
> 
> The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> shows that my review feedback early iterations of this series, that the 
> 'stacktool' name is too specific, was correct.
> 
> We really need to rename it before it gets upstream and the situation gets worse. 
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> 
> Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> would naturally express what it does:
> 
>   objtool check kernel/built-in.o
> 
> the name expresses that the tool checks object files, independently of the 
> existing toolchain. Its primary purpose right now is the checking of stack layout 
> details, but the tool is not limited to that at all.

Fair enough.  I'll rename it to objtool if there are no objections.

> 3)
> 
> There's quite a bit of overhead when running the tool on larger object files, most 
> prominently in cmd_check():
> 
>     62.06%  stacktool        stacktool                              [.] cmd_check
>      6.72%  stacktool        stacktool                              [.] find_rela_by_dest_range
> 
> I added -g to the CFLAGS, which made it visible to annotated output of perf top:
> 
>     0.00 :        40430d:       lea    0x4(%rdx,%rax,1),%r13
>          :      find_instruction():
>          :                                                  struct section *sec,
>          :                                                  unsigned long offset)
>          :      {
>          :              struct instruction *insn;
>          :
>          :              list_for_each_entry(insn, &file->insns, list)
>     0.03 :        404312:       mov    0x38(%rsp),%rax
>     0.00 :        404317:       cmp    %rbp,%rax
>     0.00 :        40431a:       jne    404334 <cmd_check+0x5b4>
>     0.00 :        40431c:       jmpq   4045ba <cmd_check+0x83a>
>     0.00 :        404321:       nopl   0x0(%rax)
>     6.14 :        404328:       mov    (%rax),%rax
>     0.00 :        40432b:       cmp    %rbp,%rax
>     2.02 :        40432e:       je     4045ba <cmd_check+0x83a>
>          :                      if (insn->sec == sec && insn->offset == offset)
>     0.55 :        404334:       cmp    %r12,0x10(%rax)
>    87.91 :        404338:       jne    404328 <cmd_check+0x5a8>
>     0.00 :        40433a:       cmp    %r13,0x18(%rax)
>     3.36 :        40433e:       jne    404328 <cmd_check+0x5a8>
>          :      get_jump_destinations():
>          :                               * later in validate_functions().
>          :                               */
>          :                              continue;
>          :                      }
>          :
>          :                      insn->jump_dest = find_instruction(file, dest_sec, dest_off);
>     0.00 :        404340:       mov    %rax,0x48(%rbx)
>     0.00 :        404344:       jmpq   4042b0 <cmd_check+0x530>
>     0.00 :        404349:       nopl   0x0(%rax)
>          :      fprintf():
>          :
>          :      # ifdef __va_arg_pack
>          :      __fortify_function int
>          :      fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
>          :      {
>          :        return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
> 
> that looks like a linear list search? That would suck with thousands of entries.
> 
> (If this is non-trivial to improve then we can delay this optimization to a later 
> patch.)

Yeah, I agree that the linear list search isn't good.  I still need to
think about the data structures a bit, so if it's ok with you, I'll fix
it with a later patch.

> 4)
> 
> I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer 
> - it's not the tool we want to name but the actual property of the code.
> 
> So instead of:
> 
>   STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
> 
> we should do something like:
> 
>   STACK_FRAME_NON_STANDARD(__bpf_prog_run);
> 
> see how much more expressive it is? It becomes a function attribute independent of 
> the tooling that makes use of it.

Ok, STACK_FRAME_NON_STANDARD sounds fine to me.

> Similarly, for the highest level 'don't check these directories' makefile flags, 
> I'd suggest, instead of using this rather opaque, tool dependent naming:
> 
>   STACKTOOL := n
> 
> something more specific, like:
> 
>   OBJECT_FILES_NON_STANDARD := y
> 
> which makes it clearer what's going on: these are special object files that are 
> not the typical kernel object files.
> 
> Stacktool (or objtool) would be one consumer of this annotation.
> 
> I think Boris made similar observations in past reviews.

Sounds reasonable.  With this approach we could probably eventually get
rid of KASAN_SANITIZE.

I'll change it to "OBJECT_FILES_NON_STANDARD := y" if there are no
objections.

Note there are also per-object ignores like:

  STACKTOOL_head_$(BITS).o := n

I can similarly change that to something like:

  OBJECT_FILES_NON_STANDARD_head_$(BITS).o := n

> 5)
> 
> Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
> we do exception table checks as well - and it does not express all the other 
> things we may check in object files in the future.
> 
> Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
> would list all the things the tool is able to checks for at the moment.

Hm, I'm not really sure about this.  Yes, the tool could potentially do
other types of checks, but is it necessary to lump them all together
into a single config option?  It does have subcommands after all ;-)

The exception table check reported above is very basic and doesn't serve
any useful purpose other than supporting the goal of validating the
stack.

However, I don't feel strong enough about it to hold up the merge any
longer, so I'll plan to make the change unless I hear back from you.

> Please send followup iterations of the series against the tip:x86/debug tree (or 
> tip:master), to keep the size of the series down.

Will do, thanks!

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Tue, Feb 23, 2016 at 11:27:17AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> > The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> > shows that my review feedback early iterations of this series, that the 
> > 'stacktool' name is too specific, was correct.
> > 
> > We really need to rename it before it gets upstream and the situation gets worse. 
> > __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> > 
> > Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> > would naturally express what it does:
> > 
> >   objtool check kernel/built-in.o
> > 
> > the name expresses that the tool checks object files, independently of the 
> > existing toolchain. Its primary purpose right now is the checking of stack layout 
> > details, but the tool is not limited to that at all.
> 
> Removing 'tool' from the tool name would be nice too :-) Making it
> easily googlable would be good too, lotsa people complain about 'perf'
> being too vague, see for a quick laugher:
> 
> http://www.brendangregg.com/perf.html
> 
> ``Searching for just "perf" finds sites on the police, petroleum, weed
> control, and a T-shirt. This is not an official perf page, for either
> perf_events or the T-shirt.''
> 
> The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg

Yeah, 'tool' in the name is kind of silly, but the above type of
situation is why I prefer 'objtool' over 'obj'.

Though I have to admit I like the idea of making a t-shirt for it ;-)

> Maybe we should ask Linus to come with some other nice, short,
> searchable, funny name like 'git'?
> 
> 'chob' as in 'check object'?

I think 'objtool' is searchable enough.  And I also like the fact that
its name at least gives you an idea of what it does (and eventually it
will do more than just "checking").

-- 
Josh

Re: [PATCH 00/33] Compile-time stack metadata validation

[Arnaldo Carvalho de Melo]

Em Tue, Feb 23, 2016 at 09:07:55AM -0600, Josh Poimboeuf escreveu:
> I think 'objtool' is searchable enough.  And I also like the fact that

Yeah, agreed, there is even documentation available for it already:

http://docs.bvstools.com/home/objtool

> its name at least gives you an idea of what it does (and eventually it
> will do more than just "checking").

:-)

- ARnaldo

Re: [PATCH 00/33] Compile-time stack metadata validation

[Ingo Molnar]

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

> Hi Ingo,
> 
> On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> > So I tried out this latest stacktool series and it looks mostly good for an 
> > upstream merge.
> > 
> > To help this effort move forward I've applied the preparatory/fix patches that are 
> > part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> > propagated all the acks that the latest submission got into the changelogs.)
> 
> Thanks very much for your review and for applying the fixes!
> 
> A few issues relating to the merge:
> 
> - The tip:x86/debug branch fails to build because it depends on
>   ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
>   is in tip:x86/asm.

Indeed...

> - As Pavel mentioned, the tip-bot seems to be spitting out garbage
>   emails from:
>   =?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.

Yeah, hpa fixed that meanwhile.

Due to the above bad base I rebased the tree (to a x86/asm base), so there will be 
a new round of (hopefully readable) tip-bot notifications. I'll push it out after 
a bit of testing.

> > 5)
> > 
> > Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
> > we do exception table checks as well - and it does not express all the other 
> > things we may check in object files in the future.
> > 
> > Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
> > would list all the things the tool is able to checks for at the moment.
> 
> Hm, I'm not really sure about this.  Yes, the tool could potentially do
> other types of checks, but is it necessary to lump them all together
> into a single config option?  It does have subcommands after all ;-)

lol ;-)

Ok, I'm fine with CONFIG_STACK_VALIDATION=y as well.

Thanks,

	Ingo

Re: [PATCH 00/33] Compile-time stack metadata validation

[Josh Poimboeuf]

On Wed, Feb 24, 2016 at 08:40:54AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > Hi Ingo,
> > 
> > On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> > > So I tried out this latest stacktool series and it looks mostly good for an 
> > > upstream merge.
> > > 
> > > To help this effort move forward I've applied the preparatory/fix patches that are 
> > > part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> > > propagated all the acks that the latest submission got into the changelogs.)
> > 
> > Thanks very much for your review and for applying the fixes!
> > 
> > A few issues relating to the merge:
> > 
> > - The tip:x86/debug branch fails to build because it depends on
> >   ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
> >   is in tip:x86/asm.
> 
> Indeed...

FYI, v17 will be based on tip:x86/debug.  But note that, when run
against that branch, it'll spit out a lot of these warnings:

  objtool: arch/x86/ia32/sys_ia32.o: __ex_table size not a multiple of 12
  objtool: arch/x86/ia32/ia32_signal.o: __ex_table size not a multiple of 12
  objtool: arch/x86/entry/common.o: __ex_table size not a multiple of 12

Those warnings mean it's expecting the new exception table format which
was added in:

  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

So that commit is needed to avoid the warnings.

Thanks!

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html