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

[Andrea Righi]

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@xxxxxxxxxxx> wrote:
> 
> > On Wed, 13 Feb 2019 01:12:44 +0900
> > Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxxx>

Thanks,
-Andrea