Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 9 Oct 2017 10:33:17 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> On Mon, 9 Oct 2017 12:20:35 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > > SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
> > > >       ftrace_ops on an architecture that does not support passing of regs
> > > >       will not fail with this flag set. But the callback must check if
> > > >       regs is NULL or not to determine if the architecture supports it.
> > > > 
> > > > RECURSION_SAFE - By default, a wrapper is added around the callback to
> > > >       make sure that recursion of the function does not occur. That is
> > > >       if a function within the callback itself is also traced, ftrace      
> > > 
> > > s/within the/called by the/    
> > 
> > I put in "within" because it is usually a function that is nested
> > within a function called by the callback. This bug has come up with
> > "gotchas", where some function that the callback calls has a path to a
> > function that was unexpectedly traced.
> > 
> > The issue hasn't been caused by a function being traced that was
> > directly called by the callback. It is usually some deeper nested
> > function.
> > 
> > I don't want to limit it to just checking functions that the callback
> > calls. Thoughts on how to stress this?  
> 
> "if a function that is called as a result of the callback's execution is
> also traced" ?

Sure, I'm not sure I could come up with a better way to say it.

> 
> > > > IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
> > > >       traced function (have another function called instead of the traced
> > > >       function), it requires setting this flag. This is what live kernel
> > > >       patches uses. Without this flag the pt_regs->ip can not be modified.
> > > >       Note, only one ftrace_ops with IPMODIFY set may be registered to
> > > >       any given function at a time.      
> > > 
> > > I assume this requires being able to get the regs too?      
> > 
> > Yes, this is why I stated "Requires SAVE_REGS" which would pass the
> > regs to the callback. Should I rewrite that somehow?  
> 
> No, just ship me another cup of coffee and that one should be OK.  Though
> perhaps if you'd spelled out the flag completely I wouldn't have been so
> dense :)

OK OK, I'll extend the names.

> 
> > > > If a glob is used to set the filter, to remove unwanted matches the
> > > > ftrace_set_notrace() can also be used.
> > > > 
> > > >   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > > > 			 int len, int reset);
> > > > 
> > > > This takes the same parameters as ftrace_set_filter() but will add the
> > > > functions it finds to not be traced. This doesn't remove them from the
> > > > filter itself, but keeps them from being traced. If @reset is set,
> > > > the filter is cleaded but the functions that match @buf will still not      
> > > 
> > > cleaded? :)    
> > 
> > Hmm, I'll have to be more descriptive.
> >   
> > >     
> > > > be traced (the callback will not be called on those functions).      
> > > 
> > > So how do you clead the "notrace" list?    
> > 
> > With passing in reset non-zero. I'll add that.  
> 
> My confusion remains here.  The text says that if reset is "set", then the
> "notrace" list remains in place.  So a non-zero "reset" value will remove
> previous notrace entries, along with the filter itself?  So if you wanted
> to clear the notrace list entirely you would use buf="", reset=1?  It would
> be good to be explicit there.

Ah I see what you mean. I'll try to be more explicit. A non-zero value
for reset means to clear the notrace buffer before making modifications.
In fact, I think the text is a bit more confusing, as I don't believe
that this function even modifies the "set_filter" list, even if reset
is set.

And yes, to clear all functions in either the set_filter or set_notrace
lists just pass in (&ops, NULL, 0, 1)

I'll start working on this document and post a real patch.

Thanks again for the review.

-- Steve