[PATCH v2 0/4] kprobes: permit use without modules

[PATCH v2 0/4] kprobes: permit use without modules

[Mark Rutland]

Currently KPROBES depends on MODULES and cannot be built when support
for modules is not enabled. This is largely an artifact of the
default/generic code for allocating insn pages using module_alloc(),
though several architectures do not use this and have no strict
dependency on MODULES. It would be nice to allow KPROBES to be used
without MODULES, as this can be useful for testing and/or in certain
constrained environments.

This series (based on v6.9-rc1) removes the artificial dependency on
MODULES. This permits (but does not require) that architectures which
don't use module_alloc() to allocate kprobe insn pages can support
kprobes when module support is not enabled.

The series deliberately avoids adding a common text allocator, as the
requirements for allocating kprobe memory van vary by architecture, and
can differ from other text allocations. However, architectures can
easily call a common allocator if they wish, and this series does not
preclude using common allocators immediately or in future.

The key change is in patch 3. This requires that architectures which
provide their own alloc function must provide the corresponding free
function and select HAVE_KPROBES_ALLOC with any appropriate dependencies
for their implementation. Architectures which use the generic functions
are left as-is with a dependency on MODULES.

The final patch allows the core kprobes code to be built without
MODULES, and removes the explicit dependency from Kconfig. This is
derived from Jarkko's recent v6 attempt:

  https://lore.kernel.org/lkml/20240326012102.27438-1-jarkko@kernel.org/
 
With the series applied, arm64 and riscv can enable KPROBES without
MODULES, while powerpc/s390/x86 are still depend on MODULES as their
alloc functions currently use module_alloc(), and all other
architectures with KPROBES uses the generic implementation that depends
on MODULES. I believe it should be relatively easy to enable
powerpc/s390/x86 to not depend on MODULES.

Since v1 [1]:
* Rebase to v6.9-rc2 (trivial)
* Update to Jarkko's v7 cleanups for core kprobes
* Fix accidental use of KPROBES_USE_MODULE_ALLOC
* Improve commit messages
* Update Cc lists

[1] https://lore.kernel.org/lkml/20240326163624.3253157-1-mark.rutland@arm.com/

Mark.

Jarkko Sakkinen (1):
  kprobes: Remove core dependency on modules

Mark Rutland (3):
  arm64: patching: always use fixmap
  kprobes/treewide: Add kprobes_ prefix to insn alloc/free functions
  kprobes/treewide: Explicitly override alloc/free functions

 arch/Kconfig                       |  5 ++-
 arch/arm64/Kconfig                 |  1 +
 arch/arm64/kernel/patching.c       | 10 ++---
 arch/arm64/kernel/probes/kprobes.c |  7 ++-
 arch/powerpc/Kconfig               |  3 +-
 arch/powerpc/kernel/kprobes.c      |  7 ++-
 arch/powerpc/kernel/optprobes.c    |  4 +-
 arch/riscv/Kconfig                 |  1 +
 arch/riscv/kernel/probes/kprobes.c |  7 ++-
 arch/s390/Kconfig                  |  3 +-
 arch/s390/kernel/kprobes.c         |  7 ++-
 arch/x86/Kconfig                   |  3 +-
 arch/x86/kernel/kprobes/core.c     |  7 ++-
 include/linux/kprobes.h            |  7 +--
 kernel/kprobes.c                   | 68 +++++++++++++++++-------------
 kernel/trace/trace_kprobe.c        | 15 ++++++-
 16 files changed, 103 insertions(+), 52 deletions(-)

-- 
2.30.2


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

[PATCH v2 1/4] arm64: patching: always use fixmap

[Mark Rutland]

For historical reasons, patch_map() won't bother to fixmap non-image
addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
use the fixmap for any non-image address.

Historically we only used patch_map() for the kernel image and modules,
but these days its also used by BPF and KPROBES to write to read-only
pages of executable text. Currently these both depend on CONFIG_MODULES,
but we'd like to change that in subsequent patches, which will require
using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.

This patch changes patch_map() to always use the fixmap, and simplifies
the logic:

* Use is_image_text() directly in the if-else, rather than using a
  temporary boolean variable.

* Use offset_in_page() to get the offset within the mapping.

* Remove uintaddr and cast the address directly when using
  is_image_text().

For kernels built with CONFIG_MODULES=y, there should be no functional
change as a result of this patch.

For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
for non-image addresses, but there are no extant users with non-image
addresses when CONFIG_MODULES=n, and hence there should be no functional
change as a result of this patch alone.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/patching.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Catalin, Will, this is a prerequisite for the final two patches in the
series. Are you happy for this go via the tracing tree?

Mark.

diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
index 2555349303684..f0f3a2a82ca5a 100644
--- a/arch/arm64/kernel/patching.c
+++ b/arch/arm64/kernel/patching.c
@@ -30,20 +30,16 @@ static bool is_image_text(unsigned long addr)
 
 static void __kprobes *patch_map(void *addr, int fixmap)
 {
-	unsigned long uintaddr = (uintptr_t) addr;
-	bool image = is_image_text(uintaddr);
 	struct page *page;
 
-	if (image)
+	if (is_image_text((unsigned long)addr))
 		page = phys_to_page(__pa_symbol(addr));
-	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-		page = vmalloc_to_page(addr);
 	else
-		return addr;
+		page = vmalloc_to_page(addr);
 
 	BUG_ON(!page);
 	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
-			(uintaddr & ~PAGE_MASK));
+					 offset_in_page(addr));
 }
 
 static void __kprobes patch_unmap(int fixmap)
-- 
2.30.2


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

[PATCH v2 2/4] kprobes/treewide: Add kprobes_ prefix to insn alloc/free functions

[Mark Rutland]

The alloc_(opt)insn_page() and free_(opt)insn_page() functions are
specific to KPROBES, but their name makes them sound more generic than
they are.

Given them a 'kprobes_' prefix to make it clear that they're part of
kprobes.

This was generated automatically with:

  sed -i 's/alloc_insn_page/kprobes_alloc_insn_page/' $(git grep -l 'alloc_insn_page')
  sed -i 's/free_insn_page/kprobes_free_insn_page/' $(git grep -l 'free_insn_page')
  sed -i 's/alloc_optinsn_page/kprobes_alloc_optinsn_page/' $(git grep -l 'alloc_optinsn_page')
  sed -i 's/free_optinsn_page/kprobes_free_optinsn_page/' $(git grep -l 'free_optinsn_page')

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
---
 arch/arm64/kernel/probes/kprobes.c |  2 +-
 arch/powerpc/kernel/kprobes.c      |  2 +-
 arch/powerpc/kernel/optprobes.c    |  4 ++--
 arch/riscv/kernel/probes/kprobes.c |  2 +-
 arch/s390/kernel/kprobes.c         |  2 +-
 arch/x86/kernel/kprobes/core.c     |  2 +-
 include/linux/kprobes.h            |  6 +++---
 kernel/kprobes.c                   | 20 ++++++++++----------
 8 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 327855a11df2f..4b6ab7b1fa211 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -129,7 +129,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	return 0;
 }
 
-void *alloc_insn_page(void)
+void *kprobes_alloc_insn_page(void)
 {
 	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
 			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bbca90a5e2ec0..0b297718d5de6 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -126,7 +126,7 @@ kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned long offse
 	return (kprobe_opcode_t *)(addr + offset);
 }
 
-void *alloc_insn_page(void)
+void *kprobes_alloc_insn_page(void)
 {
 	void *page;
 
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 004fae2044a3e..0ddbda217073f 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -27,7 +27,7 @@
 
 static bool insn_page_in_use;
 
-void *alloc_optinsn_page(void)
+void *kprobes_alloc_optinsn_page(void)
 {
 	if (insn_page_in_use)
 		return NULL;
@@ -35,7 +35,7 @@ void *alloc_optinsn_page(void)
 	return &optinsn_slot;
 }
 
-void free_optinsn_page(void *page)
+void kprobes_free_optinsn_page(void *page)
 {
 	insn_page_in_use = false;
 }
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 2f08c14a933d0..75201ce721057 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -105,7 +105,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 }
 
 #ifdef CONFIG_MMU
-void *alloc_insn_page(void)
+void *kprobes_alloc_insn_page(void)
 {
 	return  __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
 				     GFP_KERNEL, PAGE_KERNEL_READ_EXEC,
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index f0cf20d4b3c58..91ca4d501d4ef 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -34,7 +34,7 @@ struct kretprobe_blackpoint kretprobe_blacklist[] = { };
 
 static int insn_page_in_use;
 
-void *alloc_insn_page(void)
+void *kprobes_alloc_insn_page(void)
 {
 	void *page;
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index d0e49bd7c6f3f..7f01bbbfa9e2a 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -491,7 +491,7 @@ static int prepare_singlestep(kprobe_opcode_t *buf, struct kprobe *p,
 }
 
 /* Make page to RO mode when allocate it */
-void *alloc_insn_page(void)
+void *kprobes_alloc_insn_page(void)
 {
 	void *page;
 
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 0ff44d6633e33..ad4b561100f9e 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -430,10 +430,10 @@ int enable_kprobe(struct kprobe *kp);
 
 void dump_kprobe(struct kprobe *kp);
 
-void *alloc_insn_page(void);
+void *kprobes_alloc_insn_page(void);
 
-void *alloc_optinsn_page(void);
-void free_optinsn_page(void *page);
+void *kprobes_alloc_optinsn_page(void);
+void kprobes_free_optinsn_page(void *page);
 
 int kprobe_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		       char *sym);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9d9095e817928..35adf56430c9b 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -110,7 +110,7 @@ enum kprobe_slot_state {
 	SLOT_USED = 2,
 };
 
-void __weak *alloc_insn_page(void)
+void __weak *kprobes_alloc_insn_page(void)
 {
 	/*
 	 * Use module_alloc() so this page is within +/- 2GB of where the
@@ -121,15 +121,15 @@ void __weak *alloc_insn_page(void)
 	return module_alloc(PAGE_SIZE);
 }
 
-static void free_insn_page(void *page)
+static void kprobes_free_insn_page(void *page)
 {
 	module_memfree(page);
 }
 
 struct kprobe_insn_cache kprobe_insn_slots = {
 	.mutex = __MUTEX_INITIALIZER(kprobe_insn_slots.mutex),
-	.alloc = alloc_insn_page,
-	.free = free_insn_page,
+	.alloc = kprobes_alloc_insn_page,
+	.free = kprobes_free_insn_page,
 	.sym = KPROBE_INSN_PAGE_SYM,
 	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
 	.insn_size = MAX_INSN_SIZE,
@@ -333,21 +333,21 @@ int kprobe_cache_get_kallsym(struct kprobe_insn_cache *c, unsigned int *symnum,
 }
 
 #ifdef CONFIG_OPTPROBES
-void __weak *alloc_optinsn_page(void)
+void __weak *kprobes_alloc_optinsn_page(void)
 {
-	return alloc_insn_page();
+	return kprobes_alloc_insn_page();
 }
 
-void __weak free_optinsn_page(void *page)
+void __weak kprobes_free_optinsn_page(void *page)
 {
-	free_insn_page(page);
+	kprobes_free_insn_page(page);
 }
 
 /* For optimized_kprobe buffer */
 struct kprobe_insn_cache kprobe_optinsn_slots = {
 	.mutex = __MUTEX_INITIALIZER(kprobe_optinsn_slots.mutex),
-	.alloc = alloc_optinsn_page,
-	.free = free_optinsn_page,
+	.alloc = kprobes_alloc_optinsn_page,
+	.free = kprobes_free_optinsn_page,
 	.sym = KPROBE_OPTINSN_PAGE_SYM,
 	.pages = LIST_HEAD_INIT(kprobe_optinsn_slots.pages),
 	/* .insn_size is initialized later */
-- 
2.30.2


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

[PATCH v2 3/4] kprobes/treewide: Explicitly override alloc/free functions

[Mark Rutland]

Currently architectures can override kprobes_alloc_insn_page(), but
kprobes_free_insn_page() is always implemented using module_memfree(),
which might not be what an architecture needs, especially as we'd like
to make it possible to use kprobes without requiring MODULES.

It would be nicer if architectures either:

(a) Used only the generic kprobes_alloc_insn_page() and
    kprobes_free_insn_page(), implicitly depending on MODULES.

(b) Provided their own implementation of both kprobes_alloc_insn_page()
    and kprobes_free_insn_page(), handling the relevant dependencies
    themselves.

This patch applies that split treewide:

(a) Architectures using the generic kprobes_free_insn_page() and
    kprobes_free_insn_page() are left as-is. The __weak annotation is
    removed from the generic implementations so that accidental
    overrides/misuse can be detected easily.

(b) Architectures which provide their own kprobes_free_insn_page() are
    given a matching implementation of kprobes_free_insn_page(), and
    select HAVE_KPROBES_ALLOC.

    This new Kconfig symbol will allow subsequent patches to relax the
    dependency on MODULES to (MODULES || HAVE_KPROBES_ALLOC) once other
    module dependencies in the core kprobes code are cleaned up.

    Architectures which use module_alloc() are given an implementation
    using module_memfree() along with an explicit dependency on MODULES.

    Architectures using __vmalloc_node_range() are given an
    implementation using vfree(). This loses the warning for
    in_interrupt(), but vfree() can handle this via vfree_atomic(), so
    the warning isn't necessary.

    On riscv, the allocator depends on !XIP_KERNEL, which is already a
    dependency for HAVE_KPROBES in arch/riscv/Kconfig.

As of this patch arm64 and riscv have kprobe allocation functions which
do not explicitly depend on MODULES. The core kprobes code still depends
on MODULES.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/Kconfig                       | 3 +++
 arch/arm64/Kconfig                 | 1 +
 arch/arm64/kernel/probes/kprobes.c | 5 +++++
 arch/powerpc/Kconfig               | 3 ++-
 arch/powerpc/kernel/kprobes.c      | 5 +++++
 arch/riscv/Kconfig                 | 1 +
 arch/riscv/kernel/probes/kprobes.c | 5 +++++
 arch/s390/Kconfig                  | 3 ++-
 arch/s390/kernel/kprobes.c         | 5 +++++
 arch/x86/Kconfig                   | 3 ++-
 arch/x86/kernel/kprobes/core.c     | 5 +++++
 include/linux/kprobes.h            | 1 +
 kernel/kprobes.c                   | 6 ++++--
 13 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 9f066785bb71d..85bb59f7b8c07 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -206,6 +206,9 @@ config HAVE_IOREMAP_PROT
 config HAVE_KPROBES
 	bool
 
+config HAVE_KPROBES_ALLOC
+	bool
+
 config HAVE_KRETPROBES
 	bool
 
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7b11c98b3e84b..bda7913d6c9b8 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -233,6 +233,7 @@ config ARM64
 	select HAVE_STACKPROTECTOR
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
+	select HAVE_KPROBES_ALLOC
 	select HAVE_KRETPROBES
 	select HAVE_GENERIC_VDSO
 	select HOTPLUG_CORE_SYNC_DEAD if HOTPLUG_CPU
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 4b6ab7b1fa211..69d19a390cd48 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -136,6 +136,11 @@ void *kprobes_alloc_insn_page(void)
 			NUMA_NO_NODE, __builtin_return_address(0));
 }
 
+void kprobes_free_insn_page(void *page)
+{
+	vfree(page);
+}
+
 /* arm kprobe: install breakpoint in text */
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1c4be33736860..13e0fc51dcdcf 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -254,7 +254,8 @@ config PPC
 	select HAVE_KERNEL_LZMA			if DEFAULT_UIMAGE
 	select HAVE_KERNEL_LZO			if DEFAULT_UIMAGE
 	select HAVE_KERNEL_XZ			if PPC_BOOK3S || 44x
-	select HAVE_KPROBES
+	select HAVE_KPROBES			if MODULES
+	select HAVE_KPROBES_ALLOC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION if HAVE_OBJTOOL_MCOUNT && (!ARCH_USING_PATCHABLE_FUNCTION_ENTRY || (!CC_IS_GCC || GCC_VERSION >= 110100))
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 0b297718d5de6..d0332aaebab09 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -146,6 +146,11 @@ void *kprobes_alloc_insn_page(void)
 	return NULL;
 }
 
+void kprobes_free_insn_page(void *page)
+{
+	module_memfree(page);
+}
+
 int arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index be09c8836d56b..4e22549a522a5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -139,6 +139,7 @@ config RISCV
 	select HAVE_GENERIC_VDSO if MMU && 64BIT
 	select HAVE_IRQ_TIME_ACCOUNTING
 	select HAVE_KPROBES if !XIP_KERNEL
+	select HAVE_KPROBES_ALLOC
 	select HAVE_KPROBES_ON_FTRACE if !XIP_KERNEL
 	select HAVE_KRETPROBES if !XIP_KERNEL
 	# https://github.com/ClangBuiltLinux/linux/issues/1881
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 75201ce721057..37fdfa952d999 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -112,6 +112,11 @@ void *kprobes_alloc_insn_page(void)
 				     VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
 				     __builtin_return_address(0));
 }
+
+void kprobes_free_insn_page(void *page)
+{
+	vfree(page);
+}
 #endif
 
 /* install breakpoint in text */
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 8f01ada6845e3..635eddc3fce80 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -193,7 +193,8 @@ config S390
 	select HAVE_KERNEL_UNCOMPRESSED
 	select HAVE_KERNEL_XZ
 	select HAVE_KERNEL_ZSTD
-	select HAVE_KPROBES
+	select HAVE_KPROBES		if MODULES
+	select HAVE_KPROBES_ALLOC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LIVEPATCH
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index 91ca4d501d4ef..a5b142b8eb0f7 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -45,6 +45,11 @@ void *kprobes_alloc_insn_page(void)
 	return page;
 }
 
+void kprobes_free_insn_page(void *page)
+{
+	module_memfree(page);
+}
+
 static void *alloc_s390_insn_page(void)
 {
 	if (xchg(&insn_page_in_use, 1) == 1)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4fff6ed46e902..0810cd0bdeca9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -240,7 +240,8 @@ config X86
 	select HAVE_KERNEL_LZO
 	select HAVE_KERNEL_XZ
 	select HAVE_KERNEL_ZSTD
-	select HAVE_KPROBES
+	select HAVE_KPROBES			if MODULES
+	select HAVE_KPROBES_ALLOC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_FUNCTION_ERROR_INJECTION
 	select HAVE_KRETPROBES
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 7f01bbbfa9e2a..5f093b94d9b40 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -508,6 +508,11 @@ void *kprobes_alloc_insn_page(void)
 	return page;
 }
 
+void kprobes_free_insn_page(void *page)
+{
+	module_memfree(page);
+}
+
 /* Kprobe x86 instruction emulation - only regs->ip or IF flag modifiers */
 
 static void kprobe_emulate_ifmodifiers(struct kprobe *p, struct pt_regs *regs)
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index ad4b561100f9e..651c807727bea 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -431,6 +431,7 @@ int enable_kprobe(struct kprobe *kp);
 void dump_kprobe(struct kprobe *kp);
 
 void *kprobes_alloc_insn_page(void);
+void kprobes_free_insn_page(void *page);
 
 void *kprobes_alloc_optinsn_page(void);
 void kprobes_free_optinsn_page(void *page);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 35adf56430c9b..fa2ee4e59eca2 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -110,7 +110,8 @@ enum kprobe_slot_state {
 	SLOT_USED = 2,
 };
 
-void __weak *kprobes_alloc_insn_page(void)
+#ifndef CONFIG_HAVE_KPROBES_ALLOC
+void *kprobes_alloc_insn_page(void)
 {
 	/*
 	 * Use module_alloc() so this page is within +/- 2GB of where the
@@ -121,10 +122,11 @@ void __weak *kprobes_alloc_insn_page(void)
 	return module_alloc(PAGE_SIZE);
 }
 
-static void kprobes_free_insn_page(void *page)
+void kprobes_free_insn_page(void *page)
 {
 	module_memfree(page);
 }
+#endif
 
 struct kprobe_insn_cache kprobe_insn_slots = {
 	.mutex = __MUTEX_INITIALIZER(kprobe_insn_slots.mutex),
-- 
2.30.2


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

[PATCH v2 4/4] kprobes: Remove core dependency on modules

[Mark Rutland]

From: Jarkko Sakkinen <jarkko@kernel.org>

Tracing with kprobes while running a monolithic kernel is currently
impossible because KPROBES depends on MODULES. While this dependency is
necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
memory using module_alloc(), all the other module-specific code only
exist to handle the case when MODULES=y, and can be hidden behind
ifdeffery.

Add the necessary ifdeffery, and remove the dependency on MODULES=y when
HAVE_KPROBES_ALLOC=y.

As of this patch kprobes can be used when MODULES=n on arm64 and
riscv. All other architectures still depend on MODULES, either by virtue
of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
virtue of an explciit dependency on MODULES in arch code.

Other architectures can enable support by implementing their own
kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
depend on MODULES.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
[Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/Kconfig                |  2 +-
 kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
 kernel/trace/trace_kprobe.c | 15 ++++++++++--
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 85bb59f7b8c07..0df2c88547b3c 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -52,7 +52,7 @@ config GENERIC_ENTRY
 
 config KPROBES
 	bool "Kprobes"
-	depends on MODULES
+	depends on MODULES || HAVE_KPROBES_ALLOC
 	depends on HAVE_KPROBES
 	select KALLSYMS
 	select TASKS_RCU if PREEMPTION
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index fa2ee4e59eca2..ec4493a41b505 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 			goto out;
 		}
 
+#ifdef CONFIG_MODULES
 		/*
 		 * If the module freed '.init.text', we couldn't insert
 		 * kprobes in there.
@@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
 			*probed_mod = NULL;
 			ret = -ENOENT;
 		}
+#endif /* CONFIG_MODULES */
 	}
+
 out:
 	preempt_enable();
 	jump_label_unlock();
@@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
 	return 0;
 }
 
-/* Remove all symbols in given area from kprobe blacklist */
-static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
-{
-	struct kprobe_blacklist_entry *ent, *n;
-
-	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
-		if (ent->start_addr < start || ent->start_addr >= end)
-			continue;
-		list_del(&ent->list);
-		kfree(ent);
-	}
-}
-
-static void kprobe_remove_ksym_blacklist(unsigned long entry)
-{
-	kprobe_remove_area_blacklist(entry, entry + 1);
-}
-
 int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
 				   char *type, char *sym)
 {
@@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
 	return ret ? : arch_populate_kprobe_blacklist();
 }
 
+#ifdef CONFIG_MODULES
+/* Remove all symbols in given area from kprobe blacklist */
+static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
+{
+	struct kprobe_blacklist_entry *ent, *n;
+
+	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
+		if (ent->start_addr < start || ent->start_addr >= end)
+			continue;
+		list_del(&ent->list);
+		kfree(ent);
+	}
+}
+
+static void kprobe_remove_ksym_blacklist(unsigned long entry)
+{
+	kprobe_remove_area_blacklist(entry, entry + 1);
+}
+
 static void add_module_kprobe_blacklist(struct module *mod)
 {
 	unsigned long start, end;
@@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
 	mutex_unlock(&kprobe_mutex);
 	return NOTIFY_DONE;
 }
+#else
+#define kprobes_module_callback	(NULL)
+#endif /* CONFIG_MODULES */
 
 static struct notifier_block kprobe_module_nb = {
 	.notifier_call = kprobes_module_callback,
@@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
 	err = arch_init_kprobes();
 	if (!err)
 		err = register_die_notifier(&kprobe_exceptions_nb);
-	if (!err)
+
+	if (!err && IS_ENABLED(CONFIG_MODULES))
 		err = register_module_notifier(&kprobe_module_nb);
 
 	kprobes_initialized = (err == 0);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 14099cc17fc9e..c509ba776e679 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
 	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
 }
 
+#ifdef CONFIG_MODULES
 static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 {
 	char *p;
@@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 
 	return ret;
 }
+#else
+#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
+#endif /* CONFIG_MODULES */
 
 static bool trace_kprobe_is_busy(struct dyn_event *ev)
 {
@@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
 	return ret;
 }
 
+#ifdef CONFIG_MODULES
 /* Module notifier call back, checking event on the module */
 static int trace_kprobe_module_callback(struct notifier_block *nb,
 				       unsigned long val, void *data)
@@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
 
 	return NOTIFY_DONE;
 }
+#else
+#define trace_kprobe_module_callback (NULL)
+#endif /* CONFIG_MODULES */
 
 static struct notifier_block trace_kprobe_module_nb = {
 	.notifier_call = trace_kprobe_module_callback,
@@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
 	if (ret)
 		return ret;
 
-	if (register_module_notifier(&trace_kprobe_module_nb))
-		return -EINVAL;
+	if (IS_ENABLED(CONFIG_MODULES)) {
+		ret = register_module_notifier(&trace_kprobe_module_nb);
+		if (ret)
+			return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.30.2


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

Re: [PATCH v2 1/4] arm64: patching: always use fixmap

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> For historical reasons, patch_map() won't bother to fixmap non-image
> addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> use the fixmap for any non-image address.

Not familiar with the config flag but I'd guess it is essentially
w^x enforcement right for the sections?

> Historically we only used patch_map() for the kernel image and modules,
> but these days its also used by BPF and KPROBES to write to read-only
> pages of executable text. Currently these both depend on CONFIG_MODULES,
> but we'd like to change that in subsequent patches, which will require
> using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
>
> This patch changes patch_map() to always use the fixmap, and simplifies
> the logic:
>
> * Use is_image_text() directly in the if-else, rather than using a
>   temporary boolean variable.
>
> * Use offset_in_page() to get the offset within the mapping.
>
> * Remove uintaddr and cast the address directly when using
>   is_image_text().
>
> For kernels built with CONFIG_MODULES=y, there should be no functional
> change as a result of this patch.
>
> For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> for non-image addresses, but there are no extant users with non-image
> addresses when CONFIG_MODULES=n, and hence there should be no functional
> change as a result of this patch alone.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/patching.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> Catalin, Will, this is a prerequisite for the final two patches in the
> series. Are you happy for this go via the tracing tree?
>
> Mark.
>
> diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> index 2555349303684..f0f3a2a82ca5a 100644
> --- a/arch/arm64/kernel/patching.c
> +++ b/arch/arm64/kernel/patching.c
> @@ -30,20 +30,16 @@ static bool is_image_text(unsigned long addr)
>  
>  static void __kprobes *patch_map(void *addr, int fixmap)
>  {
> -	unsigned long uintaddr = (uintptr_t) addr;
> -	bool image = is_image_text(uintaddr);
>  	struct page *page;
>  
> -	if (image)
> +	if (is_image_text((unsigned long)addr))
>  		page = phys_to_page(__pa_symbol(addr));
> -	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> -		page = vmalloc_to_page(addr);
>  	else
> -		return addr;
> +		page = vmalloc_to_page(addr);
>  
>  	BUG_ON(!page);
>  	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
> -			(uintaddr & ~PAGE_MASK));
> +					 offset_in_page(addr));

nit: could be a single line but i guess it is up to the taste (and
subsystem maintainer). I.e. checkpatch will allow it at least.

I don't mind it too much just mentioning for completeness.

>  }
>  
>  static void __kprobes patch_unmap(int fixmap)

If my assumption about the config flag holds this makes sense:

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.rg>

BR, Jarkko

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

Re: [PATCH v2 2/4] kprobes/treewide: Add kprobes_ prefix to insn alloc/free functions

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> The alloc_(opt)insn_page() and free_(opt)insn_page() functions are
> specific to KPROBES, but their name makes them sound more generic than
> they are.
>
> Given them a 'kprobes_' prefix to make it clear that they're part of
> kprobes.
>
> This was generated automatically with:
>
>   sed -i 's/alloc_insn_page/kprobes_alloc_insn_page/' $(git grep -l 'alloc_insn_page')
>   sed -i 's/free_insn_page/kprobes_free_insn_page/' $(git grep -l 'free_insn_page')
>   sed -i 's/alloc_optinsn_page/kprobes_alloc_optinsn_page/' $(git grep -l 'alloc_optinsn_page')
>   sed -i 's/free_optinsn_page/kprobes_free_optinsn_page/' $(git grep -l 'free_optinsn_page')
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> ---
>  arch/arm64/kernel/probes/kprobes.c |  2 +-
>  arch/powerpc/kernel/kprobes.c      |  2 +-
>  arch/powerpc/kernel/optprobes.c    |  4 ++--
>  arch/riscv/kernel/probes/kprobes.c |  2 +-
>  arch/s390/kernel/kprobes.c         |  2 +-
>  arch/x86/kernel/kprobes/core.c     |  2 +-
>  include/linux/kprobes.h            |  6 +++---
>  kernel/kprobes.c                   | 20 ++++++++++----------
>  8 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 327855a11df2f..4b6ab7b1fa211 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -129,7 +129,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  	return 0;
>  }
>  
> -void *alloc_insn_page(void)
> +void *kprobes_alloc_insn_page(void)
>  {
>  	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
>  			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index bbca90a5e2ec0..0b297718d5de6 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -126,7 +126,7 @@ kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned long offse
>  	return (kprobe_opcode_t *)(addr + offset);
>  }
>  
> -void *alloc_insn_page(void)
> +void *kprobes_alloc_insn_page(void)
>  {
>  	void *page;
>  
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 004fae2044a3e..0ddbda217073f 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -27,7 +27,7 @@
>  
>  static bool insn_page_in_use;
>  
> -void *alloc_optinsn_page(void)
> +void *kprobes_alloc_optinsn_page(void)
>  {
>  	if (insn_page_in_use)
>  		return NULL;
> @@ -35,7 +35,7 @@ void *alloc_optinsn_page(void)
>  	return &optinsn_slot;
>  }
>  
> -void free_optinsn_page(void *page)
> +void kprobes_free_optinsn_page(void *page)
>  {
>  	insn_page_in_use = false;
>  }
> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 2f08c14a933d0..75201ce721057 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -105,7 +105,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  }
>  
>  #ifdef CONFIG_MMU
> -void *alloc_insn_page(void)
> +void *kprobes_alloc_insn_page(void)
>  {
>  	return  __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
>  				     GFP_KERNEL, PAGE_KERNEL_READ_EXEC,
> diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
> index f0cf20d4b3c58..91ca4d501d4ef 100644
> --- a/arch/s390/kernel/kprobes.c
> +++ b/arch/s390/kernel/kprobes.c
> @@ -34,7 +34,7 @@ struct kretprobe_blackpoint kretprobe_blacklist[] = { };
>  
>  static int insn_page_in_use;
>  
> -void *alloc_insn_page(void)
> +void *kprobes_alloc_insn_page(void)
>  {
>  	void *page;
>  
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index d0e49bd7c6f3f..7f01bbbfa9e2a 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -491,7 +491,7 @@ static int prepare_singlestep(kprobe_opcode_t *buf, struct kprobe *p,
>  }
>  
>  /* Make page to RO mode when allocate it */
> -void *alloc_insn_page(void)
> +void *kprobes_alloc_insn_page(void)
>  {
>  	void *page;
>  
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 0ff44d6633e33..ad4b561100f9e 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -430,10 +430,10 @@ int enable_kprobe(struct kprobe *kp);
>  
>  void dump_kprobe(struct kprobe *kp);
>  
> -void *alloc_insn_page(void);
> +void *kprobes_alloc_insn_page(void);
>  
> -void *alloc_optinsn_page(void);
> -void free_optinsn_page(void *page);
> +void *kprobes_alloc_optinsn_page(void);
> +void kprobes_free_optinsn_page(void *page);
>  
>  int kprobe_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>  		       char *sym);
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 9d9095e817928..35adf56430c9b 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -110,7 +110,7 @@ enum kprobe_slot_state {
>  	SLOT_USED = 2,
>  };
>  
> -void __weak *alloc_insn_page(void)
> +void __weak *kprobes_alloc_insn_page(void)
>  {
>  	/*
>  	 * Use module_alloc() so this page is within +/- 2GB of where the
> @@ -121,15 +121,15 @@ void __weak *alloc_insn_page(void)
>  	return module_alloc(PAGE_SIZE);
>  }
>  
> -static void free_insn_page(void *page)
> +static void kprobes_free_insn_page(void *page)
>  {
>  	module_memfree(page);
>  }
>  
>  struct kprobe_insn_cache kprobe_insn_slots = {
>  	.mutex = __MUTEX_INITIALIZER(kprobe_insn_slots.mutex),
> -	.alloc = alloc_insn_page,
> -	.free = free_insn_page,
> +	.alloc = kprobes_alloc_insn_page,
> +	.free = kprobes_free_insn_page,
>  	.sym = KPROBE_INSN_PAGE_SYM,
>  	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
>  	.insn_size = MAX_INSN_SIZE,
> @@ -333,21 +333,21 @@ int kprobe_cache_get_kallsym(struct kprobe_insn_cache *c, unsigned int *symnum,
>  }
>  
>  #ifdef CONFIG_OPTPROBES
> -void __weak *alloc_optinsn_page(void)
> +void __weak *kprobes_alloc_optinsn_page(void)
>  {
> -	return alloc_insn_page();
> +	return kprobes_alloc_insn_page();
>  }
>  
> -void __weak free_optinsn_page(void *page)
> +void __weak kprobes_free_optinsn_page(void *page)
>  {
> -	free_insn_page(page);
> +	kprobes_free_insn_page(page);
>  }
>  
>  /* For optimized_kprobe buffer */
>  struct kprobe_insn_cache kprobe_optinsn_slots = {
>  	.mutex = __MUTEX_INITIALIZER(kprobe_optinsn_slots.mutex),
> -	.alloc = alloc_optinsn_page,
> -	.free = free_optinsn_page,
> +	.alloc = kprobes_alloc_optinsn_page,
> +	.free = kprobes_free_optinsn_page,
>  	.sym = KPROBE_OPTINSN_PAGE_SYM,
>  	.pages = LIST_HEAD_INIT(kprobe_optinsn_slots.pages),
>  	/* .insn_size is initialized later */

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

Re: [PATCH v2 3/4] kprobes/treewide: Explicitly override alloc/free functions

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> Currently architectures can override kprobes_alloc_insn_page(), but
> kprobes_free_insn_page() is always implemented using module_memfree(),
> which might not be what an architecture needs, especially as we'd like
> to make it possible to use kprobes without requiring MODULES.
>
> It would be nicer if architectures either:
>
> (a) Used only the generic kprobes_alloc_insn_page() and
>     kprobes_free_insn_page(), implicitly depending on MODULES.
>
> (b) Provided their own implementation of both kprobes_alloc_insn_page()
>     and kprobes_free_insn_page(), handling the relevant dependencies
>     themselves.
>
> This patch applies that split treewide:
>
> (a) Architectures using the generic kprobes_free_insn_page() and
>     kprobes_free_insn_page() are left as-is. The __weak annotation is
>     removed from the generic implementations so that accidental
>     overrides/misuse can be detected easily.
>
> (b) Architectures which provide their own kprobes_free_insn_page() are
>     given a matching implementation of kprobes_free_insn_page(), and
>     select HAVE_KPROBES_ALLOC.
>
>     This new Kconfig symbol will allow subsequent patches to relax the
>     dependency on MODULES to (MODULES || HAVE_KPROBES_ALLOC) once other
>     module dependencies in the core kprobes code are cleaned up.
>
>     Architectures which use module_alloc() are given an implementation
>     using module_memfree() along with an explicit dependency on MODULES.
>
>     Architectures using __vmalloc_node_range() are given an
>     implementation using vfree(). This loses the warning for
>     in_interrupt(), but vfree() can handle this via vfree_atomic(), so
>     the warning isn't necessary.
>
>     On riscv, the allocator depends on !XIP_KERNEL, which is already a
>     dependency for HAVE_KPROBES in arch/riscv/Kconfig.
>
> As of this patch arm64 and riscv have kprobe allocation functions which
> do not explicitly depend on MODULES. The core kprobes code still depends
> on MODULES.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/Kconfig                       | 3 +++
>  arch/arm64/Kconfig                 | 1 +
>  arch/arm64/kernel/probes/kprobes.c | 5 +++++
>  arch/powerpc/Kconfig               | 3 ++-
>  arch/powerpc/kernel/kprobes.c      | 5 +++++
>  arch/riscv/Kconfig                 | 1 +
>  arch/riscv/kernel/probes/kprobes.c | 5 +++++
>  arch/s390/Kconfig                  | 3 ++-
>  arch/s390/kernel/kprobes.c         | 5 +++++
>  arch/x86/Kconfig                   | 3 ++-
>  arch/x86/kernel/kprobes/core.c     | 5 +++++
>  include/linux/kprobes.h            | 1 +
>  kernel/kprobes.c                   | 6 ++++--
>  13 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 9f066785bb71d..85bb59f7b8c07 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -206,6 +206,9 @@ config HAVE_IOREMAP_PROT
>  config HAVE_KPROBES
>  	bool
>  
> +config HAVE_KPROBES_ALLOC
> +	bool
> +
>  config HAVE_KRETPROBES
>  	bool
>  
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7b11c98b3e84b..bda7913d6c9b8 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -233,6 +233,7 @@ config ARM64
>  	select HAVE_STACKPROTECTOR
>  	select HAVE_SYSCALL_TRACEPOINTS
>  	select HAVE_KPROBES
> +	select HAVE_KPROBES_ALLOC
>  	select HAVE_KRETPROBES
>  	select HAVE_GENERIC_VDSO
>  	select HOTPLUG_CORE_SYNC_DEAD if HOTPLUG_CPU
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 4b6ab7b1fa211..69d19a390cd48 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -136,6 +136,11 @@ void *kprobes_alloc_insn_page(void)
>  			NUMA_NO_NODE, __builtin_return_address(0));
>  }
>  
> +void kprobes_free_insn_page(void *page)
> +{
> +	vfree(page);
> +}
> +
>  /* arm kprobe: install breakpoint in text */
>  void __kprobes arch_arm_kprobe(struct kprobe *p)
>  {
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 1c4be33736860..13e0fc51dcdcf 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -254,7 +254,8 @@ config PPC
>  	select HAVE_KERNEL_LZMA			if DEFAULT_UIMAGE
>  	select HAVE_KERNEL_LZO			if DEFAULT_UIMAGE
>  	select HAVE_KERNEL_XZ			if PPC_BOOK3S || 44x
> -	select HAVE_KPROBES
> +	select HAVE_KPROBES			if MODULES
> +	select HAVE_KPROBES_ALLOC
>  	select HAVE_KPROBES_ON_FTRACE
>  	select HAVE_KRETPROBES
>  	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION if HAVE_OBJTOOL_MCOUNT && (!ARCH_USING_PATCHABLE_FUNCTION_ENTRY || (!CC_IS_GCC || GCC_VERSION >= 110100))
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 0b297718d5de6..d0332aaebab09 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -146,6 +146,11 @@ void *kprobes_alloc_insn_page(void)
>  	return NULL;
>  }
>  
> +void kprobes_free_insn_page(void *page)
> +{
> +	module_memfree(page);
> +}
> +
>  int arch_prepare_kprobe(struct kprobe *p)
>  {
>  	int ret = 0;
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index be09c8836d56b..4e22549a522a5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -139,6 +139,7 @@ config RISCV
>  	select HAVE_GENERIC_VDSO if MMU && 64BIT
>  	select HAVE_IRQ_TIME_ACCOUNTING
>  	select HAVE_KPROBES if !XIP_KERNEL
> +	select HAVE_KPROBES_ALLOC
>  	select HAVE_KPROBES_ON_FTRACE if !XIP_KERNEL
>  	select HAVE_KRETPROBES if !XIP_KERNEL
>  	# https://github.com/ClangBuiltLinux/linux/issues/1881
> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 75201ce721057..37fdfa952d999 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -112,6 +112,11 @@ void *kprobes_alloc_insn_page(void)
>  				     VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
>  				     __builtin_return_address(0));
>  }
> +
> +void kprobes_free_insn_page(void *page)
> +{
> +	vfree(page);
> +}
>  #endif
>  
>  /* install breakpoint in text */
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 8f01ada6845e3..635eddc3fce80 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -193,7 +193,8 @@ config S390
>  	select HAVE_KERNEL_UNCOMPRESSED
>  	select HAVE_KERNEL_XZ
>  	select HAVE_KERNEL_ZSTD
> -	select HAVE_KPROBES
> +	select HAVE_KPROBES		if MODULES
> +	select HAVE_KPROBES_ALLOC
>  	select HAVE_KPROBES_ON_FTRACE
>  	select HAVE_KRETPROBES
>  	select HAVE_LIVEPATCH
> diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
> index 91ca4d501d4ef..a5b142b8eb0f7 100644
> --- a/arch/s390/kernel/kprobes.c
> +++ b/arch/s390/kernel/kprobes.c
> @@ -45,6 +45,11 @@ void *kprobes_alloc_insn_page(void)
>  	return page;
>  }
>  
> +void kprobes_free_insn_page(void *page)
> +{
> +	module_memfree(page);
> +}
> +
>  static void *alloc_s390_insn_page(void)
>  {
>  	if (xchg(&insn_page_in_use, 1) == 1)
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4fff6ed46e902..0810cd0bdeca9 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -240,7 +240,8 @@ config X86
>  	select HAVE_KERNEL_LZO
>  	select HAVE_KERNEL_XZ
>  	select HAVE_KERNEL_ZSTD
> -	select HAVE_KPROBES
> +	select HAVE_KPROBES			if MODULES
> +	select HAVE_KPROBES_ALLOC
>  	select HAVE_KPROBES_ON_FTRACE
>  	select HAVE_FUNCTION_ERROR_INJECTION
>  	select HAVE_KRETPROBES
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 7f01bbbfa9e2a..5f093b94d9b40 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -508,6 +508,11 @@ void *kprobes_alloc_insn_page(void)
>  	return page;
>  }
>  
> +void kprobes_free_insn_page(void *page)
> +{
> +	module_memfree(page);
> +}
> +
>  /* Kprobe x86 instruction emulation - only regs->ip or IF flag modifiers */
>  
>  static void kprobe_emulate_ifmodifiers(struct kprobe *p, struct pt_regs *regs)
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index ad4b561100f9e..651c807727bea 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -431,6 +431,7 @@ int enable_kprobe(struct kprobe *kp);
>  void dump_kprobe(struct kprobe *kp);
>  
>  void *kprobes_alloc_insn_page(void);
> +void kprobes_free_insn_page(void *page);
>  
>  void *kprobes_alloc_optinsn_page(void);
>  void kprobes_free_optinsn_page(void *page);
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 35adf56430c9b..fa2ee4e59eca2 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -110,7 +110,8 @@ enum kprobe_slot_state {
>  	SLOT_USED = 2,
>  };
>  
> -void __weak *kprobes_alloc_insn_page(void)
> +#ifndef CONFIG_HAVE_KPROBES_ALLOC
> +void *kprobes_alloc_insn_page(void)
>  {
>  	/*
>  	 * Use module_alloc() so this page is within +/- 2GB of where the
> @@ -121,10 +122,11 @@ void __weak *kprobes_alloc_insn_page(void)
>  	return module_alloc(PAGE_SIZE);
>  }
>  
> -static void kprobes_free_insn_page(void *page)
> +void kprobes_free_insn_page(void *page)
>  {
>  	module_memfree(page);
>  }
> +#endif
>  
>  struct kprobe_insn_cache kprobe_insn_slots = {
>  	.mutex = __MUTEX_INITIALIZER(kprobe_insn_slots.mutex),

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> Tracing with kprobes while running a monolithic kernel is currently
> impossible because KPROBES depends on MODULES. While this dependency is
> necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> memory using module_alloc(), all the other module-specific code only
> exist to handle the case when MODULES=y, and can be hidden behind
> ifdeffery.
>
> Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> HAVE_KPROBES_ALLOC=y.
>
> As of this patch kprobes can be used when MODULES=n on arm64 and
> riscv. All other architectures still depend on MODULES, either by virtue
> of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> virtue of an explciit dependency on MODULES in arch code.
>
> Other architectures can enable support by implementing their own
> kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> depend on MODULES.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/Kconfig                |  2 +-
>  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
>  kernel/trace/trace_kprobe.c | 15 ++++++++++--
>  3 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..0df2c88547b3c 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>  
>  config KPROBES
>  	bool "Kprobes"
> -	depends on MODULES
> +	depends on MODULES || HAVE_KPROBES_ALLOC
>  	depends on HAVE_KPROBES
>  	select KALLSYMS
>  	select TASKS_RCU if PREEMPTION
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fa2ee4e59eca2..ec4493a41b505 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			goto out;
>  		}
>  
> +#ifdef CONFIG_MODULES
>  		/*
>  		 * If the module freed '.init.text', we couldn't insert
>  		 * kprobes in there.
> @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			*probed_mod = NULL;
>  			ret = -ENOENT;
>  		}
> +#endif /* CONFIG_MODULES */
>  	}
> +
>  out:
>  	preempt_enable();
>  	jump_label_unlock();
> @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> -/* Remove all symbols in given area from kprobe blacklist */
> -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> -{
> -	struct kprobe_blacklist_entry *ent, *n;
> -
> -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> -		if (ent->start_addr < start || ent->start_addr >= end)
> -			continue;
> -		list_del(&ent->list);
> -		kfree(ent);
> -	}
> -}
> -
> -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> -{
> -	kprobe_remove_area_blacklist(entry, entry + 1);
> -}
> -
>  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
>  				   char *type, char *sym)
>  {
> @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
>  	return ret ? : arch_populate_kprobe_blacklist();
>  }
>  
> +#ifdef CONFIG_MODULES
> +/* Remove all symbols in given area from kprobe blacklist */
> +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> +{
> +	struct kprobe_blacklist_entry *ent, *n;
> +
> +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> +		if (ent->start_addr < start || ent->start_addr >= end)
> +			continue;
> +		list_del(&ent->list);
> +		kfree(ent);
> +	}
> +}
> +
> +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> +{
> +	kprobe_remove_area_blacklist(entry, entry + 1);
> +}
> +
>  static void add_module_kprobe_blacklist(struct module *mod)
>  {
>  	unsigned long start, end;
> @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
>  	mutex_unlock(&kprobe_mutex);
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define kprobes_module_callback	(NULL)
> +#endif /* CONFIG_MODULES */
>  
>  static struct notifier_block kprobe_module_nb = {
>  	.notifier_call = kprobes_module_callback,
> @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
> -	if (!err)
> +
> +	if (!err && IS_ENABLED(CONFIG_MODULES))
>  		err = register_module_notifier(&kprobe_module_nb);
>  
>  	kprobes_initialized = (err == 0);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 14099cc17fc9e..c509ba776e679 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
>  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
>  }
>  
> +#ifdef CONFIG_MODULES
>  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  {
>  	char *p;
> @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  
>  	return ret;
>  }
> +#else
> +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> +#endif /* CONFIG_MODULES */
>  
>  static bool trace_kprobe_is_busy(struct dyn_event *ev)
>  {
> @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_MODULES
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
>  
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define trace_kprobe_module_callback (NULL)
> +#endif /* CONFIG_MODULES */
>  
>  static struct notifier_block trace_kprobe_module_nb = {
>  	.notifier_call = trace_kprobe_module_callback,
> @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
>  	if (ret)
>  		return ret;
>  
> -	if (register_module_notifier(&trace_kprobe_module_nb))
> -		return -EINVAL;
> +	if (IS_ENABLED(CONFIG_MODULES)) {
> +		ret = register_module_notifier(&trace_kprobe_module_nb);
> +		if (ret)
> +			return -EINVAL;
> +	}
>  
>  	return 0;
>  }

I'll test the patch set asap.

BR, Jarkko

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

Re: [PATCH v2 1/4] arm64: patching: always use fixmap

[Mark Rutland]

On Wed, Apr 03, 2024 at 07:20:31PM +0300, Jarkko Sakkinen wrote:
> On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > For historical reasons, patch_map() won't bother to fixmap non-image
> > addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> > to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> > doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> > CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> > use the fixmap for any non-image address.
> 
> Not familiar with the config flag but I'd guess it is essentially
> w^x enforcement right for the sections?

Essentially, yes.

> > Historically we only used patch_map() for the kernel image and modules,
> > but these days its also used by BPF and KPROBES to write to read-only
> > pages of executable text. Currently these both depend on CONFIG_MODULES,
> > but we'd like to change that in subsequent patches, which will require
> > using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
> >
> > This patch changes patch_map() to always use the fixmap, and simplifies
> > the logic:
> >
> > * Use is_image_text() directly in the if-else, rather than using a
> >   temporary boolean variable.
> >
> > * Use offset_in_page() to get the offset within the mapping.
> >
> > * Remove uintaddr and cast the address directly when using
> >   is_image_text().
> >
> > For kernels built with CONFIG_MODULES=y, there should be no functional
> > change as a result of this patch.
> >
> > For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> > for non-image addresses, but there are no extant users with non-image
> > addresses when CONFIG_MODULES=n, and hence there should be no functional
> > change as a result of this patch alone.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/patching.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > Catalin, Will, this is a prerequisite for the final two patches in the
> > series. Are you happy for this go via the tracing tree?
> >
> > Mark.
> >
> > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> > index 2555349303684..f0f3a2a82ca5a 100644
> > --- a/arch/arm64/kernel/patching.c
> > +++ b/arch/arm64/kernel/patching.c
> > @@ -30,20 +30,16 @@ static bool is_image_text(unsigned long addr)
> >  
> >  static void __kprobes *patch_map(void *addr, int fixmap)
> >  {
> > -	unsigned long uintaddr = (uintptr_t) addr;
> > -	bool image = is_image_text(uintaddr);
> >  	struct page *page;
> >  
> > -	if (image)
> > +	if (is_image_text((unsigned long)addr))
> >  		page = phys_to_page(__pa_symbol(addr));
> > -	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> > -		page = vmalloc_to_page(addr);
> >  	else
> > -		return addr;
> > +		page = vmalloc_to_page(addr);
> >  
> >  	BUG_ON(!page);
> >  	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
> > -			(uintaddr & ~PAGE_MASK));
> > +					 offset_in_page(addr));
> 
> nit: could be a single line but i guess it is up to the taste (and
> subsystem maintainer). I.e. checkpatch will allow it at least.
> 
> I don't mind it too much just mentioning for completeness.

At that point it goes to 93 chars long, and I stuck with the existing line
wrapping at 80 chars. I'd rather have a temporary 'phys_addr_t phys' variable
and do:

	phys = page_to_phys(page) + offset_in_page(addr);
	return (void *)set_fixmap(fixmap, phys);

... but I'll leave this as-is for now.

> >  }
> >  
> >  static void __kprobes patch_unmap(int fixmap)
> 
> If my assumption about the config flag holds this makes sense:
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.rg>

Thanks! I assume that should be "kernel.org", with an 'o' ;)

Mark.

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

Re: [PATCH v2 1/4] arm64: patching: always use fixmap

[Catalin Marinas]

On Wed, Apr 03, 2024 at 04:01:51PM +0100, Mark Rutland wrote:
> For historical reasons, patch_map() won't bother to fixmap non-image
> addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> use the fixmap for any non-image address.
> 
> Historically we only used patch_map() for the kernel image and modules,
> but these days its also used by BPF and KPROBES to write to read-only
> pages of executable text. Currently these both depend on CONFIG_MODULES,
> but we'd like to change that in subsequent patches, which will require
> using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
> 
> This patch changes patch_map() to always use the fixmap, and simplifies
> the logic:
> 
> * Use is_image_text() directly in the if-else, rather than using a
>   temporary boolean variable.
> 
> * Use offset_in_page() to get the offset within the mapping.
> 
> * Remove uintaddr and cast the address directly when using
>   is_image_text().
> 
> For kernels built with CONFIG_MODULES=y, there should be no functional
> change as a result of this patch.
> 
> For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> for non-image addresses, but there are no extant users with non-image
> addresses when CONFIG_MODULES=n, and hence there should be no functional
> change as a result of this patch alone.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/patching.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> Catalin, Will, this is a prerequisite for the final two patches in the
> series. Are you happy for this go via the tracing tree?

Fine by me.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

Re: [PATCH v2 1/4] arm64: patching: always use fixmap

[Masami Hiramatsu]

On Wed, 3 Apr 2024 18:52:30 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:

> On Wed, Apr 03, 2024 at 04:01:51PM +0100, Mark Rutland wrote:
> > For historical reasons, patch_map() won't bother to fixmap non-image
> > addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> > to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> > doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> > CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> > use the fixmap for any non-image address.
> > 
> > Historically we only used patch_map() for the kernel image and modules,
> > but these days its also used by BPF and KPROBES to write to read-only
> > pages of executable text. Currently these both depend on CONFIG_MODULES,
> > but we'd like to change that in subsequent patches, which will require
> > using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
> > 
> > This patch changes patch_map() to always use the fixmap, and simplifies
> > the logic:
> > 
> > * Use is_image_text() directly in the if-else, rather than using a
> >   temporary boolean variable.
> > 
> > * Use offset_in_page() to get the offset within the mapping.
> > 
> > * Remove uintaddr and cast the address directly when using
> >   is_image_text().
> > 
> > For kernels built with CONFIG_MODULES=y, there should be no functional
> > change as a result of this patch.
> > 
> > For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> > for non-image addresses, but there are no extant users with non-image
> > addresses when CONFIG_MODULES=n, and hence there should be no functional
> > change as a result of this patch alone.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/patching.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > Catalin, Will, this is a prerequisite for the final two patches in the
> > series. Are you happy for this go via the tracing tree?
> 
> Fine by me.
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks Catalin. I'll pick this series to linux-trace tree.

Thank you!

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> Tracing with kprobes while running a monolithic kernel is currently
> impossible because KPROBES depends on MODULES. While this dependency is
> necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> memory using module_alloc(), all the other module-specific code only
> exist to handle the case when MODULES=y, and can be hidden behind
> ifdeffery.
>
> Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> HAVE_KPROBES_ALLOC=y.
>
> As of this patch kprobes can be used when MODULES=n on arm64 and
> riscv. All other architectures still depend on MODULES, either by virtue
> of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> virtue of an explciit dependency on MODULES in arch code.
>
> Other architectures can enable support by implementing their own
> kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> depend on MODULES.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/Kconfig                |  2 +-
>  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
>  kernel/trace/trace_kprobe.c | 15 ++++++++++--
>  3 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..0df2c88547b3c 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>  
>  config KPROBES
>  	bool "Kprobes"
> -	depends on MODULES
> +	depends on MODULES || HAVE_KPROBES_ALLOC
>  	depends on HAVE_KPROBES
>  	select KALLSYMS
>  	select TASKS_RCU if PREEMPTION
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fa2ee4e59eca2..ec4493a41b505 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			goto out;
>  		}
>  
> +#ifdef CONFIG_MODULES
>  		/*
>  		 * If the module freed '.init.text', we couldn't insert
>  		 * kprobes in there.
> @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			*probed_mod = NULL;
>  			ret = -ENOENT;
>  		}
> +#endif /* CONFIG_MODULES */
>  	}
> +
>  out:
>  	preempt_enable();
>  	jump_label_unlock();
> @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> -/* Remove all symbols in given area from kprobe blacklist */
> -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> -{
> -	struct kprobe_blacklist_entry *ent, *n;
> -
> -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> -		if (ent->start_addr < start || ent->start_addr >= end)
> -			continue;
> -		list_del(&ent->list);
> -		kfree(ent);
> -	}
> -}
> -
> -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> -{
> -	kprobe_remove_area_blacklist(entry, entry + 1);
> -}
> -
>  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
>  				   char *type, char *sym)
>  {
> @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
>  	return ret ? : arch_populate_kprobe_blacklist();
>  }
>  
> +#ifdef CONFIG_MODULES
> +/* Remove all symbols in given area from kprobe blacklist */
> +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> +{
> +	struct kprobe_blacklist_entry *ent, *n;
> +
> +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> +		if (ent->start_addr < start || ent->start_addr >= end)
> +			continue;
> +		list_del(&ent->list);
> +		kfree(ent);
> +	}
> +}
> +
> +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> +{
> +	kprobe_remove_area_blacklist(entry, entry + 1);
> +}
> +
>  static void add_module_kprobe_blacklist(struct module *mod)
>  {
>  	unsigned long start, end;
> @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
>  	mutex_unlock(&kprobe_mutex);
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define kprobes_module_callback	(NULL)
> +#endif /* CONFIG_MODULES */
>  
>  static struct notifier_block kprobe_module_nb = {
>  	.notifier_call = kprobes_module_callback,
> @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
> -	if (!err)
> +
> +	if (!err && IS_ENABLED(CONFIG_MODULES))
>  		err = register_module_notifier(&kprobe_module_nb);
>  
>  	kprobes_initialized = (err == 0);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 14099cc17fc9e..c509ba776e679 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
>  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
>  }
>  
> +#ifdef CONFIG_MODULES
>  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  {
>  	char *p;
> @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  
>  	return ret;
>  }
> +#else
> +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> +#endif /* CONFIG_MODULES */
>  
>  static bool trace_kprobe_is_busy(struct dyn_event *ev)
>  {
> @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_MODULES
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
>  
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define trace_kprobe_module_callback (NULL)
> +#endif /* CONFIG_MODULES */
>  
>  static struct notifier_block trace_kprobe_module_nb = {
>  	.notifier_call = trace_kprobe_module_callback,
> @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
>  	if (ret)
>  		return ret;
>  
> -	if (register_module_notifier(&trace_kprobe_module_nb))
> -		return -EINVAL;
> +	if (IS_ENABLED(CONFIG_MODULES)) {
> +		ret = register_module_notifier(&trace_kprobe_module_nb);
> +		if (ret)
> +			return -EINVAL;
> +	}
>  
>  	return 0;
>  }

2/4, 3/4, 4/4:

Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv

BR, Jarkko

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

Re: [PATCH v2 1/4] arm64: patching: always use fixmap

[Jarkko Sakkinen]

On Wed Apr 3, 2024 at 7:51 PM EEST, Mark Rutland wrote:
> On Wed, Apr 03, 2024 at 07:20:31PM +0300, Jarkko Sakkinen wrote:
> > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > For historical reasons, patch_map() won't bother to fixmap non-image
> > > addresses when CONFIG_STRICT_MODULE_RWX=n, matching the behaviour prior
> > > to the introduction of CONFIG_STRICT_MODULE_RWX. However, as arm64
> > > doesn't select CONFIG_ARCH_OPTIONAL_KERNEL_RWX, CONFIG_MODULES implies
> > > CONFIG_STRICT_MODULE_RWX, so any kernel built with module support will
> > > use the fixmap for any non-image address.
> > 
> > Not familiar with the config flag but I'd guess it is essentially
> > w^x enforcement right for the sections?
>
> Essentially, yes.
>
> > > Historically we only used patch_map() for the kernel image and modules,
> > > but these days its also used by BPF and KPROBES to write to read-only
> > > pages of executable text. Currently these both depend on CONFIG_MODULES,
> > > but we'd like to change that in subsequent patches, which will require
> > > using the fixmap regardless of CONFIG_STRICT_MODULE_RWX.
> > >
> > > This patch changes patch_map() to always use the fixmap, and simplifies
> > > the logic:
> > >
> > > * Use is_image_text() directly in the if-else, rather than using a
> > >   temporary boolean variable.
> > >
> > > * Use offset_in_page() to get the offset within the mapping.
> > >
> > > * Remove uintaddr and cast the address directly when using
> > >   is_image_text().
> > >
> > > For kernels built with CONFIG_MODULES=y, there should be no functional
> > > change as a result of this patch.
> > >
> > > For kernels built with CONFIG_MODULES=n, patch_map() will use the fixmap
> > > for non-image addresses, but there are no extant users with non-image
> > > addresses when CONFIG_MODULES=n, and hence there should be no functional
> > > change as a result of this patch alone.
> > >
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/kernel/patching.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > Catalin, Will, this is a prerequisite for the final two patches in the
> > > series. Are you happy for this go via the tracing tree?
> > >
> > > Mark.
> > >
> > > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> > > index 2555349303684..f0f3a2a82ca5a 100644
> > > --- a/arch/arm64/kernel/patching.c
> > > +++ b/arch/arm64/kernel/patching.c
> > > @@ -30,20 +30,16 @@ static bool is_image_text(unsigned long addr)
> > >  
> > >  static void __kprobes *patch_map(void *addr, int fixmap)
> > >  {
> > > -	unsigned long uintaddr = (uintptr_t) addr;
> > > -	bool image = is_image_text(uintaddr);
> > >  	struct page *page;
> > >  
> > > -	if (image)
> > > +	if (is_image_text((unsigned long)addr))
> > >  		page = phys_to_page(__pa_symbol(addr));
> > > -	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> > > -		page = vmalloc_to_page(addr);
> > >  	else
> > > -		return addr;
> > > +		page = vmalloc_to_page(addr);
> > >  
> > >  	BUG_ON(!page);
> > >  	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
> > > -			(uintaddr & ~PAGE_MASK));
> > > +					 offset_in_page(addr));
> > 
> > nit: could be a single line but i guess it is up to the taste (and
> > subsystem maintainer). I.e. checkpatch will allow it at least.
> > 
> > I don't mind it too much just mentioning for completeness.
>
> At that point it goes to 93 chars long, and I stuck with the existing line
> wrapping at 80 chars. I'd rather have a temporary 'phys_addr_t phys' variable
> and do:
>
> 	phys = page_to_phys(page) + offset_in_page(addr);
> 	return (void *)set_fixmap(fixmap, phys);
>
> ... but I'll leave this as-is for now.
>
> > >  }
> > >  
> > >  static void __kprobes patch_unmap(int fixmap)
> > 
> > If my assumption about the config flag holds this makes sense:
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.rg>
>
> Thanks! I assume that should be "kernel.org", with an 'o' ;)

Yes, that's correct, not from Gibraltar :-)

BR, Jarkko

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Jarkko Sakkinen]

On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > Tracing with kprobes while running a monolithic kernel is currently
> > impossible because KPROBES depends on MODULES. While this dependency is
> > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > memory using module_alloc(), all the other module-specific code only
> > exist to handle the case when MODULES=y, and can be hidden behind
> > ifdeffery.
> >
> > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > HAVE_KPROBES_ALLOC=y.
> >
> > As of this patch kprobes can be used when MODULES=n on arm64 and
> > riscv. All other architectures still depend on MODULES, either by virtue
> > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > virtue of an explciit dependency on MODULES in arch code.
> >
> > Other architectures can enable support by implementing their own
> > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > depend on MODULES.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/Kconfig                |  2 +-
> >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> >  3 files changed, 41 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 85bb59f7b8c07..0df2c88547b3c 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> >  
> >  config KPROBES
> >  	bool "Kprobes"
> > -	depends on MODULES
> > +	depends on MODULES || HAVE_KPROBES_ALLOC
> >  	depends on HAVE_KPROBES
> >  	select KALLSYMS
> >  	select TASKS_RCU if PREEMPTION
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index fa2ee4e59eca2..ec4493a41b505 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> >  			goto out;
> >  		}
> >  
> > +#ifdef CONFIG_MODULES
> >  		/*
> >  		 * If the module freed '.init.text', we couldn't insert
> >  		 * kprobes in there.
> > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> >  			*probed_mod = NULL;
> >  			ret = -ENOENT;
> >  		}
> > +#endif /* CONFIG_MODULES */
> >  	}
> > +
> >  out:
> >  	preempt_enable();
> >  	jump_label_unlock();
> > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> >  	return 0;
> >  }
> >  
> > -/* Remove all symbols in given area from kprobe blacklist */
> > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > -{
> > -	struct kprobe_blacklist_entry *ent, *n;
> > -
> > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > -		if (ent->start_addr < start || ent->start_addr >= end)
> > -			continue;
> > -		list_del(&ent->list);
> > -		kfree(ent);
> > -	}
> > -}
> > -
> > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > -{
> > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > -}
> > -
> >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> >  				   char *type, char *sym)
> >  {
> > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> >  	return ret ? : arch_populate_kprobe_blacklist();
> >  }
> >  
> > +#ifdef CONFIG_MODULES
> > +/* Remove all symbols in given area from kprobe blacklist */
> > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > +{
> > +	struct kprobe_blacklist_entry *ent, *n;
> > +
> > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > +		if (ent->start_addr < start || ent->start_addr >= end)
> > +			continue;
> > +		list_del(&ent->list);
> > +		kfree(ent);
> > +	}
> > +}
> > +
> > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > +{
> > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > +}
> > +
> >  static void add_module_kprobe_blacklist(struct module *mod)
> >  {
> >  	unsigned long start, end;
> > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> >  	mutex_unlock(&kprobe_mutex);
> >  	return NOTIFY_DONE;
> >  }
> > +#else
> > +#define kprobes_module_callback	(NULL)
> > +#endif /* CONFIG_MODULES */
> >  
> >  static struct notifier_block kprobe_module_nb = {
> >  	.notifier_call = kprobes_module_callback,
> > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> >  	err = arch_init_kprobes();
> >  	if (!err)
> >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > -	if (!err)
> > +
> > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> >  		err = register_module_notifier(&kprobe_module_nb);
> >  
> >  	kprobes_initialized = (err == 0);
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 14099cc17fc9e..c509ba776e679 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> >  }
> >  
> > +#ifdef CONFIG_MODULES
> >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >  {
> >  	char *p;
> > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >  
> >  	return ret;
> >  }
> > +#else
> > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > +#endif /* CONFIG_MODULES */
> >  
> >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> >  {
> > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> >  	return ret;
> >  }
> >  
> > +#ifdef CONFIG_MODULES
> >  /* Module notifier call back, checking event on the module */
> >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> >  				       unsigned long val, void *data)
> > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> >  
> >  	return NOTIFY_DONE;
> >  }
> > +#else
> > +#define trace_kprobe_module_callback (NULL)
> > +#endif /* CONFIG_MODULES */
> >  
> >  static struct notifier_block trace_kprobe_module_nb = {
> >  	.notifier_call = trace_kprobe_module_callback,
> > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > -		return -EINVAL;
> > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > +		if (ret)
> > +			return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
>
> 2/4, 3/4, 4/4:
>
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv

Hey, I tried the pci_proc_init example:

[    3.060703] ------------[ ftrace bug ]------------
[    3.060944] ftrace faulted on writing
[    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
[    3.061509] Updating ftrace call site to call a different ftrace function
[    3.061756] ftrace record flags: 80100001
[    3.061925]  (1)
[    3.061925]  expected tramp: ffffffff8000aa60
[    3.062527] ------------[ cut here ]------------
[    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
[    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
[    3.062807] Hardware name: riscv-virtio,qemu (DT)
[    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
[    3.062925] epc : ftrace_bug+0x282/0x2b8
[    3.062957]  ra : ftrace_bug+0x282/0x2b8
[    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
[    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
[    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
[    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
[    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
[    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
[    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
[    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
[    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
[    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
[    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
[    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
[    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
[    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
[    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
[    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
[    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
[    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
[    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
[    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
[    3.063676] ---[ end trace 0000000000000000 ]---
[    3.069730] ------------[ cut here ]------------
[    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
[    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
[    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
[    3.070133] Hardware name: riscv-virtio,qemu (DT)
[    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
[    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
[    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
[    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
[    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
[    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
[    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
[    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
[    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
[    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
[    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
[    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
[    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
[    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
[    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
[    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
[    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
[    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
[    3.070254] ---[ end trace 0000000000000000 ]---
[

This is with riscv64 defconfig, tracing shenanigans and the following
bootconfig and the bug was realized in QEMU:

ftrace {
	tracing_on = 0
	tracer = "function_graph"
	event {
		kprobes.pci_proc_init_begin {
			probes = "pci_proc_init"
			actions = "traceon"
		}
		kprobes.pci_proc_init_end {
			probes = "pci_proc_init%return"
			actions = "traceoff"
		}
	}
}

kernel {
	console = hvc0
	dyndbg = "file arch/riscv/kernel/* +p"
	earlycon = sbi
	memblock = debug
	memtest = 1
	tp_printk
	frace_dump_on_oops
}

BR, Jarkko

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Masami Hiramatsu]

On Thu, 04 Apr 2024 18:18:21 +0300
"Jarkko Sakkinen" <jarkko@kernel.org> wrote:

> On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > >
> > > Tracing with kprobes while running a monolithic kernel is currently
> > > impossible because KPROBES depends on MODULES. While this dependency is
> > > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > > memory using module_alloc(), all the other module-specific code only
> > > exist to handle the case when MODULES=y, and can be hidden behind
> > > ifdeffery.
> > >
> > > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > > HAVE_KPROBES_ALLOC=y.
> > >
> > > As of this patch kprobes can be used when MODULES=n on arm64 and
> > > riscv. All other architectures still depend on MODULES, either by virtue
> > > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > > virtue of an explciit dependency on MODULES in arch code.
> > >
> > > Other architectures can enable support by implementing their own
> > > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > > depend on MODULES.
> > >
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/Kconfig                |  2 +-
> > >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> > >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > index 85bb59f7b8c07..0df2c88547b3c 100644
> > > --- a/arch/Kconfig
> > > +++ b/arch/Kconfig
> > > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> > >  
> > >  config KPROBES
> > >  	bool "Kprobes"
> > > -	depends on MODULES
> > > +	depends on MODULES || HAVE_KPROBES_ALLOC
> > >  	depends on HAVE_KPROBES
> > >  	select KALLSYMS
> > >  	select TASKS_RCU if PREEMPTION
> > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > index fa2ee4e59eca2..ec4493a41b505 100644
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > >  			goto out;
> > >  		}
> > >  
> > > +#ifdef CONFIG_MODULES
> > >  		/*
> > >  		 * If the module freed '.init.text', we couldn't insert
> > >  		 * kprobes in there.
> > > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > >  			*probed_mod = NULL;
> > >  			ret = -ENOENT;
> > >  		}
> > > +#endif /* CONFIG_MODULES */
> > >  	}
> > > +
> > >  out:
> > >  	preempt_enable();
> > >  	jump_label_unlock();
> > > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > >  	return 0;
> > >  }
> > >  
> > > -/* Remove all symbols in given area from kprobe blacklist */
> > > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > -{
> > > -	struct kprobe_blacklist_entry *ent, *n;
> > > -
> > > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > -		if (ent->start_addr < start || ent->start_addr >= end)
> > > -			continue;
> > > -		list_del(&ent->list);
> > > -		kfree(ent);
> > > -	}
> > > -}
> > > -
> > > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > -{
> > > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > > -}
> > > -
> > >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > >  				   char *type, char *sym)
> > >  {
> > > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > >  	return ret ? : arch_populate_kprobe_blacklist();
> > >  }
> > >  
> > > +#ifdef CONFIG_MODULES
> > > +/* Remove all symbols in given area from kprobe blacklist */
> > > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > +{
> > > +	struct kprobe_blacklist_entry *ent, *n;
> > > +
> > > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > +		if (ent->start_addr < start || ent->start_addr >= end)
> > > +			continue;
> > > +		list_del(&ent->list);
> > > +		kfree(ent);
> > > +	}
> > > +}
> > > +
> > > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > +{
> > > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > > +}
> > > +
> > >  static void add_module_kprobe_blacklist(struct module *mod)
> > >  {
> > >  	unsigned long start, end;
> > > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> > >  	mutex_unlock(&kprobe_mutex);
> > >  	return NOTIFY_DONE;
> > >  }
> > > +#else
> > > +#define kprobes_module_callback	(NULL)
> > > +#endif /* CONFIG_MODULES */
> > >  
> > >  static struct notifier_block kprobe_module_nb = {
> > >  	.notifier_call = kprobes_module_callback,
> > > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> > >  	err = arch_init_kprobes();
> > >  	if (!err)
> > >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > > -	if (!err)
> > > +
> > > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> > >  		err = register_module_notifier(&kprobe_module_nb);
> > >  
> > >  	kprobes_initialized = (err == 0);
> > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > index 14099cc17fc9e..c509ba776e679 100644
> > > --- a/kernel/trace/trace_kprobe.c
> > > +++ b/kernel/trace/trace_kprobe.c
> > > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> > >  }
> > >  
> > > +#ifdef CONFIG_MODULES
> > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > >  {
> > >  	char *p;
> > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > >  
> > >  	return ret;
> > >  }
> > > +#else
> > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > +#endif /* CONFIG_MODULES */
> > >  
> > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > >  {
> > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > >  	return ret;
> > >  }
> > >  
> > > +#ifdef CONFIG_MODULES
> > >  /* Module notifier call back, checking event on the module */
> > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > >  				       unsigned long val, void *data)
> > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > >  
> > >  	return NOTIFY_DONE;
> > >  }
> > > +#else
> > > +#define trace_kprobe_module_callback (NULL)
> > > +#endif /* CONFIG_MODULES */
> > >  
> > >  static struct notifier_block trace_kprobe_module_nb = {
> > >  	.notifier_call = trace_kprobe_module_callback,
> > > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > > -		return -EINVAL;
> > > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > > +		if (ret)
> > > +			return -EINVAL;
> > > +	}
> > >  
> > >  	return 0;
> > >  }
> >
> > 2/4, 3/4, 4/4:
> >
> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv
> 
> Hey, I tried the pci_proc_init example:
> 
> [    3.060703] ------------[ ftrace bug ]------------
> [    3.060944] ftrace faulted on writing
> [    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
> [    3.061509] Updating ftrace call site to call a different ftrace function
> [    3.061756] ftrace record flags: 80100001
> [    3.061925]  (1)
> [    3.061925]  expected tramp: ffffffff8000aa60
> [    3.062527] ------------[ cut here ]------------
> [    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
> [    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
> [    3.062807] Hardware name: riscv-virtio,qemu (DT)
> [    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
> [    3.062925] epc : ftrace_bug+0x282/0x2b8
> [    3.062957]  ra : ftrace_bug+0x282/0x2b8
> [    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
> [    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
> [    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
> [    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
> [    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> [    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> [    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
> [    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
> [    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
> [    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
> [    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
> [    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> [    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
> [    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
> [    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
> [    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
> [    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
> [    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
> [    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
> [    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
> [    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> [    3.063676] ---[ end trace 0000000000000000 ]---
> [    3.069730] ------------[ cut here ]------------
> [    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
> [    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
> [    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
> [    3.070133] Hardware name: riscv-virtio,qemu (DT)
> [    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
> [    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
> [    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
> [    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
> [    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
> [    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
> [    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> [    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> [    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
> [    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
> [    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
> [    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
> [    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
> [    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> [    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
> [    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
> [    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
> [    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> [    3.070254] ---[ end trace 0000000000000000 ]---
> [
> 
> This is with riscv64 defconfig, tracing shenanigans and the following
> bootconfig and the bug was realized in QEMU:

So this is with CONFIG_MODULES=y?
This seems like an actual bug but not related to this series.
Can you reproduce this without this patch series?

Thank you,

> 
> ftrace {
> 	tracing_on = 0
> 	tracer = "function_graph"
> 	event {
> 		kprobes.pci_proc_init_begin {
> 			probes = "pci_proc_init"
> 			actions = "traceon"
> 		}
> 		kprobes.pci_proc_init_end {
> 			probes = "pci_proc_init%return"
> 			actions = "traceoff"
> 		}
> 	}
> }
> 
> kernel {
> 	console = hvc0
> 	dyndbg = "file arch/riscv/kernel/* +p"
> 	earlycon = sbi
> 	memblock = debug
> 	memtest = 1
> 	tp_printk
> 	frace_dump_on_oops
> }
> 
> BR, Jarkko


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Mark Rutland]

On Fri, Apr 05, 2024 at 01:10:26AM +0900, Masami Hiramatsu wrote:
> On Thu, 04 Apr 2024 18:18:21 +0300
> "Jarkko Sakkinen" <jarkko@kernel.org> wrote:
> 
> > On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> > > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > >
> > > > Tracing with kprobes while running a monolithic kernel is currently
> > > > impossible because KPROBES depends on MODULES. While this dependency is
> > > > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > > > memory using module_alloc(), all the other module-specific code only
> > > > exist to handle the case when MODULES=y, and can be hidden behind
> > > > ifdeffery.
> > > >
> > > > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > > > HAVE_KPROBES_ALLOC=y.
> > > >
> > > > As of this patch kprobes can be used when MODULES=n on arm64 and
> > > > riscv. All other architectures still depend on MODULES, either by virtue
> > > > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > > > virtue of an explciit dependency on MODULES in arch code.
> > > >
> > > > Other architectures can enable support by implementing their own
> > > > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > > > depend on MODULES.
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > > > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > > > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/Kconfig                |  2 +-
> > > >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> > > >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > > index 85bb59f7b8c07..0df2c88547b3c 100644
> > > > --- a/arch/Kconfig
> > > > +++ b/arch/Kconfig
> > > > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> > > >  
> > > >  config KPROBES
> > > >  	bool "Kprobes"
> > > > -	depends on MODULES
> > > > +	depends on MODULES || HAVE_KPROBES_ALLOC
> > > >  	depends on HAVE_KPROBES
> > > >  	select KALLSYMS
> > > >  	select TASKS_RCU if PREEMPTION
> > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > index fa2ee4e59eca2..ec4493a41b505 100644
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > >  			goto out;
> > > >  		}
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  		/*
> > > >  		 * If the module freed '.init.text', we couldn't insert
> > > >  		 * kprobes in there.
> > > > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > >  			*probed_mod = NULL;
> > > >  			ret = -ENOENT;
> > > >  		}
> > > > +#endif /* CONFIG_MODULES */
> > > >  	}
> > > > +
> > > >  out:
> > > >  	preempt_enable();
> > > >  	jump_label_unlock();
> > > > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -/* Remove all symbols in given area from kprobe blacklist */
> > > > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > -{
> > > > -	struct kprobe_blacklist_entry *ent, *n;
> > > > -
> > > > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > -		if (ent->start_addr < start || ent->start_addr >= end)
> > > > -			continue;
> > > > -		list_del(&ent->list);
> > > > -		kfree(ent);
> > > > -	}
> > > > -}
> > > > -
> > > > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > -{
> > > > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > -}
> > > > -
> > > >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > > >  				   char *type, char *sym)
> > > >  {
> > > > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > > >  	return ret ? : arch_populate_kprobe_blacklist();
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > > +/* Remove all symbols in given area from kprobe blacklist */
> > > > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > +{
> > > > +	struct kprobe_blacklist_entry *ent, *n;
> > > > +
> > > > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > +		if (ent->start_addr < start || ent->start_addr >= end)
> > > > +			continue;
> > > > +		list_del(&ent->list);
> > > > +		kfree(ent);
> > > > +	}
> > > > +}
> > > > +
> > > > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > +{
> > > > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > +}
> > > > +
> > > >  static void add_module_kprobe_blacklist(struct module *mod)
> > > >  {
> > > >  	unsigned long start, end;
> > > > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> > > >  	mutex_unlock(&kprobe_mutex);
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define kprobes_module_callback	(NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static struct notifier_block kprobe_module_nb = {
> > > >  	.notifier_call = kprobes_module_callback,
> > > > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> > > >  	err = arch_init_kprobes();
> > > >  	if (!err)
> > > >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > > > -	if (!err)
> > > > +
> > > > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> > > >  		err = register_module_notifier(&kprobe_module_nb);
> > > >  
> > > >  	kprobes_initialized = (err == 0);
> > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > index 14099cc17fc9e..c509ba776e679 100644
> > > > --- a/kernel/trace/trace_kprobe.c
> > > > +++ b/kernel/trace/trace_kprobe.c
> > > > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > > >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  {
> > > >  	char *p;
> > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > >  {
> > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  /* Module notifier call back, checking event on the module */
> > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  				       unsigned long val, void *data)
> > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_callback (NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static struct notifier_block trace_kprobe_module_nb = {
> > > >  	.notifier_call = trace_kprobe_module_callback,
> > > > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > > > -		return -EINVAL;
> > > > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > > > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > > > +		if (ret)
> > > > +			return -EINVAL;
> > > > +	}
> > > >  
> > > >  	return 0;
> > > >  }
> > >
> > > 2/4, 3/4, 4/4:
> > >
> > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv
> > 
> > Hey, I tried the pci_proc_init example:
> > 
> > [    3.060703] ------------[ ftrace bug ]------------
> > [    3.060944] ftrace faulted on writing
> > [    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
> > [    3.061509] Updating ftrace call site to call a different ftrace function
> > [    3.061756] ftrace record flags: 80100001
> > [    3.061925]  (1)
> > [    3.061925]  expected tramp: ffffffff8000aa60
> > [    3.062527] ------------[ cut here ]------------
> > [    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
> > [    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
> > [    3.062807] Hardware name: riscv-virtio,qemu (DT)
> > [    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
> > [    3.062925] epc : ftrace_bug+0x282/0x2b8
> > [    3.062957]  ra : ftrace_bug+0x282/0x2b8
> > [    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
> > [    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
> > [    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
> > [    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
> > [    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > [    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > [    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
> > [    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
> > [    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
> > [    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
> > [    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
> > [    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > [    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
> > [    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
> > [    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
> > [    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
> > [    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
> > [    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
> > [    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
> > [    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
> > [    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > [    3.063676] ---[ end trace 0000000000000000 ]---
> > [    3.069730] ------------[ cut here ]------------
> > [    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
> > [    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
> > [    3.070133] Hardware name: riscv-virtio,qemu (DT)
> > [    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
> > [    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
> > [    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
> > [    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
> > [    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > [    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > [    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
> > [    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
> > [    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
> > [    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
> > [    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
> > [    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> > [    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
> > [    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
> > [    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > [    3.070254] ---[ end trace 0000000000000000 ]---
> > [
> > 
> > This is with riscv64 defconfig, tracing shenanigans and the following
> > bootconfig and the bug was realized in QEMU:
> 
> So this is with CONFIG_MODULES=y?
> This seems like an actual bug but not related to this series.
> Can you reproduce this without this patch series?

IIUC what's going on here is:

CONFIG_MODULES=n

.. and so CONFIG_STRICT_MODULE_RWX=n

When kprobe_free_init_mem() is called, system_state == SYSTEM_FREEING_INITMEM, which causes
core_kernel_text() to return 0 for inittext:

| int notrace core_kernel_text(unsigned long addr)
| {
|         if (is_kernel_text(addr))
|                 return 1;
| 
|         if (system_state < SYSTEM_FREEING_INITMEM &&
|             is_kernel_inittext(addr))
|                 return 1;
|         return 0;
| }

This causes riscv's patch_map() to *not* fixmap the inittext, since it does:

|	if (core_kernel_text(uintaddr) || is_kernel_exittext(uintaddr))
|		page = phys_to_page(__pa_symbol(addr));
|	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
|		page = vmalloc_to_page(addr);
|	else
|		return addr;

... which fails core_kernel_text(), and IS_ENABLED(CONFIG_STRICT_MODULE_RWX),
returning the (read-only) mapping of the kernel image.

That would happen (by luck) to work with CONFIG_MODULES=because it'd be handled
by vmalloc_to_page() walking the page tables. I suspect that'll happen to work
on arm64 by virtue of patch 1, but that wasn't intentional.

I'm not sure what the right fix is here, it's annoying that core_kernel_text()
is special-cased for SYSTEM_FREEING_INITMEM, but that was deliberate as of
commit:

  d2635f2012a44e3d ("mm: create a new system state and fix core_kernel_text()")

... though I'm not sure what exactly that was trying to fix at a higher level.

I can look into this some more tomorrow.

Mark.

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Masami Hiramatsu]

On Thu, 4 Apr 2024 17:47:07 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Apr 05, 2024 at 01:10:26AM +0900, Masami Hiramatsu wrote:
> > On Thu, 04 Apr 2024 18:18:21 +0300
> > "Jarkko Sakkinen" <jarkko@kernel.org> wrote:
> > 
> > > On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> > > > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > > >
> > > > > Tracing with kprobes while running a monolithic kernel is currently
> > > > > impossible because KPROBES depends on MODULES. While this dependency is
> > > > > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > > > > memory using module_alloc(), all the other module-specific code only
> > > > > exist to handle the case when MODULES=y, and can be hidden behind
> > > > > ifdeffery.
> > > > >
> > > > > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > > > > HAVE_KPROBES_ALLOC=y.
> > > > >
> > > > > As of this patch kprobes can be used when MODULES=n on arm64 and
> > > > > riscv. All other architectures still depend on MODULES, either by virtue
> > > > > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > > > > virtue of an explciit dependency on MODULES in arch code.
> > > > >
> > > > > Other architectures can enable support by implementing their own
> > > > > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > > > > depend on MODULES.
> > > > >
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > > > > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > > > > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > ---
> > > > >  arch/Kconfig                |  2 +-
> > > > >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> > > > >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> > > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > > > index 85bb59f7b8c07..0df2c88547b3c 100644
> > > > > --- a/arch/Kconfig
> > > > > +++ b/arch/Kconfig
> > > > > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> > > > >  
> > > > >  config KPROBES
> > > > >  	bool "Kprobes"
> > > > > -	depends on MODULES
> > > > > +	depends on MODULES || HAVE_KPROBES_ALLOC
> > > > >  	depends on HAVE_KPROBES
> > > > >  	select KALLSYMS
> > > > >  	select TASKS_RCU if PREEMPTION
> > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > > index fa2ee4e59eca2..ec4493a41b505 100644
> > > > > --- a/kernel/kprobes.c
> > > > > +++ b/kernel/kprobes.c
> > > > > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > >  			goto out;
> > > > >  		}
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  		/*
> > > > >  		 * If the module freed '.init.text', we couldn't insert
> > > > >  		 * kprobes in there.
> > > > > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > >  			*probed_mod = NULL;
> > > > >  			ret = -ENOENT;
> > > > >  		}
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  	}
> > > > > +
> > > > >  out:
> > > > >  	preempt_enable();
> > > > >  	jump_label_unlock();
> > > > > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -/* Remove all symbols in given area from kprobe blacklist */
> > > > > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > > -{
> > > > > -	struct kprobe_blacklist_entry *ent, *n;
> > > > > -
> > > > > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > > -		if (ent->start_addr < start || ent->start_addr >= end)
> > > > > -			continue;
> > > > > -		list_del(&ent->list);
> > > > > -		kfree(ent);
> > > > > -	}
> > > > > -}
> > > > > -
> > > > > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > > -{
> > > > > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > > -}
> > > > > -
> > > > >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > > > >  				   char *type, char *sym)
> > > > >  {
> > > > > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > > > >  	return ret ? : arch_populate_kprobe_blacklist();
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > > +/* Remove all symbols in given area from kprobe blacklist */
> > > > > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > > +{
> > > > > +	struct kprobe_blacklist_entry *ent, *n;
> > > > > +
> > > > > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > > +		if (ent->start_addr < start || ent->start_addr >= end)
> > > > > +			continue;
> > > > > +		list_del(&ent->list);
> > > > > +		kfree(ent);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > > +{
> > > > > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > > +}
> > > > > +
> > > > >  static void add_module_kprobe_blacklist(struct module *mod)
> > > > >  {
> > > > >  	unsigned long start, end;
> > > > > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> > > > >  	mutex_unlock(&kprobe_mutex);
> > > > >  	return NOTIFY_DONE;
> > > > >  }
> > > > > +#else
> > > > > +#define kprobes_module_callback	(NULL)
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static struct notifier_block kprobe_module_nb = {
> > > > >  	.notifier_call = kprobes_module_callback,
> > > > > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> > > > >  	err = arch_init_kprobes();
> > > > >  	if (!err)
> > > > >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > > > > -	if (!err)
> > > > > +
> > > > > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> > > > >  		err = register_module_notifier(&kprobe_module_nb);
> > > > >  
> > > > >  	kprobes_initialized = (err == 0);
> > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > > index 14099cc17fc9e..c509ba776e679 100644
> > > > > --- a/kernel/trace/trace_kprobe.c
> > > > > +++ b/kernel/trace/trace_kprobe.c
> > > > > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > > > >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  {
> > > > >  	char *p;
> > > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  
> > > > >  	return ret;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > > >  {
> > > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  /* Module notifier call back, checking event on the module */
> > > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  				       unsigned long val, void *data)
> > > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  
> > > > >  	return NOTIFY_DONE;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_callback (NULL)
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static struct notifier_block trace_kprobe_module_nb = {
> > > > >  	.notifier_call = trace_kprobe_module_callback,
> > > > > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > > > > -		return -EINVAL;
> > > > > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > > > > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > > > > +		if (ret)
> > > > > +			return -EINVAL;
> > > > > +	}
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > >
> > > > 2/4, 3/4, 4/4:
> > > >
> > > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv
> > > 
> > > Hey, I tried the pci_proc_init example:
> > > 
> > > [    3.060703] ------------[ ftrace bug ]------------
> > > [    3.060944] ftrace faulted on writing
> > > [    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
> > > [    3.061509] Updating ftrace call site to call a different ftrace function
> > > [    3.061756] ftrace record flags: 80100001
> > > [    3.061925]  (1)
> > > [    3.061925]  expected tramp: ffffffff8000aa60
> > > [    3.062527] ------------[ cut here ]------------
> > > [    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
> > > [    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
> > > [    3.062807] Hardware name: riscv-virtio,qemu (DT)
> > > [    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
> > > [    3.062925] epc : ftrace_bug+0x282/0x2b8
> > > [    3.062957]  ra : ftrace_bug+0x282/0x2b8
> > > [    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
> > > [    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
> > > [    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
> > > [    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
> > > [    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > > [    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > > [    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
> > > [    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
> > > [    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
> > > [    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
> > > [    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
> > > [    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > > [    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
> > > [    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
> > > [    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
> > > [    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
> > > [    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
> > > [    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
> > > [    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
> > > [    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
> > > [    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > > [    3.063676] ---[ end trace 0000000000000000 ]---
> > > [    3.069730] ------------[ cut here ]------------
> > > [    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
> > > [    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
> > > [    3.070133] Hardware name: riscv-virtio,qemu (DT)
> > > [    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
> > > [    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
> > > [    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
> > > [    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
> > > [    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > > [    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > > [    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
> > > [    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
> > > [    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
> > > [    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
> > > [    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
> > > [    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> > > [    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
> > > [    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
> > > [    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > > [    3.070254] ---[ end trace 0000000000000000 ]---
> > > [
> > > 
> > > This is with riscv64 defconfig, tracing shenanigans and the following
> > > bootconfig and the bug was realized in QEMU:
> > 
> > So this is with CONFIG_MODULES=y?
> > This seems like an actual bug but not related to this series.
> > Can you reproduce this without this patch series?
> 
> IIUC what's going on here is:
> 
> CONFIG_MODULES=n
> 
> . and so CONFIG_STRICT_MODULE_RWX=n
> 
> When kprobe_free_init_mem() is called, system_state == SYSTEM_FREEING_INITMEM, which causes
> core_kernel_text() to return 0 for inittext:
> 
> | int notrace core_kernel_text(unsigned long addr)
> | {
> |         if (is_kernel_text(addr))
> |                 return 1;
> | 
> |         if (system_state < SYSTEM_FREEING_INITMEM &&
> |             is_kernel_inittext(addr))
> |                 return 1;
> |         return 0;
> | }
> 
> This causes riscv's patch_map() to *not* fixmap the inittext, since it does:
> 
> |	if (core_kernel_text(uintaddr) || is_kernel_exittext(uintaddr))
> |		page = phys_to_page(__pa_symbol(addr));
> |	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> |		page = vmalloc_to_page(addr);
> |	else
> |		return addr;
> 
> .. which fails core_kernel_text(), and IS_ENABLED(CONFIG_STRICT_MODULE_RWX),
> returning the (read-only) mapping of the kernel image.

Ah, I got it. Yes, kprobes is able to probe on init text.
(in the boot time and module load time)

> 
> That would happen (by luck) to work with CONFIG_MODULES=because it'd be handled
> by vmalloc_to_page() walking the page tables. I suspect that'll happen to work
> on arm64 by virtue of patch 1, but that wasn't intentional.

Yeah, even with init text, it should be handled as a part of text address.
So using phys_to_page() will be better.

> 
> I'm not sure what the right fix is here, it's annoying that core_kernel_text()
> is special-cased for SYSTEM_FREEING_INITMEM, but that was deliberate as of
> commit:
> 
>   d2635f2012a44e3d ("mm: create a new system state and fix core_kernel_text()")
> 
> .. though I'm not sure what exactly that was trying to fix at a higher level.

at the kprobe_free_init_mem() point, init text is not freed yet, so why not we check
the is_kernel_inittext() in patch_map()? I think this special case is handled in
patch_map(). (Or, we can introduce early_patch_map())

> 
> I can look into this some more tomorrow.

Thank you!

> 
> Mark.
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Jarkko Sakkinen]

On Thu Apr 4, 2024 at 7:10 PM EEST, Masami Hiramatsu (Google) wrote:
> On Thu, 04 Apr 2024 18:18:21 +0300
> "Jarkko Sakkinen" <jarkko@kernel.org> wrote:
>
> > On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> > > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > >
> > > > Tracing with kprobes while running a monolithic kernel is currently
> > > > impossible because KPROBES depends on MODULES. While this dependency is
> > > > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > > > memory using module_alloc(), all the other module-specific code only
> > > > exist to handle the case when MODULES=y, and can be hidden behind
> > > > ifdeffery.
> > > >
> > > > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > > > HAVE_KPROBES_ALLOC=y.
> > > >
> > > > As of this patch kprobes can be used when MODULES=n on arm64 and
> > > > riscv. All other architectures still depend on MODULES, either by virtue
> > > > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > > > virtue of an explciit dependency on MODULES in arch code.
> > > >
> > > > Other architectures can enable support by implementing their own
> > > > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > > > depend on MODULES.
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > > > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > > > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/Kconfig                |  2 +-
> > > >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> > > >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > > index 85bb59f7b8c07..0df2c88547b3c 100644
> > > > --- a/arch/Kconfig
> > > > +++ b/arch/Kconfig
> > > > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> > > >  
> > > >  config KPROBES
> > > >  	bool "Kprobes"
> > > > -	depends on MODULES
> > > > +	depends on MODULES || HAVE_KPROBES_ALLOC
> > > >  	depends on HAVE_KPROBES
> > > >  	select KALLSYMS
> > > >  	select TASKS_RCU if PREEMPTION
> > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > index fa2ee4e59eca2..ec4493a41b505 100644
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > >  			goto out;
> > > >  		}
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  		/*
> > > >  		 * If the module freed '.init.text', we couldn't insert
> > > >  		 * kprobes in there.
> > > > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > >  			*probed_mod = NULL;
> > > >  			ret = -ENOENT;
> > > >  		}
> > > > +#endif /* CONFIG_MODULES */
> > > >  	}
> > > > +
> > > >  out:
> > > >  	preempt_enable();
> > > >  	jump_label_unlock();
> > > > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -/* Remove all symbols in given area from kprobe blacklist */
> > > > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > -{
> > > > -	struct kprobe_blacklist_entry *ent, *n;
> > > > -
> > > > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > -		if (ent->start_addr < start || ent->start_addr >= end)
> > > > -			continue;
> > > > -		list_del(&ent->list);
> > > > -		kfree(ent);
> > > > -	}
> > > > -}
> > > > -
> > > > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > -{
> > > > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > -}
> > > > -
> > > >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > > >  				   char *type, char *sym)
> > > >  {
> > > > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > > >  	return ret ? : arch_populate_kprobe_blacklist();
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > > +/* Remove all symbols in given area from kprobe blacklist */
> > > > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > +{
> > > > +	struct kprobe_blacklist_entry *ent, *n;
> > > > +
> > > > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > +		if (ent->start_addr < start || ent->start_addr >= end)
> > > > +			continue;
> > > > +		list_del(&ent->list);
> > > > +		kfree(ent);
> > > > +	}
> > > > +}
> > > > +
> > > > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > +{
> > > > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > +}
> > > > +
> > > >  static void add_module_kprobe_blacklist(struct module *mod)
> > > >  {
> > > >  	unsigned long start, end;
> > > > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> > > >  	mutex_unlock(&kprobe_mutex);
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define kprobes_module_callback	(NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static struct notifier_block kprobe_module_nb = {
> > > >  	.notifier_call = kprobes_module_callback,
> > > > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> > > >  	err = arch_init_kprobes();
> > > >  	if (!err)
> > > >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > > > -	if (!err)
> > > > +
> > > > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> > > >  		err = register_module_notifier(&kprobe_module_nb);
> > > >  
> > > >  	kprobes_initialized = (err == 0);
> > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > index 14099cc17fc9e..c509ba776e679 100644
> > > > --- a/kernel/trace/trace_kprobe.c
> > > > +++ b/kernel/trace/trace_kprobe.c
> > > > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > > >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  {
> > > >  	char *p;
> > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > >  {
> > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  /* Module notifier call back, checking event on the module */
> > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  				       unsigned long val, void *data)
> > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_callback (NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static struct notifier_block trace_kprobe_module_nb = {
> > > >  	.notifier_call = trace_kprobe_module_callback,
> > > > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > > > -		return -EINVAL;
> > > > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > > > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > > > +		if (ret)
> > > > +			return -EINVAL;
> > > > +	}
> > > >  
> > > >  	return 0;
> > > >  }
> > >
> > > 2/4, 3/4, 4/4:
> > >
> > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv
> > 
> > Hey, I tried the pci_proc_init example:
> > 
> > [    3.060703] ------------[ ftrace bug ]------------
> > [    3.060944] ftrace faulted on writing
> > [    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
> > [    3.061509] Updating ftrace call site to call a different ftrace function
> > [    3.061756] ftrace record flags: 80100001
> > [    3.061925]  (1)
> > [    3.061925]  expected tramp: ffffffff8000aa60
> > [    3.062527] ------------[ cut here ]------------
> > [    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
> > [    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
> > [    3.062807] Hardware name: riscv-virtio,qemu (DT)
> > [    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
> > [    3.062925] epc : ftrace_bug+0x282/0x2b8
> > [    3.062957]  ra : ftrace_bug+0x282/0x2b8
> > [    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
> > [    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
> > [    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
> > [    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
> > [    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > [    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > [    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
> > [    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
> > [    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
> > [    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
> > [    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
> > [    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > [    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
> > [    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
> > [    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
> > [    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
> > [    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
> > [    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
> > [    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
> > [    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
> > [    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > [    3.063676] ---[ end trace 0000000000000000 ]---
> > [    3.069730] ------------[ cut here ]------------
> > [    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
> > [    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
> > [    3.070133] Hardware name: riscv-virtio,qemu (DT)
> > [    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
> > [    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
> > [    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
> > [    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
> > [    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > [    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > [    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
> > [    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
> > [    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
> > [    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
> > [    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
> > [    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> > [    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
> > [    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
> > [    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
> > [    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > [    3.070254] ---[ end trace 0000000000000000 ]---
> > [
> > 
> > This is with riscv64 defconfig, tracing shenanigans and the following
> > bootconfig and the bug was realized in QEMU:
>
> So this is with CONFIG_MODULES=y?
> This seems like an actual bug but not related to this series.
> Can you reproduce this without this patch series?
>
> Thank you,

Sorry I got covid early this week without knowing it first. I was
just wondering why waking up feels like rising from the grave and
I'm sleeping all the time :-)

I'll sanity check this issue hopefully tomorrow but latest on Monday
(feeling better already).

I think this was without modules but I'll sanity check everything
and come back with the results.

BR, Jarkko

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

Re: [PATCH v2 4/4] kprobes: Remove core dependency on modules

[Jarkko Sakkinen]

On Thu Apr 4, 2024 at 7:47 PM EEST, Mark Rutland wrote:
> On Fri, Apr 05, 2024 at 01:10:26AM +0900, Masami Hiramatsu wrote:
> > On Thu, 04 Apr 2024 18:18:21 +0300
> > "Jarkko Sakkinen" <jarkko@kernel.org> wrote:
> > 
> > > On Thu Apr 4, 2024 at 11:15 AM EEST, Jarkko Sakkinen wrote:
> > > > On Wed Apr 3, 2024 at 6:01 PM EEST, Mark Rutland wrote:
> > > > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > > >
> > > > > Tracing with kprobes while running a monolithic kernel is currently
> > > > > impossible because KPROBES depends on MODULES. While this dependency is
> > > > > necessary when HAVE_KPROBES_ALLOC=n and the core kprobes code allocates
> > > > > memory using module_alloc(), all the other module-specific code only
> > > > > exist to handle the case when MODULES=y, and can be hidden behind
> > > > > ifdeffery.
> > > > >
> > > > > Add the necessary ifdeffery, and remove the dependency on MODULES=y when
> > > > > HAVE_KPROBES_ALLOC=y.
> > > > >
> > > > > As of this patch kprobes can be used when MODULES=n on arm64 and
> > > > > riscv. All other architectures still depend on MODULES, either by virtue
> > > > > of the core dependency on MODULES when HAVE_KPROBES_ALLOC=n, or by
> > > > > virtue of an explciit dependency on MODULES in arch code.
> > > > >
> > > > > Other architectures can enable support by implementing their own
> > > > > kprobes_alloc_insn_page() and kprobes_free_insn_page() which do not
> > > > > depend on MODULES.
> > > > >
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > Link: https://lore.kernel.org/lkml/20240326134616.7691-1-jarkko@kernel.org/
> > > > > [Mark: Remove execmem changes, depend on HAVE_KPROBES_ALLOC]
> > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Albert Ou <aou@eecs.berkeley.edu>
> > > > > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
> > > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > > Cc: Paul Walmsley <paul.walmsley@sifive.com>
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > ---
> > > > >  arch/Kconfig                |  2 +-
> > > > >  kernel/kprobes.c            | 46 ++++++++++++++++++++++---------------
> > > > >  kernel/trace/trace_kprobe.c | 15 ++++++++++--
> > > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > > > index 85bb59f7b8c07..0df2c88547b3c 100644
> > > > > --- a/arch/Kconfig
> > > > > +++ b/arch/Kconfig
> > > > > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> > > > >  
> > > > >  config KPROBES
> > > > >  	bool "Kprobes"
> > > > > -	depends on MODULES
> > > > > +	depends on MODULES || HAVE_KPROBES_ALLOC
> > > > >  	depends on HAVE_KPROBES
> > > > >  	select KALLSYMS
> > > > >  	select TASKS_RCU if PREEMPTION
> > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > > index fa2ee4e59eca2..ec4493a41b505 100644
> > > > > --- a/kernel/kprobes.c
> > > > > +++ b/kernel/kprobes.c
> > > > > @@ -1594,6 +1594,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > >  			goto out;
> > > > >  		}
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  		/*
> > > > >  		 * If the module freed '.init.text', we couldn't insert
> > > > >  		 * kprobes in there.
> > > > > @@ -1604,7 +1605,9 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > >  			*probed_mod = NULL;
> > > > >  			ret = -ENOENT;
> > > > >  		}
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  	}
> > > > > +
> > > > >  out:
> > > > >  	preempt_enable();
> > > > >  	jump_label_unlock();
> > > > > @@ -2484,24 +2487,6 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -/* Remove all symbols in given area from kprobe blacklist */
> > > > > -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > > -{
> > > > > -	struct kprobe_blacklist_entry *ent, *n;
> > > > > -
> > > > > -	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > > -		if (ent->start_addr < start || ent->start_addr >= end)
> > > > > -			continue;
> > > > > -		list_del(&ent->list);
> > > > > -		kfree(ent);
> > > > > -	}
> > > > > -}
> > > > > -
> > > > > -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > > -{
> > > > > -	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > > -}
> > > > > -
> > > > >  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > > > >  				   char *type, char *sym)
> > > > >  {
> > > > > @@ -2566,6 +2551,25 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > > > >  	return ret ? : arch_populate_kprobe_blacklist();
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > > +/* Remove all symbols in given area from kprobe blacklist */
> > > > > +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > > > > +{
> > > > > +	struct kprobe_blacklist_entry *ent, *n;
> > > > > +
> > > > > +	list_for_each_entry_safe(ent, n, &kprobe_blacklist, list) {
> > > > > +		if (ent->start_addr < start || ent->start_addr >= end)
> > > > > +			continue;
> > > > > +		list_del(&ent->list);
> > > > > +		kfree(ent);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > > > > +{
> > > > > +	kprobe_remove_area_blacklist(entry, entry + 1);
> > > > > +}
> > > > > +
> > > > >  static void add_module_kprobe_blacklist(struct module *mod)
> > > > >  {
> > > > >  	unsigned long start, end;
> > > > > @@ -2662,6 +2666,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
> > > > >  	mutex_unlock(&kprobe_mutex);
> > > > >  	return NOTIFY_DONE;
> > > > >  }
> > > > > +#else
> > > > > +#define kprobes_module_callback	(NULL)
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static struct notifier_block kprobe_module_nb = {
> > > > >  	.notifier_call = kprobes_module_callback,
> > > > > @@ -2726,7 +2733,8 @@ static int __init init_kprobes(void)
> > > > >  	err = arch_init_kprobes();
> > > > >  	if (!err)
> > > > >  		err = register_die_notifier(&kprobe_exceptions_nb);
> > > > > -	if (!err)
> > > > > +
> > > > > +	if (!err && IS_ENABLED(CONFIG_MODULES))
> > > > >  		err = register_module_notifier(&kprobe_module_nb);
> > > > >  
> > > > >  	kprobes_initialized = (err == 0);
> > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > > index 14099cc17fc9e..c509ba776e679 100644
> > > > > --- a/kernel/trace/trace_kprobe.c
> > > > > +++ b/kernel/trace/trace_kprobe.c
> > > > > @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > > > >  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  {
> > > > >  	char *p;
> > > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  
> > > > >  	return ret;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > > >  {
> > > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  /* Module notifier call back, checking event on the module */
> > > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  				       unsigned long val, void *data)
> > > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  
> > > > >  	return NOTIFY_DONE;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_callback (NULL)
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static struct notifier_block trace_kprobe_module_nb = {
> > > > >  	.notifier_call = trace_kprobe_module_callback,
> > > > > @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > -	if (register_module_notifier(&trace_kprobe_module_nb))
> > > > > -		return -EINVAL;
> > > > > +	if (IS_ENABLED(CONFIG_MODULES)) {
> > > > > +		ret = register_module_notifier(&trace_kprobe_module_nb);
> > > > > +		if (ret)
> > > > > +			return -EINVAL;
> > > > > +	}
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > >
> > > > 2/4, 3/4, 4/4:
> > > >
> > > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> # arch/riscv
> > > 
> > > Hey, I tried the pci_proc_init example:
> > > 
> > > [    3.060703] ------------[ ftrace bug ]------------
> > > [    3.060944] ftrace faulted on writing
> > > [    3.060987] [<ffffffff8102c0da>] pci_proc_init+0x0/0x80
> > > [    3.061509] Updating ftrace call site to call a different ftrace function
> > > [    3.061756] ftrace record flags: 80100001
> > > [    3.061925]  (1)
> > > [    3.061925]  expected tramp: ffffffff8000aa60
> > > [    3.062527] ------------[ cut here ]------------
> > > [    3.062652] WARNING: CPU: 0 PID: 18 at kernel/trace/ftrace.c:2180 ftrace_bug+0x282/0x2b8
> > > [    3.062747] CPU: 0 PID: 18 Comm: migration/0 Not tainted 6.9.0-rc1 #2
> > > [    3.062807] Hardware name: riscv-virtio,qemu (DT)
> > > [    3.062868] Stopper: multi_cpu_stop+0x0/0x1a0 <- stop_machine_cpuslocked+0x140/0x18c
> > > [    3.062925] epc : ftrace_bug+0x282/0x2b8
> > > [    3.062957]  ra : ftrace_bug+0x282/0x2b8
> > > [    3.062989] epc : ffffffff80fc31f4 ra : ffffffff80fc31f4 sp : ff20000000093c70
> > > [    3.063014]  gp : ffffffff824b7780 tp : ff60000002a85940 t0 : ffffffff800923a6
> > > [    3.063037]  t1 : 0000000000000020 t2 : 6465746365707865 s0 : ff20000000093cb0
> > > [    3.063061]  s1 : ffffffff8102c0da a0 : 0000000000000022 a1 : ffffffff8229b7f0
> > > [    3.063084]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > > [    3.063108]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > > [    3.063131]  s2 : ff60000002850ab0 s3 : ffffffffffffffff s4 : 0000000000000002
> > > [    3.063154]  s5 : 0000000002000000 s6 : 0000000082000000 s7 : 0000000000000000
> > > [    3.063178]  s8 : 0000000000000001 s9 : ffffffff824bca18 s10: ff60000002845140
> > > [    3.063202]  s11: 00000000000000ab t3 : ffffffff824ce9ef t4 : ffffffff824ce9ef
> > > [    3.063225]  t5 : ffffffff824ce9f0 t6 : ff20000000093aa8
> > > [    3.063248] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > > [    3.063331] [<ffffffff80fc31f4>] ftrace_bug+0x282/0x2b8
> > > [    3.063398] [<ffffffff80108b1a>] ftrace_replace_code+0xfe/0x168
> > > [    3.063430] [<ffffffff80108c82>] ftrace_modify_all_code+0x5c/0x16a
> > > [    3.063460] [<ffffffff80108da2>] __ftrace_modify_code+0x12/0x1c
> > > [    3.063490] [<ffffffff800f299c>] multi_cpu_stop+0x118/0x1a0
> > > [    3.063519] [<ffffffff800f242e>] cpu_stopper_thread+0xb2/0x12a
> > > [    3.063548] [<ffffffff8005dece>] smpboot_thread_fn+0x1aa/0x1d2
> > > [    3.063577] [<ffffffff80057fec>] kthread+0xfe/0x106
> > > [    3.063606] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > > [    3.063676] ---[ end trace 0000000000000000 ]---
> > > [    3.069730] ------------[ cut here ]------------
> > > [    3.069861] Failed to disarm kprobe-ftrace at pci_proc_init+0x0/0x80 (error -19)
> > > [    3.070078] WARNING: CPU: 0 PID: 1 at kernel/kprobes.c:1128 __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070124] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.9.0-rc1 #2
> > > [    3.070133] Hardware name: riscv-virtio,qemu (DT)
> > > [    3.070141] epc : __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070150]  ra : __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070157] epc : ffffffff800ffcda ra : ffffffff800ffcda sp : ff2000000000be30
> > > [    3.070162]  gp : ffffffff824b7780 tp : ff60000002a70000 t0 : ffffffff800923a6
> > > [    3.070167]  t1 : 0000000000000046 t2 : 6f742064656c6961 s0 : ff2000000000be60
> > > [    3.070173]  s1 : ffffffffffffffed a0 : 0000000000000044 a1 : ffffffff8229b7f0
> > > [    3.070178]  a2 : 0000000000000010 a3 : fffffffffffffffe a4 : 0000000000000000
> > > [    3.070182]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000001
> > > [    3.070187]  s2 : ffffffff824bc940 s3 : ffffffff822ac158 s4 : ff60000002b53c80
> > > [    3.070192]  s5 : ffffffff824bc940 s6 : ffffffff822ac158 s7 : ffffffff81000000
> > > [    3.070197]  s8 : ffffffff814775f8 s9 : ffffffff824f23d8 s10: 0000000000000000
> > > [    3.070202]  s11: 0000000000000000 t3 : ffffffff824ce9d7 t4 : ffffffff824ce9d7
> > > [    3.070206]  t5 : ffffffff824ce9d8 t6 : ff2000000000bc48
> > > [    3.070211] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> > > [    3.070218] [<ffffffff800ffcda>] __disarm_kprobe_ftrace+0x9a/0xae
> > > [    3.070228] [<ffffffff80101b16>] kprobe_free_init_mem+0xc2/0x130
> > > [    3.070236] [<ffffffff80fd9b38>] kernel_init+0x46/0x14e
> > > [    3.070245] [<ffffffff80fe3d76>] ret_from_fork+0xe/0x20
> > > [    3.070254] ---[ end trace 0000000000000000 ]---
> > > [
> > > 
> > > This is with riscv64 defconfig, tracing shenanigans and the following
> > > bootconfig and the bug was realized in QEMU:
> > 
> > So this is with CONFIG_MODULES=y?
> > This seems like an actual bug but not related to this series.
> > Can you reproduce this without this patch series?
>
> IIUC what's going on here is:
>
> CONFIG_MODULES=n
>
> .. and so CONFIG_STRICT_MODULE_RWX=n
>
> When kprobe_free_init_mem() is called, system_state == SYSTEM_FREEING_INITMEM, which causes
> core_kernel_text() to return 0 for inittext:
>
> | int notrace core_kernel_text(unsigned long addr)
> | {
> |         if (is_kernel_text(addr))
> |                 return 1;
> | 
> |         if (system_state < SYSTEM_FREEING_INITMEM &&
> |             is_kernel_inittext(addr))
> |                 return 1;
> |         return 0;
> | }
>
> This causes riscv's patch_map() to *not* fixmap the inittext, since it does:
>
> |	if (core_kernel_text(uintaddr) || is_kernel_exittext(uintaddr))
> |		page = phys_to_page(__pa_symbol(addr));
> |	else if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> |		page = vmalloc_to_page(addr);
> |	else
> |		return addr;
>
> ... which fails core_kernel_text(), and IS_ENABLED(CONFIG_STRICT_MODULE_RWX),
> returning the (read-only) mapping of the kernel image.

Right not claiming to be expert with this code, so I guess this is
use to create a temporary mapping for the time period when the code
is patched into text?

>
> That would happen (by luck) to work with CONFIG_MODULES=because it'd be handled
> by vmalloc_to_page() walking the page tables. I suspect that'll happen to work
> on arm64 by virtue of patch 1, but that wasn't intentional.
>
> I'm not sure what the right fix is here, it's annoying that core_kernel_text()
> is special-cased for SYSTEM_FREEING_INITMEM, but that was deliberate as of
> commit:
>
>   d2635f2012a44e3d ("mm: create a new system state and fix core_kernel_text()")
>
> ... though I'm not sure what exactly that was trying to fix at a higher level.
>
> I can look into this some more tomorrow.

Thanks, I'll look forward to test this! Or it might be already out
as I was sick most of this week :-) Have quite a big pile of emails
to cycle through...

BR, Jarkko

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