BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Nikolay Borisov <nborisov@suse.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
Date: Sat, 30 Jan 2021 07:44:10 -0500
Message-ID: <20210130074410.6384c2e2@oasis.local.home> (raw)
In-Reply-To: <YBUYsFlxjsQxuvfB@hirez.programming.kicks-ass.net>

On Sat, 30 Jan 2021 09:28:32 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jan 29, 2021 at 04:24:54PM -0500, Steven Rostedt wrote:
> > Specifically, kprobe and ftrace callbacks may have this:
> > 
> > 	if (in_nmi())
> > 		return;
> > 
> > 	raw_spin_lock_irqsave(&lock, flags);
> > 	[..]
> > 	raw_spin_unlock_irqrestore(&lock, flags);
> > 
> > Which is totally fine to have,  
> 
> Why? There's a distinct lack of explaining here.
> 
> Note that we ripped out all such dodgy locking from kretprobes.

Actually, I think you helped explain the distinction. You mention
"kretpobes" do you mean the infrastructure of kretprobes or all its
users?

The infrastructure of ftrace and kprobes can work in any context, it
does not mean that the callbacks must. Again, these are more like
exceptions. Why have "in_nmi()"? If anything that can be called by an
NMI should just work, right? That's basically your argument for having
ftrace and kprobes set "in_nmi".

You can have locking in NMIs if the locking is *only* in NMI handlers,
right? If that's the case, then so should ftrace and kprobe callbacks.

The stack tracer checks the size of the stack, compares it to the
largest recorded size, and if it's bigger, it will save the stack. But
if this happens on two CPUs at the same time, only one can do the
recording at the same time. To synchronize this, a spin lock must be
taken. Similar to spin locks in an NMI.

But the problem here is, the callbacks can also be done from an NMI
context, so if we are in NMI, we don't want to take any locks, and
simply don't record the stack traces from NMIs.

The more I think about it, the more I hate the idea that ftrace
callbacks and kprobes are considered NMIs. Simply because they are not!

-- Steve


  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <25cd2608-03c2-94b8-7760-9de9935fde64@suse.com>
     [not found] ` <20210128001353.66e7171b395473ef992d6991@kernel.org>
     [not found]   ` <20210128002452.a79714c236b69ab9acfa986c@kernel.org>
     [not found]     ` <a35a6f15-9ab1-917c-d443-23d3e78f2d73@suse.com>
     [not found]       ` <20210128103415.d90be51ec607bb6123b2843c@kernel.org>
2021-01-28  3:38         ` Masami Hiramatsu
2021-01-28  7:11           ` Nikolay Borisov
2021-01-28 16:12           ` Nikolay Borisov
2021-01-28 16:45             ` Nikolay Borisov
2021-01-28 16:50               ` Josh Poimboeuf
2021-01-28 21:52                 ` [PATCH] x86: Disable CET instrumentation in the kernel Josh Poimboeuf
2021-01-29  6:23                   ` Nikolay Borisov
2021-01-29 10:21                   ` Borislav Petkov
     [not found]                     ` <20210129151034.iba4eaa2fuxsipqa@treble>
2021-01-29 16:30                       ` Borislav Petkov
2021-01-29 16:49                         ` Josh Poimboeuf
2021-01-29 16:54                           ` Nikolay Borisov
2021-01-29 17:03                             ` Josh Poimboeuf
2021-01-29 17:07                               ` Borislav Petkov
2021-01-29 17:58                                 ` Seth Forshee
2021-01-28 18:24               ` kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()") Peter Zijlstra
2021-01-29  1:34                 ` Alexei Starovoitov
2021-01-29  6:36                   ` Nikolay Borisov
     [not found]                   ` <YBPNyRyrkzw2echi@hirez.programming.kicks-ass.net>
     [not found]                     ` <20210129224011.81bcdb3eba1227c414e69e1f@kernel.org>
     [not found]                       ` <20210129105952.74dc8464@gandalf.local.home>
2021-01-29 16:24                         ` Peter Zijlstra
2021-01-29 17:45                           ` Alexei Starovoitov
2021-01-29 17:59                             ` Peter Zijlstra
2021-01-29 19:01                               ` Steven Rostedt
2021-01-29 21:05                                 ` Alexei Starovoitov
2021-01-30  1:41                                   ` Masami Hiramatsu
2021-01-29 21:24                                 ` Steven Rostedt
2021-01-30  8:28                                   ` Peter Zijlstra
2021-01-30 12:44                                     ` Steven Rostedt [this message]
2021-02-02 10:45                                       ` Peter Zijlstra
2021-02-02 14:52                                         ` Steven Rostedt
2021-02-02 16:45                                           ` Peter Zijlstra
2021-02-02 16:56                                             ` Steven Rostedt
2021-02-02 18:30                                               ` Peter Zijlstra
2021-02-02 21:05                                                 ` Steven Rostedt
2021-02-03 13:33                                                   ` Masami Hiramatsu
2021-02-03 13:52                                                     ` Steven Rostedt
2021-01-30  2:02                               ` Masami Hiramatsu
2021-01-30  3:08                                 ` Alexei Starovoitov
2021-01-30 12:10                                   ` 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=20210130074410.6384c2e2@oasis.local.home \
    --to=rostedt@goodmis.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=nborisov@suse.com \
    --cc=peterz@infradead.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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

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

Example config snippet for mirrors

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


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