* Re: [PATCH 1/2] kprobes: Remove kprobe::fault_handler
2021-05-25 7:25 ` [PATCH 1/2] " Peter Zijlstra
@ 2021-05-25 14:06 ` Masami Hiramatsu
2021-05-26 10:50 ` Naveen N. Rao
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2021-05-25 14:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, rostedt, naveen.n.rao, ananth, x86,
Christoph Hellwig
On Tue, 25 May 2021 09:25:19 +0200
Peter Zijlstra <peterz@infradead.org> 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@kernel.org>
Thanks,
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> 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@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/2] kprobes: Remove kprobe::fault_handler
2021-05-25 7:25 ` [PATCH 1/2] " Peter Zijlstra
2021-05-25 14:06 ` Masami Hiramatsu
@ 2021-05-26 10:50 ` Naveen N. Rao
2021-05-26 13:51 ` Masami Hiramatsu
2021-06-01 12:01 ` [PATCH] kprobes: Do not increment probe miss count in the fault handler Naveen N. Rao
2021-06-01 14:04 ` [tip: perf/core] kprobes: Remove kprobe::fault_handler tip-bot2 for Peter Zijlstra
3 siblings, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2021-05-26 10:50 UTC (permalink / raw)
To: mhiramat, Peter Zijlstra
Cc: ananth, Christoph Hellwig, linux-kernel, mingo, rostedt, x86
Peter Zijlstra 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.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> 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(-)
>
<snip>
> --- 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);
Not necessarily related, but I'm wondering why we're incrementing the
probe miss count here. Unlike what the comment above indicates, this is
not a 'fault' counter, but just a count of the number of times the probe
handler wasn't called.
> -
> - /*
> - * 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;
> }
- Naveen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] kprobes: Remove kprobe::fault_handler
2021-05-26 10:50 ` Naveen N. Rao
@ 2021-05-26 13:51 ` Masami Hiramatsu
0 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2021-05-26 13:51 UTC (permalink / raw)
To: Naveen N. Rao
Cc: mhiramat, Peter Zijlstra, ananth, Christoph Hellwig,
linux-kernel, mingo, rostedt, x86
On Wed, 26 May 2021 16:20:25 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> Peter Zijlstra 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.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > 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(-)
> >
>
> <snip>
>
> > --- 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);
>
> Not necessarily related, but I'm wondering why we're incrementing the
> probe miss count here. Unlike what the comment above indicates, this is
> not a 'fault' counter, but just a count of the number of times the probe
> handler wasn't called.
Good catch! Indeed, we have no ned to count these fault because
it anyway gets back to the user handler. (so no user_handler is skipped)
Hmm, we need to clean up these countings too.
Thank you,
>
> > -
> > - /*
> > - * 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;
> > }
>
>
> - Naveen
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] kprobes: Do not increment probe miss count in the fault handler
2021-05-25 7:25 ` [PATCH 1/2] " Peter Zijlstra
2021-05-25 14:06 ` Masami Hiramatsu
2021-05-26 10:50 ` Naveen N. Rao
@ 2021-06-01 12:01 ` Naveen N. Rao
2021-06-01 13:20 ` Peter Zijlstra
2021-06-04 13:38 ` [tip: perf/core] " tip-bot2 for Naveen N. Rao
2021-06-01 14:04 ` [tip: perf/core] kprobes: Remove kprobe::fault_handler tip-bot2 for Peter Zijlstra
3 siblings, 2 replies; 13+ messages in thread
From: Naveen N. Rao @ 2021-06-01 12:01 UTC (permalink / raw)
To: mhiramat, Peter Zijlstra
Cc: ananth, Christoph Hellwig, linux-kernel, mingo, rostedt, x86
Kprobes has a counter 'nmissed', that is used to count the number of
times a probe handler was not called. This generally happens when we hit
a kprobe while handling another kprobe.
However, if one of the probe handlers causes a fault, we are currently
incrementing 'nmissed'. The comment in fault handler indicates that this
can be used to account faults taken by the probe handlers. But, this has
never been the intention as is evident from the comment above 'nmissed'
in 'struct kprobe':
/*count the number of times this probe was temporarily disarmed */
unsigned long nmissed;
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
I'm posting this here so that these can go together, if the patch is ok
otherwise.
Thanks,
- Naveen
arch/arc/kernel/kprobes.c | 6 ------
arch/arm/probes/kprobes/core.c | 14 --------------
arch/arm64/kernel/probes/kprobes.c | 7 -------
arch/csky/kernel/probes/kprobes.c | 7 -------
arch/ia64/kernel/kprobes.c | 7 -------
arch/powerpc/kernel/kprobes.c | 7 -------
arch/riscv/kernel/probes/kprobes.c | 7 -------
arch/s390/kernel/kprobes.c | 7 -------
arch/sh/kernel/kprobes.c | 7 -------
arch/sparc/kernel/kprobes.c | 7 -------
arch/x86/kernel/kprobes/core.c | 8 --------
11 files changed, 84 deletions(-)
diff --git a/arch/arc/kernel/kprobes.c b/arch/arc/kernel/kprobes.c
index 9f5b39f387362e..5f0415fc73287b 100644
--- a/arch/arc/kernel/kprobes.c
+++ b/arch/arc/kernel/kprobes.c
@@ -317,12 +317,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned long trapnr)
* caused the fault.
*/
- /* We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned zero,
* try to fix up.
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 7b9b9a5a409bb8..27e0af78e88b02 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -348,20 +348,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
reset_current_kprobe();
}
break;
-
- case KPROBE_HIT_ACTIVE:
- case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- break;
-
- default:
- break;
}
return 0;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index f6b088e9fa70e6..004b86eff9c2d9 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -276,13 +276,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/csky/kernel/probes/kprobes.c b/arch/csky/kernel/probes/kprobes.c
index e0e973e4977037..68b22b499aebf7 100644
--- a/arch/csky/kernel/probes/kprobes.c
+++ b/arch/csky/kernel/probes/kprobes.c
@@ -294,13 +294,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 6efed4ecff9e92..441ed04b103785 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -843,13 +843,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 75b4e874269d48..3f700830169fad 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -501,13 +501,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 39eaa2091efd5d..247e33fa5bc75f 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -278,13 +278,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index ad631e33df24f3..74b0bd2c24d4c1 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -445,13 +445,6 @@ static int kprobe_trap_handler(struct pt_regs *regs, int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(p);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/sh/kernel/kprobes.c b/arch/sh/kernel/kprobes.c
index 58263420ad2a58..1c7f358ef0be1c 100644
--- a/arch/sh/kernel/kprobes.c
+++ b/arch/sh/kernel/kprobes.c
@@ -382,13 +382,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/sparc/kernel/kprobes.c b/arch/sparc/kernel/kprobes.c
index db4e341b4b6ea8..4c05a4ee6a0e71 100644
--- a/arch/sparc/kernel/kprobes.c
+++ b/arch/sparc/kernel/kprobes.c
@@ -345,13 +345,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
break;
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned
* zero, try to fix up.
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index cfcdf4b8a306f3..1b3fe0edd3299f 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1102,14 +1102,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
restore_previous_kprobe(kcb);
else
reset_current_kprobe();
- } else if (kcb->kprobe_status == KPROBE_HIT_ACTIVE ||
- kcb->kprobe_status == KPROBE_HIT_SSDONE) {
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
}
return 0;
base-commit: c2131f7e73c9e9365613e323d65c7b9e5b910f56
prerequisite-patch-id: 78c74d348e7072bc263e19753ca25e04dda45981
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] kprobes: Do not increment probe miss count in the fault handler
2021-06-01 12:01 ` [PATCH] kprobes: Do not increment probe miss count in the fault handler Naveen N. Rao
@ 2021-06-01 13:20 ` Peter Zijlstra
2021-06-01 23:48 ` Masami Hiramatsu
2021-06-04 13:38 ` [tip: perf/core] " tip-bot2 for Naveen N. Rao
1 sibling, 1 reply; 13+ messages in thread
From: Peter Zijlstra @ 2021-06-01 13:20 UTC (permalink / raw)
To: Naveen N. Rao
Cc: mhiramat, ananth, Christoph Hellwig, linux-kernel, mingo, rostedt, x86
On Tue, Jun 01, 2021 at 05:31:50PM +0530, Naveen N. Rao wrote:
> Kprobes has a counter 'nmissed', that is used to count the number of
> times a probe handler was not called. This generally happens when we hit
> a kprobe while handling another kprobe.
>
> However, if one of the probe handlers causes a fault, we are currently
> incrementing 'nmissed'. The comment in fault handler indicates that this
> can be used to account faults taken by the probe handlers. But, this has
> never been the intention as is evident from the comment above 'nmissed'
> in 'struct kprobe':
>
> /*count the number of times this probe was temporarily disarmed */
> unsigned long nmissed;
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> I'm posting this here so that these can go together, if the patch is ok
> otherwise.
I had the other two queued in perf/core and was about to push then to
tip, Masami are you good with adding this on top?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kprobes: Do not increment probe miss count in the fault handler
2021-06-01 13:20 ` Peter Zijlstra
@ 2021-06-01 23:48 ` Masami Hiramatsu
0 siblings, 0 replies; 13+ messages in thread
From: Masami Hiramatsu @ 2021-06-01 23:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Naveen N. Rao, mhiramat, ananth, Christoph Hellwig, linux-kernel,
mingo, rostedt, x86
On Tue, 1 Jun 2021 15:20:32 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 01, 2021 at 05:31:50PM +0530, Naveen N. Rao wrote:
> > Kprobes has a counter 'nmissed', that is used to count the number of
> > times a probe handler was not called. This generally happens when we hit
> > a kprobe while handling another kprobe.
> >
> > However, if one of the probe handlers causes a fault, we are currently
> > incrementing 'nmissed'. The comment in fault handler indicates that this
> > can be used to account faults taken by the probe handlers. But, this has
> > never been the intention as is evident from the comment above 'nmissed'
> > in 'struct kprobe':
> >
> > /*count the number of times this probe was temporarily disarmed */
> > unsigned long nmissed;
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > I'm posting this here so that these can go together, if the patch is ok
> > otherwise.
>
> I had the other two queued in perf/core and was about to push then to
> tip, Masami are you good with adding this on top?
Yes, those looks good to me too.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tip: perf/core] kprobes: Do not increment probe miss count in the fault handler
2021-06-01 12:01 ` [PATCH] kprobes: Do not increment probe miss count in the fault handler Naveen N. Rao
2021-06-01 13:20 ` Peter Zijlstra
@ 2021-06-04 13:38 ` tip-bot2 for Naveen N. Rao
1 sibling, 0 replies; 13+ messages in thread
From: tip-bot2 for Naveen N. Rao @ 2021-06-04 13:38 UTC (permalink / raw)
To: linux-tip-commits
Cc: Naveen N. Rao, Peter Zijlstra (Intel),
Masami Hiramatsu, x86, linux-kernel
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 2e38eb04c95e5546b71bb86ee699a891c7d212b5
Gitweb: https://git.kernel.org/tip/2e38eb04c95e5546b71bb86ee699a891c7d212b5
Author: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
AuthorDate: Tue, 01 Jun 2021 17:31:50 +05:30
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 03 Jun 2021 15:47:26 +02:00
kprobes: Do not increment probe miss count in the fault handler
Kprobes has a counter 'nmissed', that is used to count the number of
times a probe handler was not called. This generally happens when we hit
a kprobe while handling another kprobe.
However, if one of the probe handlers causes a fault, we are currently
incrementing 'nmissed'. The comment in fault handler indicates that this
can be used to account faults taken by the probe handlers. But, this has
never been the intention as is evident from the comment above 'nmissed'
in 'struct kprobe':
/*count the number of times this probe was temporarily disarmed */
unsigned long nmissed;
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Link: https://lkml.kernel.org/r/20210601120150.672652-1-naveen.n.rao@linux.vnet.ibm.com
---
arch/arc/kernel/kprobes.c | 6 ------
arch/arm/probes/kprobes/core.c | 14 --------------
arch/arm64/kernel/probes/kprobes.c | 7 -------
arch/csky/kernel/probes/kprobes.c | 7 -------
arch/ia64/kernel/kprobes.c | 7 -------
arch/powerpc/kernel/kprobes.c | 7 -------
arch/riscv/kernel/probes/kprobes.c | 7 -------
arch/s390/kernel/kprobes.c | 7 -------
arch/sh/kernel/kprobes.c | 7 -------
arch/sparc/kernel/kprobes.c | 7 -------
arch/x86/kernel/kprobes/core.c | 8 --------
11 files changed, 84 deletions(-)
diff --git a/arch/arc/kernel/kprobes.c b/arch/arc/kernel/kprobes.c
index 9f5b39f..5f0415f 100644
--- a/arch/arc/kernel/kprobes.c
+++ b/arch/arc/kernel/kprobes.c
@@ -317,12 +317,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned long trapnr)
* caused the fault.
*/
- /* We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
/*
* In case the user-specified fault handler returned zero,
* try to fix up.
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 7b9b9a5..27e0af7 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -348,20 +348,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
reset_current_kprobe();
}
break;
-
- case KPROBE_HIT_ACTIVE:
- case KPROBE_HIT_SSDONE:
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- break;
-
- default:
- break;
}
return 0;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index f6b088e..004b86e 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -277,13 +277,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/csky/kernel/probes/kprobes.c b/arch/csky/kernel/probes/kprobes.c
index e0e973e..68b22b4 100644
--- a/arch/csky/kernel/probes/kprobes.c
+++ b/arch/csky/kernel/probes/kprobes.c
@@ -295,13 +295,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 6efed4e..441ed04 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -844,13 +844,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 75b4e87..3f70083 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -502,13 +502,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 923b5ea..9b71a63 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -277,13 +277,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index ad631e3..74b0bd2 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -446,13 +446,6 @@ static int kprobe_trap_handler(struct pt_regs *regs, int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(p);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/sh/kernel/kprobes.c b/arch/sh/kernel/kprobes.c
index 5826342..1c7f358 100644
--- a/arch/sh/kernel/kprobes.c
+++ b/arch/sh/kernel/kprobes.c
@@ -383,13 +383,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/sparc/kernel/kprobes.c b/arch/sparc/kernel/kprobes.c
index db4e341..4c05a4e 100644
--- a/arch/sparc/kernel/kprobes.c
+++ b/arch/sparc/kernel/kprobes.c
@@ -346,13 +346,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
case KPROBE_HIT_ACTIVE:
case KPROBE_HIT_SSDONE:
/*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
-
- /*
* In case the user-specified fault handler returned
* zero, try to fix up.
*/
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index cfcdf4b..1b3fe0e 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1102,14 +1102,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
restore_previous_kprobe(kcb);
else
reset_current_kprobe();
- } else if (kcb->kprobe_status == KPROBE_HIT_ACTIVE ||
- kcb->kprobe_status == KPROBE_HIT_SSDONE) {
- /*
- * We increment the nmissed count for accounting,
- * we can also use npre/npostfault count for accounting
- * these specific fault cases.
- */
- kprobes_inc_nmissed_count(cur);
}
return 0;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [tip: perf/core] kprobes: Remove kprobe::fault_handler
2021-05-25 7:25 ` [PATCH 1/2] " Peter Zijlstra
` (2 preceding siblings ...)
2021-06-01 12:01 ` [PATCH] kprobes: Do not increment probe miss count in the fault handler Naveen N. Rao
@ 2021-06-01 14:04 ` tip-bot2 for Peter Zijlstra
3 siblings, 0 replies; 13+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2021-06-01 14:04 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel),
Christoph Hellwig, Masami Hiramatsu, x86, linux-kernel
The following commit has been merged into the perf/core branch of tip:
Commit-ID: ec6aba3d2be1ed75b3f4c894bb64a36d40db1f55
Gitweb: https://git.kernel.org/tip/ec6aba3d2be1ed75b3f4c894bb64a36d40db1f55
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 25 May 2021 09:25:19 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 01 Jun 2021 16:00:08 +02:00
kprobes: Remove kprobe::fault_handler
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.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Link: https://lore.kernel.org/r/20210525073213.561116662@infradead.org
---
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(-)
diff --git a/Documentation/trace/kprobes.rst b/Documentation/trace/kprobes.rst
index b757b6d..998149c 100644
--- 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 @@ User's post-handler (kp->post_handler)::
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
------------------
diff --git a/arch/arc/kernel/kprobes.c b/arch/arc/kernel/kprobes.c
index cabef45..9f5b39f 100644
--- a/arch/arc/kernel/kprobes.c
+++ b/arch/arc/kernel/kprobes.c
@@ -324,16 +324,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned long trapnr)
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.
*/
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index a965311..7b9b9a5 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -358,15 +358,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
*/
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:
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d607c99..f6b088e 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -284,16 +284,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
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.
*/
diff --git a/arch/csky/kernel/probes/kprobes.c b/arch/csky/kernel/probes/kprobes.c
index 589f090..e0e973e 100644
--- a/arch/csky/kernel/probes/kprobes.c
+++ b/arch/csky/kernel/probes/kprobes.c
@@ -302,16 +302,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
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.
*/
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index fc1ff8a..6efed4e 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -851,15 +851,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
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.
*/
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index 54dfba8..75bff0f 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -403,9 +403,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
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;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 01ab216..75b4e87 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -509,16 +509,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
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.
*/
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 10b965c..923b5ea 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -284,16 +284,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
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.
*/
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index aae24dc..ad631e3 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -453,16 +453,6 @@ static int kprobe_trap_handler(struct pt_regs *regs, int trapnr)
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.
*/
diff --git a/arch/sh/kernel/kprobes.c b/arch/sh/kernel/kprobes.c
index 756100b..5826342 100644
--- a/arch/sh/kernel/kprobes.c
+++ b/arch/sh/kernel/kprobes.c
@@ -390,16 +390,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
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.
*/
diff --git a/arch/sparc/kernel/kprobes.c b/arch/sparc/kernel/kprobes.c
index 217c21a..db4e341 100644
--- a/arch/sparc/kernel/kprobes.c
+++ b/arch/sparc/kernel/kprobes.c
@@ -353,16 +353,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
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.
*/
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index d3d6554..cfcdf4b 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1110,16 +1110,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
* 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;
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1883a4a..523ffc7 100644
--- 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;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 745f08f..e41385a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1183,23 +1183,6 @@ static void aggr_post_handler(struct kprobe *p, struct pt_regs *regs,
}
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 kprobe *ap, struct kprobe *p)
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)
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) {
diff --git a/samples/kprobes/kprobe_example.c b/samples/kprobes/kprobe_example.c
index c495664..4b2f318 100644
--- a/samples/kprobes/kprobe_example.c
+++ b/samples/kprobes/kprobe_example.c
@@ -94,26 +94,11 @@ static void __kprobes handler_post(struct kprobe *p, struct pt_regs *regs,
#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) {
^ permalink raw reply related [flat|nested] 13+ messages in thread