On 5/4/21 2:03 PM, Madhavan T. Venkataraman wrote:
>
>
> On 5/4/21 11:05 AM, Mark Brown wrote:
>> On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>>
>>> Create a sym_code_ranges[] array to cover the following text sections that
>>> contain functions defined as SYM_CODE_*(). These functions are low-level
>>
>> This makes sense to me - a few of bikesheddy comments below but nothing
>> really substantive.
>>
>
> OK.
>
>>> +static struct code_range *lookup_range(unsigned long pc)
>>
>> This feels like it should have a prefix on the name (eg, unwinder_)
>> since it looks collision prone. Or lookup_code_range() rather than just
>> plain lookup_range().
>>
>
> I will add the prefix.
>
>>> +{
>> + struct code_range *range;
>> +
>> + for (range = sym_code_ranges; range->start; range++) {
>>
>> It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
>> the array can't be empty.
>>
>
> If there is a match, I return the matched range. Else, I return the sentinel.
> This is just so I don't have to check for range == NULL after calling
> lookup_range().
>
> I will change it to what you have suggested and check for NULL explicitly.
> It is not a problem.
>
>>> + range = lookup_range(frame->pc);
>>> +
>>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> if (tsk->ret_stack &&
>>> frame->pc == (unsigned long)return_to_handler) {
>>> @@ -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.
>
> 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?
>
If you prefer, I could do something like this:
check_pc:
if (!__kernel_text_address(frame->pc))
frame->reliable = false;
range = lookup_range(frame->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
frame->pc == (unsigned long)return_to_handler) {
...
frame->pc = ret_stack->ret;
frame->pc = ptrauth_strip_insn_pac(frame->pc);
goto check_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Is that acceptable?
Madhavan
