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 11:04:34AM +0200, Björn Töpel wrote:
>> 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.
>
> That's what I understand too.
>
>> If that's not the case, there's a another bug, that your patches
>> addresses.
>
> I think it's a bug regardless. is_trap_insn() is used by uprobes to figure out
> if there is an instruction that generates trap exception, not just instructions
> that are "SWBP". The reason is because when there is a trap, but uprobes doesn't
> see a probe installed here, it needs is_trap_insn() to figure out if the trap
> is generated by ebreak from something else, or because the probe is just removed.
> In the latter case, uprobes will return back, because probe has already been removed,
> so it should be safe to do so. That's why I think the incorrect is_swbp_insn()
> would cause a hang, because uprobes incorrectly thinks there is no ebreak there,
> so it should be okay to go back, but there actually is.
>
> So, from my understanding, if uprobes encounter a 32-bit ebreak for any reason,
> the kernel would hang. I think your patch is a great addition nonetheless, but I
> am guessing that it only masks the problem by preventing uprobes from seeing the
> 32-bit ebreak in the specific test, not really solve it. So, if there is a 32-bit
> ebreak in userspace, the bug still causes the kernel to hang.
>
> I am still quite confident of my logic, so I would be very suprised if my fix
> doesn't solve the reported hang. Do you mind testing my patch? My potato of a
> laptop unfortunately cannot run the test :(

Maybe I wasn't clear, sorry for that! I did take the patch for a spin,
and it did not solve this particular problem.

When we're taking a trap from *kernel*mode, we should never deal with
uprobes at all. Have a look at uprobe_pre_sstep_notifier(), this
function returns 1, which then means that the trap handler exit
premature.

The code you're referring to (called from uprobe_notify_resume()), and
will never be entered, because we're not exiting the trap to
userland. Have a look in kernel/entry/common.c (search for
e.g. TIF_UPROBE).

Now, for your concern, which I see as a potential different bug. Not at
all related to my issue "trap from kernelmode touches uprobe
incorrectly"; A "random" ebreak from *userland* is trapped, when uprobes
is enabled will set the kernel in a hang. I suggest you construct try to
write a simple program to reproduce this!

I had a quick look in the uprobe handling code, and AFAIU the was used
when installing the uprobe as an additional check, and when searching
for an active uprobe. I'm still a bit puzzled how the issue you're
describing could trigger. A reproducer would help!


Cheers,
Björn