Compile-time stack validation

This adds a CONFIG_STACK_VALIDATION option which enables a host tool named stackvalidate which runs at compile time. It analyzes every .o file and ensures the validity of its stack metadata. It enforces a set of rules on asm code and C inline assembly code so that stack traces can be reliable.

Currently it checks frame pointer usage. I plan to add DWARF CFI validation as well.

Quoted 8 lines · Josh Poimboeuf

This adds a CONFIG_STACK_VALIDATION option which enables a host tool named stackvalidate which runs at compile time. It analyzes every .o file and ensures the validity of its stack metadata. It enforces a set of rules on asm code and C inline assembly code so that stack traces can be reliable.

Currently it checks frame pointer usage. I plan to add DWARF CFI validation as well.

Validation or Annotation, as in the generation of DWARF CFI info?

This adds a CONFIG_STACK_VALIDATION option which enables a host tool named stackvalidate which runs at compile time. It analyzes every .o file and ensures the validity of its stack metadata. It enforces a set of rules on asm code and C inline assembly code so that stack traces can be reliable.

Currently it checks frame pointer usage. I plan to add DWARF CFI validation as well.

For each function, it recursively follows all possible code paths and validates the correct frame pointer state at each instruction.

It also follows code paths involving special sections, like .altinstructions, __jump_table, and __ex_table, which can add alternative execution paths to a given instruction (or set of instructions). Similarly, it knows how to follow switch statements, for which gcc sometimes uses jump tables.

To achieve the validation, stackvalidate enforces the following rules:

1. Each callable function must be annotated as such with the ELF function type. In asm code, this is typically done using the ENTRY/ENDPROC macros. If stackvalidate finds a return instruction outside of a function, it flags an error since that usually indicates callable code which should be annotated accordingly.

2. Conversely, each section of code which is *not* callable should *not* be annotated as an ELF function. The ENDPROC macro shouldn't be used in this case.

3. Each callable function which calls another function must have the correct frame pointer logic, if required by CONFIG_FRAME_POINTER or the architecture's back chain rules. This can by done in asm code with the FRAME/ENDFRAME macros.

4. Dynamic jumps and jumps to undefined symbols are only allowed if:

a) the jump is part of a switch statement; or

b) the jump matches sibling call semantics and the frame pointer has the same value it had on function entry.

5. A callable function may not execute kernel entry/exit instructions. The only code which needs such instructions is kernel entry code, which shouldn't be in callable functions anyway.

Quoted 42 lines · Josh Poimboeuf

This adds a CONFIG_STACK_VALIDATION option which enables a host tool named stackvalidate which runs at compile time. It analyzes every .o file and ensures the validity of its stack metadata. It enforces a set of rules on asm code and C inline assembly code so that stack traces can be reliable.

Currently it checks frame pointer usage. I plan to add DWARF CFI validation as well.

For each function, it recursively follows all possible code paths and validates the correct frame pointer state at each instruction.

It also follows code paths involving special sections, like .altinstructions, __jump_table, and __ex_table, which can add alternative execution paths to a given instruction (or set of instructions). Similarly, it knows how to follow switch statements, for which gcc sometimes uses jump tables.

To achieve the validation, stackvalidate enforces the following rules:

1. Each callable function must be annotated as such with the ELF function type. In asm code, this is typically done using the ENTRY/ENDPROC macros. If stackvalidate finds a return instruction outside of a function, it flags an error since that usually indicates callable code which should be annotated accordingly.

2. Conversely, each section of code which is *not* callable should *not* be annotated as an ELF function. The ENDPROC macro shouldn't be used in this case.

3. Each callable function which calls another function must have the correct frame pointer logic, if required by CONFIG_FRAME_POINTER or the architecture's back chain rules. This can by done in asm code with the FRAME/ENDFRAME macros.

4. Dynamic jumps and jumps to undefined symbols are only allowed if:

a) the jump is part of a switch statement; or

b) the jump matches sibling call semantics and the frame pointer has the same value it had on function entry.

Quoted 3 lines

5. A callable function may not execute kernel entry/exit instructions. The only code which needs such instructions is kernel entry code, which shouldn't be in callable functions anyway.

How (if it does at all) deal with function-trace / -pg -fprofile-arcs things? Does it silently ignore the __mcount calls and assumes ftrace knows wtf its doing? ;-)

Quoted 3 lines

How (if it does at all) deal with function-trace / -pg -fprofile-arcs things? Does it silently ignore the __mcount calls and assumes ftrace knows wtf its doing? ;-)

Adding Steven to CC to keep me honest.

In the case of "-pg -mfentry", which is what ftrace has relied on for

the past few years for newer versions of gcc, stackvalidate silently ignores __fentry__ calls and assumes that ftrace indeed knows wtf it's doing. I don't see a problem there as long as the ftrace handler doesn't sleep.

I haven't run stackvalidate on the old "-pg" mcount non-fentry stuff, but I think it creates a stack frame before calling mcount, so it should be fine.

I don't know much about -fprofile-arcs, but as far as I can tell, it's only used for gcov.

Quoted 4 lines · Josh Poimboeuf

the past few years for newer versions of gcc, stackvalidate silently ignores __fentry__ calls and assumes that ftrace indeed knows wtf it's doing. I don't see a problem there as long as the ftrace handler doesn't sleep.

They should not indeed, however it would be very nice if backtraces would still be 'good'.

scripts/stackvalidate/elf.c new file +54 -0

+struct elf *elf_open(const char *name)
+{
+       struct elf *elf;
+
+       elf_version(EV_CURRENT);
+
+       elf = malloc(sizeof(*elf));
+       if (!elf) {
+               perror("malloc");
+               return NULL;
+       }
+       memset(elf, 0, sizeof(*elf));
+
+       INIT_LIST_HEAD(&elf->sections);
+
+       elf->name = strdup(name);
+       if (!elf->name) {
+               perror("strdup");
+               goto err;
+       }
+
+       elf->fd = open(name, O_RDONLY);
+       if (elf->fd == -1) {
+               perror("open");
+               goto err;
+       }
+
+       elf->elf = elf_begin(elf->fd, ELF_C_READ_MMAP, NULL);
+       if (!elf->elf) {
+               perror("elf_begin");
+               goto err;
+       }
+
+       if (!gelf_getehdr(elf->elf, &elf->ehdr)) {
+               perror("gelf_getehdr");
+               goto err;
+       }
+
+       if (read_sections(elf))
+               goto err;
+
+       if (read_symbols(elf))
+               goto err;
+
+       if (read_relas(elf))
+               goto err;
+
+       return elf;
+
+err:
+       elf_close(elf);
+       return NULL;
+}
+

scripts/stackvalidate/elf.c new file +11 -0

+               list_del(&sec->list);
+               free(sec);
+       }
+       if (elf->name)
+               free(elf->name);
+       if (elf->fd > 0)
+               close(elf->fd);
+       if (elf->elf)
+               elf_end(elf->elf);
+       free(elf);
+}

scripts/stackvalidate/stackvalidate.c new file +19 -0

+int main(int argc, char *argv[])
+{
+       struct elf *elf;
+       int ret = 0, warnings = 0;
+
+       argp_parse(&argp, argc, argv, 0, 0, &args);
+
+       elf = elf_open(args.args[0]);
+       if (!elf) {
+               fprintf(stderr, "error reading elf file %s\n", args.args[0]);
+               return 1;
+       }
+
+       ret = decode_sections(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+       ret = validate_functions(elf);

scripts/stackvalidate/stackvalidate.c new file +5 -1

+       /* ignore warnings for now until we get all the code cleaned up */
+       if (ret || warnings)
+               return 0;
+       return 0;
+}
--

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

Just nitpicks below..

Quoted 4 lines

scripts/stackvalidate/elf.c new file +4 -0

+void elf_close(struct elf *elf)
+{
+       struct section *sec, *tmpsec;
+       struct symbol *sym, *tmpsym;

scripts/stackvalidate/elf.c line 411

Quoted 6 lines

scripts/stackvalidate/elf.c new file +6 -0

+
+       list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
+               list_for_each_entry_safe(sym, tmpsym, &sec->symbols, list) {
+                       list_del(&sym->list);
+                       free(sym);
+               }

It seems that it needs to free entries in sec->relas too here.

Quoted 23 lines

scripts/stackvalidate/stackvalidate.c new file +23 -0

+int main(int argc, char *argv[])
+{
+       struct elf *elf;
+       int ret = 0, warnings = 0;
+
+       argp_parse(&argp, argc, argv, 0, 0, &args);
+
+       elf = elf_open(args.args[0]);
+       if (!elf) {
+               fprintf(stderr, "error reading elf file %s\n", args.args[0]);
+               return 1;
+       }
+
+       ret = decode_sections(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+       ret = validate_functions(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+

scripts/stackvalidate/stackvalidate.c line 876

Quoted 6 lines

scripts/stackvalidate/stackvalidate.c new file +6 -0

+       ret = validate_uncallable_instructions(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+out:

elf_close(elf); ??

Quoted 23 lines

Quoted 23 lines

scripts/stackvalidate/stackvalidate.c new file +23 -0

+int main(int argc, char *argv[])
+{
+       struct elf *elf;
+       int ret = 0, warnings = 0;
+
+       argp_parse(&argp, argc, argv, 0, 0, &args);
+
+       elf = elf_open(args.args[0]);
+       if (!elf) {
+               fprintf(stderr, "error reading elf file %s\n", args.args[0]);
+               return 1;
+       }
+
+       ret = decode_sections(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+       ret = validate_functions(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+

Quoted 8 lines

scripts/stackvalidate/stackvalidate.c line 876

Quoted 6 lines

scripts/stackvalidate/stackvalidate.c new file +6 -0

+       ret = validate_uncallable_instructions(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+out:

elf_close(elf); ??

I intentionally left out the call to elf_close() here, since this is the exit path and the kernel will free the memory anyway.

Quoted 32 lines · Josh Poimboeuf

Quoted 31 lines

Quoted 19 lines

scripts/stackvalidate/stackvalidate.c new file +19 -0

+int main(int argc, char *argv[])
+{
+       struct elf *elf;
+       int ret = 0, warnings = 0;
+
+       argp_parse(&argp, argc, argv, 0, 0, &args);
+
+       elf = elf_open(args.args[0]);
+       if (!elf) {
+               fprintf(stderr, "error reading elf file %s\n", args.args[0]);
+               return 1;
+       }
+
+       ret = decode_sections(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+       ret = validate_functions(elf);

scripts/stackvalidate/stackvalidate.c line 876

Quoted 10 lines

scripts/stackvalidate/stackvalidate.c new file +10 -0

+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+       ret = validate_uncallable_instructions(elf);
+       if (ret < 0)
+               goto out;
+       warnings += ret;
+
+out:

elf_close(elf); ??

Quoted 2 lines

I intentionally left out the call to elf_close() here, since this is the exit path and the kernel will free the memory anyway.

OTOH it makes Valgrind runs harder to interpret, as real leaks won't be visible.

So I'd only do intentional leaks only if it's truly, measurably improves performance.

It's perhaps distasteful, but it improves performance. And I'm a pragmatist at heart ;-)

On Mon, 2015-07-20 at 12:50 -0500, Josh Poimboeuf wrote:
[...]

Quoted 2 lines

It's perhaps distasteful, but it improves performance. And I'm a pragmatist at heart ;-)

And you measured the time gain guaranteeing that it actually saves that much time. Usually that isn't actually measurable ....

Posting a listing of reported warnings in a reply to this email.

Quoted 1 line · Josh Poimboeuf

Posting a listing of reported warnings in a reply to this email.

These are the reported stackvalidate warnings on tip/master with my Fedora-based config. There were 1399 warnings in 26 .c files and 16 .S files. It was mostly repeat warnings so I removed a lot of the repeats to keep it short.

Quoted 1 line · Josh Poimboeuf

On Tue, Jul 14, 2015 at 12:14:06PM -0500, Josh Poimboeuf wrote:

Quoted 5 lines

Quoted 1 line

Posting a listing of reported warnings in a reply to this email.

These are the reported stackvalidate warnings on tip/master with my Fedora-based config. There were 1399 warnings in 26 .c files and 16 .S files. It was mostly repeat warnings so I removed a lot of the repeats to keep it short.

Are most of these warnings legit, what's the false positive rate in your impression?

You might want to start fixing a few typical types, just to see what it involves exactly and whether we want to fix it that way.

Add a C inline asm string version of the STACKVALIDATE_IGNORE_INSN macro which tells stackvalidate to ignore the subsequent instruction.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/include/asm/stackvalidate.h | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/x86/include/asm/stackvalidate.h +0 -0

index 2d23c23..dac935c 100644

Quoted 13 lines · Josh Poimboeuf

Add a C inline asm string version of the STACKVALIDATE_IGNORE_INSN macro which tells stackvalidate to ignore the subsequent instruction.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/include/asm/stackvalidate.h | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/x86/include/asm/stackvalidate.h +0 -0

index 2d23c23..dac935c 100644

arch/x86/include/asm/stackvalidate.h line 27

Quoted 5 lines

arch/x86/include/asm/stackvalidate.h +2 -0

@@ -23,6 +23,15 @@
 	.endif
 .endm
 
+#else /* !__ASSEMBLY__ */
+

#ifdef CONFIG_STACK_VALIDATION

arch/x86/include/asm/stackvalidate.h line 34

Quoted 7 lines

arch/x86/include/asm/stackvalidate.h +7 -0

+#define STACKVALIDATE_IGNORE_INSN				\
+	".Ltemp" __stringify(__LINE__) ":;"			\
+	".pushsection __stackvalidate_ignore_insn, \"a\";"	\
+	_ASM_ALIGN ";"						\
+	".long .Ltemp" __stringify(__LINE__) " - .;"		\
+	".popsection;"
+

#endif

Also, you should end your lines with "\n" so that the .s output looks a bit more readable, not like now:

#APP
# 30 "./arch/x86/include/asm/arch_hweight.h" 1

        661:
        .Ltemp32:;.pushsection __stackvalidate_ignore_insn, "a"; .balign 8 ;.long .Ltemp32 - .;.popsection;call __sw_hweight32

662:

arch/x86/include/asm/stackvalidate.h line 29

Quoted 4 lines · Josh Poimboeuf

arch/x86/include/asm/stackvalidate.h +3 -0

+
+#define STACKVALIDATE_IGNORE_INSN                              \
+       ".Ltemp" __stringify(__LINE__) ":;"                     \

This is wrong. It won't work for people who do multiple of these on the same line (think macros etc).

For temporary labels, you should just use numeric labels. Think Pascal style ones. Then use "b" or "f" after the number when referring to it to say "back" or "forward" reference.

So the code for an endless loop with a true temporary label looks like:

1: code
jmp 1b

and a branch over would look like

          jne 7f
        .. code ...
    7:   ...

and now you can have multiple of these truly temp labels without ever getting errors from having a label redefined.

If __arch_hweight32() or __arch_hweight64() is inlined at the beginning of a function, gcc can insert the call instruction before setting up a stack frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace.

Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by listing the stack pointer as an output operand for the inline asm statement.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/include/asm/arch_hweight.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/x86/include/asm/arch_hweight.h +3 -1

index 9686c3d..e438a0d 100644
@@ -23,10 +23,11 @@
  */
 static inline unsigned int __arch_hweight32(unsigned int w)
 {
+	register void *__sp asm("esp");
 	unsigned int res = 0;
 
 	asm (ALTERNATIVE("call __sw_hweight32", POPCNT32, X86_FEATURE_POPCNT)
-		     : "="REG_OUT (res)
+		     : "="REG_OUT (res), "+r" (__sp)
 		     : REG_IN (w));
 
 	return res;

@@ -44,6 +45,7 @@ static inline unsigned int __arch_hweight8(unsigned int w)
 
 static inline unsigned long __arch_hweight64(__u64 w)
 {
+	register void __maybe_unused *__sp asm("rsp");
 	unsigned long res = 0;
 
 #ifdef CONFIG_X86_32

Quoted 40 lines · Josh Poimboeuf

If __arch_hweight32() or __arch_hweight64() is inlined at the beginning of a function, gcc can insert the call instruction before setting up a stack frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace.

Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by listing the stack pointer as an output operand for the inline asm statement.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/include/asm/arch_hweight.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/x86/include/asm/arch_hweight.h +3 -1

index 9686c3d..e438a0d 100644
@@ -23,10 +23,11 @@
  */
 static inline unsigned int __arch_hweight32(unsigned int w)
 {
+	register void *__sp asm("esp");
 	unsigned int res = 0;
 
 	asm (ALTERNATIVE("call __sw_hweight32", POPCNT32, X86_FEATURE_POPCNT)
-		     : "="REG_OUT (res)
+		     : "="REG_OUT (res), "+r" (__sp)
 		     : REG_IN (w));
 
 	return res;

@@ -44,6 +45,7 @@ static inline unsigned int __arch_hweight8(unsigned int w)
 
 static inline unsigned long __arch_hweight64(__u64 w)
 {
+	register void __maybe_unused *__sp asm("rsp");
 	unsigned long res = 0;
 
 #ifdef CONFIG_X86_32

arch/x86/include/asm/arch_hweight.h static inline unsigned long __arch_hweight64(__u64 w) · line 58

Quoted 7 lines

arch/x86/include/asm/arch_hweight.h +1 -1

@@ -51,7 +53,7 @@ static inline unsigned long __arch_hweight64(__u64 w)
 		__arch_hweight32((u32)(w >> 32));
 #else
 	asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT)
-		     : "="REG_OUT (res)
+		     : "="REG_OUT (res), "+r" (__sp)
 		     : REG_IN (w));
 #endif /* CONFIG_X86_32 */

Eeew, useless code so that some compile-time validation is done. Let's not add this clutter please.

In this particular case, the majority of CPUs out there will get POPCNT patched in and that CALL is gone. And for the remaining cases where we do end up using the __sw_* variants, I'd prefer to rather not do the validation instead of polluting the code with that fake rsp dependency.

Well, but this isn't some whitelist code to make stackvalidate happy.

It's actually a real runtime frame pointer bug, and the rsp dependency is real. If it does the call without first creating the stack frame then it breaks frame pointer based stack traces.

Quoted 5 lines · Josh Poimboeuf

Well, but this isn't some whitelist code to make stackvalidate happy.

It's actually a real runtime frame pointer bug, and the rsp dependency is real. If it does the call without first creating the stack frame then it breaks frame pointer based stack traces.

I think we can live with the stack trace being a little wrong in those __sw_* variants. And besides, we're talking about the very very small percentage of machines (which keeps getting smaller) which don't support POPCNT. And from those, only for the cases where the arg is not __builtin_constant_p() because there we do the __const_hweight* thing.

I'd prefer to not clutter the code more in that case.

Ok, so would you rather adding a whitelist to tell stackvalidate to ignore it? Something like this?

Quoted 2 lines · Josh Poimboeuf

Ok, so would you rather adding a whitelist to tell stackvalidate to ignore it? Something like this?

I tried it and maybe I'm missing something but that doesn't work:

$ make drivers/gpu/drm/i915/intel_ringbuffer.o
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/bounds.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      drivers/gpu/drm/i915/intel_ringbuffer.o

./arch/x86/include/asm/arch_hweight.h: Assembler messages:
./arch/x86/include/asm/arch_hweight.h:31: Error: symbol `.Ltemp32' is already defined
./arch/x86/include/asm/arch_hweight.h:31: Error: symbol `.Ltemp32' is already defined
./arch/x86/include/asm/arch_hweight.h:31: Error: symbol `.Ltemp32' is already defined
scripts/Makefile.build:258: recipe for target 'drivers/gpu/drm/i915/intel_ringbuffer.o' failed
make[1]: *** [drivers/gpu/drm/i915/intel_ringbuffer.o] Error 1
Makefile:1528: recipe for target 'drivers/gpu/drm/i915/intel_ringbuffer.o' failed
make: *** [drivers/gpu/drm/i915/intel_ringbuffer.o] Error 2

Also, that label temp32 could be more descriptive.

so you see that a CALL instruction gets replaced with a POPCNT and the feature bit used is 4*32+23 which is X86_FEATURE_POPCNT. This information is enough to detect that particular case and add the offset ".long 661b - ." to the list of instructions which stackvalidate should ignore.

Anyway, this is what I'd do.

IMNSVHO, we must be very conservative and not add some markers/helpers/etc to code only so that tools can do their job. Not if it can be helped. Instead, tools should do the hard work and we should keep kernel code clean.

Currently, when stackvalidate sees an ALTERNATIVE, it assumes that either code path is possible, so it follows both paths in parallel.

If I understand right, you're proposing that stackvalidate should only follow the POPCNT path and never follow the !POPCNT path?

In general, I agree, and I like the original patch much better. IMO, it achieved the goal of keeping the kernel code clean, while fixing the frame pointer bug.

Quoted 5 lines · Josh Poimboeuf

Currently, when stackvalidate sees an ALTERNATIVE, it assumes that either code path is possible, so it follows both paths in parallel.

If I understand right, you're proposing that stackvalidate should only follow the POPCNT path and never follow the !POPCNT path?

Actually, you don't even need to follow the POPCNT case either because it is a single instruction - no stack operations there.

So yeah, either that or special-case the case where the original insn is CALL and the replacement is a POPCNT and ignore those CALL locations.

The advantage is that the burden is put on the tool and not by adding markers to kernel code paths.

Quoted 3 lines

In general, I agree, and I like the original patch much better. IMO, it achieved the goal of keeping the kernel code clean, while fixing the frame pointer bug.

And I think that in that case, adding that rSP dependency is too much because even though it fixes the "bug", it is very very unlikely any stack trace will have __sw_hweight* in it for reasons pointed out earlier and also because those functions can't fail and they get integral types as args which can't fail when deref-fing either. And even if they do, they don't call any other functions so rIP pointing to them is already enough.

Enough for oopses, perhaps, but maybe not enough for perf.

It sounds like you want CFI unwinding :)

Quoted 3 lines · Andy Lutomirski

Enough for oopses, perhaps, but maybe not enough for perf.

It sounds like you want CFI unwinding :)

What would you want to unwind in those __sw_hweight* almost-trivial, tail functions?

So your shiny perf profile shows cumulative time in whatever called them. Sure, this is arguably silly if we're stuck with frame pointers.

stackvalidate reports the following warnings for __schedule():

stackvalidate: kernel/sched/core.o: __schedule()+0x3e7: duplicate frame pointer save
stackvalidate: kernel/sched/core.o: __schedule()+0x424: sibling call from callable instruction with changed frame pointer
stackvalidate: kernel/sched/core.o: __schedule()+0x431: call without frame pointer save/setup
stackvalidate: kernel/sched/core.o: __schedule()+0x8b8: frame pointer state mismatch
stackvalidate: kernel/sched/core.o: __schedule()+0x447: frame pointer state mismatch

__schedule() is obviously a special case which is allowed to do unusual things with the frame pointer.

Quoted 8 lines · Josh Poimboeuf

stackvalidate reports the following warnings for __schedule():

stackvalidate: kernel/sched/core.o: __schedule()+0x3e7: duplicate frame pointer save
stackvalidate: kernel/sched/core.o: __schedule()+0x424: sibling call from callable instruction with changed frame pointer
stackvalidate: kernel/sched/core.o: __schedule()+0x431: call without frame pointer save/setup
stackvalidate: kernel/sched/core.o: __schedule()+0x8b8: frame pointer state mismatch
stackvalidate: kernel/sched/core.o: __schedule()+0x447: frame pointer state mismatch

Quoted 2 lines

__schedule() is obviously a special case which is allowed to do unusual things with the frame pointer.

Yes, but is the code actually correct? We can't dismiss the warnings just on that basis alone.

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Josh Poimboeuf

stackvalidate reports the following warnings for __schedule():

stackvalidate: kernel/sched/core.o: __schedule()+0x3e7: duplicate frame pointer save
stackvalidate: kernel/sched/core.o: __schedule()+0x424: sibling call from callable instruction with changed frame pointer
stackvalidate: kernel/sched/core.o: __schedule()+0x431: call without frame pointer save/setup
stackvalidate: kernel/sched/core.o: __schedule()+0x8b8: frame pointer state mismatch
stackvalidate: kernel/sched/core.o: __schedule()+0x447: frame pointer state mismatch

Quoted 5 lines

Quoted 2 lines

__schedule() is obviously a special case which is allowed to do unusual things with the frame pointer.

Yes, but is the code actually correct? We can't dismiss the warnings just on that basis alone.

It's really only __switch_to that does weird things, right?

I kinda
want to rework that thing anyway to have a well-defined saved state format anyway, which would have the nice benefit of letting us get rid of all the ret_from_fork crap.

That is, we'd context switch like:

static inline void __switch_to(...) {
switch extra stuff;
switch everything except gpregs;
asm volatile ("call __switch_stack_and_ip" : [__sp thing goes here]
: "S" (&prev->bottom_of_stack), "D" (&next->bottom_of_stack) :
"basically all regs and flags");
}

Then the low level bit would be:

__switch_stack_and_ip:
pushq %rbp
mov %rsp, (%rsi)
mov (%rdi), %rsp
popq %rbp
ret

Now, when we create a new task, we can set up its stack directly rather than setting some TI flag, because we actually know the layout.

Hmm?

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Josh Poimboeuf

stackvalidate reports the following warnings for __schedule():

stackvalidate: kernel/sched/core.o: __schedule()+0x3e7: duplicate frame pointer save
stackvalidate: kernel/sched/core.o: __schedule()+0x424: sibling call from callable instruction with changed frame pointer
stackvalidate: kernel/sched/core.o: __schedule()+0x431: call without frame pointer save/setup
stackvalidate: kernel/sched/core.o: __schedule()+0x8b8: frame pointer state mismatch
stackvalidate: kernel/sched/core.o: __schedule()+0x447: frame pointer state mismatch

Quoted 5 lines

Quoted 2 lines

__schedule() is obviously a special case which is allowed to do unusual things with the frame pointer.

Yes, but is the code actually correct? We can't dismiss the warnings just on that basis alone.

It's really only __switch_to that does weird things, right? I kinda want to rework that thing anyway to have a well-defined saved state format anyway, which would have the nice benefit of letting us get rid of all the ret_from_fork crap.

That is, we'd context switch like:

static inline void __switch_to(...) {
switch extra stuff;
switch everything except gpregs;
asm volatile ("call __switch_stack_and_ip" : [__sp thing goes here]
: "S" (&prev->bottom_of_stack), "D" (&next->bottom_of_stack) :
"basically all regs and flags");
}

Then the low level bit would be:

__switch_stack_and_ip:
pushq %rbp
mov %rsp, (%rsi)
mov (%rdi), %rsp
popq %rbp
ret

Now, when we create a new task, we can set up its stack directly rather than setting some TI flag, because we actually know the layout.

Hmm?

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Josh Poimboeuf

stackvalidate reports the following warnings for __schedule():

stackvalidate: kernel/sched/core.o: __schedule()+0x3e7: duplicate frame pointer save
stackvalidate: kernel/sched/core.o: __schedule()+0x424: sibling call from callable instruction with changed frame pointer
stackvalidate: kernel/sched/core.o: __schedule()+0x431: call without frame pointer save/setup
stackvalidate: kernel/sched/core.o: __schedule()+0x8b8: frame pointer state mismatch
stackvalidate: kernel/sched/core.o: __schedule()+0x447: frame pointer state mismatch

Quoted 5 lines

Quoted 2 lines

__schedule() is obviously a special case which is allowed to do unusual things with the frame pointer.

Yes, but is the code actually correct? We can't dismiss the warnings just on that basis alone.

It's really only __switch_to that does weird things, right? I kinda want to rework that thing anyway to have a well-defined saved state format anyway, which would have the nice benefit of letting us get rid of all the ret_from_fork crap.

That is, we'd context switch like:

static inline void __switch_to(...) {
switch extra stuff;
switch everything except gpregs;
asm volatile ("call __switch_stack_and_ip" : [__sp thing goes here]
: "S" (&prev->bottom_of_stack), "D" (&next->bottom_of_stack) :
"basically all regs and flags");
}

Then the low level bit would be:

__switch_stack_and_ip:
pushq %rbp
mov %rsp, (%rsi)
mov (%rdi), %rsp
popq %rbp
ret

Now, when we create a new task, we can set up its stack directly rather than setting some TI flag, because we actually know the layout.

Hmm?

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

Quoted 39 lines · Ingo Molnar

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

--
Josh

Quoted 48 lines · Josh Poimboeuf

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Quoted 9 lines

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

This really is about frame pointers, right? How about ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return, whatever?

Quoted 59 lines · Andy Lutomirski

Quoted 57 lines · Josh Poimboeuf

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

Quoted 3 lines

This really is about frame pointers, right? How about ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return, whatever?

Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of a function?

--
Josh

Quoted 64 lines · Josh Poimboeuf

Quoted 62 lines · Andy Lutomirski

Quoted 48 lines · Josh Poimboeuf

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

Review ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

(quoted diff) +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Quoted 9 lines

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

This really is about frame pointers, right? How about ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return, whatever?

Quoted 2 lines

Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of a function?

I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz (ENTRY is debatable, but that's what we currently have). ENDPROC could easily be replaced with anything else.

Quoted 68 lines · Andy Lutomirski

Quoted 66 lines · Josh Poimboeuf

Quoted 59 lines · Andy Lutomirski

Quoted 48 lines · Josh Poimboeuf

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

Review ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

(quoted diff) +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Quoted 9 lines

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

Quoted 3 lines

This really is about frame pointers, right? How about ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return, whatever?

Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of a function?

Quoted 3 lines

I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz (ENTRY is debatable, but that's what we currently have). ENDPROC could easily be replaced with anything else.

So do you mean ENTRY_FRAMEPTR_PROLOGUE and ENDPROC_FRAMEPTR_EPILOGUE? Or something else?

--
Josh

Quoted 73 lines · Josh Poimboeuf

Quoted 71 lines · Andy Lutomirski

Quoted 60 lines · Josh Poimboeuf

Quoted 59 lines · Andy Lutomirski

Quoted 57 lines · Josh Poimboeuf

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

Review ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

(quoted diff) +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

Quoted 6 lines

Quoted 3 lines

This really is about frame pointers, right? How about ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return, whatever?

Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of a function?

I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz (ENTRY is debatable, but that's what we currently have). ENDPROC could easily be replaced with anything else.

Quoted 3 lines

So do you mean ENTRY_FRAMEPTR_PROLOGUE and ENDPROC_FRAMEPTR_EPILOGUE? Or something else?

I like it. I think this bikeshed might be well painted now!

Actually I'm not done painting. Personally it seems a little too verbose. I still like ENTRY_FRAME and ENDPROC_FRAME_RETURN :p

Quoted 2 lines · Josh Poimboeuf

Actually I'm not done painting. Personally it seems a little too verbose. I still like ENTRY_FRAME and ENDPROC_FRAME_RETURN :p

Let's balance it out even more:

ENTRY_FRAME(..)

	...

ENDPROC_FRAME(..)

I like the balance, but the "ret" is still non-obvious.

Quoted 1 line · Josh Poimboeuf

I like the balance, but the "ret" is still non-obvious.

Does it have to be obvious?

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Ingo Molnar

arch/x86/crypto/aesni-intel_asm.S ENTRY(aesni_set_key) · line 1912

Quoted 12 lines · Josh Poimboeuf

ENTRY(aesni_set_key)

arch/x86/crypto/aesni-intel_asm.S +2 -0

+     FRAME
 #ifndef __x86_64__
      pushl KEYP
      movl 8(%esp), KEYP              # ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
      popl KEYP
 #endif
+     ENDFRAME
      ret
 ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

        ENTRY(aesni_set_key)
                FRAME
        ...
                ENDFRAME
                ret
        ENDPROC(aesni_set_key)

How about writing this as:

        FUNCTION_ENTRY(aesni_set_key)
        ...
        FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry declaration, but it will now generate real code.

Quoted 6 lines

Quoted 4 lines

OTOH if people find this intuitive enough then it's a lot harder to mess it up, and I think 'RETURN' makes it clear enough that there's a real instruction generated there.

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Perhaps the macro name should describe what the epilogue does, since frame pointers aren't required for _all_ functions, only those which don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN? The ending macro is kind of long, but at least it a) matches the existing ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame pointers are involved; and c) lets you know that the return is there.

Note what the names _don't_ contain: that we generate debug info! That fact is not present in the naming, and that's very much intentional, because the precise form of debug info is conditional:

- if CONFIG_FRAME_POINTERS=y then we push/pop a stack frame

- if (later on) we do CFI annotations we don't push/pop a stack frame but emit CFI debuginfo

In that sense 'FRAME' should never be in these names I think, nor 'PROC' (which is not symmetric).

Plus all 3 variants I suggested are very easy to remember, why I'd always have to look up any non-symmetric macro name called 'PROC'...

Quoted 4 lines · Ingo Molnar

Note what the names _don't_ contain: that we generate debug info! That fact is not present in the naming, and that's very much intentional, because the precise form of debug info is conditional:

Quoted 4 lines

- if CONFIG_FRAME_POINTERS=y then we push/pop a stack frame

- if (later on) we do CFI annotations we don't push/pop a stack frame but emit CFI debuginfo

According to current plan, the macro won't add CFI annotations. That will be done instead by a separate tool. So the macro really is frame pointer specific.

Quoted 5 lines

In that sense 'FRAME' should never be in these names I think, nor 'PROC' (which is not symmetric).

Plus all 3 variants I suggested are very easy to remember, why I'd always have to look up any non-symmetric macro name called 'PROC'...

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to distinguish them from the already ubiquitous ENTRY and ENDPROC. So as a kernel developer it seems confusing to me, e.g. how do I remember when to use FUNCTION_ENTRY vs ENTRY?

Quoted 3 lines

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Well, we could use LEAF_FUNCTION to mark that fact.

Wether a function written in assembly is a leaf function or not is a higher level (and thus more valuable) piece of information whether we generate frame pointer debuginfo or not.

Quoted 4 lines

Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to distinguish them from the already ubiquitous ENTRY and ENDPROC. So as a kernel developer it seems confusing to me, e.g. how do I remember when to use FUNCTION_ENTRY vs ENTRY?

'ENDPROC' is really leftover from older debuginfo cruft, it's not a valuable construct IMHO, even if it's (sadly) ubiquitious.

We want to create new, clean, as minimal as possible and as clearly named as possible debuginfo constructs from first principles.

Quoted 15 lines · Ingo Molnar

Quoted 3 lines

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Well, we could use LEAF_FUNCTION to mark that fact.

Wether a function written in assembly is a leaf function or not is a higher level (and thus more valuable) piece of information whether we generate frame pointer debuginfo or not.

Quoted 4 lines

Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to distinguish them from the already ubiquitous ENTRY and ENDPROC. So as a kernel developer it seems confusing to me, e.g. how do I remember when to use FUNCTION_ENTRY vs ENTRY?

Quoted 5 lines

'ENDPROC' is really leftover from older debuginfo cruft, it's not a valuable construct IMHO, even if it's (sadly) ubiquitious.

We want to create new, clean, as minimal as possible and as clearly named as possible debuginfo constructs from first principles.

Ok. So if I understand right, the proposal is:

Replace *all* x86 usage of ENTRY/ENDPROC with either:

FUNCTION_ENTRY(func)
FUNCTION_RETURN(func)

or

LEAF_FUNCTION_ENTRY(func)
LEAF_FUNCTION_RETURN(func)

Those sound fine to me.

I should point out that there are still a few cases where the more granular FRAME/ENDFRAME and ENTRY/ENDPROC macros would still be needed.

For example, if the function ends with a jump instead of a ret. If the jump is a sibling call, the code would look like:

FUNCTION_ENTRY(func)

	...
	ENDFRAME
	jmp another_func

ENDPROC(func)

Or if it's a jump within the function to an internal ret:

FUNCTION_ENTRY(func)

	...

1: ...
ENDFRAME
ret
2: ...
jmp 1b
ENDPROC(func)

Or if it jumps to some shared code before returning:

FUNCTION_ENTRY(func_1)

	...
	jmp common_return

ENDPROC(func_1)

FUNCTION_ENTRY(func_2)

	...
	jmp common_return

ENDPROC(func_2)

common_return:

	...
	ENDFRAME
	ret

So in some cases we'd still need the more granular macros, unless we decided to make special macros for these cases as well.

Quoted 38 lines · Josh Poimboeuf

On Mon, Jul 20, 2015 at 09:56:11AM +0200, Ingo Molnar wrote:
I should point out that there are still a few cases where the more granular FRAME/ENDFRAME and ENTRY/ENDPROC macros would still be needed.

For example, if the function ends with a jump instead of a ret. If the jump is a sibling call, the code would look like:

FUNCTION_ENTRY(func)

	...
	ENDFRAME
	jmp another_func

ENDPROC(func)

Or if it's a jump within the function to an internal ret:

FUNCTION_ENTRY(func)

	...

1: ...
ENDFRAME
ret
2: ...
jmp 1b
ENDPROC(func)

Or if it jumps to some shared code before returning:

FUNCTION_ENTRY(func_1)

	...
	jmp common_return

ENDPROC(func_1)

FUNCTION_ENTRY(func_2)

	...
	jmp common_return

ENDPROC(func_2)

Quoted 8 lines

common_return:

	...
	ENDFRAME
	ret

So in some cases we'd still need the more granular macros, unless we decided to make special macros for these cases as well.

Ok, I see how the naming scheme I proposed won't work with all that very well, but I'd still suggest using consistently named patterns.

Let me suggest yet another approach. How about open-coding something like this:

FUNCTION_START(func)

	push_bp
	mov_sp_bp

	...

	pop_bp
	ret

FUNCTION_END(func)

This is just two easy things:

- a redefine of the FUNCTION_ENTRY and ENDPROC names

- the introduction of three quasi-mnemonics: push_bp, mov_sp_bp, pop_bp - which all look very similar to a real frame setup sequence, except that we can easily make them go away in the !CONFIG_FRAME_POINTERS case.

The advantage of this approach would be:

- it looks pretty 'natural' and very close to how the real disassembly looks like in CONFIG_FRAME_POINTERS=y kernels. So while it's not as compact as some of the other variants, it's close to what the real instruction sequence looks like and that is a positive quality in itself.

- it also makes it apparent 'on sight' that it's probably a bug to have unbalanced push/pop sequences in a regular function, to any reasonably alert assembly coder.

- if we ever unsupport framepointer kernels in the (far far) future, we can get rid of all lines with those 3 mnemonics and be done with it.

- it's finegrained enough so that we can express all the special function/tail variants you listed above.

What do you think?

I'd still keep existing frame setup functionality and names and only use these in fixes, new code and new annotations - and do a full rename and cleanup once the dust has settled.

Quoted 43 lines · Ingo Molnar

* Josh Poimboeuf <[email protected]> wrote:
Ok, I see how the naming scheme I proposed won't work with all that very well, but I'd still suggest using consistently named patterns.

Let me suggest yet another approach. How about open-coding something like this:

FUNCTION_START(func)

	push_bp
	mov_sp_bp

	...

	pop_bp
	ret

FUNCTION_END(func)

This is just two easy things:

- a redefine of the FUNCTION_ENTRY and ENDPROC names

- the introduction of three quasi-mnemonics: push_bp, mov_sp_bp, pop_bp - which all look very similar to a real frame setup sequence, except that we can easily make them go away in the !CONFIG_FRAME_POINTERS case.

The advantage of this approach would be:

- it looks pretty 'natural' and very close to how the real disassembly looks like in CONFIG_FRAME_POINTERS=y kernels. So while it's not as compact as some of the other variants, it's close to what the real instruction sequence looks like and that is a positive quality in itself.

- it also makes it apparent 'on sight' that it's probably a bug to have unbalanced push/pop sequences in a regular function, to any reasonably alert assembly coder.

- if we ever unsupport framepointer kernels in the (far far) future, we can get rid of all lines with those 3 mnemonics and be done with it.

- it's finegrained enough so that we can express all the special function/tail variants you listed above.

Quoted 1 line

What do you think?

I agree that the edge cases make FUNCTION_ENTRY and FUNCTION_RETURN less attractive. Slowly we are circling around to where we started :-)

Personally, I prefer FRAME/ENDFRAME instead of push_bp/mov_sp_bp/pop_bp, because it more communicates *what* it's doing rather than how. IMO, it's easier to grok with a quick glance.

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Quoted 4 lines · Josh Poimboeuf

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Could someone remind me why it isn't needed for leaf functions?

If a function doesn't call any other functions, then it won't ever show up in a stack trace unless:

a) the function itself walks the stack, in which case the frame pointer isn't necessary; or

b) The function gets hit by an interrupt/exception, in which case frame pointers can't be 100% relied upon anyway.

I've noticed that gcc *does* seem to create stack frames for leaf functions. But it's inconsistent, because the early exit path of some functions will skip the stack frame creation and go straight to the return.

We could probably get a good performance boost with the -momit-leaf-frame-pointer flag. Though it would make stack traces less reliable when a leaf function gets interrupted.

Quoted 8 lines · Josh Poimboeuf

If a function doesn't call any other functions, then it won't ever show up in a stack trace unless:

a) the function itself walks the stack, in which case the frame pointer isn't necessary; or

b) The function gets hit by an interrupt/exception, in which case frame pointers can't be 100% relied upon anyway.

In case the interrupt happens whilst setting up the frame, right?

Quoted 8 lines

I've noticed that gcc *does* seem to create stack frames for leaf functions. But it's inconsistent, because the early exit path of some functions will skip the stack frame creation and go straight to the return.

We could probably get a good performance boost with the -momit-leaf-frame-pointer flag. Though it would make stack traces less reliable when a leaf function gets interrupted.

So the information we'd loose in that case would be the location in the calling function, right?

Which isn't a problem, if the current function (as obtained through RIP) is only ever called once. However if there's multiple call sites this might be a wee bit confusing.

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Josh Poimboeuf

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Could someone remind me why it isn't needed for leaf functions?

If a function doesn't call any other functions, then it won't ever show up in a stack trace unless:

a) the function itself walks the stack, in which case the frame pointer isn't necessary; or

b) The function gets hit by an interrupt/exception, in which case frame pointers can't be 100% relied upon anyway.

I've noticed that gcc *does* seem to create stack frames for leaf functions. But it's inconsistent, because the early exit path of some functions will skip the stack frame creation and go straight to the return.

We could probably get a good performance boost with the -momit-leaf-frame-pointer flag. Though it would make stack traces less reliable when a leaf function gets interrupted.

Quoted 22 lines · Josh Poimboeuf

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Josh Poimboeuf

The reason I suggested to put FRAME in the macro name is to try to prevent it from being accidentally used for leaf functions, where it isn't needed.

Could someone remind me why it isn't needed for leaf functions?

If a function doesn't call any other functions, then it won't ever show up in a stack trace unless:

a) the function itself walks the stack, in which case the frame pointer isn't necessary; or

b) The function gets hit by an interrupt/exception, in which case frame pointers can't be 100% relied upon anyway.

I've noticed that gcc *does* seem to create stack frames for leaf functions. But it's inconsistent, because the early exit path of some functions will skip the stack frame creation and go straight to the return.

Quoted 3 lines

We could probably get a good performance boost with the -momit-leaf-frame-pointer flag. Though it would make stack traces less reliable when a leaf function gets interrupted.

So in theory we could resolve this during the stack walk: when we pass from the IRQ stack to the process stack we actually know the RIP of the interrupted context, and could include that.

These patches fix many of the warnings reported by stackvalidate. They're based on top of the "Compile-time stack validation" v7 patch set [1].

They've been compile-tested and boot tested in a VM, but I haven't attempted any meaningful testing for most of them. This should give an idea of what kinds of changes I think are needed.

[1] https://lkml.kernel.org/r/[email protected]

Quoted 8 lines · Josh Poimboeuf

These patches fix many of the warnings reported by stackvalidate. They're based on top of the "Compile-time stack validation" v7 patch set [1].

They've been compile-tested and boot tested in a VM, but I haven't attempted any meaningful testing for most of them. This should give an idea of what kinds of changes I think are needed.

Quoted 2 lines

[1] https://lkml.kernel.org/r/[email protected]

Nothing here looks all that bad, but I think the extra frame pointers staring us in the face (as opposed to the ones that gcc adds transparently) might serve as added incentive to suck it up and get CFI unwinding working.