Re: [PATCH -tip] x86/kprobes: Handle removed INT3 in do_int3()

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 25 Nov 2022 08:41:19 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Nov 25, 2022 at 10:09:02AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since x86 doesn't use stop_machine() to patch the kernel text,
> > there is a small chance that the another CPU removes the INT3
> > during do_int3(). In this case, if no INT3 notifier callbacks
> > handled that, the kernel calls die() because of a stray INT3.
> 
> Please clarify; how would that happen? Should not everybody modifying
> text take text_mutex ?

The text_mutex doesn't stop executing do_int3() since do_int3() is
an exception and must not be blocked. That mutex is only blocking
the other kernel text modifiers, not INT3 handling.

If there is only kprobe using INT3, this must not happen because
kprobe_int3_handler() always find a struct kprobe corresponding
to the INT3 address. (from this reason, the current code is wrong too)

However, if there are other INT3 callbacks (e.g. alternatives and
ftrace via text_poke_bp*()) managing the INT3, this can happen.
The possible scenario is here;

1. CPU0 hits an INT3 which is managed by text_poke_bp().

2. CPU1 removes the INT3 from the text and *start* calling
   text_poke_sync(). (note that text_poke_sync() will sync core to
    serialize pipeline, thus the memory and dcache already updated)

3. CPU0 calls kprobe_int3_handler(), but it can not find the
   corresponding kprobe from the address. Thus it reads the instruction
   at the address, and find it is not INT3 anymore.

4. CPU0's kprobe_int3_handler() sets the address to the regs->ip,
   and returns 1, which means "this INT3 is handled".

5. CPU0 returns from do_int3() and exit the exception, then it
   handles the do_sync_core() via IPI.

6. CPU1 finish the text_poke_sync().

This works fine, but it is not handled by the INT3 owner's callback.

Oh! maybe we don't need to handle remove INT3 case, because all
callback MUST handle its own INT3. This can be done simply using
text_poke_sync() because it use an IPI, and the IPI is not handled
until all running INT3 handlers exit.

OK, let me update the patch to just drop the removed INT3 handling
from kprobes.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>