Re: [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users

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>
Subject: Re: [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users
Date: Thu, 29 Nov 2018 23:29:27 +0900	[thread overview]
Message-ID: <20181129232927.74ca5f294e97fc58b15bf8c6@kernel.org> (raw)
In-Reply-To: <20181126113215.4259d473@gandalf.local.home>

Hi Steve,

On Mon, 26 Nov 2018 11:32:15 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 26 Nov 2018 18:21:12 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> 
> > > Note, if another fgraph_ops is registered in the same location, its
> > > retfunc may be called that was set by a previous fgraph_ops. This
> > > is not a regression because that's what can happen today if you unregister
> > > a callback from the current function_graph tracer and register another
> > > one. If this is an issue, there are ways to solve it.  
> > 
> > Yeah, I need the solution, maybe an API to get correct return address? :)
> 
> 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.

> > By the way, are there any way to hold a private data on each ret_stack entry?
> > Since kretprobe supports "entry data" passed from entry_handler to
> > return handler, we have to store the data or data-instance on the ret_stack.
> > 
> > This feature is used by systemtap to save the function entry data, like
> > function parameters etc., so that return handler analyzes the parameters
> > with return value.
> 
> Yes, I remember you telling me about this at plumbers, and while I was
> writing this code I had that in mind. It wouldn't be too hard to
> implement, I just left it off for now. I also left it off because I
> have some questions about what exactly is needed. What size do you
> require to be stored. Especially if we want to keep the shadow stack
> smaller. I was going to find a way to implement some of the data that
> is already stored via the ret_stack with this instead, and make the
> ret_stack entry smaller. Should we allow just sizeof(long)*3? or just
> let user choose any size and if they run out of stack, then too bad. We
> just wont let it crash.

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 :-)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>