Re: [PATCH v2 3/3] x86/kprobes: Fix to avoid kretprobe recursion

[Masami Hiramatsu]

On Wed, 9 Jan 2019 11:08:16 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> On Tue,  8 Jan 2019 13:45:22 +0900
> Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> 
> > Fix to avoid kretprobe recursion loop by setting a dummy
> > kprobes to current_kprobe per-cpu variable.
> > 
> > This bug has been introduced with the asm-coded trampoline
> > code, since previously it used another kprobe for hooking
> > the function return placeholder (which only has a nop) and
> > trampoline handler was called from that kprobe.
> > 
> > This revives the old lost kprobe again.
> > 
> > With this fix, we don't see deadlock anymore.
> > 
> > # echo "r:event_1 __fdget" >> kprobe_events
> > # echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> > # echo 1 > events/kprobes/enable
> > 
> > And you can see that all inner-called kretprobe are skipped.
> > 
> > # cat kprobe_profile
> >   event_1                                  235               0
> >   event_2                                19375           19612
> > 
> > The 1st column is recorded count and the 2nd is missed count.
> > Above shows (event_1 rec) + (event_2 rec) ~= (event_2 missed)
> 
> I don't quite understand the above. Is the miss count because we missed
> event_2 events for both event_1 and event_2?
> 
>  trace raw_spin_lock()
>     handler calls raw_spin_lock()
>           trace raw_spin_lock() [ skip ]

Yes, both events(kretprobe) eventually hits event_2 in trampoline_handler()'s
spin_lock_irqsave().

> 
> I'm also guessing that the 2 extra (19612 - (235 + 19375) = 2) are
> possibly due to the displaying being racy?

Yes, it can be racy.

Thank you,

> 
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > Reported-by: Andrea Righi <righi.andrea@xxxxxxxxx>
> > Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  arch/x86/kernel/kprobes/core.c |   22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 69b6400d1ce2..f4b954ff5b89 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -749,11 +749,16 @@ asm(
> >  NOKPROBE_SYMBOL(kretprobe_trampoline);
> >  STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> >  
> > +static struct kprobe kretprobe_kprobe = {
> > +	.addr = (void *)kretprobe_trampoline,
> > +};
> > +
> >  /*
> >   * Called from kretprobe_trampoline
> >   */
> >  static __used void *trampoline_handler(struct pt_regs *regs)
> >  {
> > +	struct kprobe_ctlblk *kcb;
> >  	struct kretprobe_instance *ri = NULL;
> >  	struct hlist_head *head, empty_rp;
> >  	struct hlist_node *tmp;
> > @@ -763,6 +768,17 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> >  	void *frame_pointer;
> >  	bool skipped = false;
> >  
> > +	preempt_disable();
> > +
> > +	/*
> > +	 * Set a dummy kprobe for avoiding kretprobe recursion.
> > +	 * Since kretprobe never run in kprobe handler, kprobe must not
> > +	 * be running at this point.
> > +	 */
> > +	kcb = get_kprobe_ctlblk();
> > +	__this_cpu_write(current_kprobe, &kretprobe_kprobe);
> 
> If an interrupt comes in here, is this still safe, if the interrupt
> handler has a kretprobe too?
> 
> -- Steve
> 
> > +	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > +
> >  	INIT_HLIST_HEAD(&empty_rp);
> >  	kretprobe_hash_lock(current, &head, &flags);
> >  	/* fixup registers */
> > @@ -838,10 +854,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> >  		orig_ret_address = (unsigned long)ri->ret_addr;
> >  		if (ri->rp && ri->rp->handler) {
> >  			__this_cpu_write(current_kprobe, &ri->rp->kp);
> > -			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> >  			ri->ret_addr = correct_ret_addr;
> >  			ri->rp->handler(ri, regs);
> > -			__this_cpu_write(current_kprobe, NULL);
> > +			__this_cpu_write(current_kprobe, &kretprobe_kprobe);
> >  		}
> >  
> >  		recycle_rp_inst(ri, &empty_rp);
> > @@ -857,6 +872,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> >  
> >  	kretprobe_hash_unlock(current, &flags);
> >  
> > +	__this_cpu_write(current_kprobe, NULL);
> > +	preempt_enable();
> > +
> >  	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> >  		hlist_del(&ri->hlist);
> >  		kfree(ri);
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>