Re: x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint)

[Masami Hiramatsu]

Hi Eddy,

On Mon, 24 Aug 2020 16:41:58 +0000
"Eddy_Wu@xxxxxxxxxxxxxx" <Eddy_Wu@xxxxxxxxxxxxxx> wrote:

> > -----Original Message-----
> > From: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > Sent: Monday, August 24, 2020 11:54 PM
> > To: Eddy Wu (RD-TW) <Eddy_Wu@xxxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; David S. Miller <davem@xxxxxxxxxxxxx>
> > Subject: Re: x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint)
> >
> >
> > This message was sent from outside of Trend Micro. Please do not click links or open attachments unless you recognise the source of this
> > email and know the content is safe.
> >
> >
> > On Mon, 24 Aug 2020 12:02:58 +0000
> > "Eddy_Wu@xxxxxxxxxxxxxx" <Eddy_Wu@xxxxxxxxxxxxxx> wrote:
> >
> > > Greetings!
> > >
> > > Starting from kernel 5.8 (x86_64), kretprobe handler will always missed if corresponding kprobe on function entry is not optimized
> > (using break point instead).
> >
> > Oops, good catch. I always enabled ftrace hook for kretprobe, I didn't noticed that.
> >
> > > Step to reproduce this:
> > > 1) Build the kretprobe example module (CONFIG_SAMPLE_KRETPROBES=m)
> > > 2) Disable jump optimization (`sysctl debug.kprobes-optimization=0` or register any kprobe.post_handler at same location)
> > > 3) Insert the kretprobe_example module
> > > 4) Launch some process to trigger _do_fork
> > > 5) Remove kretprobe_example module
> > > 6) dmesg shows that all probing instances are missed
> > >
> > > Example output:
> > > # sysctl debug.kprobes-optimization=0
> > > debug.kprobes-optimization = 0
> > > # insmod samples/kprobes/kretprobe_example.ko
> > > # ls > /dev/null
> > > # rmmod kretprobe_example
> > > # dmesg
> > > [48555.067295] Planted return probe at _do_fork: 0000000038ae0211
> > > [48560.229459] kretprobe at 0000000038ae0211 unregistered
> > > [48560.229460] Missed probing 3 instances of _do_fork
> > >
> > > After bisecting, I found this behavior seems to introduce by this commit: (5.8-rc1)
> > > 0d00449c7a28a1514595630735df383dec606812 x86: Replace ist_enter() with nmi_enter()
> > > This make kprobe_int3_handler() effectively running as NMI context, which pre_handler_kretprobe() explicitly checked to prevent
> > recursion.
> >
> > Thanks for the bisecting!
> >
> > >
> > > (in_nmi() check appears from v3.17)
> > > f96f56780ca584930bb3a2769d73fd9a101bcbbe kprobes: Skip kretprobe hit in NMI context to avoid deadlock
> > >
> > > To make kretprobe work again with int3 breakpoint, I think we can replace the in_nmi() check with in_nmi() == (1 << NMI_SHIFT) at
> > kprobe_int3_handler() and skip kretprobe if nested NMI.
> >
> > Ah, I see. Now int3 is a kind of NMI, so in the handler in_nmi() always returns !0.
> >
> > > Did a quick test on 5.9-rc2 and it seems to be working.
> > > I'm not sure if it is the best way to do since it may also require change to other architecture as well, any thought?
> >
> > Hmm, this behavior is arch-dependent. So I think we need an weak function like this.
> >
> > @kernel/kprobes.c
> >
> > bool __weak arch_kprobe_in_nmi(void)
> > {
> >         return in_nmi()
> > }
> >
> > @arch/x86/kernel/kprobes/core.c
> >
> > bool arch_kprobe_in_nmi(void)
> > {
> >        /*
> >         * Since the int3 is one of NMI, we have to check in_nmi() is
> >         * bigger than 1 << NMI_SHIFT instead of !0.
> >         */
> >        return in_nmi() > (1 << NMI_SHIFT);
> > }
> >
> > And use arch_kprobe_in_nmi() instead of in_nmi() in kprobes.c.
> >
> > Thanks,
> >
> > --
> > Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> 
> Kretprobe might still trigger from NMI with nmi counter == 1 (if entry kprobe is jump-optimized).

Ah, right. Hmm, in that case, we can store the int3 status in 
the kprobe_ctlblk and refer it in the handler.


> The arch- dependent weak function looks cleaner than doing this in kprobe_int3_handler() under x86/, but I don't know if there is a way to check if called by specific int3 handler or not.
> 
> My original patch below, need to change all architecture support kretprobe though

OK, here is my fix. This will not change the other arches. please try it.