Message ID | 167448024501.3253718.13037333683110512967.stgit@devnote3 (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | kprobes: Fix to handle forcibly unoptimized kprobes on freeing_list | expand |
On Mon, 23 Jan 2023 22:24:05 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Sinec forcibly unoptimized kprobes will be put on the freeing_list directly "Since" > in the unoptimize_kprobe(), do_unoptimize_kprobes() must continue to check > the freeing_list even if unoptimizing_list is empty. > > This bug can be happen if a kprobe is put in an instruction which is in the "This bug can happen if" > middle of the jump-replaced instruction sequence of an optprobe, *and* the > optprobe is recently unregistered and queued on unoptimizing_list. > In this case, the optprobe will be unoptimized forcibly (means immediately) > and put it into the freeing_list, expecting the optprobe will be handled in > do_unoptimize_kprobe(). > But if there is no other optprobes on the unoptimizing_list, current code > returns from the do_unoptimize_kprobe() soon and do not handle the optprobe "and does not handle' > which is on the freeing_list, and it will hit the WARN_ON_ONCE() in the > do_free_cleaned_kprobes(), because it is not handled in the latter loop of > the do_unoptimize_kprobe(). > > To solve this issue, do not return from do_unoptimize_kprobes() immediately > even if unoptimizing_list is empty. > > Moreover, this change affects another case. kill_optimized_kprobes() expects > kprobe_optimizer() will just free the optprobe on freeing_list. > So I changed it to just do list_move() to freeing_list if optprobes are on > unoptimizing list. And the do_unoptimize_kprobe() will skip > arch_disarm_kprobe() if the probe on freeing_list has gone flag. > > Link: https://lore.kernel.org/all/Y8URdIfVr3pq2X8w@xpf.sh.intel.com/ > > Fixes: e4add247789e ("kprobes: Fix optimize_kprobe()/unoptimize_kprobe() cancellation logic") > Reported-by: Pengfei Xu <pengfei.xu@intel.com> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/kprobes.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index 1c18ecf9f98b..73b150fad936 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -555,17 +555,15 @@ static void do_unoptimize_kprobes(void) > /* See comment in do_optimize_kprobes() */ > lockdep_assert_cpus_held(); > > - /* Unoptimization must be done anytime */ > - if (list_empty(&unoptimizing_list)) > - return; > + if (!list_empty(&unoptimizing_list)) > + arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); > > - arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); > - /* Loop on 'freeing_list' for disarming */ > + /* Loop on 'freeing_list' for disarming and removing from kprobe hash list */ > list_for_each_entry_safe(op, tmp, &freeing_list, list) { > /* Switching from detour code to origin */ > op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; > - /* Disarm probes if marked disabled */ > - if (kprobe_disabled(&op->kp)) > + /* Disarm probes if marked disabled and not gone */ > + if (kprobe_disabled(&op->kp) && !kprobe_gone(&op->kp)) > arch_disarm_kprobe(&op->kp); > if (kprobe_unused(&op->kp)) { > /* > @@ -797,14 +795,13 @@ static void kill_optimized_kprobe(struct kprobe *p) > op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; > > if (kprobe_unused(p)) { > - /* Enqueue if it is unused */ > - list_add(&op->list, &freeing_list); > /* > - * Remove unused probes from the hash list. After waiting > - * for synchronization, this probe is reclaimed. > - * (reclaiming is done by do_free_cleaned_kprobes().) > + * Unused kprobe is on unoptimizing or freeing list. We move it > + * to freeing_list and let the kprobe_optimizer() removes it from "remove it" > + * the kprobe hash list and frees it. "and free it." > */ > - hlist_del_rcu(&op->kp.hlist); > + if (optprobe_queued_unopt(op)) > + list_move(&op->list, &freeing_list); > } > > /* Don't touch the code, because it is already freed. */ Other than the spelling issues, Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> -- Steve
On Mon, 23 Jan 2023 13:39:31 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 23 Jan 2023 22:24:05 +0900 > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > Sinec forcibly unoptimized kprobes will be put on the freeing_list directly > > "Since" > > > in the unoptimize_kprobe(), do_unoptimize_kprobes() must continue to check > > the freeing_list even if unoptimizing_list is empty. > > > > This bug can be happen if a kprobe is put in an instruction which is in the > > "This bug can happen if" > > > middle of the jump-replaced instruction sequence of an optprobe, *and* the > > optprobe is recently unregistered and queued on unoptimizing_list. > > In this case, the optprobe will be unoptimized forcibly (means immediately) > > and put it into the freeing_list, expecting the optprobe will be handled in > > do_unoptimize_kprobe(). > > But if there is no other optprobes on the unoptimizing_list, current code > > returns from the do_unoptimize_kprobe() soon and do not handle the optprobe > > "and does not handle' > > > which is on the freeing_list, and it will hit the WARN_ON_ONCE() in the > > do_free_cleaned_kprobes(), because it is not handled in the latter loop of > > the do_unoptimize_kprobe(). > > > > To solve this issue, do not return from do_unoptimize_kprobes() immediately > > even if unoptimizing_list is empty. > > > > Moreover, this change affects another case. kill_optimized_kprobes() expects > > kprobe_optimizer() will just free the optprobe on freeing_list. > > So I changed it to just do list_move() to freeing_list if optprobes are on > > unoptimizing list. And the do_unoptimize_kprobe() will skip > > arch_disarm_kprobe() if the probe on freeing_list has gone flag. > > > > Link: https://lore.kernel.org/all/Y8URdIfVr3pq2X8w@xpf.sh.intel.com/ > > > > Fixes: e4add247789e ("kprobes: Fix optimize_kprobe()/unoptimize_kprobe() cancellation logic") > > Reported-by: Pengfei Xu <pengfei.xu@intel.com> > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > --- > > kernel/kprobes.c | 23 ++++++++++------------- > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index 1c18ecf9f98b..73b150fad936 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -555,17 +555,15 @@ static void do_unoptimize_kprobes(void) > > /* See comment in do_optimize_kprobes() */ > > lockdep_assert_cpus_held(); > > > > - /* Unoptimization must be done anytime */ > > - if (list_empty(&unoptimizing_list)) > > - return; > > + if (!list_empty(&unoptimizing_list)) > > + arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); > > > > - arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); > > - /* Loop on 'freeing_list' for disarming */ > > + /* Loop on 'freeing_list' for disarming and removing from kprobe hash list */ > > list_for_each_entry_safe(op, tmp, &freeing_list, list) { > > /* Switching from detour code to origin */ > > op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; > > - /* Disarm probes if marked disabled */ > > - if (kprobe_disabled(&op->kp)) > > + /* Disarm probes if marked disabled and not gone */ > > + if (kprobe_disabled(&op->kp) && !kprobe_gone(&op->kp)) > > arch_disarm_kprobe(&op->kp); > > if (kprobe_unused(&op->kp)) { > > /* > > @@ -797,14 +795,13 @@ static void kill_optimized_kprobe(struct kprobe *p) > > op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; > > > > if (kprobe_unused(p)) { > > - /* Enqueue if it is unused */ > > - list_add(&op->list, &freeing_list); > > /* > > - * Remove unused probes from the hash list. After waiting > > - * for synchronization, this probe is reclaimed. > > - * (reclaiming is done by do_free_cleaned_kprobes().) > > + * Unused kprobe is on unoptimizing or freeing list. We move it > > + * to freeing_list and let the kprobe_optimizer() removes it from > > "remove it" > > > + * the kprobe hash list and frees it. > > "and free it." > > > */ > > - hlist_del_rcu(&op->kp.hlist); > > + if (optprobe_queued_unopt(op)) > > + list_move(&op->list, &freeing_list); > > } > > > > /* Don't touch the code, because it is already freed. */ > > Other than the spelling issues, > > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> Thanks for review! I'll fix typos and put on probes/urgent. Also, Cc: stable@vger.kernel.org Thank you, > > -- Steve
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 1c18ecf9f98b..73b150fad936 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -555,17 +555,15 @@ static void do_unoptimize_kprobes(void) /* See comment in do_optimize_kprobes() */ lockdep_assert_cpus_held(); - /* Unoptimization must be done anytime */ - if (list_empty(&unoptimizing_list)) - return; + if (!list_empty(&unoptimizing_list)) + arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); - arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list); - /* Loop on 'freeing_list' for disarming */ + /* Loop on 'freeing_list' for disarming and removing from kprobe hash list */ list_for_each_entry_safe(op, tmp, &freeing_list, list) { /* Switching from detour code to origin */ op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; - /* Disarm probes if marked disabled */ - if (kprobe_disabled(&op->kp)) + /* Disarm probes if marked disabled and not gone */ + if (kprobe_disabled(&op->kp) && !kprobe_gone(&op->kp)) arch_disarm_kprobe(&op->kp); if (kprobe_unused(&op->kp)) { /* @@ -797,14 +795,13 @@ static void kill_optimized_kprobe(struct kprobe *p) op->kp.flags &= ~KPROBE_FLAG_OPTIMIZED; if (kprobe_unused(p)) { - /* Enqueue if it is unused */ - list_add(&op->list, &freeing_list); /* - * Remove unused probes from the hash list. After waiting - * for synchronization, this probe is reclaimed. - * (reclaiming is done by do_free_cleaned_kprobes().) + * Unused kprobe is on unoptimizing or freeing list. We move it + * to freeing_list and let the kprobe_optimizer() removes it from + * the kprobe hash list and frees it. */ - hlist_del_rcu(&op->kp.hlist); + if (optprobe_queued_unopt(op)) + list_move(&op->list, &freeing_list); } /* Don't touch the code, because it is already freed. */