RISC-V uprobe bug (Was: Re: WARNING: CPU: 3 PID: 261 at kernel/bpf/memalloc.c:342)

[Björn Töpel]

Nam Cao <namcaov at gmail.com> writes:

> On Sun, Aug 27, 2023 at 10:11:25AM +0200, Björn Töpel wrote:
>> The default implementation of is_trap_insn() which RISC-V is using calls
>> is_swbp_insn(), which is doing what your patch does. Your patch does not
>> address the issue.
>
> is_swbp_insn() does this:
>
>         #ifdef CONFIG_RISCV_ISA_C
>                 return (*insn & 0xffff) == UPROBE_SWBP_INSN;
>         #else
>                 return *insn == UPROBE_SWBP_INSN;
>         #endif
>
> ...so it doesn't even check for 32-bit ebreak if C extension is on. My patch
> is not the same.

Ah, was too quick.

AFAIU uprobes *always* uses c.ebreak when CONFIG_RISCV_ISA_C is set, and
ebreak otherwise. That's the reason is_swbp_insn() is implemented like
that. If that's not the case, there's a another bug, that your patches
addresses.

In that case we need an arch specific set_swbp() implementation together
with your patch.

Guo, thoughts? ...text patching on RISC-V is still a big WIP.

> But okay, if it doesn't solve the problem, then I must be wrong somewhere.

Yes, this bug is another issue.

>> We're taking an ebreak trap from kernel space. In this case we should
>> never look for a userland (uprobe) handler at all, only the kprobe
>> handlers should be considered.
>> 
>> In this case, the TIF_UPROBE is incorrectly set, and incorrectly (not)
>> handled in the "common entry" exit path, which takes us to the infinite
>> loop.
>
> This change makes a lot of sense, no reason to check for uprobes if exception
> comes from the kernel.

Ok! I sent a patch proper for this.


Björn