[Fwd: Re: [PATCH] Return probe]

[Vara Prasad]

Prasanna S Panchamukhi wrote:

>Hi Jim, Hien,
>
>Now this patch looks much better.  Please note that this patch requires to call
>kprobe_flush_task() at every do_exit()/do_execv() which adds to some amount of
>performance overhead. 
>
Yes this does add few instructions to do the cleanup but i am not sure 
if this is really a performance overhead.
In a data center environment process are long lived hence exit and exec 
account for a very small portion of the time. However this is a concern 
in the case of short lived process like in an interactive shell script 
or compile kind of environment. In the case of exit process is dyeing 
anyway hence few extra instructions as part of the death i don't think 
makes any difference. In the case of exec it is probably more relevant 
to be concerned. I suggest we do a small benchmark by compiling kernel 
sources with and without return probes and see if the addition of these 
few lines of code makes any perceivable performance overhead. If we 
notice any performance problem we can make cleanup inline.  Even after 
code tightening we still feel performance impact is significant, we 
should investigate the issue further.

>Also this method could not use multiple handlers feature 
>in a cleaner way. 
>
>  
>
Can you give more details of what you mean by cleaner way?

>Just a thought if a probe inside the tampoline can be avoided, since we change
> the return address on stack to tampoline and now within the tampoline we can 
>call the handlers. Then the return from tampoline can be changed to actual 
>return address.  We may need to get the pt_regs structure and avoid preemption 
>during the handler execution .
>
>Do you feel that there are some performance benefits by avoiding int3 
>in a tampoline?
>  
>
I will let Jim/Hien answer this one.

>Can we make this patch still cleaner by using multiple handlers feature?
>Should we proceed wth this patch now?
>
>  
>
I thought Hien already made this patch work with latest multiple 
handlers patch from Ananth.
I think we should proceed with this patch unless you have some strong 
technical objections and a better approach solve those objections.

>Also from discussions followed on the mailing lists that this method does not 
>handle the case where the return address stored on the stack is corrupted. This
>can be avoided by reistering a watchpoint at the place where the return 
>address is stored. There are other issue such as watchpoint register depletion
>etc, which might add some amount of complexity.
>
>  
>
I think we considered the watch point approach but didn't implement 
because number of watchpoints in most architectures are quiet limited 
than the number of return probes we need to activate. Coming to the 
problem of stack overruns and corruptions we might detect the problem of 
corruption with the watch points but i am not sure how we can really 
still track return of the function using this approach. When we detect 
corruption using watch point, i guess all we can do is trying to 
gracefully get out of the mess, anything else we can do?

>Should we proceed with this limitation? 
>  
>
If we suspect a stack corruption can we first use watchpoint probes to 
solve the corruption problem before using return probes for its purpose.

>If this limitation is not acceptable should we prototype the above approach?
>
>  
>
We have this limitation with this approach but watch point approach for 
return probes to me has much severe limitation than this occasional 
corruption bugs. Should we change our design due to these kinds of 
occasional corruption problems? Let me be a devils advocate and say what 
if the kprobes structure itself gets corrupted due to a bad pointer, 
should we change the kprobes design for such occasional bugs.

>Just a thought if we can implement the exit probe feature in much cleaner and 
>simpler way.
>One of the alternative methods mentioned in your design document that 
>"inserting a probepoint every time a probed function is entered, and the 
>removal of the probepoint after the function returns" seems to be much simpler.
>Also the problem mentioned in your design document about this method that "the 
>instruction at the RA may be executed even if the probed function isn't called"
>can be solved by tracking the current task associated with probe.
>  
>
We can probably track the current task but to me this has two 
disadvantages one is we will get way many traps then function returns. 
The second the task of keeping track of which traps are real and which 
are false makes it difficult to associate exit calls with corresponding 
entry calls hence prone to bugs.

>Ofcourse the problem of return address corruption will exist here also.
>
>How about prototyping this alternative approach just to see how this looks ?
>  
>
I am not sure we are going to learn lot of insight by prototyping this 
approach.

>Is there a simpler way of implementing the exit probe feature ?
>  
>
I think if there is an obvious better way someone would have suggested, 
that makes me feel it is not easy to come up with much more simpler than 
the current approach. It is possible that we can make the current 
implementation better by cleaning up the code.

Just to summarize your main concerns i hear about this approach are
1) We are adding little bit of additional cleanup code in the exit and 
exec paths
2) If there is a stack corruption we have no clue of when the function 
returns
I tried my best to address these concerns in this reply.

Did i miss any other concerns?

>Your suggestions are welcome.
>
>Thanks
>Prasanna
>
>  
>
bye,
Vara Prasad