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

Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
fails to find a kprobe corresponding to the address.

Since arm64 kprobes uses stop_machine based text patching for removing
BRK, it ensures all running kprobe_break_handler() is done at that point.
And after removing the BRK, it removes the kprobe from its hash list.
Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
there is a bug.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |   79 +++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d2ae37f89774..ea56b22d4da8 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
 	return 0;
 }
 
-static void __kprobes kprobe_handler(struct pt_regs *regs)
+static int __kprobes
+kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
 {
 	struct kprobe *p, *cur_kprobe;
 	struct kprobe_ctlblk *kcb;
@@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
 	cur_kprobe = kprobe_running();
 
 	p = get_kprobe((kprobe_opcode_t *) addr);
+	if (WARN_ON_ONCE(!p)) {
+		/*
+		 * Something went wrong. This must be put by kprobe, but we
+		 * could not find corresponding kprobes. Let the kernel handle
+		 * this error case.
+		 */
+		return DBG_HOOK_ERROR;
+	}
 
-	if (p) {
-		if (cur_kprobe) {
-			if (reenter_kprobe(p, regs, kcb))
-				return;
-		} else {
-			/* Probe hit */
-			set_current_kprobe(p);
-			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
-
-			/*
-			 * If we have no pre-handler or it returned 0, we
-			 * continue with normal processing.  If we have a
-			 * pre-handler and it returned non-zero, it will
-			 * modify the execution path and no need to single
-			 * stepping. Let's just reset current kprobe and exit.
-			 */
-			if (!p->pre_handler || !p->pre_handler(p, regs)) {
-				setup_singlestep(p, regs, kcb, 0);
-			} else
-				reset_current_kprobe();
-		}
+	if (cur_kprobe) {
+		/* Hit a kprobe inside another kprobe */
+		if (!reenter_kprobe(p, regs, kcb))
+			return DBG_HOOK_ERROR;
+	} else {
+		/* Probe hit */
+		set_current_kprobe(p);
+		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+
+		/*
+		 * If we have no pre-handler or it returned 0, we
+		 * continue with normal processing.  If we have a
+		 * pre-handler and it returned non-zero, it will
+		 * modify the execution path and no need to single
+		 * stepping. Let's just reset current kprobe and exit.
+		 */
+		if (!p->pre_handler || !p->pre_handler(p, regs))
+			setup_singlestep(p, regs, kcb, 0);
+		else
+			reset_current_kprobe();
 	}
-	/*
-	 * The breakpoint instruction was removed right
-	 * after we hit it.  Another cpu has removed
-	 * either a probepoint or a debugger breakpoint
-	 * at this address.  In either case, no further
-	 * handling of this interrupt is appropriate.
-	 * Return back to original instruction, and continue.
-	 */
+
+	return DBG_HOOK_HANDLED;
 }
 
+static struct break_hook kprobes_break_hook = {
+	.imm = KPROBES_BRK_IMM,
+	.fn = kprobe_breakpoint_handler,
+};
+
 static int __kprobes
 kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
 {
@@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
 	.fn = kprobe_breakpoint_ss_handler,
 };
 
-static int __kprobes
-kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
-{
-	kprobe_handler(regs);
-	return DBG_HOOK_HANDLED;
-}
-
-static struct break_hook kprobes_break_hook = {
-	.imm = KPROBES_BRK_IMM,
-	.fn = kprobe_breakpoint_handler,
-};
-
 /*
  * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
  * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).

On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
> fails to find a kprobe corresponding to the address.
> 
> Since arm64 kprobes uses stop_machine based text patching for removing
> BRK, it ensures all running kprobe_break_handler() is done at that point.
> And after removing the BRK, it removes the kprobe from its hash list.
> Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
> there is a bug.

IIUC this relies on BRK handling not being preemptible, which is something
we've repeatedly considered changing along with a bunch of other debug
exception handling.

In case we do try to change that in future, it would be good to have a comment
somewhere to that effect.

I think there are other ways we could synchronise against that (e.g. using RCU
tasks rude) if we ever do that, and this patch looks good to me.

> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  arch/arm64/kernel/probes/kprobes.c |   79 +++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d2ae37f89774..ea56b22d4da8 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
>  	return 0;
>  }
>  
> -static void __kprobes kprobe_handler(struct pt_regs *regs)
> +static int __kprobes
> +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
>  {
>  	struct kprobe *p, *cur_kprobe;
>  	struct kprobe_ctlblk *kcb;
> @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
>  	cur_kprobe = kprobe_running();
>  
>  	p = get_kprobe((kprobe_opcode_t *) addr);
> +	if (WARN_ON_ONCE(!p)) {
> +		/*
> +		 * Something went wrong. This must be put by kprobe, but we
> +		 * could not find corresponding kprobes. Let the kernel handle
> +		 * this error case.
> +		 */

Could we make this:

		/*
		 * Something went wrong. This BRK used an immediate reserved
		 * for kprobes, but we couldn't find any corresponding probe.
		 */

> +		return DBG_HOOK_ERROR;
> +	}
>  
> -	if (p) {
> -		if (cur_kprobe) {
> -			if (reenter_kprobe(p, regs, kcb))
> -				return;
> -		} else {
> -			/* Probe hit */
> -			set_current_kprobe(p);
> -			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> -
> -			/*
> -			 * If we have no pre-handler or it returned 0, we
> -			 * continue with normal processing.  If we have a
> -			 * pre-handler and it returned non-zero, it will
> -			 * modify the execution path and no need to single
> -			 * stepping. Let's just reset current kprobe and exit.
> -			 */
> -			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> -				setup_singlestep(p, regs, kcb, 0);
> -			} else
> -				reset_current_kprobe();
> -		}
> +	if (cur_kprobe) {
> +		/* Hit a kprobe inside another kprobe */
> +		if (!reenter_kprobe(p, regs, kcb))
> +			return DBG_HOOK_ERROR;
> +	} else {
> +		/* Probe hit */
> +		set_current_kprobe(p);
> +		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> +
> +		/*
> +		 * If we have no pre-handler or it returned 0, we
> +		 * continue with normal processing.  If we have a
> +		 * pre-handler and it returned non-zero, it will
> +		 * modify the execution path and no need to single
> +		 * stepping. Let's just reset current kprobe and exit.
> +		 */

Minor wording nit: could we replace:

	no need to single stepping.

With:
	
	not need to single-step.

Thanks,
Mark.

> +		if (!p->pre_handler || !p->pre_handler(p, regs))
> +			setup_singlestep(p, regs, kcb, 0);
> +		else
> +			reset_current_kprobe();
>  	}
> -	/*
> -	 * The breakpoint instruction was removed right
> -	 * after we hit it.  Another cpu has removed
> -	 * either a probepoint or a debugger breakpoint
> -	 * at this address.  In either case, no further
> -	 * handling of this interrupt is appropriate.
> -	 * Return back to original instruction, and continue.
> -	 */
> +
> +	return DBG_HOOK_HANDLED;
>  }
>  
> +static struct break_hook kprobes_break_hook = {
> +	.imm = KPROBES_BRK_IMM,
> +	.fn = kprobe_breakpoint_handler,
> +};
> +
>  static int __kprobes
>  kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
>  {
> @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
>  	.fn = kprobe_breakpoint_ss_handler,
>  };
>  
> -static int __kprobes
> -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> -{
> -	kprobe_handler(regs);
> -	return DBG_HOOK_HANDLED;
> -}
> -
> -static struct break_hook kprobes_break_hook = {
> -	.imm = KPROBES_BRK_IMM,
> -	.fn = kprobe_breakpoint_handler,
> -};
> -
>  /*
>   * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
>   * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
>

On Thu, 1 Dec 2022 15:08:52 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
> > fails to find a kprobe corresponding to the address.
> > 
> > Since arm64 kprobes uses stop_machine based text patching for removing
> > BRK, it ensures all running kprobe_break_handler() is done at that point.
> > And after removing the BRK, it removes the kprobe from its hash list.
> > Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
> > there is a bug.
> 
> IIUC this relies on BRK handling not being preemptible, which is something
> we've repeatedly considered changing along with a bunch of other debug
> exception handling.

Interesting idea... and it also need many changes in kprobe itself.

> 
> In case we do try to change that in future, it would be good to have a comment
> somewhere to that effect.

Hmm, it would fundamentally change the assumptions that kprobes relies on,
and would require a lot of thought again. (e.g. current running kprobe is
stored in per-cpu variable, it should be per-task. etc.)

> 
> I think there are other ways we could synchronise against that (e.g. using RCU
> tasks rude) if we ever do that, and this patch looks good to me.
> 
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  arch/arm64/kernel/probes/kprobes.c |   79 +++++++++++++++++-------------------
> >  1 file changed, 37 insertions(+), 42 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index d2ae37f89774..ea56b22d4da8 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
> >  	return 0;
> >  }
> >  
> > -static void __kprobes kprobe_handler(struct pt_regs *regs)
> > +static int __kprobes
> > +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> >  {
> >  	struct kprobe *p, *cur_kprobe;
> >  	struct kprobe_ctlblk *kcb;
> > @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
> >  	cur_kprobe = kprobe_running();
> >  
> >  	p = get_kprobe((kprobe_opcode_t *) addr);
> > +	if (WARN_ON_ONCE(!p)) {
> > +		/*
> > +		 * Something went wrong. This must be put by kprobe, but we
> > +		 * could not find corresponding kprobes. Let the kernel handle
> > +		 * this error case.
> > +		 */
> 
> Could we make this:
> 
> 		/*
> 		 * Something went wrong. This BRK used an immediate reserved
> 		 * for kprobes, but we couldn't find any corresponding probe.
> 		 */

OK.

> 
> > +		return DBG_HOOK_ERROR;
> > +	}
> >  
> > -	if (p) {
> > -		if (cur_kprobe) {
> > -			if (reenter_kprobe(p, regs, kcb))
> > -				return;
> > -		} else {
> > -			/* Probe hit */
> > -			set_current_kprobe(p);
> > -			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > -
> > -			/*
> > -			 * If we have no pre-handler or it returned 0, we
> > -			 * continue with normal processing.  If we have a
> > -			 * pre-handler and it returned non-zero, it will
> > -			 * modify the execution path and no need to single
> > -			 * stepping. Let's just reset current kprobe and exit.
> > -			 */
> > -			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > -				setup_singlestep(p, regs, kcb, 0);
> > -			} else
> > -				reset_current_kprobe();
> > -		}
> > +	if (cur_kprobe) {
> > +		/* Hit a kprobe inside another kprobe */
> > +		if (!reenter_kprobe(p, regs, kcb))
> > +			return DBG_HOOK_ERROR;
> > +	} else {
> > +		/* Probe hit */
> > +		set_current_kprobe(p);
> > +		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > +
> > +		/*
> > +		 * If we have no pre-handler or it returned 0, we
> > +		 * continue with normal processing.  If we have a
> > +		 * pre-handler and it returned non-zero, it will
> > +		 * modify the execution path and no need to single
> > +		 * stepping. Let's just reset current kprobe and exit.
> > +		 */
> 
> Minor wording nit: could we replace:
> 
> 	no need to single stepping.
> 
> With:
> 	
> 	not need to single-step.

OK, I'll update both in v2.

Thank you!

> 
> Thanks,
> Mark.
> 
> > +		if (!p->pre_handler || !p->pre_handler(p, regs))
> > +			setup_singlestep(p, regs, kcb, 0);
> > +		else
> > +			reset_current_kprobe();
> >  	}
> > -	/*
> > -	 * The breakpoint instruction was removed right
> > -	 * after we hit it.  Another cpu has removed
> > -	 * either a probepoint or a debugger breakpoint
> > -	 * at this address.  In either case, no further
> > -	 * handling of this interrupt is appropriate.
> > -	 * Return back to original instruction, and continue.
> > -	 */
> > +
> > +	return DBG_HOOK_HANDLED;
> >  }
> >  
> > +static struct break_hook kprobes_break_hook = {
> > +	.imm = KPROBES_BRK_IMM,
> > +	.fn = kprobe_breakpoint_handler,
> > +};
> > +
> >  static int __kprobes
> >  kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
> >  {
> > @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
> >  	.fn = kprobe_breakpoint_ss_handler,
> >  };
> >  
> > -static int __kprobes
> > -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > -{
> > -	kprobe_handler(regs);
> > -	return DBG_HOOK_HANDLED;
> > -}
> > -
> > -static struct break_hook kprobes_break_hook = {
> > -	.imm = KPROBES_BRK_IMM,
> > -	.fn = kprobe_breakpoint_handler,
> > -};
> > -
> >  /*
> >   * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
> >   * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
> > 


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

On Fri, Dec 02, 2022 at 01:07:13AM +0900, Masami Hiramatsu wrote:
> On Thu, 1 Dec 2022 15:08:52 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
> > > fails to find a kprobe corresponding to the address.
> > > 
> > > Since arm64 kprobes uses stop_machine based text patching for removing
> > > BRK, it ensures all running kprobe_break_handler() is done at that point.
> > > And after removing the BRK, it removes the kprobe from its hash list.
> > > Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
> > > there is a bug.
> > 
> > IIUC this relies on BRK handling not being preemptible, which is something
> > we've repeatedly considered changing along with a bunch of other debug
> > exception handling.
> 
> Interesting idea... and it also need many changes in kprobe itself.
> 
> > 
> > In case we do try to change that in future, it would be good to have a comment
> > somewhere to that effect.
> 
> Hmm, it would fundamentally change the assumptions that kprobes relies on,
> and would require a lot of thought again. (e.g. current running kprobe is
> stored in per-cpu variable, it should be per-task. etc.)

Ah; I had not considered that.

Feel free to ignore the above; with the comments as below:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> 
> > 
> > I think there are other ways we could synchronise against that (e.g. using RCU
> > tasks rude) if we ever do that, and this patch looks good to me.
> > 
> > > 
> > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > ---
> > >  arch/arm64/kernel/probes/kprobes.c |   79 +++++++++++++++++-------------------
> > >  1 file changed, 37 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > index d2ae37f89774..ea56b22d4da8 100644
> > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
> > >  	return 0;
> > >  }
> > >  
> > > -static void __kprobes kprobe_handler(struct pt_regs *regs)
> > > +static int __kprobes
> > > +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > >  {
> > >  	struct kprobe *p, *cur_kprobe;
> > >  	struct kprobe_ctlblk *kcb;
> > > @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
> > >  	cur_kprobe = kprobe_running();
> > >  
> > >  	p = get_kprobe((kprobe_opcode_t *) addr);
> > > +	if (WARN_ON_ONCE(!p)) {
> > > +		/*
> > > +		 * Something went wrong. This must be put by kprobe, but we
> > > +		 * could not find corresponding kprobes. Let the kernel handle
> > > +		 * this error case.
> > > +		 */
> > 
> > Could we make this:
> > 
> > 		/*
> > 		 * Something went wrong. This BRK used an immediate reserved
> > 		 * for kprobes, but we couldn't find any corresponding probe.
> > 		 */
> 
> OK.
> 
> > 
> > > +		return DBG_HOOK_ERROR;
> > > +	}
> > >  
> > > -	if (p) {
> > > -		if (cur_kprobe) {
> > > -			if (reenter_kprobe(p, regs, kcb))
> > > -				return;
> > > -		} else {
> > > -			/* Probe hit */
> > > -			set_current_kprobe(p);
> > > -			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > -
> > > -			/*
> > > -			 * If we have no pre-handler or it returned 0, we
> > > -			 * continue with normal processing.  If we have a
> > > -			 * pre-handler and it returned non-zero, it will
> > > -			 * modify the execution path and no need to single
> > > -			 * stepping. Let's just reset current kprobe and exit.
> > > -			 */
> > > -			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > > -				setup_singlestep(p, regs, kcb, 0);
> > > -			} else
> > > -				reset_current_kprobe();
> > > -		}
> > > +	if (cur_kprobe) {
> > > +		/* Hit a kprobe inside another kprobe */
> > > +		if (!reenter_kprobe(p, regs, kcb))
> > > +			return DBG_HOOK_ERROR;
> > > +	} else {
> > > +		/* Probe hit */
> > > +		set_current_kprobe(p);
> > > +		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > +
> > > +		/*
> > > +		 * If we have no pre-handler or it returned 0, we
> > > +		 * continue with normal processing.  If we have a
> > > +		 * pre-handler and it returned non-zero, it will
> > > +		 * modify the execution path and no need to single
> > > +		 * stepping. Let's just reset current kprobe and exit.
> > > +		 */
> > 
> > Minor wording nit: could we replace:
> > 
> > 	no need to single stepping.
> > 
> > With:
> > 	
> > 	not need to single-step.
> 
> OK, I'll update both in v2.
> 
> Thank you!
> 
> > 
> > Thanks,
> > Mark.
> > 
> > > +		if (!p->pre_handler || !p->pre_handler(p, regs))
> > > +			setup_singlestep(p, regs, kcb, 0);
> > > +		else
> > > +			reset_current_kprobe();
> > >  	}
> > > -	/*
> > > -	 * The breakpoint instruction was removed right
> > > -	 * after we hit it.  Another cpu has removed
> > > -	 * either a probepoint or a debugger breakpoint
> > > -	 * at this address.  In either case, no further
> > > -	 * handling of this interrupt is appropriate.
> > > -	 * Return back to original instruction, and continue.
> > > -	 */
> > > +
> > > +	return DBG_HOOK_HANDLED;
> > >  }
> > >  
> > > +static struct break_hook kprobes_break_hook = {
> > > +	.imm = KPROBES_BRK_IMM,
> > > +	.fn = kprobe_breakpoint_handler,
> > > +};
> > > +
> > >  static int __kprobes
> > >  kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
> > >  {
> > > @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
> > >  	.fn = kprobe_breakpoint_ss_handler,
> > >  };
> > >  
> > > -static int __kprobes
> > > -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > > -{
> > > -	kprobe_handler(regs);
> > > -	return DBG_HOOK_HANDLED;
> > > -}
> > > -
> > > -static struct break_hook kprobes_break_hook = {
> > > -	.imm = KPROBES_BRK_IMM,
> > > -	.fn = kprobe_breakpoint_handler,
> > > -};
> > > -
> > >  /*
> > >   * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
> > >   * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
> > > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Thu, 1 Dec 2022 17:21:55 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Dec 02, 2022 at 01:07:13AM +0900, Masami Hiramatsu wrote:
> > On Thu, 1 Dec 2022 15:08:52 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > 
> > > > Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
> > > > fails to find a kprobe corresponding to the address.
> > > > 
> > > > Since arm64 kprobes uses stop_machine based text patching for removing
> > > > BRK, it ensures all running kprobe_break_handler() is done at that point.
> > > > And after removing the BRK, it removes the kprobe from its hash list.
> > > > Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
> > > > there is a bug.
> > > 
> > > IIUC this relies on BRK handling not being preemptible, which is something
> > > we've repeatedly considered changing along with a bunch of other debug
> > > exception handling.
> > 
> > Interesting idea... and it also need many changes in kprobe itself.
> > 
> > > 
> > > In case we do try to change that in future, it would be good to have a comment
> > > somewhere to that effect.
> > 
> > Hmm, it would fundamentally change the assumptions that kprobes relies on,
> > and would require a lot of thought again. (e.g. current running kprobe is
> > stored in per-cpu variable, it should be per-task. etc.)
> 
> Ah; I had not considered that.
> 
> Feel free to ignore the above; with the comments as below:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

OK, Thanks!

> 
> Thanks,
> Mark.
> 
> > 
> > > 
> > > I think there are other ways we could synchronise against that (e.g. using RCU
> > > tasks rude) if we ever do that, and this patch looks good to me.
> > > 
> > > > 
> > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > ---
> > > >  arch/arm64/kernel/probes/kprobes.c |   79 +++++++++++++++++-------------------
> > > >  1 file changed, 37 insertions(+), 42 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > > index d2ae37f89774..ea56b22d4da8 100644
> > > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > > @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static void __kprobes kprobe_handler(struct pt_regs *regs)
> > > > +static int __kprobes
> > > > +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > > >  {
> > > >  	struct kprobe *p, *cur_kprobe;
> > > >  	struct kprobe_ctlblk *kcb;
> > > > @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
> > > >  	cur_kprobe = kprobe_running();
> > > >  
> > > >  	p = get_kprobe((kprobe_opcode_t *) addr);
> > > > +	if (WARN_ON_ONCE(!p)) {
> > > > +		/*
> > > > +		 * Something went wrong. This must be put by kprobe, but we
> > > > +		 * could not find corresponding kprobes. Let the kernel handle
> > > > +		 * this error case.
> > > > +		 */
> > > 
> > > Could we make this:
> > > 
> > > 		/*
> > > 		 * Something went wrong. This BRK used an immediate reserved
> > > 		 * for kprobes, but we couldn't find any corresponding probe.
> > > 		 */
> > 
> > OK.
> > 
> > > 
> > > > +		return DBG_HOOK_ERROR;
> > > > +	}
> > > >  
> > > > -	if (p) {
> > > > -		if (cur_kprobe) {
> > > > -			if (reenter_kprobe(p, regs, kcb))
> > > > -				return;
> > > > -		} else {
> > > > -			/* Probe hit */
> > > > -			set_current_kprobe(p);
> > > > -			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > > -
> > > > -			/*
> > > > -			 * If we have no pre-handler or it returned 0, we
> > > > -			 * continue with normal processing.  If we have a
> > > > -			 * pre-handler and it returned non-zero, it will
> > > > -			 * modify the execution path and no need to single
> > > > -			 * stepping. Let's just reset current kprobe and exit.
> > > > -			 */
> > > > -			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > > > -				setup_singlestep(p, regs, kcb, 0);
> > > > -			} else
> > > > -				reset_current_kprobe();
> > > > -		}
> > > > +	if (cur_kprobe) {
> > > > +		/* Hit a kprobe inside another kprobe */
> > > > +		if (!reenter_kprobe(p, regs, kcb))
> > > > +			return DBG_HOOK_ERROR;
> > > > +	} else {
> > > > +		/* Probe hit */
> > > > +		set_current_kprobe(p);
> > > > +		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > > +
> > > > +		/*
> > > > +		 * If we have no pre-handler or it returned 0, we
> > > > +		 * continue with normal processing.  If we have a
> > > > +		 * pre-handler and it returned non-zero, it will
> > > > +		 * modify the execution path and no need to single
> > > > +		 * stepping. Let's just reset current kprobe and exit.
> > > > +		 */
> > > 
> > > Minor wording nit: could we replace:
> > > 
> > > 	no need to single stepping.
> > > 
> > > With:
> > > 	
> > > 	not need to single-step.
> > 
> > OK, I'll update both in v2.
> > 
> > Thank you!
> > 
> > > 
> > > Thanks,
> > > Mark.
> > > 
> > > > +		if (!p->pre_handler || !p->pre_handler(p, regs))
> > > > +			setup_singlestep(p, regs, kcb, 0);
> > > > +		else
> > > > +			reset_current_kprobe();
> > > >  	}
> > > > -	/*
> > > > -	 * The breakpoint instruction was removed right
> > > > -	 * after we hit it.  Another cpu has removed
> > > > -	 * either a probepoint or a debugger breakpoint
> > > > -	 * at this address.  In either case, no further
> > > > -	 * handling of this interrupt is appropriate.
> > > > -	 * Return back to original instruction, and continue.
> > > > -	 */
> > > > +
> > > > +	return DBG_HOOK_HANDLED;
> > > >  }
> > > >  
> > > > +static struct break_hook kprobes_break_hook = {
> > > > +	.imm = KPROBES_BRK_IMM,
> > > > +	.fn = kprobe_breakpoint_handler,
> > > > +};
> > > > +
> > > >  static int __kprobes
> > > >  kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
> > > >  {
> > > > @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
> > > >  	.fn = kprobe_breakpoint_ss_handler,
> > > >  };
> > > >  
> > > > -static int __kprobes
> > > > -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > > > -{
> > > > -	kprobe_handler(regs);
> > > > -	return DBG_HOOK_HANDLED;
> > > > -}
> > > > -
> > > > -static struct break_hook kprobes_break_hook = {
> > > > -	.imm = KPROBES_BRK_IMM,
> > > > -	.fn = kprobe_breakpoint_handler,
> > > > -};
> > > > -
> > > >  /*
> > > >   * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
> > > >   * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
> > > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


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