LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Ingo Molnar <mingo@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
	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, Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH -tip v3 0/3] kprobes: Fix kretprobe issues
Date: Fri, 22 Mar 2019 15:37:31 -0400
Message-ID: <20190322153731.27065002@gandalf.local.home> (raw)
In-Reply-To: <155094056180.6137.4208463927265038015.stgit@devbox>


Hi Ingo,

I was looking through at urgent patches that need to go upstream (as
I'm working on a couple of patches now), and saw this series. It
doesn't look like it was applied yet.

Do you want to take this, or would you want me to? It touches arch/x86,
so I'll take it if one of the x86 maintainers acks it. Or you can take
it as I already acked the generic patch.

-- Steve


On Sun, 24 Feb 2019 01:49:22 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Ingo,
> 
> Here is v3 series of fixing kretprobe incorrect stacking order patches.
> 
> I found that this series did not merged yet, so I just ported it
> on the tip tree.
> 
> In this version, I just added Steve's Ack and sort tags.
> Please push it as an urgent fix.
> 
> (1) kprobe incorrct stacking order problem
> 
> On recent talk with Andrea, I started more precise investigation on
> the kernel panic with kretprobes on notrace functions, which Francis
> had been reported last year ( https://lkml.org/lkml/2017/7/14/466 ).
> 
> See the investigation details in 
> https://lkml.kernel.org/r/154686789378.15479.2886543882215785247.stgit@devbox
> 
> When we put a kretprobe on ftrace_ops_assist_func() and put another
> kretprobe on probed-function, below happens
> 
> <caller>
>  -><probed-function>
>    ->fentry
>     ->ftrace_ops_assist_func()
>      ->int3
>       ->kprobe_int3_handler()  
>       ...->pre_handler_kretprobe()
>            push the return address (*fentry*) of ftrace_ops_assist_func() to
>            top of the kretprobe list and replace it with kretprobe_trampoline.
>       <-kprobe_int3_handler()
>      <-(int3)
>      ->kprobe_ftrace_handler()  
>       ...->pre_handler_kretprobe()
>            push the return address (caller) of probed-function to top of the
>            kretprobe list and replace it with kretprobe_trampoline.
>      <-(kprobe_ftrace_handler())
>     <-(ftrace_ops_assist_func())
>     [kretprobe_trampoline]
>      ->tampoline_handler()  
>        pop the return address (caller) from top of the kretprobe list
>      <-(trampoline_handler())
>     <caller>
>     [run caller with incorrect stack information]
>    <-(<caller>)
>   !!KERNEL PANIC!!
> 
> Therefore, this kernel panic happens only when we put 2 k*ret*probes on
> ftrace_ops_assist_func() and other functions. If we put kprobes, it
> doesn't cause any issue, since it doesn't change the return address.
> 
> To fix (or just avoid) this issue, we can introduce a frame pointer
> verification to skip wrong order entries. And I also would like to
> blacklist those functions because those are part of ftrace-based 
> kprobe handling routine.
> 
> (2) kretprobe trampoline recursion problem
> 
> This was found by Andrea in the previous thread
> https://lkml.kernel.org/r/20190107183444.GA5966@xps-13
> 
> ----
>  echo "r:event_1 __fdget" >> kprobe_events
>  echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
>  echo 1 > events/kprobes/enable
>  [DEADLOCK]
> ----
> 
> Because kretprobe trampoline_handler uses spinlock for protecting
> hash table, if we probe the spinlock itself, it causes deadlock.
> Thank you Andrea and Steve for discovering this root cause!!
> 
> 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.
> 
> To fix this bug, I introduced a dummy kprobe and set it in
> current_kprobe as we did in old days.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       x86/kprobes: Verify stack frame on kretprobe
>       kprobes: Mark ftrace mcount handler functions nokprobe
>       x86/kprobes: Fix to avoid kretprobe recursion
> 
> 
>  arch/x86/kernel/kprobes/core.c |   48 ++++++++++++++++++++++++++++++++++++++--
>  include/linux/kprobes.h        |    1 +
>  kernel/trace/ftrace.c          |    6 ++++-
>  3 files changed, 52 insertions(+), 3 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


      parent reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-23 16:49 Masami Hiramatsu
2019-02-23 16:49 ` [PATCH -tip v3 1/3] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu
2019-04-19 12:52   ` [tip:perf/urgent] " tip-bot for Masami Hiramatsu
2019-02-23 16:50 ` [PATCH -tip v3 2/3] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu
2019-04-19 12:53   ` [tip:perf/urgent] " tip-bot for Masami Hiramatsu
2019-02-23 16:50 ` [PATCH -tip v3 3/3] x86/kprobes: Fix to avoid kretprobe recursion Masami Hiramatsu
2019-04-19 12:54   ` [tip:perf/urgent] x86/kprobes: Avoid kretprobe recursion bug tip-bot for Masami Hiramatsu
2019-03-22 19:37 ` Steven Rostedt [this message]

Reply instructions:

You may reply publically 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=20190322153731.27065002@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=righi.andrea@gmail.com \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git