BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Nikolay Borisov <nborisov@suse.com>,
	Masami Hiramatsu <masami.hiramatsu@gmail.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	bpf@vger.kernel.org, Josh Poimboeuf <jpoimboe@redhat.com>,
	Steven Rostedt <rostedt@goodmis.org>
Subject: Re: kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
Date: Thu, 28 Jan 2021 17:34:52 -0800
Message-ID: <20210129013452.njuh3fomws62m4rc@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <YBMBTsY1uuQb9wCP@hirez.programming.kicks-ass.net>

On Thu, Jan 28, 2021 at 07:24:14PM +0100, Peter Zijlstra wrote:
> On Thu, Jan 28, 2021 at 06:45:56PM +0200, Nikolay Borisov wrote:
> > it would be placed on the __fentry__ (and not endbr64) hence it works.
> > So perhaps a workaround outside of bpf could essentially detect this
> > scenario and adjust the probe to be on the __fentry__ and not preceding
> > instruction if it's detected to be endbr64 ?
> 
> Arguably the fentry handler should also set the nmi context, it can,
> after all, interrupt pretty much any other context by construction.

But that doesn't make it a real nmi.
nmi can actually interrupt anything. Whereas kprobe via int3 cannot
do nokprobe and noinstr sections. The exposure is a lot different.
ftrace is even more contained. It's only at the start of the functions.
It's even smaller subset of places than kprobes.
So ftrace < kprobe < nmi.
Grouping them all into nmi doesn't make sense to me.
That bpf breaking change came unnoticed mostly because people use
kprobes in the beginning of the functions only, but there are cases
where kprobes are in the middle too. People just didn't upgrade kernels
fast enough to notice.
imo appropriate solution would be to have some distinction between
ftrace < kprobe_via_int3 < nmi, so that bpf side can react accordingly.
That code in trace_call_bpf:
  if (in_nmi()) /* not supported yet */
was necessary in the past. Now we have all sorts of protections built in.
So it's probably ok to just drop it, but I think adding
called_via_ftrace vs called_via_kprobe_int3 vs called_via_nmi
is more appropriate solution long term.

  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 [this message]
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
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=20210129013452.njuh3fomws62m4rc@ast-mbp.dhcp.thefacebook.com \
    --to=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=masami.hiramatsu@gmail.com \
    --cc=mhiramat@kernel.org \
    --cc=nborisov@suse.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.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