Messages in this thread |  | | Date | Tue, 27 Dec 2016 15:13:15 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH tip/master v2] kprobes: extable: Identify kprobes' insn-slots as kernel text area |
| |
Oops, I found I missed an rcu_read_unlock on the fast path... I'll send v3.
Thanks,
On Tue, 27 Dec 2016 00:34:20 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Make __kernel_text_address()/kernel_text_address() returns > true if the given address is on a kprobe's instruction slot, > which is generated by kprobes as a trampoline code. > This can help stacktraces to determine the address is on a > text area or not. > > To implement this without any sleep in is_kprobe_*_slot(), > this also modify insn_cache page list as a rcu list. It may > increase processing deley (not processing time) for garbage > slot collection, because it requires to wait an additional > rcu grance period when freeing a page from the list. > However, since it is not a hot path, we may not take care of it. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > --- > V2: Fix build error when CONFIG_KPROBES=n > > Hi Josh, could check this patch fixes your issue? It will > enable unwinder code to validate return address by using > __kernel_text_address() again. > --- > include/linux/kprobes.h | 22 ++++++++++++++- > kernel/extable.c | 9 +++++- > kernel/kprobes.c | 70 ++++++++++++++++++++++++++++++++++++----------- > 3 files changed, 82 insertions(+), 19 deletions(-) > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 8f68490..f0496b0 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -281,6 +281,9 @@ struct kprobe_insn_cache { > extern kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c); > extern void __free_insn_slot(struct kprobe_insn_cache *c, > kprobe_opcode_t *slot, int dirty); > +/* sleep-less address checking routine */ > +extern bool __is_insn_slot_addr(struct kprobe_insn_cache *c, > + unsigned long addr); > > #define DEFINE_INSN_CACHE_OPS(__name) \ > extern struct kprobe_insn_cache kprobe_##__name##_slots; \ > @@ -294,6 +297,11 @@ static inline void free_##__name##_slot(kprobe_opcode_t *slot, int dirty)\ > { \ > __free_insn_slot(&kprobe_##__name##_slots, slot, dirty); \ > } \ > + \ > +static inline bool is_kprobe_##__name##_slot(unsigned long addr) \ > +{ \ > + return __is_insn_slot_addr(&kprobe_##__name##_slots, addr); \ > +} > > DEFINE_INSN_CACHE_OPS(insn); > > @@ -330,7 +338,6 @@ extern int proc_kprobes_optimization_handler(struct ctl_table *table, > int write, void __user *buffer, > size_t *length, loff_t *ppos); > #endif > - > #endif /* CONFIG_OPTPROBES */ > #ifdef CONFIG_KPROBES_ON_FTRACE > extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > @@ -481,6 +488,19 @@ static inline int enable_jprobe(struct jprobe *jp) > return enable_kprobe(&jp->kp); > } > > +#ifndef CONFIG_KPROBES > +static inline bool is_kprobe_insn_slot(unsigned long addr) > +{ > + return false; > +} > +#endif > +#ifndef CONFIG_OPTPROBES > +static inline bool is_kprobe_optinsn_slot(unsigned long addr) > +{ > + return false; > +} > +#endif > + > #ifdef CONFIG_KPROBES > /* > * Blacklist ganerating macro. Specify functions which is not probed > diff --git a/kernel/extable.c b/kernel/extable.c > index e820cce..81c9633 100644 > --- a/kernel/extable.c > +++ b/kernel/extable.c > @@ -20,6 +20,7 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/init.h> > +#include <linux/kprobes.h> > > #include <asm/sections.h> > #include <asm/uaccess.h> > @@ -104,6 +105,8 @@ int __kernel_text_address(unsigned long addr) > return 1; > if (is_ftrace_trampoline(addr)) > return 1; > + if (is_kprobe_optinsn_slot(addr) || is_kprobe_insn_slot(addr)) > + return 1; > /* > * There might be init symbols in saved stacktraces. > * Give those symbols a chance to be printed in > @@ -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..1bd1c17 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -149,9 +149,11 @@ kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c) > struct kprobe_insn_page *kip; > kprobe_opcode_t *slot = NULL; > > + /* Since the slot array is not protected by rcu, we need a mutex */ > mutex_lock(&c->mutex); > retry: > - list_for_each_entry(kip, &c->pages, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(kip, &c->pages, list) { > if (kip->nused < slots_per_page(c)) { > int i; > for (i = 0; i < slots_per_page(c); i++) { > @@ -167,6 +169,7 @@ kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c) > WARN_ON(1); > } > } > + rcu_read_unlock(); > > /* If there are any garbage slots, collect it and try again. */ > if (c->nr_garbage && collect_garbage_slots(c) == 0) > @@ -193,13 +196,15 @@ kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c) > kip->nused = 1; > kip->ngarbage = 0; > kip->cache = c; > - list_add(&kip->list, &c->pages); > + list_add_rcu(&kip->list, &c->pages); > slot = kip->insns; > out: > mutex_unlock(&c->mutex); > return slot; > } > > + > + > /* Return 1 if all garbages are collected, otherwise 0. */ > static int collect_one_slot(struct kprobe_insn_page *kip, int idx) > { > @@ -213,7 +218,8 @@ static int collect_one_slot(struct kprobe_insn_page *kip, int idx) > * next time somebody inserts a probe. > */ > if (!list_is_singular(&kip->list)) { > - list_del(&kip->list); > + list_del_rcu(&kip->list); > + synchronize_rcu(); > kip->cache->free(kip->insns); > kfree(kip); > } > @@ -248,29 +254,59 @@ void __free_insn_slot(struct kprobe_insn_cache *c, > kprobe_opcode_t *slot, int dirty) > { > struct kprobe_insn_page *kip; > + long idx; > > mutex_lock(&c->mutex); > - list_for_each_entry(kip, &c->pages, list) { > - long idx = ((long)slot - (long)kip->insns) / > - (c->insn_size * sizeof(kprobe_opcode_t)); > - if (idx >= 0 && idx < slots_per_page(c)) { > - WARN_ON(kip->slot_used[idx] != SLOT_USED); > - if (dirty) { > - kip->slot_used[idx] = SLOT_DIRTY; > - kip->ngarbage++; > - if (++c->nr_garbage > slots_per_page(c)) > - collect_garbage_slots(c); > - } else > - collect_one_slot(kip, idx); > + rcu_read_lock(); > + list_for_each_entry_rcu(kip, &c->pages, list) { > + idx = ((long)slot - (long)kip->insns) / > + (c->insn_size * sizeof(kprobe_opcode_t)); > + if (idx >= 0 && idx < slots_per_page(c)) > goto out; > - } > } > - /* Could not free this slot. */ > + /* Could not find this slot. */ > WARN_ON(1); > + kip = NULL; > out: > + rcu_read_unlock(); > + /* Mark and sweep: this may sleep */ > + if (kip) { > + /* Check double free */ > + WARN_ON(kip->slot_used[idx] != SLOT_USED); > + if (dirty) { > + kip->slot_used[idx] = SLOT_DIRTY; > + kip->ngarbage++; > + if (++c->nr_garbage > slots_per_page(c)) > + collect_garbage_slots(c); > + } else > + collect_one_slot(kip, idx); > + } > mutex_unlock(&c->mutex); > } > > +/* > + * 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; > +} > + > #ifdef CONFIG_OPTPROBES > /* For optimized_kprobe buffer */ > struct kprobe_insn_cache kprobe_optinsn_slots = { >
-- Masami Hiramatsu <mhiramat@kernel.org>
|  |