Re: [PATCH] kprobes: add kprobe_is_function_probed()

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Oct 21, 2014 at 10:19:30PM +0200, Jiri Kosina wrote:
> On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
> 
> > > Add a function that allows external users (such as live patching 
> > > mechanisms) to check whether a given function (identified by symbol name) 
> > > has a kprobe installed in it.
> > 
> > Functions aren't uniquely identifiable by name.  Perhaps it should be
> > something like kprobe_is_addr_range_probed()?
> 
> Hi Josh,
> 
> first, thanks a lot for the review.
> 
> This is a rather difficult call actually. I am of course aware of the fact 
> that kernel fucntions can't be uniquely identified by name, but when 
> thinking about this, one has to consider:
> 
> - ftrace primary userspace interface (set_ftrace_filter) is based on 
>   function names
> - kprobe tracer and uprobe trace primary userspace interface 
>   (/sys/kernel/debug/tracing/kprobe_events and uprobe_events respectively)
>   are primarily based on function names

True, the user space interfaces are done by name, but the kernel
interfaces aren't necessarily so (see ftrace_set_filter_ip and struct
kprobe.addr).  This is a kernel interface so we can be more precise.

> - the number of conflicts is probably zero, or very close to zero. Try to 
>   run this 
> 
> 	cut -f3 -d' ' /proc/kallsyms | sort  | uniq -c
> 
>   So it's questionable whether all the back and forth name->address->name 
>   translations all over the place are worth all the trouble.

On my kernel:

$ grep ' t \| T ' /proc/kallsyms | awk '{print $3}' |sort |uniq -d |wc -l
395

So there are at least 395 places where this could go wrong...

With kpatch, we're setting the ftrace filter by address so we don't
patch the wrong function.  So we already have the address.  We also have
the function length, which is needed for our backtrace safety checks.

I'm guessing kGraft doesn't have the address + length?  I think you
could call kallsyms_lookup() to get both values.

Maybe we should see what our unified live patching code ends up looking
like before deciding what interface(s) we need here?

> 
>   I do agree though that we should make it obvious that the lookup 
>   interface works on symbol names only ... perhaps by adding '_by_name()' 
>   or so?
> 
> > Should we refuse to patch a function which has a kprobe installed inside 
> > it?  
> 
> I think warning about it is a good thing to do.

I think that's probably ok.

> 
> > Is there a race-free way to do that?
> 
> Do we need to be race-free here? If userspace is both instantiating new 
> krpobe and initiating live kernel patching at the "same time", I don't 
> think kernel should try to solve such race ... it's simply there by 
> definition, depending on, for example, scheduling decisions.

If we're not preventing it, but instead just printing a warning, then
yeah, that sounds fine to me.

I think it would also be nice to do the reverse, i.e. have kprobes emit
a warning if someone tries to probe a replaced function.

-- 
Josh