linux-kernel - Re: [RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <sotffno5desd3ajyjd42rq52yrtztddwjbbh3xpa6v7fb63v36@bwoq4s7j5why>
Date: Thu, 17 Jul 2025 21:51:01 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Jens Remus <jremus@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	bpf@...r.kernel.org, x86@...nel.org, Steven Rostedt <rostedt@...nel.org>, 
	Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>, 
	Ilya Leoshkevich <iii@...ux.ibm.com>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Andrii Nakryiko <andrii@...nel.org>, 
	Indu Bhagat <indu.bhagat@...cle.com>, "Jose E. Marchesi" <jemarch@....org>, 
	Beau Belgrave <beaub@...ux.microsoft.com>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>, 
	Florian Weimer <fweimer@...hat.com>, Sam James <sam@...too.org>
Subject: Re: [RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA =
 SP_callsite + offset

On Thu, Jul 17, 2025 at 11:27:45AM +0200, Jens Remus wrote:
> On 16.07.2025 23:32, Josh Poimboeuf wrote:
> > On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote:
> >> Most architectures define their CFA as the value of the stack pointer
> >> (SP) at the call site in the previous frame, as suggested by the DWARF
> >> standard:
> >>
> >>   CFA = <SP at call site>
> >>
> >> Enable unwinding of user space for architectures, such as s390, which
> >> define their CFA as the value of the SP at the call site in the previous
> >> frame with an offset:
> >>
> >>   CFA = <SP at call site> + offset
> > 
> > This is a bit confusing, as the comment and code define it as
> > 
> >     SP = CFA + offset
> > 
> > Should the commit log be updated to match that?
> 
> I agree that the commit message is confusing. Would it help if I replace
> it with the following:
> 
> Most architectures define their CFA as the value of the stack pointer
> (SP) at the call site in the previous frame, as suggested by the DWARF
> standard.  Therefore the SP at call site can be unwound using an
> implicitly assumed value offset from CFA rule with an offset of zero:
> 
>   .cfi_val_offset <SP>, 0
> 
> As a result the SP at call site computes as follows:
> 
>   SP = CFA
> 
> Enable unwinding of user space for architectures, such as s390, which
> define their CFA as the value of the SP at the call site in the previous
> frame with an offset.  Do so by enabling architectures to override the
> default SP value offset from CFA of zero with an architecture-specific
> one:
> 
>   .cfi_val_offset <SP>, offset
>   
> So that the SP at call site computes as follows:
> 
>   SP = CFA + offset

Looks good to me, thanks!

> >> +++ b/arch/x86/include/asm/unwind_user.h
> >> @@ -8,6 +8,7 @@
> >>  	.cfa_off	= (s32)sizeof(long) *  2,				\
> >>  	.ra_off		= (s32)sizeof(long) * -1,				\
> >>  	.fp_off		= (s32)sizeof(long) * -2,				\
> >> +	.sp_val_off	= (s32)0,						\
> > 
> > IIUC, this is similar to ra_off and fp_off in that its an offset from
> > the CFA.  Can we call it "sp_off"?
> 
> My intent was to use the terminology from DWARF CFI (i.e. "offset(N)"
> and "val_offset(N)") and the related assembler CFI directives:
> 
>   .cfi_offset register, offset:  Previous value of register is saved at
>                                  offset from CFA.
> 
>   .cfi_val_offset register, offset:  Previous value of register is
>                                      CFA + offset. 

The distinction between "cfi_offset" and "cfi_val_offset" is confusing,
unless one already happens to know CFI syntax (not likely for us kernel
developers).

We don't need to match the DWARF CFI directive naming.  Let's instead
optimize for readability.

I think "sp_off" is fine here, its semantics are similar to the existing
cfa_off field.

The semantics of ra_off and fp_off are different, but those are getting
removed in favor of nested structs in a later patch anyway.

-- 
Josh