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

Sinec forcibly unoptimized kprobes will be put on the freeing_list directly
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
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
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
+		 * 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. */

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


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