Re: [PATCH tip/master v3] kprobes: extable: Identify kprobes' insn-slots as kernel text area

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue, 3 Jan 2017 11:54:02 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Dec 27, 2016 at 03:14:10PM +0900, Masami Hiramatsu wrote:
> 
> > diff --git a/kernel/extable.c b/kernel/extable.c
> > index e820cce..81c9633 100644
> > --- a/kernel/extable.c
> > +++ b/kernel/extable.c
> 
> > @@ -123,7 +126,11 @@ int kernel_text_address(unsigned long addr)
> >  		return 1;
> >  	if (is_module_text_address(addr))
> >  		return 1;
> > -	return is_ftrace_trampoline(addr);
> > +	if (is_ftrace_trampoline(addr))
> > +		return 1;
> > +	if (is_kprobe_optinsn_slot(addr) || is_kprobe_insn_slot(addr))
> > +		return 1;
> > +	return 0;
> >  }
> 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index d630954..be41f6d 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> 
> > +/*
> > + * Check given address is on the page of kprobe instruction slots.
> > + * This will be used for checking whether the address on a stack
> > + * is on a text area or not.
> > + */
> > +bool __is_insn_slot_addr(struct kprobe_insn_cache *c, unsigned long addr)
> > +{
> > +	struct kprobe_insn_page *kip;
> > +	bool ret = false;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(kip, &c->pages, list) {
> > +		if (addr >= (unsigned long)kip->insns &&
> > +		    addr < (unsigned long)kip->insns + PAGE_SIZE) {
> > +			ret = true;
> > +			break;
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	return ret;
> > +}
> 
> How many entries should one expect on that list? I spend quite a bit of
> time reducing the cost of is_module_text_address() a while back and see
> that both ftrace (which actually needs this to be fast) and now
> kprobes have linear list walks in here.

It depends on how many probes are used and optimized. However, in most
cases, there should be one entry (unless user defines optimized probes
over 32 on x86, from my experience, it is very rare case. :) )

> I'm assuming the ftrace thing to be mostly empty, since I never saw it
> on my benchmarks back then, but it is something Steve should look at I
> suppose.
> 
> Similarly, the changelog here should include some talk about worst case
> costs.

Would you have any good benchmark to measure it?

Thanks,

> 
> FWIW, see commit: 93c2e105f6bc ("module: Optimize __module_address() using a latched RB-tree")


-- 
Masami Hiramatsu <mhiramat@kernel.org>