linux-kernel - Re: [PATCH v11 08/15] unwind_user/sframe: Wire up unwind

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: <6b5c0c64-c4da-416d-a103-8d6ec2f06a9b@linux.ibm.com>
Date: Fri, 24 Oct 2025 16:29:07 +0200
From: Jens Remus <jremus@...ux.ibm.com>
To: Peter Zijlstra <peterz@...radead.org>, Steven Rostedt <rostedt@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        bpf@...r.kernel.org, x86@...nel.org, linux-mm@...ck.org,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Masami Hiramatsu
 <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...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>,
        Florian Weimer <fweimer@...hat.com>, Kees Cook <kees@...nel.org>,
        "Carlos O'Donell" <codonell@...hat.com>, Sam James <sam@...too.org>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        David Hildenbrand <david@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Michal Hocko
 <mhocko@...e.com>, Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        "Steven Rostedt (Google)" <rostedt@...dmis.org>
Subject: Re: [PATCH v11 08/15] unwind_user/sframe: Wire up unwind_user to
 sframe

On 10/24/2025 3:44 PM, Peter Zijlstra wrote:
> On Wed, Oct 22, 2025 at 04:43:19PM +0200, Jens Remus wrote:
> 
>> @@ -26,12 +27,10 @@ get_user_word(unsigned long *word, unsigned long base, int off, unsigned int ws)
>>  	return get_user(*word, addr);
>>  }
>>  
>> -static int unwind_user_next_fp(struct unwind_user_state *state)
>> +static int unwind_user_next_common(struct unwind_user_state *state,
>> +				   const struct unwind_user_frame *frame,
>> +				   struct pt_regs *regs)
>>  {
> 
> What is pt_regs for? AFAICT it isn't actually used in any of the
> following patches.

Good catch!  No idea.  It started to appear in v9 of the series:

[PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
https://lore.kernel.org/all/20250708021159.386608979@kernel.org/

[PATCH v9 06/11] unwind_user/sframe: Wire up unwind_user to sframe
https://lore.kernel.org/all/20250717012936.619600891@kernel.org/

My s390 support for unwind user sframe will make use of it, but it
should better be introduced there then.

@Steven: Any idea why you added pt_regs?  Your v9 even had this other
instance of unused pt_regs:

+static struct unwind_user_frame *get_fp_frame(struct pt_regs *regs)
+{
+	return &fp_frame;
+}

>> @@ -67,6 +66,26 @@ static int unwind_user_next_fp(struct unwind_user_state *state)
>>  	return 0;
>>  }
>>  
>> +static int unwind_user_next_sframe(struct unwind_user_state *state)
>> +{
>> +	struct unwind_user_frame _frame, *frame;
>> +
>> +	/* sframe expects the frame to be local storage */
>> +	frame = &_frame;
>> +	if (sframe_find(state->ip, frame))
>> +		return -ENOENT;
>> +	return unwind_user_next_common(state, frame, task_pt_regs(current));
>> +}
> 
> Would it not be simpler to write:
> 
> static int unwind_user_next_sframe(struct unwind_user_state *state)
> {
> 	struct unwind_user_frame frame;
> 
> 	/* sframe expects the frame to be local storage */
> 	if (sframe_find(state->ip, &frame))
> 		return -ENOENT;
> 	return unwind_user_next_common(state, &frame, task_pt_regs(current));
> }
> 
> hmm?

I agree.  Must have been a leftover from changes from v8 to v9.

>> @@ -80,6 +99,16 @@ static int unwind_user_next(struct unwind_user_state *state)
>>  
>>  		state->current_type = type;
>>  		switch (type) {
>> +		case UNWIND_USER_TYPE_SFRAME:
>> +			switch (unwind_user_next_sframe(state)) {
>> +			case 0:
>> +				return 0;
>> +			case -ENOENT:
>> +				continue;	/* Try next method. */
>> +			default:
>> +				state->done = true;
>> +			}
>> +			break;
> 
> Should it remove SFRAME from state->available_types at this point?

In the -ENOENT case?  If the reason is that there was either no SFrame
section or no SFrame information (SFrame FRE) for the IP, then SFRAME
could potentially be successful with the next IP in the call chain.
Provided the other unwind methods do correctly unwind both SP and FP.

@Steven: What is your opinion on this?

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@...ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/