Re: [PATCH v10 08/11] arm64: Rename unwinder functions, prevent them from being traced and kprobed

[Madhavan T. Venkataraman]

On 10/27/21 12:53 PM, Mark Rutland wrote:
> On Thu, Oct 14, 2021 at 09:58:44PM -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> Rename unwinder functions for consistency and better naming.
>>
>> 	- Rename start_backtrace() to unwind_start().
>> 	- Rename unwind_frame() to unwind_next().
>> 	- Rename walk_stackframe() to unwind().
> 
> This looks good to me.
> 

Thanks.

> Could we split this from the krpbes/tracing changes? I think this stands
> on it's own, and (as below) the kprobes/tracing changes need some more
> explanation, and would make sense as a separate patch.
> 

OK. I will split the patches.

>> Prevent the following unwinder functions from being traced:
>>
>> 	- unwind_start()
>> 	- unwind_next()
>>
>> 	unwind() is already prevented from being traced.
> 
> This could do with an explanation in the commis message as to why we
> need to do this. If this is fixing a latent issue, it should be in a
> preparatory patch that we can backport.
> 
> I dug into this a bit, and from taking a look, we prohibited ftrace in commit:
> 
>   0c32706dac1b0a72 ("arm64: stacktrace: avoid tracing arch_stack_walk()")
> 
> ... which is just one special case of graph return stack unbalancing,
> and should be addressed by using HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so
> with the patch making us use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, that's
> no longer necessary.
> 
> So we no longer seem to have a specific reason to prohibit ftrace
> here.
> 

OK, I will think about this and add a comment.

>> Prevent the following unwinder functions from being kprobed:
>>
>> 	- unwind_start()
>>
>> 	unwind_next() and unwind() are already prevented from being kprobed.
> 
> Likewise, I think this needs some explanation. From diggin, we
> prohibited kprobes in commit:
> 
>   ee07b93e7721ccd5 ("arm64: unwind: Prohibit probing on return_address()")
> 
> ... and the commit message says we need to do this because this is
> (transitively) called by trace_hardirqs_off(), which is kprobes
> blacklisted, but doesn't explain the actual problem this results in.
> 

OK. I will think about this and add a comment.

> AFAICT x86 directly uses __builtin_return_address() here, but that won't
> recover rewritten addresses, which seems like a bug (or at least a
> limitation) on x86, assuming I've read that correctly.
> 

OK.

Thanks,

Madhavan