linux-kernel - Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk

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: <f2dfa6cd-7a23-e1b7-09d5-737d4a95b90c@linux.microsoft.com>
Date:   Tue, 30 Nov 2021 11:13:28 -0600
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, jmorris@...ei.org,
        linux-arm-kernel@...ts.infradead.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within
 walk_stackframe()



On 11/30/21 9:05 AM, Mark Rutland wrote:
> On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@...ux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
>>
>> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
>> separately. There is no need to do that. Instead, call start_backtrace()
>> from within walk_stackframe(). In other words, walk_stackframe() is the only
>> unwind function a consumer needs to call.
>>
>> Currently, the only consumer is arch_stack_walk(). In the future,
>> arch_stack_walk_reliable() will be another consumer.
>>
>> Currently, there is a check for a NULL task in unwind_frame(). It is not
>> needed since all current consumers pass a non-NULL task.
> 
> Can you split the NULL check change into a preparatory patch? That change is
> fine in isolation (and easier to review/ack), and it's nicer for future
> bisection to not group that with unrelated changes.
> 

Will do this in the next version.

>> Use struct stackframe only within the unwind functions.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@...ux.microsoft.com>
>> ---
>>  arch/arm64/kernel/stacktrace.c | 41 ++++++++++++++++++----------------
>>  1 file changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 0fb58fed54cb..7217c4f63ef7 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -69,9 +69,6 @@ static int notrace unwind_frame(struct task_struct *tsk,
>>  	unsigned long fp = frame->fp;
>>  	struct stack_info info;
>>  
>> -	if (!tsk)
>> -		tsk = current;
>> -
>>  	/* Final frame; nothing to unwind */
>>  	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>>  		return -ENOENT;
>> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
>>  NOKPROBE_SYMBOL(unwind_frame);
>>  
>>  static void notrace walk_stackframe(struct task_struct *tsk,
>> -				    struct stackframe *frame,
>> +				    unsigned long fp, unsigned long pc,
>>  				    bool (*fn)(void *, unsigned long), void *data)
>>  {
>> +	struct stackframe frame;
>> +
>> +	start_backtrace(&frame, fp, pc);
>> +
>>  	while (1) {
>>  		int ret;
>>  
>> -		if (!fn(data, frame->pc))
>> +		if (!fn(data, frame.pc))
>>  			break;
>> -		ret = unwind_frame(tsk, frame);
>> +		ret = unwind_frame(tsk, &frame);
>>  		if (ret < 0)
>>  			break;
>>  	}
>> @@ -195,17 +196,19 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>>  			      void *cookie, struct task_struct *task,
>>  			      struct pt_regs *regs)
>>  {
>> -	struct stackframe frame;
>> -
>> -	if (regs)
>> -		start_backtrace(&frame, regs->regs[29], regs->pc);
>> -	else if (task == current)
>> -		start_backtrace(&frame,
>> -				(unsigned long)__builtin_frame_address(1),
>> -				(unsigned long)__builtin_return_address(0));
>> -	else
>> -		start_backtrace(&frame, thread_saved_fp(task),
>> -				thread_saved_pc(task));
>> -
>> -	walk_stackframe(task, &frame, consume_entry, cookie);
>> +	unsigned long fp, pc;
>> +
>> +	if (regs) {
>> +		fp = regs->regs[29];
>> +		pc = regs->pc;
>> +	} else if (task == current) {
>> +		/* Skip arch_stack_walk() in the stack trace. */
>> +		fp = (unsigned long)__builtin_frame_address(1);
>> +		pc = (unsigned long)__builtin_return_address(0);
>> +	} else {
>> +		/* Caller guarantees that the task is not running. */
>> +		fp = thread_saved_fp(task);
>> +		pc = thread_saved_pc(task);
>> +	}
>> +	walk_stackframe(task, fp, pc, consume_entry, cookie);
> 
> I'd prefer to leave this as-is. The new and old structure are largely
> equivalent, so we haven't made this any simpler, but we have added more
> arguments to walk_stackframe().
> 

This is just to simplify things when we eventually add arch_stack_walk_reliable().
That is all. All of the unwinding is done by a single unwinding function and
there are two consumers of that unwinding function - arch_stack_walk() and
arch_stack_walk_reliable().


> One thing I *would* like to do is move tsk into strcut stackframe, so we only
> need to pass that around, which'll make it easier to refactor the core unwind
> logic.
> 

Will do this in the next version.

Thanks,

Madhavan