lkml.org 
[lkml]   [2023]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] kprobes: Fix to handle forcibly unoptimized kprobes on freeing_list
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


--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2023-03-26 23:53    [W:0.050 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site