Re: [PATCH] riscv: provide riscv-specific is_trap_insn()

[Nam Cao]

On Wed, Aug 30, 2023 at 9:32 AM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>
> Nam Cao <namcaov@xxxxxxxxx> writes:
>
> > On Tue, Aug 29, 2023 at 08:14:59AM +0200, Björn Töpel wrote:
> >> Nam Cao <namcaov@xxxxxxxxx> writes:
> >>
> >> > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote:
> >> >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote:
> >> >> > Nam Cao <namcaov@xxxxxxxxx> writes:
> >> >> >
> >> >> > > uprobes expects is_trap_insn() to return true for any trap instructions,
> >> >> > > not just the one used for installing uprobe. The current default
> >> >> > > implementation only returns true for 16-bit c.ebreak if C extension is
> >> >> > > enabled. This can confuse uprobes if a 32-bit ebreak generates a trap
> >> >> > > exception from userspace: uprobes asks is_trap_insn() who says there is no
> >> >> > > trap, so uprobes assume a probe was there before but has been removed, and
> >> >> > > return to the trap instruction. This cause an infinite loop of entering
> >> >> > > and exiting trap handler.
> >> >> > >
> >> >> > > Instead of using the default implementation, implement this function
> >> >> > > speficially for riscv which checks for both ebreak and c.ebreak.
> >> >> >
> >> >> > I took this for a spin, and it indeed fixes this new hang! Nice!
> >> >>
> >> >> Great! Thanks for testing it.
> >> >>
> >> >> > However, when I tried setting an uprobe on the ebreak instruction
> >> >> > (offset 0x118) from your example [1], the probe does not show up in the
> >> >> > trace buffer.
> >> >> >
> >> >> > Any ideas?
> >> >>
> >> >> >From my understanding, both uprobes and kprobes refuse to install break points
> >> >> into existing trap instructions. Otherwise, we may conflict with something else
> >> >> that is also using trap instructions.
> >> >
> >> > I just realize you probably ask this because uprobe can still be installed before
> >> > applying the patch. But I think that is another bug that my patch also
> >> > accidentally fix: uprobes should not install breakpoint into ebreak instructions,
> >> > but it incorrectly does so because it does not even know about the existence of
> >> > 32-bit ebreak.
> >>
> >> FWIW, I can still install the uprobe at an ebreak with you patch. It's
> >> not hit, but succeeds to install.
> >
> > It seems uprobes install failures are completely silent (see uprobe_mmap() in
> > kernel/events/uprobes.c). So I think although uprobes install seems fine, it
> > actually is not.
>
> Huh, so there's no check if the instruction is a valid one at event
> register point?

There are some checks (eg. if the probe is within the binary), but
they are not complete.

The actual checks for the validity of the instruction is not done
until installation.

Best regards,
Nam