From ce7f27dcd7e7721527648e27908e5200d1a3e3b0 Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Tue, 13 Aug 2024 19:53:33 +0800 Subject: [PATCH 1/4] kprobes: Cleanup the config comment The CONFIG_KPROBES_ON_FTRACE #if/#else/#endif section is small and doesn't nest additional #ifdefs so the comment is useless and should be removed, but the __ARCH_WANT_KPROBES_INSN_SLOT and CONFIG_OPTPROBES() nest is long, it is better to add comment for reading. Link: https://lore.kernel.org/all/20240813115334.3922580-3-ruanjinjie@huawei.com/ Signed-off-by: Jinjie Ruan Signed-off-by: Masami Hiramatsu (Google) --- kernel/kprobes.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/kprobes.c b/kernel/kprobes.c index da59c68df841..5f4117b1ff70 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -353,8 +353,8 @@ struct kprobe_insn_cache kprobe_optinsn_slots = { /* .insn_size is initialized later */ .nr_garbage = 0, }; -#endif -#endif +#endif /* CONFIG_OPTPROBES */ +#endif /* __ARCH_WANT_KPROBES_INSN_SLOT */ /* We have preemption disabled.. so it is safe to use __ versions */ static inline void set_kprobe_instance(struct kprobe *kp) @@ -1543,7 +1543,7 @@ static int check_ftrace_location(struct kprobe *p) if (ftrace_location(addr) == addr) { #ifdef CONFIG_KPROBES_ON_FTRACE p->flags |= KPROBE_FLAG_FTRACE; -#else /* !CONFIG_KPROBES_ON_FTRACE */ +#else return -EINVAL; #endif } From da93dd931baa921ee31796e0cd6402d995e1e4fc Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Tue, 13 Aug 2024 19:53:34 +0800 Subject: [PATCH 2/4] kprobes: Cleanup collect_one_slot() and __disable_kprobe() If kip->nused is not zero, collect_one_slot() return false, otherwise do a lot of linked list operations, reverse the processing order to make the code if nesting more concise. __disable_kprobe() is the same as well. Link: https://lore.kernel.org/all/20240813115334.3922580-4-ruanjinjie@huawei.com/ Signed-off-by: Jinjie Ruan Signed-off-by: Masami Hiramatsu (Google) --- kernel/kprobes.c | 79 ++++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 5f4117b1ff70..5381c6571fa2 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -206,29 +206,29 @@ static bool collect_one_slot(struct kprobe_insn_page *kip, int idx) { kip->slot_used[idx] = SLOT_CLEAN; kip->nused--; - if (kip->nused == 0) { + if (kip->nused != 0) + return false; + + /* + * Page is no longer in use. Free it unless + * it's the last one. We keep the last one + * so as not to have to set it up again the + * next time somebody inserts a probe. + */ + if (!list_is_singular(&kip->list)) { /* - * Page is no longer in use. Free it unless - * it's the last one. We keep the last one - * so as not to have to set it up again the - * next time somebody inserts a probe. + * Record perf ksymbol unregister event before removing + * the page. */ - if (!list_is_singular(&kip->list)) { - /* - * Record perf ksymbol unregister event before removing - * the page. - */ - perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_OOL, - (unsigned long)kip->insns, PAGE_SIZE, true, - kip->cache->sym); - list_del_rcu(&kip->list); - synchronize_rcu(); - kip->cache->free(kip->insns); - kfree(kip); - } - return true; + perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_OOL, + (unsigned long)kip->insns, PAGE_SIZE, true, + kip->cache->sym); + list_del_rcu(&kip->list); + synchronize_rcu(); + kip->cache->free(kip->insns); + kfree(kip); } - return false; + return true; } static int collect_garbage_slots(struct kprobe_insn_cache *c) @@ -1725,28 +1725,29 @@ static struct kprobe *__disable_kprobe(struct kprobe *p) if (unlikely(orig_p == NULL)) return ERR_PTR(-EINVAL); - if (!kprobe_disabled(p)) { - /* Disable probe if it is a child probe */ - if (p != orig_p) - p->flags |= KPROBE_FLAG_DISABLED; + if (kprobe_disabled(p)) + return orig_p; - /* Try to disarm and disable this/parent probe */ - if (p == orig_p || aggr_kprobe_disabled(orig_p)) { - /* - * Don't be lazy here. Even if 'kprobes_all_disarmed' - * is false, 'orig_p' might not have been armed yet. - * Note arm_all_kprobes() __tries__ to arm all kprobes - * on the best effort basis. - */ - if (!kprobes_all_disarmed && !kprobe_disabled(orig_p)) { - ret = disarm_kprobe(orig_p, true); - if (ret) { - p->flags &= ~KPROBE_FLAG_DISABLED; - return ERR_PTR(ret); - } + /* Disable probe if it is a child probe */ + if (p != orig_p) + p->flags |= KPROBE_FLAG_DISABLED; + + /* Try to disarm and disable this/parent probe */ + if (p == orig_p || aggr_kprobe_disabled(orig_p)) { + /* + * Don't be lazy here. Even if 'kprobes_all_disarmed' + * is false, 'orig_p' might not have been armed yet. + * Note arm_all_kprobes() __tries__ to arm all kprobes + * on the best effort basis. + */ + if (!kprobes_all_disarmed && !kprobe_disabled(orig_p)) { + ret = disarm_kprobe(orig_p, true); + if (ret) { + p->flags &= ~KPROBE_FLAG_DISABLED; + return ERR_PTR(ret); } - orig_p->flags |= KPROBE_FLAG_DISABLED; } + orig_p->flags |= KPROBE_FLAG_DISABLED; } return orig_p; From 3fbff988892993a144d0f47db0348c18b4621393 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Wed, 30 Oct 2024 09:14:49 -0700 Subject: [PATCH 3/4] kprobes: Use struct_size() in __get_insn_slot() __get_insn_slot() allocates 'struct kprobe_insn_page' using a custom structure size calculation macro, KPROBE_INSN_PAGE_SIZE. Replace KPROBE_INSN_PAGE_SIZE with the struct_size() macro, which is the preferred way to calculate the size of flexible structures in the kernel because it handles overflow and makes it easier to change and audit how flexible structures are allocated across the entire tree. Link: https://lore.kernel.org/all/20241030-kprobes-fix-counted-by-annotation-v1-2-8f266001fad0@kernel.org/ (Masami modofied this to be applicable without the 1st patch in the series.) Signed-off-by: Nathan Chancellor Signed-off-by: Masami Hiramatsu (Google) --- kernel/kprobes.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 5381c6571fa2..b027a4030976 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -95,10 +95,6 @@ struct kprobe_insn_page { char slot_used[]; }; -#define KPROBE_INSN_PAGE_SIZE(slots) \ - (offsetof(struct kprobe_insn_page, slot_used) + \ - (sizeof(char) * (slots))) - static int slots_per_page(struct kprobe_insn_cache *c) { return PAGE_SIZE/(c->insn_size * sizeof(kprobe_opcode_t)); @@ -175,7 +171,7 @@ kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c) goto retry; /* All out of space. Need to allocate a new page. */ - kip = kmalloc(KPROBE_INSN_PAGE_SIZE(slots_per_page(c)), GFP_KERNEL); + kip = kmalloc(struct_size(kip, slot_used, slots_per_page(c)), GFP_KERNEL); if (!kip) goto out; From 4638d7ebeff5a741e22139cf00aaa9db0f668a8c Mon Sep 17 00:00:00 2001 From: "Masami Hiramatsu (Google)" Date: Mon, 21 Oct 2024 21:39:22 +0900 Subject: [PATCH 4/4] x86/kprobes: Cleanup kprobes on ftrace code Cleanup kprobes on ftrace code for x86. - Set instruction pointer (ip + MCOUNT_INSN_SIZE) after pre_handler only when p->post_handler exists. - Use INT3_INSN_SIZE instead of 1. - Use instruction_pointer/instruction_pointer_set() functions instead of accessing regs->ip directly. Link: https://lore.kernel.org/all/172951436219.167263.18330240454389154327.stgit@devnote2/ Signed-off-by: Masami Hiramatsu (Google) --- arch/x86/kernel/kprobes/ftrace.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c index 15af7e98e161..2be55ec3f392 100644 --- a/arch/x86/kernel/kprobes/ftrace.c +++ b/arch/x86/kernel/kprobes/ftrace.c @@ -9,6 +9,7 @@ #include #include #include +#include #include "common.h" @@ -36,23 +37,25 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, if (kprobe_running()) { kprobes_inc_nmissed_count(p); } else { - unsigned long orig_ip = regs->ip; + unsigned long orig_ip = instruction_pointer(regs); + /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */ - regs->ip = ip + sizeof(kprobe_opcode_t); + instruction_pointer_set(regs, ip + INT3_INSN_SIZE); __this_cpu_write(current_kprobe, p); kcb->kprobe_status = KPROBE_HIT_ACTIVE; if (!p->pre_handler || !p->pre_handler(p, regs)) { - /* - * Emulate singlestep (and also recover regs->ip) - * as if there is a 5byte nop - */ - regs->ip = (unsigned long)p->addr + MCOUNT_INSN_SIZE; if (unlikely(p->post_handler)) { + /* + * Emulate singlestep (and also recover regs->ip) + * as if there is a 5byte nop + */ + instruction_pointer_set(regs, ip + MCOUNT_INSN_SIZE); kcb->kprobe_status = KPROBE_HIT_SSDONE; p->post_handler(p, regs, 0); } - regs->ip = orig_ip; + /* Recover IP address */ + instruction_pointer_set(regs, orig_ip); } /* * If pre_handler returns !0, it changes regs->ip. We have to