All of lore.kernel.org
 help / color / mirror / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>
Cc: x86@kernel.org, Masami Hiramatsu <mhiramat@kernel.org>,
	Yang Bo <yangbo@deepin.com>, Ingo Molnar <mingo@redhat.com>,
	"H . Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org,
	Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Laura Abbott <labbott@redhat.com>, Josef Bacik <jbacik@fb.com>,
	Alexei Starovoitov <ast@kernel.org>
Subject: [RFC PATCH -tip 7/9] x86: kprobes: Do not disable preempt on int3 path
Date: Fri,  9 Mar 2018 21:38:46 +0900	[thread overview]
Message-ID: <152059912643.4214.16269633432646853400.stgit@devbox> (raw)
In-Reply-To: <152059891712.4214.9539716544254881611.stgit@devbox>

Since int3 and debug exception(for singlestep) are run with
IRQ disabled and while running single stepping we drop IF
from regs->flags, that path must not be preemptible. So we
can remove the preempt disable/enable calls from that path.

Note that, this changes the behavior of execution path override
which is done by modifying regs->ip in pre_handler().
Previously it requires reset_current_kprobe(), enable preempt
and return !0.
With this change, preempt count is not changed on int3 path, so
user no need to enable preempt. To fit this behavior, this
modifies ftrace-based kprobe too. In total, pre_handler() does
not need to recover preempt count even if it changes regs->ip.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/kprobes.txt        |   11 +++++------
 arch/x86/kernel/kprobes/core.c   |   14 +++-----------
 arch/x86/kernel/kprobes/ftrace.c |    5 ++---
 arch/x86/kernel/kprobes/opt.c    |    1 -
 4 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 94df43224c6c..679cba3380e4 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -566,12 +566,11 @@ the same handler) may run concurrently on different CPUs.
 Kprobes does not use mutexes or allocate memory except during
 registration and unregistration.
 
-Probe handlers are run with preemption disabled.  Depending on the
-architecture and optimization state, handlers may also run with
-interrupts disabled (e.g., kretprobe handlers and optimized kprobe
-handlers run without interrupt disabled on x86/x86-64).  In any case,
-your handler should not yield the CPU (e.g., by attempting to acquire
-a semaphore).
+Probe handlers are run with preemption disabled or interrupt disabled,
+which depends on the architecture and optimization state.  (e.g.,
+kretprobe handlers and optimized kprobe handlers run without interrupt
+disabled on x86/x86-64).  In any case, your handler should not yield
+the CPU (e.g., by attempting to acquire a semaphore, or waiting I/O).
 
 Since a return probe is implemented by replacing the return
 address with the trampoline's address, stack backtraces and calls
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 7663f23aa860..b771a01d51f4 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -592,7 +592,6 @@ static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
 		 * stepping.
 		 */
 		regs->ip = (unsigned long)p->ainsn.insn;
-		preempt_enable_no_resched();
 		return;
 	}
 #endif
@@ -665,12 +664,10 @@ int kprobe_int3_handler(struct pt_regs *regs)
 
 	addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
 	/*
-	 * We don't want to be preempted for the entire
-	 * duration of kprobe processing. We conditionally
-	 * re-enable preemption at the end of this function,
-	 * and also in reenter_kprobe() and setup_singlestep().
+	 * We don't want to be preempted for the entire duration of kprobe
+	 * processing. Since int3 and debug trap disables irqs and we clear
+	 * IF while singlestepping, it must be no preemptible.
 	 */
-	preempt_disable();
 
 	kcb = get_kprobe_ctlblk();
 	p = get_kprobe(addr);
@@ -704,11 +701,9 @@ int kprobe_int3_handler(struct pt_regs *regs)
 		 * the original instruction.
 		 */
 		regs->ip = (unsigned long)addr;
-		preempt_enable_no_resched();
 		return 1;
 	} /* else: not a kprobe fault; let the kernel handle it */
 
-	preempt_enable_no_resched();
 	return 0;
 }
 NOKPROBE_SYMBOL(kprobe_int3_handler);
@@ -959,8 +954,6 @@ int kprobe_debug_handler(struct pt_regs *regs)
 	}
 	reset_current_kprobe();
 out:
-	preempt_enable_no_resched();
-
 	/*
 	 * if somebody else is singlestepping across a probe point, flags
 	 * will have TF set, in which case, continue the remaining processing
@@ -1007,7 +1000,6 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 			restore_previous_kprobe(kcb);
 		else
 			reset_current_kprobe();
-		preempt_enable_no_resched();
 	} else if (kcb->kprobe_status == KPROBE_HIT_ACTIVE ||
 		   kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 		/*
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index c8696f2a583f..f1ac65d6b352 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -67,10 +67,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		preempt_disable();
 		__this_cpu_write(current_kprobe, p);
 		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
-		if (!p->pre_handler || !p->pre_handler(p, regs)) {
+		if (!p->pre_handler || !p->pre_handler(p, regs))
 			skip_singlestep(p, regs, kcb, orig_ip);
-			preempt_enable_no_resched();
-		}
+		preempt_enable_no_resched();
 		/*
 		 * If pre_handler returns !0, it sets regs->ip and
 		 * resets current kprobe, and keep preempt count +1.
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 203d398802a3..eaf02f2e7300 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -491,7 +491,6 @@ int setup_detour_execution(struct kprobe *p, struct pt_regs *regs, int reenter)
 		regs->ip = (unsigned long)op->optinsn.insn + TMPL_END_IDX;
 		if (!reenter)
 			reset_current_kprobe();
-		preempt_enable_no_resched();
 		return 1;
 	}
 	return 0;

  parent reply	other threads:[~2018-03-09 12:39 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-09 12:35 [RFC PATCH -tip 0/9] kprobes: Cleanup jprobe implementation Masami Hiramatsu
2018-03-09 12:35 ` [RFC PATCH -tip 1/9] kprobes: Remove jprobe API implementation Masami Hiramatsu
2018-03-09 12:36 ` [RFC PATCH -tip 2/9] x86: kprobes: Remove jprobe implementation Masami Hiramatsu
2018-03-09 12:36 ` [RFC PATCH -tip 3/9] kprobes: Remove jprobe data structure and interfaces Masami Hiramatsu
2018-03-09 12:37 ` [RFC PATCH -tip 4/9] kprobes: Ignore break_handler Masami Hiramatsu
2018-03-09 12:37 ` [RFC PATCH -tip 5/9] x86: " Masami Hiramatsu
2018-03-09 12:38 ` [RFC PATCH -tip 6/9] kprobes: Remove break_handler from struct kprobe Masami Hiramatsu
2018-03-09 12:38 ` Masami Hiramatsu [this message]
2018-03-09 12:39 ` [RFC PATCH -tip 8/9] error-injection: Fix to not enabling preemption in pre_handler Masami Hiramatsu
2018-03-10  8:22   ` Ingo Molnar
2018-03-11 15:05     ` Masami Hiramatsu
2018-03-09 12:39 ` [RFC PATCH -tip 9/9] tracing: kprobes: Fix to not enabling preemption Masami Hiramatsu
2018-03-09 23:54 ` [RFC PATCH -tip 0/9] kprobes: Cleanup jprobe implementation Steven Rostedt
2018-03-10 13:55   ` Masami Hiramatsu
2018-03-10  8:24 ` Ingo Molnar
2018-03-11 15:13   ` 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=152059912643.4214.16269633432646853400.stgit@devbox \
    --to=mhiramat@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ananth@linux.vnet.ibm.com \
    --cc=ast@kernel.org \
    --cc=hpa@zytor.com \
    --cc=jbacik@fb.com \
    --cc=labbott@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --cc=yangbo@deepin.com \
    /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.