Re: [RFC] kretprobe: Prevent triggering kretprobe from within kprobe_flush_task

[Ziqian SUN (Zamir)]

On 4/10/20 4:13 AM, Jiri Olsa wrote:
> On Thu, Apr 09, 2020 at 08:45:01PM +0200, Jiri Olsa wrote:
> > On Thu, Apr 09, 2020 at 11:41:01PM +0900, Masami Hiramatsu wrote:
> >
> > SNIP
> >
> > > > ---
> > > >   kernel/kprobes.c | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > index 2625c241ac00..b13247cae752 100644
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -1236,6 +1236,10 @@ __releases(hlist_lock)
> > > >   }
> > > >   NOKPROBE_SYMBOL(kretprobe_table_unlock);
> > > >   
> > > > +static struct kprobe kretprobe_dummy = {
> > > > +        .addr = (void *)kretprobe_trampoline,
> > > > +};
> > > > +
> > > >   /*
> > > >    * This function is called from finish_task_switch when task tk becomes dead,
> > > >    * so that we can recycle any function-return probe instances associated
> > > > @@ -1256,12 +1260,14 @@ void kprobe_flush_task(struct task_struct *tk)
> > > >   	INIT_HLIST_HEAD(&empty_rp);
> > > >   	hash = hash_ptr(tk, KPROBE_HASH_BITS);
> > > >   	head = &kretprobe_inst_table[hash];
> > > > +	__this_cpu_write(current_kprobe, &kretprobe_dummy);
> > >
> > > Can you also set the kcb->kprobe_state = KPROBE_HIT_ACTIVE?
> > >
> > > BTW, we may be better to introduce a common kprobe_reject_section_start()
> > > and kprobe_reject_section_end() so that the user don't need to prepare
> > > dummy kprobes.
> >
> > sure, will do
> >
> > thank you both for review
>
> ok, found out it's actually arch code..  would you guys be ok with something like below?
>
> jirka

Hi Jiri,

In my origin test lockup happens on both x86_64 and ppc64le. So I would 
appreciate if you can also come up with a solution for both of the 
architectures.

> ---
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 4d7022a740ab..081d0f366c99 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -757,12 +757,33 @@ static struct kprobe kretprobe_kprobe = {
>   	.addr = (void *)kretprobe_trampoline,
>   };
>   
> +void arch_kprobe_reject_section_start(void)
> +{
> +	struct kprobe_ctlblk *kcb;
> +
> +	preempt_disable();
> +
> +	/*
> +	 * Set a dummy kprobe for avoiding kretprobe recursion.
> +	 * Since kretprobe never run in kprobe handler, kprobe must not
> +	 * be running behind this point.
> +	 */
> +	__this_cpu_write(current_kprobe, &kretprobe_kprobe);
> +	kcb = get_kprobe_ctlblk();
> +	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> +}
> +
> +void arch_kprobe_reject_section_end(void)
> +{
> +	__this_cpu_write(current_kprobe, NULL);
> +	preempt_enable();
> +}
> +
>   /*
>    * Called from kretprobe_trampoline
>    */
>   __used __visible 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;
> @@ -772,16 +793,7 @@ __used __visible 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);
> -	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> +	arch_kprobe_reject_section_start();
>   
>   	INIT_HLIST_HEAD(&empty_rp);
>   	kretprobe_hash_lock(current, &head, &flags);
> @@ -873,8 +885,7 @@ __used __visible void *trampoline_handler(struct pt_regs *regs)
>   
>   	kretprobe_hash_unlock(current, &flags);
>   
> -	__this_cpu_write(current_kprobe, NULL);
> -	preempt_enable();
> +	arch_kprobe_reject_section_end();
>   
>   	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
>   		hlist_del(&ri->hlist);
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2625c241ac00..935dd8c87705 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1236,6 +1236,14 @@ __releases(hlist_lock)
>   }
>   NOKPROBE_SYMBOL(kretprobe_table_unlock);
>   
> +void __weak arch_kprobe_reject_section_start(void)
> +{
> +}
> +
> +void __weak arch_kprobe_reject_section_end(void)
> +{
> +}
> +
>   /*
>    * This function is called from finish_task_switch when task tk becomes dead,
>    * so that we can recycle any function-return probe instances associated
> @@ -1253,6 +1261,8 @@ void kprobe_flush_task(struct task_struct *tk)
>   		/* Early boot.  kretprobe_table_locks not yet initialized. */
>   		return;
>   
> +	arch_kprobe_reject_section_start();
> +
>   	INIT_HLIST_HEAD(&empty_rp);
>   	hash = hash_ptr(tk, KPROBE_HASH_BITS);
>   	head = &kretprobe_inst_table[hash];
> @@ -1266,6 +1276,8 @@ void kprobe_flush_task(struct task_struct *tk)
>   		hlist_del(&ri->hlist);
>   		kfree(ri);
>   	}
> +
> +	arch_kprobe_reject_section_end();
>   }
>   NOKPROBE_SYMBOL(kprobe_flush_task);
>   

--
Ziqian SUN (Zamir)
9F Raycom office (NAY)
Red Hat Software (Beijing) Co.,Ltd
IRC: zsun (internal and freenode)
Tel: +86 10 65627458
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E