BPF List
 help / color / mirror / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: "Andrii Nakryiko" <andrii.nakryiko@gmail.com>,
	"Jiri Olsa" <jolsa@redhat.com>, "Jiri Olsa" <jolsa@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andriin@fb.com>,
	Networking <netdev@vger.kernel.org>, 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>, "Daniel Xu" <dxu@dxuuu.xyz>,
	"Jesper Brouer" <jbrouer@redhat.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Viktor Malik" <vmalik@redhat.com>
Subject: Re: [PATCHv2 RFC bpf-next 0/7] bpf: Add support for ftrace probe
Date: Mon, 19 Apr 2021 23:29:34 +0900	[thread overview]
Message-ID: <20210419232934.4bce1d424b7cd133d20c8be4@kernel.org> (raw)
In-Reply-To: <20210416124834.05862233@gandalf.local.home>

On Fri, 16 Apr 2021 12:48:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 17 Apr 2021 00:03:04 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > Anyway, IIRC, Masami wasn't sure that the full regs was ever needed for the
> > > return (who cares about the registers on return, except for the return
> > > value?)  
> > 
> > I think kretprobe and ftrace are for a bit different usage. kretprobe can be
> > used for something like debugger. In that case, accessing full regs stack
> > will be more preferrable. (BTW, what the not "full regs" means? Does that
> > save partial registers?)
> 
> When the REGS flag is not set in the ftrace_ops (where kprobes uses the
> REGS flags), the regs parameter is not a full set of regs, but holds just
> enough to get access to the parameters. This just happened to be what was
> saved in the mcount/fentry trampoline, anyway, because tracing the start of
> the program, you had to save the arguments before calling the trace code,
> otherwise you would corrupt the parameters of the function being traced.

Yes, if we trace the function as a blackbox, it is correct. It should trace
the parameter at the entry and trace result at the exit.

> I just tweaked it so that by default, the ftrace callbacks now have access
> to the saved regs (call ftrace_regs, to not let a callback get confused and
> think it has full regs when it does not).

Ah, I got it. kretprobe allows user to set a custom region in its instance
so that the user handler can store the parameter at entry point. Sometimes
such "saved regs" is not enough because if the parameter passed via
pointer, actual data can be changed.
Anyway, for the kprobe event, that can be integrated seemlessly. But for the
low-level kretprobe, I think if we integrate it, we should better to update
kretprobe handler interface.

> Now for the exit of a function, what does having the full pt_regs give you?

It may allow user to debug kernel function if the user thinks any suspicious
behavior does/doesn't come from the compiler issue. (I would like to recommend
them to use kprobe for that purpose, but there is kretprobe already ...)

> Besides the information to get the return value, the rest of the regs are
> pretty much meaningless. Is there any example that someone wants access to
> the regs at the end of a function besides getting the return value?

Yes, as far as we can confident that the code is not corrupted. But, for example,
if user would like to make sure the collee saved register (or some flags)
is correctly saved and restored, ensuring raw register access will be helpful.

(Yeah, but I know it is very rare case.)

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

  reply	other threads:[~2021-04-19 14:29 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13 12:15 [PATCHv2 RFC bpf-next 0/7] bpf: Add support for ftrace probe Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 1/7] bpf: Move bpf_prog_start/end functions to generic place Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 2/7] bpf: Add bpf_functions object Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 3/7] bpf: Add support to attach program to ftrace probe Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 4/7] libbpf: Add btf__find_by_pattern_kind function Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 5/7] libbpf: Add support to load and attach ftrace probe Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 6/7] selftests/bpf: Add ftrace probe to fentry test Jiri Olsa
2021-04-13 12:15 ` [PATCHv2 RFC bpf-next 7/7] selftests/bpf: Add ftrace probe test Jiri Olsa
2021-04-14  1:04 ` [PATCHv2 RFC bpf-next 0/7] bpf: Add support for ftrace probe Andrii Nakryiko
2021-04-14 12:19   ` Jiri Olsa
2021-04-14 22:46     ` Andrii Nakryiko
2021-04-15 14:00       ` Jiri Olsa
2021-04-15 15:10       ` Steven Rostedt
2021-04-15 17:39         ` Jiri Olsa
2021-04-15 18:18           ` Steven Rostedt
2021-04-15 18:21             ` Steven Rostedt
2021-04-15 21:49               ` Jiri Olsa
2021-04-15 23:30                 ` Steven Rostedt
2021-04-19 20:51                   ` Jiri Olsa
2021-04-19 22:00                     ` Steven Rostedt
2021-04-15 18:31             ` Yonghong Song
2021-04-15 20:45         ` Andrii Nakryiko
2021-04-15 21:00           ` Steven Rostedt
2021-04-16 15:03             ` Masami Hiramatsu
2021-04-16 16:48               ` Steven Rostedt
2021-04-19 14:29                 ` Masami Hiramatsu [this message]
2021-04-20 12:51                 ` Jiri Olsa
2021-04-20 15:33                   ` Alexei Starovoitov
2021-04-20 16:33                     ` Steven Rostedt
2021-04-20 16:52                     ` Jiri Olsa
2021-04-20 23:38                       ` Alexei Starovoitov
2021-04-21 13:40                         ` Jiri Olsa
2021-04-21 14:05                           ` Steven Rostedt
2021-04-21 18:52                             ` Andrii Nakryiko
2021-04-21 19:18                               ` Jiri Olsa
2021-04-22 14:24                                 ` Steven Rostedt
2021-04-21 21:37                             ` Jiri Olsa
2021-04-22  1:17                               ` Steven Rostedt
2021-04-20  4:51               ` Andrii Nakryiko

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=20210419232934.4bce1d424b7cd133d20c8be4@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dxu@dxuuu.xyz \
    --cc=jbrouer@redhat.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=jolsa@redhat.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=netdev@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=vmalik@redhat.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