All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arm64: kprobes: Remove redundant kprobe_step_ctx
Date: Fri, 30 Oct 2020 09:27:19 +0100	[thread overview]
Message-ID: <20201030082719.GA122147@myrica> (raw)
In-Reply-To: <160402038829.313559.6664989279479113647.stgit@devnote2>

On Fri, Oct 30, 2020 at 10:13:08AM +0900, Masami Hiramatsu wrote:
> The kprobe_step_ctx (kcb->ss_ctx) has ss_pending and match_addr, but
> those are redundant because those can be replaced by KPROBE_HIT_SS and
> &cur_kprobe->ainsn.api.insn[1] respectively.
> To simplify the code, remove the kprobe_step_ctx.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---
>  arch/arm64/include/asm/kprobes.h   |    7 -----
>  arch/arm64/kernel/probes/kprobes.c |   53 ++++++++----------------------------
>  2 files changed, 12 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> index 8699ce30f587..5d38ff4a4806 100644
> --- a/arch/arm64/include/asm/kprobes.h
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -28,18 +28,11 @@ struct prev_kprobe {
>  	unsigned int status;
>  };
>  
> -/* Single step context for kprobe */
> -struct kprobe_step_ctx {
> -	unsigned long ss_pending;
> -	unsigned long match_addr;
> -};
> -
>  /* per-cpu kprobe control block */
>  struct kprobe_ctlblk {
>  	unsigned int kprobe_status;
>  	unsigned long saved_irqflag;
>  	struct prev_kprobe prev_kprobe;
> -	struct kprobe_step_ctx ss_ctx;
>  };
>  
>  void arch_remove_kprobe(struct kprobe *);
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index ec1446ceacc9..3c74fc9e2acf 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -34,7 +34,7 @@ DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
>  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  
>  static void __kprobes
> -post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
> +post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
>  
>  static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opc1, u32 opc2)
>  {
> @@ -81,7 +81,7 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
>  		p->ainsn.api.handler((u32)p->opcode, (long)p->addr, regs);
>  
>  	/* single step simulated, now go for post processing */
> -	post_kprobe_handler(kcb, regs);
> +	post_kprobe_handler(p, kcb, regs);
>  }
>  
>  int __kprobes arch_prepare_kprobe(struct kprobe *p)
> @@ -184,19 +184,6 @@ static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
>  	regs->pstate |= kcb->saved_irqflag;
>  }
>  
> -static void __kprobes
> -set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr)
> -{
> -	kcb->ss_ctx.ss_pending = true;
> -	kcb->ss_ctx.match_addr = addr + sizeof(kprobe_opcode_t);
> -}
> -
> -static void __kprobes clear_ss_context(struct kprobe_ctlblk *kcb)
> -{
> -	kcb->ss_ctx.ss_pending = false;
> -	kcb->ss_ctx.match_addr = 0;
> -}
> -
>  static void __kprobes setup_singlestep(struct kprobe *p,
>  				       struct pt_regs *regs,
>  				       struct kprobe_ctlblk *kcb, int reenter)
> @@ -216,7 +203,6 @@ static void __kprobes setup_singlestep(struct kprobe *p,
>  		/* prepare for single stepping */
>  		slot = (unsigned long)p->ainsn.api.insn;
>  
> -		set_ss_context(kcb, slot);	/* mark pending ss */
>  		kprobes_save_local_irqflag(kcb, regs);
>  		instruction_pointer_set(regs, slot);
>  	} else {
> @@ -250,13 +236,8 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
>  }
>  
>  static void __kprobes
> -post_kprobe_handler(struct kprobe_ctlblk *kcb, struct pt_regs *regs)
> +post_kprobe_handler(struct kprobe *cur, struct kprobe_ctlblk *kcb, struct pt_regs *regs)
>  {
> -	struct kprobe *cur = kprobe_running();
> -
> -	if (!cur)
> -		return;
> -
>  	/* return addr restore if non-branching insn */
>  	if (cur->ainsn.api.restore != 0)
>  		instruction_pointer_set(regs, cur->ainsn.api.restore);
> @@ -371,33 +352,23 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
>  	 */
>  }
>  
> -static int __kprobes
> -kprobe_ss_hit(struct kprobe_ctlblk *kcb, unsigned long addr)
> -{
> -	if ((kcb->ss_ctx.ss_pending)
> -	    && (kcb->ss_ctx.match_addr == addr)) {
> -		clear_ss_context(kcb);	/* clear pending ss */
> -		return DBG_HOOK_HANDLED;
> -	}
> -	/* not ours, kprobes should ignore it */
> -	return DBG_HOOK_ERROR;
> -}
> -
>  static int __kprobes
>  kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned int esr)
>  {
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> -	int retval;
> -
> -	/* return error if this is not our step */
> -	retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
> +	unsigned long addr = instruction_pointer(regs);
> +	struct kprobe *cur = kprobe_running();
>  
> -	if (retval == DBG_HOOK_HANDLED) {
> +	if (cur && (kcb->kprobe_status == KPROBE_HIT_SS)
> +	    && ((unsigned long)&cur->ainsn.api.insn[1] == addr)) {
>  		kprobes_restore_local_irqflag(kcb, regs);
> -		post_kprobe_handler(kcb, regs);
> +		post_kprobe_handler(cur, kcb, regs);
> +
> +		return DBG_HOOK_HANDLED;
>  	}
>  
> -	return retval;
> +	/* not ours, kprobes should ignore it */
> +	return DBG_HOOK_ERROR;
>  }
>  
>  static struct break_hook kprobes_break_ss_hook = {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-10-30  8:29 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29 17:34 [PATCH] arm64: kprobes: Use BRK instead of single-step when executing instructions out-of-line Jean-Philippe Brucker
2020-10-29 23:34 ` Masami Hiramatsu
2020-10-30  1:13 ` [PATCH] arm64: kprobes: Remove redundant kprobe_step_ctx Masami Hiramatsu
2020-10-30  8:27   ` Jean-Philippe Brucker [this message]
2020-10-30 16:06 ` [PATCH] arm64: kprobes: Use BRK instead of single-step when executing instructions out-of-line Will Deacon
2020-10-30 16:13   ` Jean-Philippe Brucker
2020-11-02 11:41 ` Will Deacon
2020-11-02 13:52   ` Masami Hiramatsu
2020-11-03  9:13     ` Jean-Philippe Brucker
2020-11-03  9:23       ` Will Deacon
2020-11-17 17:28         ` Jean-Philippe Brucker
2020-11-24  2:34           ` Masami Hiramatsu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201030082719.GA122147@myrica \
    --to=jean-philippe@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=mhiramat@kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.