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

[Huacai Chen <chenhuacai@kernel.org>]

On Fri, Jul 21, 2023 at 9:50 AM Enze Li <lienze@kylinos.cn> wrote:
>
> Hi Huacai,
>
> Thanks for your review.
>
> On Wed, Jul 19 2023 at 11:17:14 PM +0800, Huacai Chen wrote:
>
> > Hi, Enze,
> >
> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
> >>
> >> Currently, executing 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 | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
> >> index 2463d2fea21f..21f60811e26f 100644
> >> --- a/arch/loongarch/kernel/stacktrace.c
> >> +++ b/arch/loongarch/kernel/stacktrace.c
> >> @@ -18,16 +18,20 @@ 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 {
> > When "task != current", we don't need to handle the "regs != NULL" case?
> >
> > Huacai
> >
>
> So far, I have not encountered this situation.  I'm not sure what
> problems would arise from extending the modifications with "task !=
> current".
>
> However, these modifications now are sufficient for the KFENCE
> system.  I would suggest that we don't modify other parts until we
> encounter problems.  This way, we can forge ahead steadily.
I don't think so. In my opinion, "partial stack information" is a
clear requirement, whether the task is current or not.

So, if  the input regs is not NULL, we should always
memcpy(&dummyregs, regs, sizeof(*regs));

Or we may listen to Tiezhu's idea?

Huacai
>
> Best Regards,
> Enze
>
> >> -               regs->regs[3] = thread_saved_fp(task);
> >> -               regs->csr_era = thread_saved_ra(task);
> >> +               dummyregs.regs[3] = thread_saved_fp(task);
> >> +               dummyregs.csr_era = thread_saved_ra(task);
> >>         }
> >>
> >> +       regs = &dummyregs;
> >> +
> >>         regs->regs[1] = 0;
> >>         for (unwind_start(&state, task, regs);
> >>              !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
> >> --
> >> 2.34.1
> >>
> >>