[PATCH 0/3] Refactor patch text interfaces and mechanism

[PATCH 0/3] Refactor patch text interfaces and mechanism

[Zong Li]

This patch set contains the difference from the newest strict memory
permission. These changes are suggested by Masami Hiramatsu, including
deprecating old style of kprobe annotation, lock protection and so on.

Zong Li (3):
  riscv: Remove the 'riscv_' prefix of function name
  riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation
  riscv: Use text_mutex instead of patch_lock

 arch/riscv/include/asm/patch.h |  4 +--
 arch/riscv/kernel/ftrace.c     | 15 ++++++++++-
 arch/riscv/kernel/patch.c      | 46 ++++++++++++++++++++--------------
 3 files changed, 43 insertions(+), 22 deletions(-)

-- 
2.26.1

[PATCH 1/3] riscv: Remove the 'riscv_' prefix of function name

[Zong Li]

Refactor the function name by removing the 'riscv_' prefix, it would be
better unless it could mix up with arch-independent functions.

Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/include/asm/patch.h |  4 ++--
 arch/riscv/kernel/ftrace.c     |  2 +-
 arch/riscv/kernel/patch.c      | 22 +++++++++++-----------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
index b5918a6e0615..9a7d7346001e 100644
--- a/arch/riscv/include/asm/patch.h
+++ b/arch/riscv/include/asm/patch.h
@@ -6,7 +6,7 @@
 #ifndef _ASM_RISCV_PATCH_H
 #define _ASM_RISCV_PATCH_H
 
-int riscv_patch_text_nosync(void *addr, const void *insns, size_t len);
-int riscv_patch_text(void *addr, u32 insn);
+int patch_text_nosync(void *addr, const void *insns, size_t len);
+int patch_text(void *addr, u32 insn);
 
 #endif /* _ASM_RISCV_PATCH_H */
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index ce69b34ff55d..fb1e2b8fe254 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -51,7 +51,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
 	make_call(hook_pos, target, call);
 
 	/* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
-	if (riscv_patch_text_nosync
+	if (patch_text_nosync
 	    ((void *)hook_pos, enable ? call : nops, MCOUNT_INSN_SIZE))
 		return -EPERM;
 
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 8a4fc65ee022..de28f23f65cb 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -11,7 +11,7 @@
 #include <asm/cacheflush.h>
 #include <asm/fixmap.h>
 
-struct riscv_insn_patch {
+struct patch_insn {
 	void *addr;
 	u32 insn;
 	atomic_t cpu_count;
@@ -43,7 +43,7 @@ static void __kprobes patch_unmap(int fixmap)
 	clear_fixmap(fixmap);
 }
 
-static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
+static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
@@ -69,18 +69,18 @@ static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
 	return ret;
 }
 #else
-static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
+static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	return probe_kernel_write(addr, insn, len);
 }
 #endif /* CONFIG_MMU */
 
-int __kprobes riscv_patch_text_nosync(void *addr, const void *insns, size_t len)
+int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
 {
 	u32 *tp = addr;
 	int ret;
 
-	ret = riscv_insn_write(tp, insns, len);
+	ret = patch_insn_write(tp, insns, len);
 
 	if (!ret)
 		flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
@@ -88,14 +88,14 @@ int __kprobes riscv_patch_text_nosync(void *addr, const void *insns, size_t len)
 	return ret;
 }
 
-static int __kprobes riscv_patch_text_cb(void *data)
+static int __kprobes patch_text_cb(void *data)
 {
-	struct riscv_insn_patch *patch = data;
+	struct patch_insn *patch = data;
 	int ret = 0;
 
 	if (atomic_inc_return(&patch->cpu_count) == 1) {
 		ret =
-		    riscv_patch_text_nosync(patch->addr, &patch->insn,
+		    patch_text_nosync(patch->addr, &patch->insn,
 					    GET_INSN_LENGTH(patch->insn));
 		atomic_inc(&patch->cpu_count);
 	} else {
@@ -107,14 +107,14 @@ static int __kprobes riscv_patch_text_cb(void *data)
 	return ret;
 }
 
-int __kprobes riscv_patch_text(void *addr, u32 insn)
+int __kprobes patch_text(void *addr, u32 insn)
 {
-	struct riscv_insn_patch patch = {
+	struct patch_insn patch = {
 		.addr = addr,
 		.insn = insn,
 		.cpu_count = ATOMIC_INIT(0),
 	};
 
-	return stop_machine_cpuslocked(riscv_patch_text_cb,
+	return stop_machine_cpuslocked(patch_text_cb,
 				       &patch, cpu_online_mask);
 }
-- 
2.26.1

[PATCH 2/3] riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation

[Zong Li]

The __kprobes annotation is old style, so change it to NOKPROBE_SYMBOL().

Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/kernel/patch.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index de28f23f65cb..8acb9ae2da08 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -20,7 +20,7 @@ struct patch_insn {
 #ifdef CONFIG_MMU
 static DEFINE_RAW_SPINLOCK(patch_lock);
 
-static void __kprobes *patch_map(void *addr, int fixmap)
+static void *patch_map(void *addr, int fixmap)
 {
 	uintptr_t uintaddr = (uintptr_t) addr;
 	struct page *page;
@@ -37,13 +37,15 @@ static void __kprobes *patch_map(void *addr, int fixmap)
 	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
 					 (uintaddr & ~PAGE_MASK));
 }
+NOKPROBE_SYMBOL(patch_map);
 
-static void __kprobes patch_unmap(int fixmap)
+static void patch_unmap(int fixmap)
 {
 	clear_fixmap(fixmap);
 }
+NOKPROBE_SYMBOL(patch_unmap);
 
-static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
+static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
@@ -68,14 +70,16 @@ static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
 
 	return ret;
 }
+NOKPROBE_SYMBOL(patch_insn_write);
 #else
-static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
+static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	return probe_kernel_write(addr, insn, len);
 }
+NOKPROBE_SYMBOL(patch_insn_write);
 #endif /* CONFIG_MMU */
 
-int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
+int patch_text_nosync(void *addr, const void *insns, size_t len)
 {
 	u32 *tp = addr;
 	int ret;
@@ -87,8 +91,9 @@ int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
 
 	return ret;
 }
+NOKPROBE_SYMBOL(patch_text_nosync);
 
-static int __kprobes patch_text_cb(void *data)
+static int patch_text_cb(void *data)
 {
 	struct patch_insn *patch = data;
 	int ret = 0;
@@ -106,8 +111,9 @@ static int __kprobes patch_text_cb(void *data)
 
 	return ret;
 }
+NOKPROBE_SYMBOL(patch_text_cb);
 
-int __kprobes patch_text(void *addr, u32 insn)
+int patch_text(void *addr, u32 insn)
 {
 	struct patch_insn patch = {
 		.addr = addr,
@@ -118,3 +124,4 @@ int __kprobes patch_text(void *addr, u32 insn)
 	return stop_machine_cpuslocked(patch_text_cb,
 				       &patch, cpu_online_mask);
 }
+NOKPROBE_SYMBOL(patch_text);
-- 
2.26.1

[PATCH 3/3] riscv: Use text_mutex instead of patch_lock

[Zong Li]

We don't need the additional lock protection when patching the text.

There are two patching interfaces here:
 - patch_text: patch code and always synchronize with stop_machine()
 - patch_text_nosync: patch code without synchronization, it's caller's
                      responsibility to synchronize all CPUs if needed.

For the first one, stop_machine() is protected by its own mutex, and
also the irq is already disabled here.

For the second one, in risc-v real case now, it would be used to ftrace
patching the mcount function, since it already running under
kstop_machine(), no other thread will run, so we could use text_mutex
on ftrace side.

Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/kernel/ftrace.c | 13 +++++++++++++
 arch/riscv/kernel/patch.c  | 13 +++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index fb1e2b8fe254..08396614d6f4 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -7,10 +7,23 @@
 
 #include <linux/ftrace.h>
 #include <linux/uaccess.h>
+#include <linux/memory.h>
 #include <asm/cacheflush.h>
 #include <asm/patch.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)
+{
+	mutex_lock(&text_mutex);
+	return 0;
+}
+
+int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
+{
+	mutex_unlock(&text_mutex);
+	return 0;
+}
+
 static int ftrace_check_current_call(unsigned long hook_pos,
 				     unsigned int *expected)
 {
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 8acb9ae2da08..5805791cd5b5 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -5,6 +5,7 @@
 
 #include <linux/spinlock.h>
 #include <linux/mm.h>
+#include <linux/memory.h>
 #include <linux/uaccess.h>
 #include <linux/stop_machine.h>
 #include <asm/kprobes.h>
@@ -18,8 +19,6 @@ struct patch_insn {
 };
 
 #ifdef CONFIG_MMU
-static DEFINE_RAW_SPINLOCK(patch_lock);
-
 static void *patch_map(void *addr, int fixmap)
 {
 	uintptr_t uintaddr = (uintptr_t) addr;
@@ -49,10 +48,14 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
-	unsigned long flags = 0;
 	int ret;
 
-	raw_spin_lock_irqsave(&patch_lock, flags);
+	/*
+	 * Before reaching here, it was expected to lock the text_mutex
+	 * already, so we don't need to give another lock here and could
+	 * ensure that it was safe between each cores.
+	 */
+	lockdep_assert_held(&text_mutex);
 
 	if (across_pages)
 		patch_map(addr + len, FIX_TEXT_POKE1);
@@ -66,8 +69,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
 	if (across_pages)
 		patch_unmap(FIX_TEXT_POKE1);
 
-	raw_spin_unlock_irqrestore(&patch_lock, flags);
-
 	return ret;
 }
 NOKPROBE_SYMBOL(patch_insn_write);
-- 
2.26.1

Re: [PATCH 0/3] Refactor patch text interfaces and mechanism

[Palmer Dabbelt]

On Tue, 21 Apr 2020 00:29:58 PDT (-0700), zong.li@sifive.com wrote:
> This patch set contains the difference from the newest strict memory
> permission. These changes are suggested by Masami Hiramatsu, including
> deprecating old style of kprobe annotation, lock protection and so on.
>
> Zong Li (3):
>   riscv: Remove the 'riscv_' prefix of function name
>   riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation
>   riscv: Use text_mutex instead of patch_lock
>
>  arch/riscv/include/asm/patch.h |  4 +--
>  arch/riscv/kernel/ftrace.c     | 15 ++++++++++-
>  arch/riscv/kernel/patch.c      | 46 ++++++++++++++++++++--------------
>  3 files changed, 43 insertions(+), 22 deletions(-)

Thanks, this is on for-next.