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
