Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] kprobes: premature kprobe publication onto ap->list in register_aggr_kprobe() - [High] kprobes: unchecked rollback arm_kprobe_ftrace() in register_aggr_kprobe() --
commit 90db6f90ad3cb02409cec15b4dabeb1b192665e6 Author: Masami Hiramatsu (Google) <[email protected]> kprobes: Switch ftrace ops when adding probe with post_handler This commit switches an armed ftrace-based kprobe from kprobe_ftrace_ops to kprobe_ipmodify_ops in register_aggr_kprobe() when a new kprobe with a post_handler is added to an aggregated kprobe that previously had no post_handler. It also restores the ftrace ops registration and counter if ftrace_set_filter_ip() fails in __disarm_kprobe_ftrace(). > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index 0b41d69540ae4..135fec5d49cf5 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c [ ... ] > @@ -1654,18 +1657,51 @@ static int register_aggr_kprobe(struct kprobe > *orig_p, struct kprobe *p) > | KPROBE_FLAG_DISABLED; > } > > + if (p->post_handler && !ap->post_handler) { > + reset_post_handler = true; > + if (kprobe_ftrace(ap) && !kprobes_all_disarmed && > + !kprobe_disabled(ap)) > + ftrace_switch_ops = true; > + else > + ap->post_handler = aggr_post_handler; > + } > + > /* Copy the insn slot of 'p' to 'ap'. */ > copy_kprobe(ap, p); > - ret = add_new_kprobe(ap, p); > + add_new_kprobe(ap, p); > + } [Severity: High] When ftrace_switch_ops is true in register_aggr_kprobe(), add_new_kprobe(ap, p) publishes p onto ap->list via list_add_rcu() while ap is still armed on kprobe_ftrace_ops and ap->post_handler is still NULL. If another CPU hits the probed function before disarm_kprobe_ftrace(ap) finishes updating the ftrace callsite, kprobe_ftrace_handler() will call aggr_pre_handler() -> p->pre_handler(), and then skip aggr_post_handler() because ap->post_handler is NULL: arch/x86/kernel/kprobes/ftrace.c:kprobe_ftrace_handler() { ... if (!p->pre_handler || !p->pre_handler(p, regs)) { if (unlikely(p->post_handler)) { instruction_pointer_set(regs, ip + MCOUNT_INSN_SIZE); kcb->kprobe_status = KPROBE_HIT_SSDONE; p->post_handler(p, regs, 0); } ... } In addition, p->pre_handler() executes under kprobe_ftrace_ops before arm_kprobe_ftrace(ap) has verified that kprobe_ipmodify_ops (FTRACE_OPS_FL_IPMODIFY) does not conflict with an active livepatch. A similar window also occurs in the error path below if arm_kprobe_ftrace(ap) fails, where ap->post_handler is reset to NULL and ap is re-armed on kprobe_ftrace_ops before list_del_rcu(&p->list) removes p. Could add_new_kprobe(ap, p) be deferred until after ap has been switched to kprobe_ipmodify_ops, or could p be removed from ap->list before re-arming ap on rollback? > + > + if (ftrace_switch_ops) { > + /* > + * Must switch ftrace ops outside of text_mutex and > cpus_read_lock > + * to prevent deadlocks when ftrace code modification acquires > text_mutex. > + */ > + ret = disarm_kprobe_ftrace(ap); > + if (ret) { > + list_del_rcu(&p->list); > + synchronize_rcu(); > + return ret; > + } > + ap->post_handler = aggr_post_handler; > + ret = arm_kprobe_ftrace(ap); > + if (ret) { > + ap->post_handler = NULL; > + arm_kprobe_ftrace(ap); > + list_del_rcu(&p->list); > + synchronize_rcu(); > + return ret; > + } > } [Severity: High] What happens if the rollback call to arm_kprobe_ftrace(ap) in register_aggr_kprobe() also fails (for example, with -ENOMEM in ftrace_set_filter_ip() -> alloc_and_copy_ftrace_hash())? Because the return value of the rollback arm_kprobe_ftrace(ap) is ignored and KPROBE_FLAG_DISABLED is not set on ap->flags, ap remains marked as enabled even though it is no longer registered in kprobe_ftrace_ops's filter hash. When the remaining probe on ap is later disabled or unregistered via unregister_kprobe() -> __unregister_kprobe_top() -> __disable_kprobe(), __disable_kprobe() sees !kprobe_disabled(orig_p) and calls disarm_kprobe_ftrace(), which fails with -ENOENT in ftrace_set_filter_ip(), triggers the WARN_ONCE() in __disarm_kprobe_ftrace(), and aborts unregistration while leaving the probe hashed in kprobe_table: kernel/kprobes.c:__disable_kprobe() { ... 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); } } ... } Should the rollback path check the return value of arm_kprobe_ftrace(ap) and set KPROBE_FLAG_DISABLED on ap->flags if re-arming fails? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/179375029807.247640.13024754173032838618.stgit@devnote2?part=4
