From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>,
peterz@infradead.org,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Andrea Righi <righi.andrea@gmail.com>,
stable@vger.kernel.org
Subject: Re: [PATCH v2 3/3] x86/kprobes: Fix to avoid kretprobe recursion
Date: Thu, 10 Jan 2019 09:11:27 +0900 [thread overview]
Message-ID: <20190110091127.17e902c318578b192051eeb0@kernel.org> (raw)
In-Reply-To: <20190109110816.72a7ae85@gandalf.local.home>
On Wed, 9 Jan 2019 11:08:16 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 8 Jan 2019 13:45:22 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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@kernel.org>
> > Reported-by: Andrea Righi <righi.andrea@gmail.com>
> > Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
> > Cc: stable@vger.kernel.org
> > ---
> > 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@kernel.org>
next prev parent reply other threads:[~2019-01-10 0:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-08 4:43 [PATCH v2 0/3] kprobes: Fix kretprobe issues Masami Hiramatsu
2019-01-08 4:44 ` [PATCH v2 1/3] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu
2019-01-08 4:44 ` [PATCH v2 2/3] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu
2019-01-09 14:31 ` Steven Rostedt
2019-01-08 4:45 ` [PATCH v2 3/3] x86/kprobes: Fix to avoid kretprobe recursion Masami Hiramatsu
2019-01-09 16:08 ` Steven Rostedt
2019-01-10 0:11 ` Masami Hiramatsu [this message]
2019-01-08 10:31 ` [PATCH v2 0/3] kprobes: Fix kretprobe issues Andrea Righi
2019-01-09 4:37 ` Masami Hiramatsu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190110091127.17e902c318578b192051eeb0@kernel.org \
--to=mhiramat@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=righi.andrea@gmail.com \
--cc=rostedt@goodmis.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).