Date:   Thu, 29 Nov 2018 11:46:52 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        Andy Lutomirski <luto@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple
 users

On Thu, 29 Nov 2018 23:29:27 +0900
Masami Hiramatsu <mhiramat@...nel.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:

	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(...);

We only call the retfunc of a fgraph_ops if it returned non-zero from
its entryfunc(). The return can happen a long time from now (which is
why I don't save the &fgraph_ops on the ret_stack, because then we would
never be able to free it).

In the mean time, lets say we unregistered (and freed) fgraph_ops_2 and
then added fgraph_ops_3, so the array looks like:

 array: | &fgraph_ops_1 | &fgraph_ops_3 | &fgraph_ops_stub | ...

Then a function that was called when fgraph_ops_2 was on the stack
returns, it will call array[1]->retfunc() which now belongs to
fgraph_ops_3 and not fgraph_ops_2.

But if we add a counter array that gets updated when new ops are added
to the array, we have this:

 cnt_array: |       4       |     2         |       0          |
     array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...

And do:

	for (i = 0; i < array_entries; i++) {
		if (array[i]->entryfunc(...)) {
			idx = cnt_array[i] << 8 | i;
			push idx onto ret_stack;
		}
	}

Then on return we have:

	idx = pop ret_stack;

	if (idx >> 8 == cnt_array[idx & 0xff])
		array[idx & 0xff]->retfunc(...);

It wouldn't call fgraph_ops_3 because we would change the cnt_array
when we remove fgraph_ops_2 and the return would not match, as
cnt_array[1] would then be "3".

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

Awesome, then this shouldn't be too hard to implement.

-- Steve

Powered by blists - more mailing lists