Re: [PATCH 2/4 v2] LoongArch: Get stack without NMI when providing regs parameter

[Enze Li <lienze@kylinos.cn>]

On Wed, Jul 26 2023 at 10:59:06 AM +0800, Jinyang He wrote:

> On 2023-07-25 14:14, Enze Li wrote:
>
>> Currently, arch_stack_walk() can only get the full stack information
>> including NMI.  This is because the implementation of arch_stack_walk()
>> is forced to ignore the information passed by the regs parameter and use
>> the current stack information instead.
>>
>> For some detection systems like KFENCE, only partial stack information
>> is needed.  In particular, the stack frame where the interrupt occurred.
>>
>> To support KFENCE, this patch modifies the implementation of the
>> arch_stack_walk() function so that if this function is called with the
>> regs argument passed, it retains all the stack information in regs and
>> uses it to provide accurate information.
>>
>> Before the patch applied, I get,
>> [    1.531195 ] ==================================================================
>> [    1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
>> [    1.531442 ]
>> [    1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
>> [    1.532046 ]  stack_trace_save_regs+0x48/0x6c
>> [    1.532169 ]  kfence_report_error+0xa4/0x528
>> [    1.532276 ]  kfence_handle_page_fault+0x124/0x270
>> [    1.532388 ]  no_context+0x50/0x94
>> [    1.532453 ]  do_page_fault+0x1a8/0x36c
>> [    1.532524 ]  tlb_do_page_fault_0+0x118/0x1b4
>> [    1.532623 ]  test_out_of_bounds_read+0xa0/0x1d8
>> [    1.532745 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    1.532854 ]  kthread+0x124/0x130
>> [    1.532922 ]  ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> With this patch applied, I get the correct stack information.
>> [    1.320220 ] ==================================================================
>> [    1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
>> [    1.320401 ]
>> [    1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
>> [    1.321134 ]  test_out_of_bounds_read+0xa8/0x1d8
>> [    1.321264 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    1.321392 ]  kthread+0x124/0x130
>> [    1.321459 ]  ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>   arch/loongarch/kernel/stacktrace.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
>> index 2463d2fea21f..9dab30ae68ec 100644
>> --- a/arch/loongarch/kernel/stacktrace.c
>> +++ b/arch/loongarch/kernel/stacktrace.c
>> @@ -18,16 +18,24 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>>   	struct pt_regs dummyregs;
>>   	struct unwind_state state;
>>   -	regs = &dummyregs;
>> -
>>   	if (task == current) {
>> -		regs->regs[3] = (unsigned long)__builtin_frame_address(0);
>> -		regs->csr_era = (unsigned long)__builtin_return_address(0);
>> +		if (regs)
>> +			memcpy(&dummyregs, regs, sizeof(*regs));
>> +		else {
>> +			dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
>> +			dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
>> +		}
>>   	} else {
>> -		regs->regs[3] = thread_saved_fp(task);
>> -		regs->csr_era = thread_saved_ra(task);
>> +		if (regs)
>> +			memcpy(&dummyregs, regs, sizeof(*regs));
>> +		else {
>> +			dummyregs.regs[3] = thread_saved_fp(task);
>> +			dummyregs.csr_era = thread_saved_ra(task);
>> +		}
>>   	}
>>   +	regs = &dummyregs;
>> +

Hi Jinyang,

>
> if (!regs) {
>     regs = &dummyregs;
>
>     if (task == current) {
>         regs->regs[3] = (unsigned long)__builtin_frame_address(0);
>         regs->csr_era = (unsigned long)__builtin_return_address(0);
>     } else {
>         regs->regs[3] = thread_saved_fp(task);
>         regs->csr_era = thread_saved_ra(task);
>     }
>     regs->regs[1] = 0;
> }

Excellent!  FWIW, it looks easy to understand.
I've tested this patch, and it works well.  Thank you.

Cheers!
Enze

>
> BTW, I remembered that __unwind_start() deals with this issue in regs,
> task and current. arch_stack_walk() is unnecessary to provide current
> or task regs if we fix the unwind_start() skip its parent frame
> (caller is arch_stack_walk). But the current state is better, I think.
>
>
> Thanks,
>
> Jinyang
>
>>   	regs->regs[1] = 0;
>>   	for (unwind_start(&state, task, regs);
>>   	     !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {