Messages in this thread | | | Date | Thu, 29 Nov 2018 11:46:52 -0500 | From | Steven Rostedt <> | 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@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:
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
| |