Re: [RFC PATCH v5 2/2] arm64: Create a list of SYM_CODE functions, check return PC against list

[Madhavan T. Venkataraman]

On 6/15/21 8:52 PM, Suraj Jitindar Singh wrote:
> On Wed, 2021-05-26 at 16:49 -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> The unwinder should check if the return PC falls in any function that
>> is considered unreliable from an unwinding perspective. If it does,
>> mark the stack trace unreliable.
>>
> 
> [snip]
> 
> Correct me if I'm wrong, but do you not need to move the final frame
> check to before the unwinder_is_unreliable() call?
> 

That is done in a patch series that has been merged into for-next/stacktrace branch.
When I merge this patch series with that, the final frame check will be done prior.

I have mentioned this in the cover letter:

Last stack frame
================

If a SYM_CODE function occurs in the very last frame in the stack trace,
then the stack trace is not considered unreliable. This is because there
is no more unwinding to do. Examples:

        - EL0 exception stack traces end in the top level EL0 exception
          handlers.

        - All kernel thread stack traces end in ret_from_fork().

Madhavan

> Userland threads which have ret_from_fork as the last entry on the
> stack will always be marked unreliable as they will always have a
> SYM_CODE entry on their stack (the ret_from_fork).
> 


> Also given that this means the last frame has been reached and as such
> there's no more unwinding to do, I don't think we care if the last pc
> is a code address.
> 
> - Suraj
> 
>>   *
>> @@ -133,7 +236,20 @@ int notrace unwind_frame(struct task_struct
>> *tsk, struct stackframe *frame)
>>  	 *	- Foreign code (e.g. EFI runtime services)
>>  	 *	- Procedure Linkage Table (PLT) entries and veneer
>> functions
>>  	 */
>> -	if (!__kernel_text_address(frame->pc))
>> +	if (!__kernel_text_address(frame->pc)) {
>> +		frame->reliable = false;
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * If the final frame has been reached, there is no more
>> unwinding
>> +	 * to do. There is no need to check if the return PC is
>> considered
>> +	 * unreliable by the unwinder.
>> +	 */
>> +	if (!frame->fp)
>> +		return 0;
> 
> if (frame->fp == (unsigned long)task_pt_regs(tsk)->stackframe)
> 	return -ENOENT;
> 
>> +
>> +	if (unwinder_is_unreliable(frame->pc))
>>  		frame->reliable = false;
>>  
>>  	return 0;
>> diff --git a/arch/arm64/kernel/vmlinux.lds.S
>> b/arch/arm64/kernel/vmlinux.lds.S
>> index 7eea7888bb02..32e8d57397a1 100644
>> --- a/arch/arm64/kernel/vmlinux.lds.S
>> +++ b/arch/arm64/kernel/vmlinux.lds.S
>> @@ -103,6 +103,12 @@ jiffies = jiffies_64;
>>  #define TRAMP_TEXT
>>  #endif
>>  
>> +#define SYM_CODE_FUNCTIONS                                     \
>> +       . = ALIGN(16);                                           \
>> +       __sym_code_functions_start = .;                         \
>> +       KEEP(*(sym_code_functions))                             \
>> +       __sym_code_functions_end = .;
>> +
>>  /*
>>   * The size of the PE/COFF section that covers the kernel image,
>> which
>>   * runs from _stext to _edata, must be a round multiple of the
>> PE/COFF
>> @@ -218,6 +224,7 @@ SECTIONS
>>  		CON_INITCALL
>>  		INIT_RAM_FS
>>  		*(.init.altinstructions .init.bss)	/* from the
>> EFI stub */
>> +               SYM_CODE_FUNCTIONS
>>  	}
>>  	.exit.data : {
>>  		EXIT_DATA