From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
Peter Zijlstra <peterz@infradead.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Frederic Weisbecker <frederic@kernel.org>,
Joel Fernandes <joel@joelfernandes.org>,
Andy Lutomirski <luto@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
systemtap@sourceware.org, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users
Date: Fri, 30 Nov 2018 23:11:58 +0900 [thread overview]
Message-ID: <20181130231158.d095ee9483e38d964ba26741@kernel.org> (raw)
In-Reply-To: <20181129222435.26fad0ea@vmware.local.home>
On Thu, 29 Nov 2018 22:24:35 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 30 Nov 2018 11:26:58 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > On Thu, 29 Nov 2018 11:46:52 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > On Thu, 29 Nov 2018 23:29:27 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > > > One way to solve this is to also have a counter array that gets updated
> > > > > every time the index array gets updated. And save the counter to the
> > > > > shadow stack index as well. This way, we only call the return if the
> > > > > counter on the stack matches what's in the counter on the counter array
> > > > > for the index.
> > > >
> > > > Hmm, but we already know the current stack "header" entry when calling
> > > > handlers, don't we? I thought we just calcurate out from curr_ret_stack.
> > >
> > > Basically we have this:
> > >
> > > array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...
> > >
> > > On entry of function we do:
> > >
> > push header(including original ret_addr) onto ret_stack
>
> We can't put the ret_addr of the callback on the stack. What if that
> ret_addr is a module, and it gets unloaded? We must not call it.
But in that case, how can we recover the original addr on the kernel (real)
stack? I don't call the entry, but kretprobe handler will need the info
to record as a caller-address.
> >
> > > for (i = 0; i < array_entries; i++) {
> > > if (array[i]->entryfunc(...)) {
> > > push i onto ret_stack;
> > > }
> > > }
> > >
> > > On the return side, we do:
> > >
> > > idx = pop ret_stack;
> > >
> > > array[idx]->retfunc(...);
> >
> > So at this point we have the header on ret_stack, don't we? :)
> >
> > Anyway, I think we may provide an API for unwinder to find correct
> > original return address form ret_stack. That is OK for me.
>
> Yes. In fact, I have something that worked for that. I'll have to test
> it some more.
Great! I think it will be enough for kretprobe.
> > > > I need only sizeof(unsigned long). If the kretprobe user requires more,
> > > > it will be fall back to current method -- get an "instance" and store
> > > > its address to the entry :-)
> > >
> > > Awesome, then this shouldn't be too hard to implement.
> >
> > Oops, anyway I noticed that I must store a value on each area so that we can
> > identify which kretprobe is using that if there are several kretprobes on same
> > function. So, kretprobe implementation will be something like below.
> >
> > kretprobe_retfunc(trace, regs)
> > {
> > kp = get_kprobe(trace->func);
> >
> > if (private == to_kretprobe(kp)) // this is directly mapped to current kprobe
> > goto found_kretprobe;
> >
> > if (!list_empty(&kp->list)) { // we need to find from multiple kretprobes
> > list_for_each_entry(kp, &kp->list, list)
> > if (private == kp)
> > goto found_kretprobe;
> > }
> >
> > // Or this must be an instance
> > struct kretprobe_instance *ri = trace->private;
> > rp = ri->rp;
> > if (valid_kretprobe(rp))
> > rp->handler(ri, regs);
> > kretprobe_recycle_instance(ri);
> > goto out;
> >
> > found_kretprobe:
> > struct kretprobe_instance rii = {.rp = to_kretprobe(kp),
> > .ret_addr=trace->ret, .task = current}
> > rp->handler(&rii, regs);
> >
> > out:
> > return 0;
> > }
> >
> > I think we talked about pt_regs, which is redundant for return probe, so it should
> > be just a return value. (but how we pass it? trace->retval?)
>
> Yeah, we can add that.
OK, then I will start with making a fake pt_regs on stack and call handler,
which will be something like,
struct pt_regs regs = {};
regs_set_return_value(®s, trace->retval);
rp->handler(ri, ®s);
Thank you,
> > That is OK for ftrace (but the transition needs more code).
> > And I would like to ask ebpf and systemtap people that is OK since it will change
> > the kernel ABI.
>
> I agree.
>
> -- Steve
--
Masami Hiramatsu <mhiramat@kernel.org>
prev parent reply other threads:[~2018-11-30 14:12 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-22 1:27 [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 01/14] fgraph: Create a fgraph.c file to store function graph infrastructure Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 02/14] fgraph: Have set_graph_notrace only affect function_graph tracer Steven Rostedt
2018-11-23 0:01 ` Namhyung Kim
2018-11-23 17:37 ` Steven Rostedt
2018-11-24 5:49 ` Namhyung Kim
2018-11-24 18:41 ` Steven Rostedt
2018-11-26 4:54 ` Namhyung Kim
2018-11-22 1:27 ` [RFC][PATCH 03/14] arm64: function_graph: Remove use of FTRACE_NOTRACE_DEPTH Steven Rostedt
2018-11-27 19:31 ` Will Deacon
2018-11-27 19:50 ` Steven Rostedt
2018-12-05 21:50 ` [PATCH 03/14 v2] " Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 04/14] function_graph: Remove the " Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 05/14] ftrace: Create new ftrace-internal.h header Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 06/14] fgraph: Move function graph specific code into fgraph.c Steven Rostedt
2018-11-23 6:11 ` Joel Fernandes
2018-11-23 17:58 ` Steven Rostedt
2018-11-23 18:11 ` Steven Rostedt
2018-11-23 22:13 ` Joel Fernandes
2018-11-26 7:25 ` Masami Hiramatsu
2018-11-22 1:27 ` [RFC][PATCH 07/14] fgraph: Add new fgraph_ops structure to enable function graph hooks Steven Rostedt
2018-11-23 2:59 ` Joel Fernandes
2018-11-23 18:25 ` Steven Rostedt
2018-11-26 11:30 ` Masami Hiramatsu
2018-11-26 21:06 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 08/14] function_graph: Remove unused task_curr_ret_stack() Steven Rostedt
2018-11-26 7:40 ` Masami Hiramatsu
2018-11-26 21:26 ` Steven Rostedt
2018-11-26 10:02 ` Joey Pabalinas
2018-11-26 21:27 ` Steven Rostedt
2018-11-26 21:37 ` Joey Pabalinas
2018-11-22 1:27 ` [RFC][PATCH 09/14] function_graph: Move ftrace_graph_get_addr() to fgraph.c Steven Rostedt
2018-11-23 3:13 ` Joel Fernandes
2018-11-23 19:25 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 10/14] function_graph: Have profiler use new helper ftrace_graph_get_ret_stack() Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 11/14] function_graph: Convert ret_stack to a series of longs Steven Rostedt
2018-11-24 5:31 ` Joel Fernandes
2018-11-26 16:07 ` Masami Hiramatsu
2018-11-26 16:26 ` Steven Rostedt
2018-11-28 1:38 ` Joel Fernandes
2018-11-26 21:31 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 12/14] function_graph: Add an array structure that will allow multiple callbacks Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 13/14] function_graph: Allow multiple users to attach to function graph Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 14/14] function_graph: Allow for more than one callback to be registered Steven Rostedt
2018-11-22 10:08 ` [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users Peter Zijlstra
2018-11-22 12:46 ` Steven Rostedt
2018-11-22 13:42 ` Peter Zijlstra
2018-11-26 9:21 ` Masami Hiramatsu
2018-11-26 16:32 ` Steven Rostedt
2018-11-29 14:29 ` Masami Hiramatsu
2018-11-29 16:46 ` Steven Rostedt
2018-11-30 2:26 ` Masami Hiramatsu
2018-11-30 3:24 ` Steven Rostedt
2018-11-30 14:11 ` Masami Hiramatsu [this message]
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=20181130231158.d095ee9483e38d964ba26741@kernel.org \
--to=mhiramat@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=frederic@kernel.org \
--cc=joel@joelfernandes.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=systemtap@sourceware.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
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;
as well as URLs for NNTP newsgroup(s).