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/[email protected]/
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.
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 | 34 ++++++++++++++++++++----------
kernel/trace/trace_kprobe.c | 15 +++++++++++--
16 files changed, 87 insertions(+), 34 deletions(-)
--
2.30.2
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 <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Naveen N. Rao <[email protected]>
---
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
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.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Naveen N. Rao <[email protected]>
---
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 39886bab943a8..bdd327b0124e2 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
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.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/kernel/patching.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
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
From: Jarkko Sakkinen <[email protected]>
Tracing with kprobes while running a monolithic kernel is currently
impossible because KPROBES depends on MODULES. While this dependency is
necessary when KPROBES_USE_MODULE_ALLOC=y, 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=N when
KPROBES_USE_MODULE_ALLOC=n.
Currently this allows kprobes to be used when CONFIG_MODULES=n on arm64
and riscv, and 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 <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
[Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
Signed-off-by: Mark Rutland <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Naveen N. Rao <[email protected]>
---
arch/Kconfig | 2 +-
kernel/kprobes.c | 12 +++++++++++-
kernel/trace/trace_kprobe.c | 15 +++++++++++++--
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 85bb59f7b8c07..cf43de9ffb5b9 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -52,7 +52,7 @@ config GENERIC_ENTRY
config KPROBES
bool "Kprobes"
- depends on MODULES
+ depends on MODULES || !KPROBES_USE_MODULE_ALLOC
depends on HAVE_KPROBES
select KALLSYMS
select TASKS_RCU if PREEMPTION
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index fa2ee4e59eca2..7c2f0b504cdcb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1582,6 +1582,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
goto out;
}
+#ifdef CONFIG_MODULES
/* Check if 'p' is probing a module. */
*probed_mod = __module_text_address((unsigned long) p->addr);
if (*probed_mod) {
@@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
ret = -ENOENT;
}
}
+#endif
+
out:
preempt_enable();
jump_label_unlock();
@@ -2484,6 +2487,7 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
return 0;
}
+#ifdef CONFIG_MODULES
/* Remove all symbols in given area from kprobe blacklist */
static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
{
@@ -2501,6 +2505,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
{
kprobe_remove_area_blacklist(entry, entry + 1);
}
+#endif /* CONFIG_MODULES */
int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
char *type, char *sym)
@@ -2566,6 +2571,7 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
return ret ? : arch_populate_kprobe_blacklist();
}
+#ifdef CONFIG_MODULES
static void add_module_kprobe_blacklist(struct module *mod)
{
unsigned long start, end;
@@ -2662,6 +2668,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 +2735,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
On Tue Mar 26, 2024 at 6:36 PM EET, 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 <[email protected]>
> Cc: Anil S Keshavamurthy <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Naveen N. Rao <[email protected]>
> ---
> 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 */
OK, great, I'll test this.
BR, Jarkko
On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> From: Jarkko Sakkinen <[email protected]>
>
> Tracing with kprobes while running a monolithic kernel is currently
> impossible because KPROBES depends on MODULES. While this dependency is
> necessary when KPROBES_USE_MODULE_ALLOC=y, 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=N when
> KPROBES_USE_MODULE_ALLOC=n.
>
> Currently this allows kprobes to be used when CONFIG_MODULES=n on arm64
> and riscv, and 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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Anil S Keshavamurthy <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Naveen N. Rao <[email protected]>
> ---
> arch/Kconfig | 2 +-
> kernel/kprobes.c | 12 +++++++++++-
> kernel/trace/trace_kprobe.c | 15 +++++++++++++--
> 3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..cf43de9ffb5b9 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> + depends on MODULES || !KPROBES_USE_MODULE_ALLOC
> depends on HAVE_KPROBES
> select KALLSYMS
> select TASKS_RCU if PREEMPTION
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fa2ee4e59eca2..7c2f0b504cdcb 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1582,6 +1582,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> goto out;
> }
>
> +#ifdef CONFIG_MODULES
> /* Check if 'p' is probing a module. */
> *probed_mod = __module_text_address((unsigned long) p->addr);
> if (*probed_mod) {
> @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> ret = -ENOENT;
> }
> }
> +#endif
This can be scoped a bit more (see v7 of my patch set).
> +
> out:
> preempt_enable();
> jump_label_unlock();
> @@ -2484,6 +2487,7 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> return 0;
> }
>
> +#ifdef CONFIG_MODULES
> /* Remove all symbols in given area from kprobe blacklist */
> static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> {
> @@ -2501,6 +2505,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
> {
> kprobe_remove_area_blacklist(entry, entry + 1);
> }
> +#endif /* CONFIG_MODULES */
>
> int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> char *type, char *sym)
> @@ -2566,6 +2571,7 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> return ret ? : arch_populate_kprobe_blacklist();
> }
>
> +#ifdef CONFIG_MODULES
> static void add_module_kprobe_blacklist(struct module *mod)
> {
> unsigned long start, end;
> @@ -2662,6 +2668,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 +2735,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 */
The last two CONFIG_MODULES sections could be combined. This was also in
v7.
>
> 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;
> }
Other than lgtm.
BR, Jarkko
On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > +#ifdef CONFIG_MODULES
> > /* Check if 'p' is probing a module. */
> > *probed_mod = __module_text_address((unsigned long) p->addr);
> > if (*probed_mod) {
> > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > ret = -ENOENT;
> > }
> > }
> > +#endif
>
> This can be scoped a bit more (see v7 of my patch set).
> > +#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 */
>
> The last two CONFIG_MODULES sections could be combined. This was also in
> v7.
> Other than lgtm.
Great! I've folded your v7 changes in, and pushed that out to:
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
I'll hold off sending that out to the list until other folk have had a chance
to comment.
Mark.
On Tue, 26 Mar 2024 17:38:18 +0000
Mark Rutland <[email protected]> wrote:
> On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
>
> > > +#ifdef CONFIG_MODULES
> > > /* Check if 'p' is probing a module. */
> > > *probed_mod = __module_text_address((unsigned long) p->addr);
> > > if (*probed_mod) {
> > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > ret = -ENOENT;
> > > }
> > > }
> > > +#endif
> >
> > This can be scoped a bit more (see v7 of my patch set).
>
> > > +#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 */
> >
> > The last two CONFIG_MODULES sections could be combined. This was also in
> > v7.
>
> > Other than lgtm.
>
> Great! I've folded your v7 changes in, and pushed that out to:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
>
> I'll hold off sending that out to the list until other folk have had a chance
> to comment.
Yeah, the updated one looks good to me too.
Thanks!
>
> Mark.
--
Masami Hiramatsu (Google) <[email protected]>
On Wed, 2024-03-27 at 09:01 +0900, Masami Hiramatsu wrote:
> On Tue, 26 Mar 2024 17:38:18 +0000
> Mark Rutland <[email protected]> wrote:
>
> > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> >
> > > > +#ifdef CONFIG_MODULES
> > > > /* Check if 'p' is probing a module. */
> > > > *probed_mod = __module_text_address((unsigned long) p-
> > > > >addr);
> > > > if (*probed_mod) {
> > > > @@ -1605,6 +1606,8 @@ static int
> > > > check_kprobe_address_safe(struct kprobe *p,
> > > > ret = -ENOENT;
> > > > }
> > > > }
> > > > +#endif
> > >
> > > This can be scoped a bit more (see v7 of my patch set).
> >
> > > > +#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 */
> > >
> > > The last two CONFIG_MODULES sections could be combined. This was
> > > also in
> > > v7.
> >
> > > Other than lgtm.
> >
> > Great! I've folded your v7 changes in, and pushed that out to:
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> >
> > I'll hold off sending that out to the list until other folk have
> > had a chance
> > to comment.
>
> Yeah, the updated one looks good to me too.
>
> Thanks!
Yeah, I'm also planning to test this with x86 instrumenting sgx_* calls
as I need to test the cgroups support for it so can help with the
coverage both RISC-V and x86 (as I find a good time slot).
BR, Jarkko
On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> On Tue, 26 Mar 2024 17:38:18 +0000
> Mark Rutland <[email protected]> wrote:
>
> > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> >
> > > > +#ifdef CONFIG_MODULES
> > > > /* Check if 'p' is probing a module. */
> > > > *probed_mod = __module_text_address((unsigned long) p->addr);
> > > > if (*probed_mod) {
> > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > ret = -ENOENT;
> > > > }
> > > > }
> > > > +#endif
> > >
> > > This can be scoped a bit more (see v7 of my patch set).
> >
> > > > +#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 */
> > >
> > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > v7.
> >
> > > Other than lgtm.
> >
> > Great! I've folded your v7 changes in, and pushed that out to:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> >
> > I'll hold off sending that out to the list until other folk have had a chance
> > to comment.
>
> Yeah, the updated one looks good to me too.
>
> Thanks!
As for RISC-V:
Tested-by: Jarkko Sakkinen <[email protected]> # arch/riscv
I'm fine with adding to all patches because it would be hard
to place tested-by to any specific patch (e.g. if this was a
syscall I would give tested-by just for that patch).
Just adding disclaimer because depending on subsystem people
are more or less strict with this tag :-)
BR, Jarkko
On Wed, 27 Mar 2024 19:46:50 +0200
"Jarkko Sakkinen" <[email protected]> wrote:
> On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> > On Tue, 26 Mar 2024 17:38:18 +0000
> > Mark Rutland <[email protected]> wrote:
> >
> > > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > >
> > > > > +#ifdef CONFIG_MODULES
> > > > > /* Check if 'p' is probing a module. */
> > > > > *probed_mod = __module_text_address((unsigned long) p->addr);
> > > > > if (*probed_mod) {
> > > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > > ret = -ENOENT;
> > > > > }
> > > > > }
> > > > > +#endif
> > > >
> > > > This can be scoped a bit more (see v7 of my patch set).
> > >
> > > > > +#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 */
> > > >
> > > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > > v7.
> > >
> > > > Other than lgtm.
> > >
> > > Great! I've folded your v7 changes in, and pushed that out to:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > >
> > > I'll hold off sending that out to the list until other folk have had a chance
> > > to comment.
> >
> > Yeah, the updated one looks good to me too.
> >
> > Thanks!
>
> As for RISC-V:
>
> Tested-by: Jarkko Sakkinen <[email protected]> # arch/riscv
Thank you for testing!
>
> I'm fine with adding to all patches because it would be hard
> to place tested-by to any specific patch (e.g. if this was a
> syscall I would give tested-by just for that patch).
Except for the 1st patch because that is for arm64, right? :)
>
> Just adding disclaimer because depending on subsystem people
> are more or less strict with this tag :-)
>
> BR, Jarkko
Thanks,
--
Masami Hiramatsu (Google) <[email protected]>
On Thu Mar 28, 2024 at 1:47 AM EET, Masami Hiramatsu (Google) wrote:
> On Wed, 27 Mar 2024 19:46:50 +0200
> "Jarkko Sakkinen" <[email protected]> wrote:
>
> > On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> > > On Tue, 26 Mar 2024 17:38:18 +0000
> > > Mark Rutland <[email protected]> wrote:
> > >
> > > > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > > >
> > > > > > +#ifdef CONFIG_MODULES
> > > > > > /* Check if 'p' is probing a module. */
> > > > > > *probed_mod = __module_text_address((unsigned long) p->addr);
> > > > > > if (*probed_mod) {
> > > > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > > > ret = -ENOENT;
> > > > > > }
> > > > > > }
> > > > > > +#endif
> > > > >
> > > > > This can be scoped a bit more (see v7 of my patch set).
> > > >
> > > > > > +#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 */
> > > > >
> > > > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > > > v7.
> > > >
> > > > > Other than lgtm.
> > > >
> > > > Great! I've folded your v7 changes in, and pushed that out to:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > > >
> > > > I'll hold off sending that out to the list until other folk have had a chance
> > > > to comment.
> > >
> > > Yeah, the updated one looks good to me too.
> > >
> > > Thanks!
> >
> > As for RISC-V:
> >
> > Tested-by: Jarkko Sakkinen <[email protected]> # arch/riscv
>
> Thank you for testing!
>
> >
> > I'm fine with adding to all patches because it would be hard
> > to place tested-by to any specific patch (e.g. if this was a
> > syscall I would give tested-by just for that patch).
>
> Except for the 1st patch because that is for arm64, right? :)
Right! For that not required :-)
>
> >
> > Just adding disclaimer because depending on subsystem people
> > are more or less strict with this tag :-)
> >
> > BR, Jarkko
>
> Thanks,
BR, Jarkko
On Tue, Mar 26, 2024 at 04:36:24PM +0000, Mark Rutland wrote:
> From: Jarkko Sakkinen <[email protected]>
>
> Tracing with kprobes while running a monolithic kernel is currently
> impossible because KPROBES depends on MODULES. While this dependency is
> necessary when KPROBES_USE_MODULE_ALLOC=y, 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=N when
> KPROBES_USE_MODULE_ALLOC=n.
>
> Currently this allows kprobes to be used when CONFIG_MODULES=n on arm64
> and riscv, and 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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Anil S Keshavamurthy <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Naveen N. Rao <[email protected]>
> ---
> arch/Kconfig | 2 +-
> kernel/kprobes.c | 12 +++++++++++-
> kernel/trace/trace_kprobe.c | 15 +++++++++++++--
> 3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..cf43de9ffb5b9 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> + depends on MODULES || !KPROBES_USE_MODULE_ALLOC
Whoops; that should be:
depends on MODULES || HAVE_KPROBES_ALLOC
.. with similar fixups in the commit message to describe HAVE_KPROBES_ALLOC
rather than KPROBES_USE_MODULE_ALLOC (which does not exist in any version of
the series that got sent to the list).
I'll send a v2 with that fixed (and the other changes from Jarkko's v7 base
patch) once I've locally tested that for architectures with and without
HAVE_KPROBES_ALLOC.
Mark.
On Wed Apr 3, 2024 at 2:20 PM EEST, Mark Rutland wrote:
> On Tue, Mar 26, 2024 at 04:36:24PM +0000, Mark Rutland wrote:
> > From: Jarkko Sakkinen <[email protected]>
> >
> > Tracing with kprobes while running a monolithic kernel is currently
> > impossible because KPROBES depends on MODULES. While this dependency is
> > necessary when KPROBES_USE_MODULE_ALLOC=y, 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=N when
> > KPROBES_USE_MODULE_ALLOC=n.
> >
> > Currently this allows kprobes to be used when CONFIG_MODULES=n on arm64
> > and riscv, and 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 <[email protected]>
> > Link: https://lore.kernel.org/all/[email protected]/
> > [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> > Signed-off-by: Mark Rutland <[email protected]>
> > Cc: Anil S Keshavamurthy <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: Jarkko Sakkinen <[email protected]>
> > Cc: Masami Hiramatsu <[email protected]>
> > Cc: Naveen N. Rao <[email protected]>
> > ---
> > arch/Kconfig | 2 +-
> > kernel/kprobes.c | 12 +++++++++++-
> > kernel/trace/trace_kprobe.c | 15 +++++++++++++--
> > 3 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 85bb59f7b8c07..cf43de9ffb5b9 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> >
> > config KPROBES
> > bool "Kprobes"
> > - depends on MODULES
> > + depends on MODULES || !KPROBES_USE_MODULE_ALLOC
>
> Whoops; that should be:
>
> depends on MODULES || HAVE_KPROBES_ALLOC
>
> ... with similar fixups in the commit message to describe HAVE_KPROBES_ALLOC
> rather than KPROBES_USE_MODULE_ALLOC (which does not exist in any version of
> the series that got sent to the list).
>
> I'll send a v2 with that fixed (and the other changes from Jarkko's v7 base
> patch) once I've locally tested that for architectures with and without
> HAVE_KPROBES_ALLOC.
OK, please put to me to the CC list as I'm not ATM subscribed
to the tracing list.
BR, Jarkko
On Tue, Mar 26, 2024 at 04:36:23PM +0000, Mark Rutland wrote:
Hi Mark,
..
> (a) Architectures using the generic kprobes_free_insn_page() and
kprobes_alloc_insn_page()?
> 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
kprobes_alloc_insn_page()?
> given a matching implementation of kprobes_free_insn_page(), and
> select HAVE_KPROBES_ALLOC.
.
> arch/s390/Kconfig | 3 ++-
> arch/s390/kernel/kprobes.c | 5 +++++
I tried the repo:
git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git kprobes/without-modules
For s390:
Acked-by: Alexander Gordeev <[email protected]>
Thanks!