linux-kernel - Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

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: <0c31a02c-183e-cac6-8826-82330c6fd830@linux.microsoft.com>
Date:   Sun, 26 Jun 2022 23:51:23 -0500
From:   "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     broonie@...nel.org, jpoimboe@...hat.com, ardb@...nel.org,
        nobuta.keiya@...itsu.com, sjitindarsingh@...il.com,
        catalin.marinas@....com, will@...nel.org,
        jamorris@...ux.microsoft.com, linux-arm-kernel@...ts.infradead.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar
 to other architectures



On 6/26/22 03:21, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:14PM -0500, madvenka@...ux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
>>
>> Change the loop in unwind()
>> ===========================
>>
>> Change the unwind loop in unwind() to:
>>
>> 	while (unwind_continue(state, consume_entry, cookie))
>> 		unwind_next(state);
>>
>> This is easy to understand and maintain.
>> New function unwind_continue()
>> ==============================
>>
>> Define a new function unwind_continue() that is used in the unwind loop
>> to check for conditions that terminate a stack trace.
>>
>> The conditions checked are:
>>
>> 	- If the bottom of the stack (final frame) has been reached,
>> 	  terminate.
>>
>> 	- If the consume_entry() function returns false, the caller of
>> 	  unwind has asked to terminate the stack trace. So, terminate.
>>
>> 	- If unwind_next() failed for some reason (like stack corruption),
>> 	  terminate.
> 
> I'm a bit confused as to why this structure, since AFAICT this doesn't match
> other architectures (looking at x86, powerpc, and s390). I note that x86 has:
> 
> * In arch_stack_walk():
> 
>         for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...))
> 			break;
> 		...
> 	}
> 
> * In arch_stack_walk_reliable():
> 
>         for (unwind_start(&state, task, NULL, NULL);
>              !unwind_done(&state) && !unwind_error(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...)
> 			return -EINVAL;
> 	}
> 
> ... and back in v6 I suggeted exactly that shape:
> 
>   https://lore.kernel.org/linux-arm-kernel/20210728165635.GA47345@C02TD0UTHF1T.local/
> 

OK. I will take a look at your suggestion and resend this patch.

>>
>> Do not return an error value from unwind_next()
>> ===============================================
>>
>> We want to check for terminating conditions only in unwind_continue() from
>> the unwinder loop. So, do not return an error value from unwind_next().
>> Simply set a flag in unwind_state and check the flag in unwind_continue().
> 
> I'm fine with the concept of moving ghe return value out of unwind_next() (e.g.
> if we go with an x86-like structure), but I don't think that we should
> centralize the other checks *and* the consumption within unwind_continue(), as
> I think those are two separate things.
> 

OK. I will address this in the next version.

>>
>> Final FP
>> ========
>>
>> Introduce a new field "final_fp" in "struct unwind_state". Initialize this
>> to the final frame of the stack trace:
>>
>> 	task_pt_regs(task)->stackframe
>>
>> This is where the stacktrace must terminate if it is successful. Add an
>> explicit comment to that effect.
> 
> Can we please make this change as a preparatory step, as with the 'task' field?
> 
> We can wrap this in a helper like:
> 
> static bool is_final_frame(struct unwind state *state)
> {
> 	return state->fp == state->final_fp;
> }
> 
> ... and use that in the main loop.
> 

OK. I will make these changes.

Thanks.

Madhavan