arch/x86/include/asm/asm.h | 4 ++++ 1 file changed, 4 insertions(+)
The following commit has been merged into the x86/asm branch of tip:
Commit-ID: 05844663b4fcf22bb3a1494615ae3f25852c9abc
Gitweb: https://git.kernel.org/tip/05844663b4fcf22bb3a1494615ae3f25852c9abc
Author: Josh Poimboeuf <jpoimboe@kernel.org>
AuthorDate: Sun, 02 Mar 2025 17:21:03 -08:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 04 Mar 2025 11:21:40 +01:00
x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
asm statement with a call instruction to force the compiler to set up
the frame pointer before doing the call.
Without frame pointers, no such constraint is needed. Make it
conditional on frame pointers.
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: linux-kernel@vger.kernel.org
---
arch/x86/include/asm/asm.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 0d268e6..f1db9e8 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
* gets set up by the containing function. If you forget to do this, objtool
* may print a "call without frame pointer save/setup" warning.
*/
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
#define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
+#else
+#define ASM_CALL_CONSTRAINT
+#endif
#endif /* __ASSEMBLY__ */On March 4, 2025 2:36:24 AM PST, tip-bot2 for Josh Poimboeuf <tip-bot2@linutronix.de> wrote: >The following commit has been merged into the x86/asm branch of tip: > >Commit-ID: 05844663b4fcf22bb3a1494615ae3f25852c9abc >Gitweb: https://git.kernel.org/tip/05844663b4fcf22bb3a1494615ae3f25852c9abc >Author: Josh Poimboeuf <jpoimboe@kernel.org> >AuthorDate: Sun, 02 Mar 2025 17:21:03 -08:00 >Committer: Ingo Molnar <mingo@kernel.org> >CommitterDate: Tue, 04 Mar 2025 11:21:40 +01:00 > >x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers > >With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline >asm statement with a call instruction to force the compiler to set up >the frame pointer before doing the call. > >Without frame pointers, no such constraint is needed. Make it >conditional on frame pointers. > >Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> >Signed-off-by: Ingo Molnar <mingo@kernel.org> >Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> >Cc: Linus Torvalds <torvalds@linux-foundation.org> >Cc: Brian Gerst <brgerst@gmail.com> >Cc: H. Peter Anvin <hpa@zytor.com> >Cc: linux-kernel@vger.kernel.org >--- > arch/x86/include/asm/asm.h | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h >index 0d268e6..f1db9e8 100644 >--- a/arch/x86/include/asm/asm.h >+++ b/arch/x86/include/asm/asm.h >@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP); > * gets set up by the containing function. If you forget to do this, objtool > * may print a "call without frame pointer save/setup" warning. > */ >+#ifdef CONFIG_UNWINDER_FRAME_POINTER > #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0)) >+#else >+#define ASM_CALL_CONSTRAINT >+#endif > > #endif /* __ASSEMBLY__ */ > So we are going to be using this version despite the gcc maintainers telling us it is not supported?
* H. Peter Anvin <hpa@zytor.com> wrote: > > #endif /* __ASSEMBLY__ */ > > So we are going to be using this version despite the gcc maintainers > telling us it is not supported? No, neither patches are in the x86 tree at the moment. Thanks, Ingo
On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote: > > * H. Peter Anvin <hpa@zytor.com> wrote: > > > > #endif /* __ASSEMBLY__ */ > > > > So we are going to be using this version despite the gcc maintainers > > telling us it is not supported? > > No, neither patches are in the x86 tree at the moment. FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this patch wouldn't have changed anything in that respect. Regardless I plan to post a new patch set soon with a bunch of cleanups. It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will use the new __builtin_frame_address(0) input constraint for Clang only. There will be a new asm_call() interface to hide the mess. -- Josh
On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote: >On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote: >> >> * H. Peter Anvin <hpa@zytor.com> wrote: >> >> > > #endif /* __ASSEMBLY__ */ >> > >> > So we are going to be using this version despite the gcc maintainers >> > telling us it is not supported? >> >> No, neither patches are in the x86 tree at the moment. > >FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this >patch wouldn't have changed anything in that respect. > >Regardless I plan to post a new patch set soon with a bunch of cleanups. > >It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will >use the new __builtin_frame_address(0) input constraint for Clang only. > >There will be a new asm_call() interface to hide the mess. > Alternatively, you can co-opt the gcc BR I already filed on this and argue there that there are new reasons to support the alternate construct.
On Fri, Mar 07, 2025 at 03:29:00PM -0800, H. Peter Anvin wrote: > On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote: > >On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote: > >> > >> * H. Peter Anvin <hpa@zytor.com> wrote: > >> > >> > > #endif /* __ASSEMBLY__ */ > >> > > >> > So we are going to be using this version despite the gcc maintainers > >> > telling us it is not supported? > >> > >> No, neither patches are in the x86 tree at the moment. > > > >FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this > >patch wouldn't have changed anything in that respect. > > > >Regardless I plan to post a new patch set soon with a bunch of cleanups. > > > >It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will > >use the new __builtin_frame_address(0) input constraint for Clang only. > > > >There will be a new asm_call() interface to hide the mess. > > > > Alternatively, you can co-opt the gcc BR I already filed on this and > argue there that there are new reasons to support the alternate > construct. We hopefully won't need those hacks much longer anyway, as I'll have another series to propose removing frame pointers for x86-64. x86-32 can keep frame pointers, but doesn't need the constraints. It's not supported for livepatch so it doesn't need to be 100% reliable. Worst case, an unwind skips a frame, but the call address still shows up on stack trace dumps prepended with '?'. I plan to do the asm_call() series before the FP removal series because it's presumably less disruptive, and it has a bunch more orthogonal cleanups. -- Josh
On March 7, 2025 5:38:14 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote: >On Fri, Mar 07, 2025 at 03:29:00PM -0800, H. Peter Anvin wrote: >> On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote: >> >On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote: >> >> >> >> * H. Peter Anvin <hpa@zytor.com> wrote: >> >> >> >> > > #endif /* __ASSEMBLY__ */ >> >> > >> >> > So we are going to be using this version despite the gcc maintainers >> >> > telling us it is not supported? >> >> >> >> No, neither patches are in the x86 tree at the moment. >> > >> >FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this >> >patch wouldn't have changed anything in that respect. >> > >> >Regardless I plan to post a new patch set soon with a bunch of cleanups. >> > >> >It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will >> >use the new __builtin_frame_address(0) input constraint for Clang only. >> > >> >There will be a new asm_call() interface to hide the mess. >> > >> >> Alternatively, you can co-opt the gcc BR I already filed on this and >> argue there that there are new reasons to support the alternate >> construct. > >We hopefully won't need those hacks much longer anyway, as I'll have >another series to propose removing frame pointers for x86-64. > >x86-32 can keep frame pointers, but doesn't need the constraints. It's >not supported for livepatch so it doesn't need to be 100% reliable. >Worst case, an unwind skips a frame, but the call address still shows up >on stack trace dumps prepended with '?'. > >I plan to do the asm_call() series before the FP removal series because >it's presumably less disruptive, and it has a bunch more orthogonal >cleanups. > I should probably clarify that this wasn't flippant, but a serious request. If this works by accident on existing gcc, and works on clang, that is a very good reason for making it the supported way of doing this going forward for both compilers. Per-compiler hacks are nasty, and although we are pretty good about coping with them in the kernel, some user space app developer is guaranteed to get it wrong. Frame pointers are actually more relevant in user space because user space tends to be compiled with a wider range of debug and architecture options, and of course there is simply way more user space code out there.
On Mon, Mar 10, 2025 at 07:49:56AM -0700, H. Peter Anvin wrote: > >> Alternatively, you can co-opt the gcc BR I already filed on this and > >> argue there that there are new reasons to support the alternate > >> construct. > > I should probably clarify that this wasn't flippant, but a serious > request. > > If this works by accident on existing gcc, and works on clang, that is > a very good reason for making it the supported way of doing this going > forward for both compilers. Per-compiler hacks are nasty, and although > we are pretty good about coping with them in the kernel, some user > space app developer is guaranteed to get it wrong. > > Frame pointers are actually more relevant in user space because user > space tends to be compiled with a wider range of debug and > architecture options, and of course there is simply way more user > space code out there. I opened a gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119279 -- Josh
On Fri, 7 Mar 2025 17:38:14 -0800 Josh Poimboeuf <jpoimboe@kernel.org> wrote: ... > We hopefully won't need those hacks much longer anyway, as I'll have > another series to propose removing frame pointers for x86-64. > > x86-32 can keep frame pointers, but doesn't need the constraints. It's > not supported for livepatch so it doesn't need to be 100% reliable. > Worst case, an unwind skips a frame, but the call address still shows up > on stack trace dumps prepended with '?'. Doesn't 'user copy hardening' also do stack following? That needs to find all the stack frames (that have locals) and I think is is more reliable with frame pointers. David
On Sat, Mar 08, 2025 at 08:15:30AM +0000, David Laight wrote: > On Fri, 7 Mar 2025 17:38:14 -0800 > Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > ... > > We hopefully won't need those hacks much longer anyway, as I'll have > > another series to propose removing frame pointers for x86-64. > > > > x86-32 can keep frame pointers, but doesn't need the constraints. It's > > not supported for livepatch so it doesn't need to be 100% reliable. > > Worst case, an unwind skips a frame, but the call address still shows up > > on stack trace dumps prepended with '?'. > > Doesn't 'user copy hardening' also do stack following? > That needs to find all the stack frames (that have locals) and I think > is is more reliable with frame pointers. Yeah, that's arch_within_stack_frames(), which is frame pointer only. ORC would actually be more reliable than frame pointers, but IIRC, hardened usercopy didn't get an ORC implementation due to performance concerns about doing an ORC unwind for every usercopy to/from the stack. So yeah, hardened usercopy is one minor benefit of frame pointers vs ORC. -- Josh
© 2016 - 2026 Red Hat, Inc.