Re: [PATCH v2] riscv: fix oops caused by irq on/off tracer

From: Changbin Du
Date: Thu Feb 10 2022 - 22:07:15 EST


I reconsidered the problem and found my previous analysis is flawed. So let's re-explain.

The fault happens on code generated by CALLER_ADDR1 (aka.__builtin_return_address(1)):
0xffffffff8011510e <+80>: ld a1,-16(s0)
0xffffffff80115112 <+84>: ld s2,-8(a1) # <-- paging fault here,a1=0x0000000000000100

This because the assembly entry code doesn't setup a valid frame pointer, and the fp(aka. s0) register is used for other purpose.
resume_kernel:
REG_L s0, TASK_TI_PREEMPT_COUNT(tp)
bnez s0, restore_all
REG_L s0, TASK_TI_FLAGS(tp)
andi s0, s0, _TIF_NEED_RESCHED
beqz s0, restore_all
call preempt_schedule_irq
j restore_all

So, there is two solutions:
1) Invoke trace_hardirqs_on/off in C function, so the compiler will take care of frame pointer. This what I did.
2) Always setup vaild frame pointer in assembly entry code. I think this is what JiSheng suggested?

I prefer #1 since we don't need to setup frame pointer if irqoff tracer is not enabled.

On Thu, Feb 10, 2022 at 11:37:06PM +0800, Jisheng Zhang wrote:
> On Thu, Feb 10, 2022 at 11:27:21PM +0800, Jisheng Zhang wrote:
> > On Thu, Feb 10, 2022 at 09:37:58PM +0800, Changbin Du wrote:
> > > On Thu, Feb 10, 2022 at 01:32:59AM +0800, Jisheng Zhang wrote:
> > > [snip]
> > > > Hi Changbin,
> > > >
> > > > I read the code and find that current riscv frame records during
> > > > exception isn't as completed as other architectures. riscv only
> > > > records frames from the ret_from_exception(). If we add completed
> > > What do you mean for 'record'?
> > >
> >
> > stack frame record.
> >
> > > > frame records as other arch do, then the issue you saw can also
> > > > be fixed at the same time.
> > > >
> > > I don't think so. The problem is __builtin_return_address(1) trigger page fault
> > > here.
> >
> > There's misunderstanding here. I interpret this bug as incomplete
> > stackframes.
> >
> > This is current riscv stackframe during exception:
> >
> > high
> > ----------------
> > top | | <- ret_from_exception
> > ----------------
> > | | <- trace_hardirqs_on
> > -----------------
> > low
>
> sorry, the "top" is wrongly placed.
> high
> ----------------
> | | <- ret_from_exception
> ----------------
> | | <- trace_hardirqs_on
> -----------------
> top
>
> low
>
>
>
> >
> > As you said, the CALLER_ADDR1 a.k.a __builtin_return_address(1) needs
> > at least two parent call frames.
> >
> > If we complete the stackframes during exception as other arch does:
> >
> > high
> > ----------------
> > top | | <- the synthetic stackframe from the interrupted point
> > ----------------
> > .....
> > ----------------
> > | | <- ret_from_exception
> > ----------------
> > | | <- trace_hardirqs_on
> > -----------------
> > low
>
> ditto
>
> >
> >
> > Then we meet the "at least two parent call frames" requirement. IOW, my
> > solution solve the problem from the entry.S side. One of the advantages
> > would be we let interrupted point show up in dump_stack() as other arch
> > do. What I'm not sure is whether it's safe to do so now since rc3 is
> > released.
> >

--
Cheers,
Changbin Du