From: Masami Hiramatsu <mhiramat@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Eddy_Wu@trendmicro.com" <Eddy_Wu@trendmicro.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"x86@kernel.org" <x86@kernel.org>,
"David S. Miller" <davem@davemloft.net>
Subject: Re: x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint)
Date: Tue, 25 Aug 2020 03:15:03 +0900 [thread overview]
Message-ID: <20200825031503.57d08355bf44900af7d70536@kernel.org> (raw)
In-Reply-To: <20200824141429.GA3982@worktop.programming.kicks-ass.net>
Hi Peter,
On Mon, 24 Aug 2020 16:14:29 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Aug 24, 2020 at 12:02:58PM +0000, Eddy_Wu@trendmicro.com wrote:
> > 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.
> >
> > (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. 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?
>
> Masami, would it be possible to have a kretprobe specific recursion
> count here?
Hmm, good point. As I commented in f96f56780ca5 ("kprobes: Skip kretprobe
hit in NMI context to avoid deadlock"), this check is for avoiding the
deadlock with kretprobe_table_lock which is used in pre_handler *and*
kretprobe trampoline handler.
>
> I did the below, but i'm not at all sure that isn't horrible broken. I
> can't really find many rp->lock sites and this might break things by
> limiting contention.
This is not enough. For checking the recursion of kretprobes, we might
need kretprobe_table_trylock() or kretprobe_table_busy() (but both
can be false positive)
Note that rp->lock shouldn't matter unless we will support recursive
kprobe itself. (even though, we can use raw_spin_trylock_irqsave())
Thank you,
>
> ---
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 9be1bff4f586..0bff314cc800 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -153,6 +153,7 @@ struct kretprobe {
> size_t data_size;
> struct hlist_head free_instances;
> raw_spinlock_t lock;
> + atomic_t recursion;
> };
>
> struct kretprobe_instance {
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 287b263c9cb9..27fd096bcb9a 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1934,22 +1934,17 @@ unsigned long __weak arch_deref_entry_point(void *entry)
> static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> {
> struct kretprobe *rp = container_of(p, struct kretprobe, kp);
> - unsigned long hash, flags = 0;
> struct kretprobe_instance *ri;
> -
> - /*
> - * To avoid deadlocks, prohibit return probing in NMI contexts,
> - * just skip the probe and increase the (inexact) 'nmissed'
> - * statistical counter, so that the user is informed that
> - * something happened:
> - */
> - if (unlikely(in_nmi())) {
> - rp->nmissed++;
> - return 0;
> - }
> + unsigned long hash, flags;
> + int rec;
>
> /* TODO: consider to only swap the RA after the last pre_handler fired */
> hash = hash_ptr(current, KPROBE_HASH_BITS);
> + rec = atomic_fetch_inc_acquire(&rp->recursion);
> + if (rec) {
> + rp->nmissed++;
> + goto out;
> + }
> raw_spin_lock_irqsave(&rp->lock, flags);
> if (!hlist_empty(&rp->free_instances)) {
> ri = hlist_entry(rp->free_instances.first,
> @@ -1964,7 +1959,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> raw_spin_lock_irqsave(&rp->lock, flags);
> hlist_add_head(&ri->hlist, &rp->free_instances);
> raw_spin_unlock_irqrestore(&rp->lock, flags);
> - return 0;
> + goto out;
> }
>
> arch_prepare_kretprobe(ri, regs);
> @@ -1978,6 +1973,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> rp->nmissed++;
> raw_spin_unlock_irqrestore(&rp->lock, flags);
> }
> +out:
> + atomic_dec(&rp->recursion);
> return 0;
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
--
Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2020-08-24 18:15 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-24 12:02 x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint) Eddy_Wu
2020-08-24 14:14 ` Peter Zijlstra
2020-08-24 16:18 ` Eddy_Wu
2020-08-24 18:15 ` Masami Hiramatsu [this message]
2020-08-25 7:36 ` peterz
2020-08-24 15:54 ` Masami Hiramatsu
2020-08-24 16:41 ` Eddy_Wu
2020-08-25 6:15 ` Masami Hiramatsu
2020-08-25 8:33 ` Eddy_Wu
2020-08-25 11:06 ` [PATCH] kprobes/x86: Fixes NMI context check on x86 kernel test robot
2020-08-25 12:09 ` x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint) peterz
2020-08-25 13:15 ` Masami Hiramatsu
2020-08-25 13:30 ` peterz
2020-08-25 13:59 ` Masami Hiramatsu
2020-08-25 14:15 ` peterz
2020-08-25 14:10 ` peterz
2020-08-25 14:19 ` Masami Hiramatsu
2020-08-27 9:02 ` peterz
2020-08-26 7:07 ` Eddy_Wu
2020-08-26 8:22 ` Masami Hiramatsu
2020-08-26 9:06 ` Masami Hiramatsu
2020-08-26 10:00 ` Masami Hiramatsu
2020-08-26 10:25 ` peterz
2020-08-26 13:36 ` Eddy_Wu
2020-08-26 13:51 ` Masami Hiramatsu
2020-08-26 9:01 ` peterz
2020-08-26 9:21 ` peterz
2020-08-26 8:31 ` Masami Hiramatsu
2020-08-25 12:20 ` [PATCH] kprobes/x86: Fixes NMI context check on x86 kernel test robot
2020-08-25 12:25 ` kernel test robot
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=20200825031503.57d08355bf44900af7d70536@kernel.org \
--to=mhiramat@kernel.org \
--cc=Eddy_Wu@trendmicro.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=x86@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).