public inbox for bpf@vger.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Jiri Olsa <olsajiri@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Ingo Molnar <mingo@redhat.com>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [RFC] ftrace: Add support to keep some functions out of ftrace
Date: Mon, 15 Aug 2022 11:32:40 -0400	[thread overview]
Message-ID: <20220815113240.71edf5cf@gandalf.local.home> (raw)
In-Reply-To: <CAADnVQKX5xJz5N_mVyf7wg4BT8Q2cNh8ze-SxTRfk6KtcFQ0=Q@mail.gmail.com>

On Mon, 15 Aug 2022 08:17:42 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> Ask yourself: should static_call patching logic go through
> ftrace infra ? No. Right?

I agree that static_call (and jump_labels) are not part of the ftrace
infrastructure (but ftrace was a strong motivator for those).

> static_call has nothing to do with ftrace (function tracing).

Besides the motivation, I agree.

> Same thing here. bpf dispatching logic is nothing to do with
> function tracing.

But it used fentry, which is part of function tracing. Which is what I'm
against. And why it broke ftrace.

> In this case bpf_dispatcher_xdp_func is a placeholder written C.
> If it was written in asm, fentry recording wouldn't have known about it.

And I would not have had an issue with that approach (for ftrace that is).
But that brings up other concerns (see below).

> And that's more or less what Jiri patch is doing.
> It's hiding a fake function from ftrace, since it's not a function
> and ftrace infra shouldn't show it tracing logs.
> In other words it's a _notrace_ function with nop5.

On the ftrace side, I'm perfectly happy with Jiri's approach (the one I
help extend).

But dynamic code modification is something we need to take very seriously.
It's very similar to writing your own locking primitives (which Linus
always says "Don't do"). It's complex and easy to get wrong. The more
dynamic code modifications we have, the less secure the kernel is.

Here's the list of dynamic code modification infrastructures:

 ftrace
 kprobes
 jump_labels
 static_calls

We now have the bpf dispatcher.

The ftrace, kprobes, jump_labels and static_calls developers work together
to make sure that we are all in line, not breaking anything, and try to
consolidate when possible. We also review each others code.

The issue I have is that BPF is largely doing it alone, and not
communicating with the others. This gives me cause for concern on both a
robustness and security point of view.

-- Steve

  parent reply	other threads:[~2022-08-15 15:33 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-22 11:08 [RFC] ftrace: Add support to keep some functions out of ftrace Jiri Olsa
2022-07-22 11:26 ` Steven Rostedt
2022-07-22 16:04   ` Alexei Starovoitov
2022-07-22 16:08     ` Steven Rostedt
2022-07-22 16:25       ` Steven Rostedt
2022-07-22 16:53         ` Alexei Starovoitov
2022-07-22 17:14           ` Steven Rostedt
2022-07-22 21:05         ` Jiri Olsa
2022-07-22 21:41           ` Steven Rostedt
2022-07-23  3:53             ` Steven Rostedt
2022-07-23  3:56               ` Steven Rostedt
2022-07-25  7:00                 ` Jiri Olsa
2022-07-23 21:39             ` Jiri Olsa
2022-08-12 21:18               ` Jiri Olsa
2022-08-12 21:50                 ` Steven Rostedt
2022-08-13 19:02                 ` Steven Rostedt
2022-08-14 11:32                   ` Steven Rostedt
2022-08-14 15:22                   ` Jiri Olsa
2022-08-15  2:07                     ` Chen Zhongjin
2022-08-15  8:04                       ` Jiri Olsa
2022-08-15 11:01                         ` Björn Töpel
2022-08-15 11:29                           ` Jiri Olsa
2022-08-15 12:19                             ` Björn Töpel
2022-08-15 12:30                               ` Björn Töpel
2022-08-15  8:03                   ` Peter Zijlstra
2022-08-15  9:44                     ` Jiri Olsa
2022-08-15 12:37                       ` Peter Zijlstra
2022-08-15 14:25                         ` Alexei Starovoitov
2022-08-15 14:33                           ` Peter Zijlstra
2022-08-15 14:45                             ` Alexei Starovoitov
2022-08-15 15:02                               ` Peter Zijlstra
2022-08-15 15:17                                 ` Alexei Starovoitov
2022-08-15 15:28                                   ` Peter Zijlstra
2022-08-15 15:35                                     ` Alexei Starovoitov
2022-08-15 15:44                                       ` Steven Rostedt
2022-08-15 15:53                                         ` Alexei Starovoitov
2022-08-15 16:13                                           ` Steven Rostedt
2022-08-15 15:48                                       ` Peter Zijlstra
2022-08-16  6:56                                         ` Jiri Olsa
2022-08-17  9:29                                           ` Jiri Olsa
2022-08-17 16:57                                             ` Alexei Starovoitov
2022-08-17 19:39                                               ` Jiri Olsa
2022-08-15 15:41                                     ` Peter Zijlstra
2022-08-15 15:49                                       ` Alexei Starovoitov
2022-08-15 16:08                                         ` Steven Rostedt
2022-08-18 20:27                                       ` Jiri Olsa
2022-08-18 20:50                                         ` Steven Rostedt
2022-08-18 21:00                                           ` Alexei Starovoitov
2022-08-18 21:05                                             ` Steven Rostedt
2022-08-18 21:32                                             ` Jiri Olsa
2022-08-19 11:45                                               ` Jiri Olsa
2022-08-23 17:23                                                 ` Alexei Starovoitov
2022-08-26  8:00                                                   ` Jiri Olsa
2022-08-18 21:14                                           ` Jiri Olsa
2022-08-15 15:32                                   ` Steven Rostedt [this message]
2022-07-22 16:26       ` Jiri Olsa
2022-07-22 16:37         ` Steven Rostedt
2022-07-22 16:54           ` Alexei Starovoitov

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=20220815113240.71edf5cf@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jpoimboe@redhat.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=olsajiri@gmail.com \
    --cc=peterz@infradead.org \
    --cc=sdf@google.com \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    /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