On 5/5/21 11:34 AM, Mark Brown wrote: > On Tue, May 04, 2021 at 02:03:14PM -0500, Madhavan T. Venkataraman wrote: >> On 5/4/21 11:05 AM, Mark Brown wrote: > >>>> @@ -118,9 +160,21 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) >>>> return -EINVAL; >>>> frame->pc = ret_stack->ret; >>>> frame->pc = ptrauth_strip_insn_pac(frame->pc); >>>> + return 0; >>>> } > >>> Do we not need to look up the range of the restored pc and validate >>> what's being pointed to here? It's not immediately obvious why we do >>> the lookup before handling the function graph tracer, especially given >>> that we never look at the result and there's now a return added skipping >>> further reliability checks. At the very least I think this needs some >>> additional comments so the code is more obvious. > >> I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges. >> Unwindable ranges will be special ranges such as the return_to_handler() and >> kretprobe_trampoline() functions for which the unwinder has (or will have) >> special code to unwind. So, the lookup_range() has to happen before the >> function graph code. Please look at the last patch in the series for >> the fix for the above function graph code. > > That sounds reasonable but like I say should probably be called out in > the code so it's clear to people working with it. > OK. To make this better, I will do the lookup_range() after the function graph code to begin with. Then, in the last patch for the function graph code, I will move it up. This way, the code is clear and your comment is addressed. >> On the question of "should the original return address be checked against >> sym_code_ranges[]?" - I assumed that if there is a function graph trace on a >> function, it had to be an ftraceable function. It would not be a part >> of sym_code_ranges[]. Is that a wrong assumption on my part? > > I can't think of any cases where it wouldn't be right now, but it seems > easier to just do a redundant check than to have the assumption in the > code and have to think about if it's missing. > Agreed. Will do the check. Madhavan
