Re: [PATCH 1/2] kprobes: Remove kprobe::fault_handler

[Masami Hiramatsu]

On Tue, 25 May 2021 09:25:19 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> The reason for kprobe::fault_handler(), as given by their comment:
> 
>  * We come here because instructions in the pre/post
>  * handler caused the page_fault, this could happen
>  * if handler tries to access user space by
>  * copy_from_user(), get_user() etc. Let the
>  * user-specified handler try to fix it first.
> 
> Is just plain bad. Those other handlers are ran from non-preemptible
> context and had better use _nofault() functions. Also, there is no
> upstream usage of this.

OK, this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thanks,

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  Documentation/trace/kprobes.rst    |   24 +++++-------------------
>  arch/arc/kernel/kprobes.c          |   10 ----------
>  arch/arm/probes/kprobes/core.c     |    9 ---------
>  arch/arm64/kernel/probes/kprobes.c |   10 ----------
>  arch/csky/kernel/probes/kprobes.c  |   10 ----------
>  arch/ia64/kernel/kprobes.c         |    9 ---------
>  arch/mips/kernel/kprobes.c         |    3 ---
>  arch/powerpc/kernel/kprobes.c      |   10 ----------
>  arch/riscv/kernel/probes/kprobes.c |   10 ----------
>  arch/s390/kernel/kprobes.c         |   10 ----------
>  arch/sh/kernel/kprobes.c           |   10 ----------
>  arch/sparc/kernel/kprobes.c        |   10 ----------
>  arch/x86/kernel/kprobes/core.c     |   10 ----------
>  include/linux/kprobes.h            |    8 --------
>  kernel/kprobes.c                   |   19 -------------------
>  samples/kprobes/kprobe_example.c   |   15 ---------------
>  16 files changed, 5 insertions(+), 172 deletions(-)
> 
> --- a/Documentation/trace/kprobes.rst
> +++ b/Documentation/trace/kprobes.rst
> @@ -362,14 +362,11 @@ register_kprobe
>  	#include <linux/kprobes.h>
>  	int register_kprobe(struct kprobe *kp);
>  
> -Sets a breakpoint at the address kp->addr.  When the breakpoint is
> -hit, Kprobes calls kp->pre_handler.  After the probed instruction
> -is single-stepped, Kprobe calls kp->post_handler.  If a fault
> -occurs during execution of kp->pre_handler or kp->post_handler,
> -or during single-stepping of the probed instruction, Kprobes calls
> -kp->fault_handler.  Any or all handlers can be NULL. If kp->flags
> -is set KPROBE_FLAG_DISABLED, that kp will be registered but disabled,
> -so, its handlers aren't hit until calling enable_kprobe(kp).
> +Sets a breakpoint at the address kp->addr.  When the breakpoint is hit, Kprobes
> +calls kp->pre_handler.  After the probed instruction is single-stepped, Kprobe
> +calls kp->post_handler.  Any or all handlers can be NULL. If kp->flags is set
> +KPROBE_FLAG_DISABLED, that kp will be registered but disabled, so, its handlers
> +aren't hit until calling enable_kprobe(kp).
>  
>  .. note::
>  
> @@ -415,17 +412,6 @@ the breakpoint was hit.  Return 0 here u
>  p and regs are as described for the pre_handler.  flags always seems
>  to be zero.
>  
> -User's fault-handler (kp->fault_handler)::
> -
> -	#include <linux/kprobes.h>
> -	#include <linux/ptrace.h>
> -	int fault_handler(struct kprobe *p, struct pt_regs *regs, int trapnr);
> -
> -p and regs are as described for the pre_handler.  trapnr is the
> -architecture-specific trap number associated with the fault (e.g.,
> -on i386, 13 for a general protection fault or 14 for a page fault).
> -Returns 1 if it successfully handled the exception.
> -
>  register_kretprobe
>  ------------------
>  
> --- a/arch/arc/kernel/kprobes.c
> +++ b/arch/arc/kernel/kprobes.c
> @@ -324,16 +324,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned zero,
>  		 * try to fix up.
>  		 */
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -358,15 +358,6 @@ int __kprobes kprobe_fault_handler(struc
>  		 */
>  		kprobes_inc_nmissed_count(cur);
>  
> -		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, fsr))
> -			return 1;
>  		break;
>  
>  	default:
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -283,16 +283,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, fsr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/csky/kernel/probes/kprobes.c
> +++ b/arch/csky/kernel/probes/kprobes.c
> @@ -302,16 +302,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/ia64/kernel/kprobes.c
> +++ b/arch/ia64/kernel/kprobes.c
> @@ -851,15 +851,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/mips/kernel/kprobes.c
> +++ b/arch/mips/kernel/kprobes.c
> @@ -403,9 +403,6 @@ int kprobe_fault_handler(struct pt_regs
>  	struct kprobe *cur = kprobe_running();
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  
> -	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -		return 1;
> -
>  	if (kcb->kprobe_status & KPROBE_HIT_SS) {
>  		resume_execution(cur, regs, kcb);
>  		regs->cp0_status |= kcb->kprobe_old_SR;
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -509,16 +509,6 @@ int kprobe_fault_handler(struct pt_regs
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -275,16 +275,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/s390/kernel/kprobes.c
> +++ b/arch/s390/kernel/kprobes.c
> @@ -453,16 +453,6 @@ static int kprobe_trap_handler(struct pt
>  		kprobes_inc_nmissed_count(p);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (p->fault_handler && p->fault_handler(p, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/sh/kernel/kprobes.c
> +++ b/arch/sh/kernel/kprobes.c
> @@ -390,16 +390,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/sparc/kernel/kprobes.c
> +++ b/arch/sparc/kernel/kprobes.c
> @@ -353,16 +353,6 @@ int __kprobes kprobe_fault_handler(struc
>  		kprobes_inc_nmissed_count(cur);
>  
>  		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -
> -		/*
>  		 * In case the user-specified fault handler returned
>  		 * zero, try to fix up.
>  		 */
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -947,16 +947,6 @@ int kprobe_fault_handler(struct pt_regs
>  		 * these specific fault cases.
>  		 */
>  		kprobes_inc_nmissed_count(cur);
> -
> -		/*
> -		 * We come here because instructions in the pre/post
> -		 * handler caused the page_fault, this could happen
> -		 * if handler tries to access user space by
> -		 * copy_from_user(), get_user() etc. Let the
> -		 * user-specified handler try to fix it first.
> -		 */
> -		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> -			return 1;
>  	}
>  
>  	return 0;
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -54,8 +54,6 @@ struct kretprobe_instance;
>  typedef int (*kprobe_pre_handler_t) (struct kprobe *, struct pt_regs *);
>  typedef void (*kprobe_post_handler_t) (struct kprobe *, struct pt_regs *,
>  				       unsigned long flags);
> -typedef int (*kprobe_fault_handler_t) (struct kprobe *, struct pt_regs *,
> -				       int trapnr);
>  typedef int (*kretprobe_handler_t) (struct kretprobe_instance *,
>  				    struct pt_regs *);
>  
> @@ -83,12 +81,6 @@ struct kprobe {
>  	/* Called after addr is executed, unless... */
>  	kprobe_post_handler_t post_handler;
>  
> -	/*
> -	 * ... called if executing addr causes a fault (eg. page fault).
> -	 * Return 1 if it handled fault, otherwise kernel will see it.
> -	 */
> -	kprobe_fault_handler_t fault_handler;
> -
>  	/* Saved opcode (which has been replaced with breakpoint) */
>  	kprobe_opcode_t opcode;
>  
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1183,23 +1183,6 @@ static void aggr_post_handler(struct kpr
>  }
>  NOKPROBE_SYMBOL(aggr_post_handler);
>  
> -static int aggr_fault_handler(struct kprobe *p, struct pt_regs *regs,
> -			      int trapnr)
> -{
> -	struct kprobe *cur = __this_cpu_read(kprobe_instance);
> -
> -	/*
> -	 * if we faulted "during" the execution of a user specified
> -	 * probe handler, invoke just that probe's fault handler
> -	 */
> -	if (cur && cur->fault_handler) {
> -		if (cur->fault_handler(cur, regs, trapnr))
> -			return 1;
> -	}
> -	return 0;
> -}
> -NOKPROBE_SYMBOL(aggr_fault_handler);
> -
>  /* Walks the list and increments nmissed count for multiprobe case */
>  void kprobes_inc_nmissed_count(struct kprobe *p)
>  {
> @@ -1330,7 +1313,6 @@ static void init_aggr_kprobe(struct kpro
>  	ap->addr = p->addr;
>  	ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED;
>  	ap->pre_handler = aggr_pre_handler;
> -	ap->fault_handler = aggr_fault_handler;
>  	/* We don't care the kprobe which has gone. */
>  	if (p->post_handler && !kprobe_gone(p))
>  		ap->post_handler = aggr_post_handler;
> @@ -2014,7 +1996,6 @@ int register_kretprobe(struct kretprobe
>  
>  	rp->kp.pre_handler = pre_handler_kretprobe;
>  	rp->kp.post_handler = NULL;
> -	rp->kp.fault_handler = NULL;
>  
>  	/* Pre-allocate memory for max kretprobe instances */
>  	if (rp->maxactive <= 0) {
> --- a/samples/kprobes/kprobe_example.c
> +++ b/samples/kprobes/kprobe_example.c
> @@ -86,26 +86,11 @@ static void __kprobes handler_post(struc
>  #endif
>  }
>  
> -/*
> - * fault_handler: this is called if an exception is generated for any
> - * instruction within the pre- or post-handler, or when Kprobes
> - * single-steps the probed instruction.
> - */
> -static int handler_fault(struct kprobe *p, struct pt_regs *regs, int trapnr)
> -{
> -	pr_info("fault_handler: p->addr = 0x%p, trap #%dn", p->addr, trapnr);
> -	/* Return 0 because we don't handle the fault. */
> -	return 0;
> -}
> -/* NOKPROBE_SYMBOL() is also available */
> -NOKPROBE_SYMBOL(handler_fault);
> -
>  static int __init kprobe_init(void)
>  {
>  	int ret;
>  	kp.pre_handler = handler_pre;
>  	kp.post_handler = handler_post;
> -	kp.fault_handler = handler_fault;
>  
>  	ret = register_kprobe(&kp);
>  	if (ret < 0) {
> 
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>