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