Re: [RFC PATCH v3 1/4] arm64: Introduce stack trace reliability checks in the unwinder

[Madhavan T. Venkataraman]

On 5/4/21 10:50 AM, Mark Brown wrote:
> On Mon, May 03, 2021 at 12:36:12PM -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
> 
>> +	/*
>> +	 * First, make sure that the return address is a proper kernel text
>> +	 * address. A NULL or invalid return address probably means there's
>> +	 * some generated code which __kernel_text_address() doesn't know
>> +	 * about. Mark the stack trace as not reliable.
>> +	 */
>> +	if (!__kernel_text_address(frame->pc)) {
>> +		frame->reliable = false;
>> +		return 0;
>> +	}
> 
> Do we want the return here?  It means that...
> 
>> +
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  	if (tsk->ret_stack &&
>> -		(ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) {
>> +		frame->pc == (unsigned long)return_to_handler) {
>>  		struct ftrace_ret_stack *ret_stack;
>>  		/*
>>  		 * This is a case where function graph tracer has
>> @@ -103,11 +117,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>  		if (WARN_ON_ONCE(!ret_stack))
>>  			return -EINVAL;
>>  		frame->pc = ret_stack->ret;
>> +		frame->pc = ptrauth_strip_insn_pac(frame->pc);
>>  	}
> 
> ...we skip this handling in the case where we're not in kernel code.  I
> don't know off hand if that's a case that can happen right now but it
> seems more robust to run through this and anything else we add later,
> even if it's not relevant now changes either in the unwinder itself or
> resulting from some future work elsewhere may mean it later becomes
> important.  Skipping futher reliability checks is obviously fine if
> we've already decided things aren't reliable but this is more than just
> a reliability check.
> 

AFAICT, currently, all the functions that the unwinder checks do have
valid kernel text addresses. However, I don't think there is any harm
in letting it fall through and make all the checks. So, I will remove the
return statement.

Thanks!

Madhavan