Re: [PATCH -tip v3 04/10] x86/kprobes: Prohibit probing on IRQ handlers directly

Andrea Righi
2019-03-26T15:17:20Z

On Tue, Mar 26, 2019 at 11:50:52PM +0900, Masami Hiramatsu wrote:
> On Mon, 25 Mar 2019 17:23:34 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Wed, 13 Feb 2019 01:12:44 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > > Prohibit probing on IRQ handlers in irqentry_text because
> > > if it interrupts user mode, at that point we haven't changed
> > > to kernel space yet and which eventually leads a double fault.
> > > E.g.
> > >
> > > # echo p apic_timer_interrupt > kprobe_events
> >
> > Hmm, this breaks one of my tests (which I probe on do_IRQ).
>
> OK, it seems this patch is a bit redundant, because
> I found that these interrupt handler issue has been fixed
> by Andrea's commit before merge this patch.
>
> commit a50480cb6d61d5c5fc13308479407b628b6bc1c5
> Author: Andrea Righi <righi.andrea@gmail.com>
> Date: Thu Dec 6 10:56:48 2018 +0100
>
> kprobes/x86: Blacklist non-attachable interrupt functions
>
> These interrupt functions are already non-attachable by kprobes.
> Blacklist them explicitly so that they can show up in
> /sys/kernel/debug/kprobes/blacklist and tools like BCC can use this
> additional information.
>
> This description is a bit odd (maybe his patch is after mine?) I think
> while updating this series, the patches were merged out of order.
> Anyway, with above patch, the core problematic probe points are blacklisted.

This is the previous thread when I posted my patch (not sure if it helps
to figure out what happened - maybe it was just an out of order merge
issue, like you said):

https://lkml.org/lkml/2018/12/6/212

>
> >
> > It's been working for years.
> >
> >
> > > # echo 1 > events/kprobes/enable
> > > PANIC: double fault, error_code: 0x0
> > > CPU: 1 PID: 814 Comm: less Not tainted 4.20.0-rc3+ #30
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> > > RIP: 0010:error_entry+0x12/0xf0
> > > [snip]
> > > Call Trace:
> > > <ENTRY_TRAMPOLINE>
> > > ? native_iret+0x7/0x7
> > > ? async_page_fault+0x8/0x30
> > > ? trace_hardirqs_on_thunk+0x1c/0x1c
> > > ? error_entry+0x7c/0xf0
> > > ? async_page_fault+0x8/0x30
> > > ? native_iret+0x7/0x7
> > > ? int3+0xa/0x20
> > > ? trace_hardirqs_on_thunk+0x1c/0x1c
> > > ? error_entry+0x7c/0xf0
> > > ? int3+0xa/0x20
> > > ? apic_timer_interrupt+0x1/0x20
> > > </ENTRY_TRAMPOLINE>
> > > Kernel panic - not syncing: Machine halted.
> > > Kernel Offset: disabled
> >
> > I'm not able to reproduce this (by removing this commit).
>
> I ensured that if I revert both of this patch and Andrea's patch,
> I can reproduce this with probing on apic_timer_interrupt().
>
> > I'm thinking something else may have changed, as I've been tracing
> > interrupt entries for years, and interrupting userspace while doing
> > this.
> >
> > I've even added probes where ftrace isn't (where it uses an int3) and
> > still haven't hit a problem.
> >
> > I think this patch is swatting a symptom of a bug and not addressing
> > the bug itself. Can you send me the config that triggers this?
>
> Yes, it seems you're right. Andrea's commit specifically fixed the
> issue and mine is redundant. (I'm not sure why do_IRQ is in
> __irqentry_text...)

Not sure if there are specific reasons for that, but do_IRQ is part of
__irqentry_text because it's explicitly marked with __irq_entry.

>
> So, Ingo, please revert this, since this bug already has been fixed by
> commit a50480cb6d61 ("kprobes: x86_64: blacklist non-attachable interrupt
> functions")
>
> BTW, for further error investigation, I attached my kconfig which is
> usually I'm testing (some options can be changed) on Qemu.
> I'm using my mini-container shellscript ( https://github.com/mhiramat/mincs
> ) which supports qemu-container.
>
>
> Thank you,
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

Thanks,
-Andrea