Remove MODULES dependency and migrate from module_alloc to the new
text_alloc() API Right now one has to compile LKM support only to enable
kprobes. With this change applied, it is somewhat easier to create
custom test kernel's with a proper debugging capabilities, thus making
Linux more developer friendly.
Jarkko Sakkinen (3):
kprobes: Add text_alloc() and text_free()
module: Add lock_modules() and unlock_modules()
kprobes: Flag out CONFIG_MODULES dependent code
arch/Kconfig | 2 +-
arch/x86/Kconfig | 3 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/module.c | 49 -------------------------
arch/x86/kernel/text.c | 71 +++++++++++++++++++++++++++++++++++++
include/linux/module.h | 29 +++++++++++----
include/linux/text.h | 17 +++++++++
kernel/kprobes.c | 22 ++++++++++--
kernel/livepatch/core.c | 8 ++---
kernel/module.c | 70 ++++++++++++++++++++----------------
kernel/trace/trace_kprobe.c | 20 +++++++++--
11 files changed, 196 insertions(+), 96 deletions(-)
create mode 100644 arch/x86/kernel/text.c
create mode 100644 include/linux/text.h
--
2.25.1
Introduce new API for allocating space for code generaed at run-time
leveraging from module_alloc() and module_memfree() code. Use this to
perform memory allocations in the kprobes code in order to loose the
bound between kprobes and modules subsystem.
Initially, use this API only with arch/x86 and define a new config
flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
new API.
Cc: Andi Kleen <[email protected]>
Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
arch/Kconfig | 2 +-
arch/x86/Kconfig | 3 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/module.c | 49 ---------------------------
arch/x86/kernel/text.c | 71 ++++++++++++++++++++++++++++++++++++++++
include/linux/text.h | 17 ++++++++++
kernel/kprobes.c | 11 +++++++
kernel/module.c | 10 ++++++
8 files changed, 114 insertions(+), 50 deletions(-)
create mode 100644 arch/x86/kernel/text.c
create mode 100644 include/linux/text.h
diff --git a/arch/Kconfig b/arch/Kconfig
index 8cc35dc556c7..e3d6b6868ccb 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -61,7 +61,7 @@ config OPROFILE_NMI_TIMER
config KPROBES
bool "Kprobes"
- depends on MODULES
+ depends on MODULES || ARCH_HAS_TEXT_ALLOC
depends on HAVE_KPROBES
select KALLSYMS
help
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0dea7fdd7a00..a4ee5d1300f6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2035,6 +2035,9 @@ config KEXEC_FILE
config ARCH_HAS_KEXEC_PURGATORY
def_bool KEXEC_FILE
+config ARCH_HAS_TEXT_ALLOC
+ def_bool y
+
config KEXEC_SIG
bool "Verify kernel signature during kexec_file_load() syscall"
depends on KEXEC_FILE
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index e77261db2391..2878e4b753a0 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -68,6 +68,7 @@ obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
obj-y += irqflags.o
+obj-y += text.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 34b153cbd4ac..261df078f127 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -36,55 +36,6 @@ do { \
} while (0)
#endif
-#ifdef CONFIG_RANDOMIZE_BASE
-static unsigned long module_load_offset;
-
-/* Mutex protects the module_load_offset. */
-static DEFINE_MUTEX(module_kaslr_mutex);
-
-static unsigned long int get_module_load_offset(void)
-{
- if (kaslr_enabled()) {
- mutex_lock(&module_kaslr_mutex);
- /*
- * Calculate the module_load_offset the first time this
- * code is called. Once calculated it stays the same until
- * reboot.
- */
- if (module_load_offset == 0)
- module_load_offset =
- (get_random_int() % 1024 + 1) * PAGE_SIZE;
- mutex_unlock(&module_kaslr_mutex);
- }
- return module_load_offset;
-}
-#else
-static unsigned long int get_module_load_offset(void)
-{
- return 0;
-}
-#endif
-
-void *module_alloc(unsigned long size)
-{
- void *p;
-
- if (PAGE_ALIGN(size) > MODULES_LEN)
- return NULL;
-
- p = __vmalloc_node_range(size, MODULE_ALIGN,
- MODULES_VADDR + get_module_load_offset(),
- MODULES_END, GFP_KERNEL,
- PAGE_KERNEL, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
- if (p && (kasan_module_alloc(p, size) < 0)) {
- vfree(p);
- return NULL;
- }
-
- return p;
-}
-
#ifdef CONFIG_X86_32
int apply_relocate(Elf32_Shdr *sechdrs,
const char *strtab,
diff --git a/arch/x86/kernel/text.c b/arch/x86/kernel/text.c
new file mode 100644
index 000000000000..986b92ff1434
--- /dev/null
+++ b/arch/x86/kernel/text.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Kernel module help for x86.
+ * Copyright (C) 2001 Rusty Russell.
+ */
+
+#include <linux/kasan.h>
+#include <linux/mm.h>
+#include <linux/moduleloader.h>
+#include <linux/vmalloc.h>
+#include <asm/setup.h>
+
+#ifdef CONFIG_RANDOMIZE_BASE
+static unsigned long module_load_offset;
+
+/* Mutex protects the module_load_offset. */
+static DEFINE_MUTEX(module_kaslr_mutex);
+
+static unsigned long get_module_load_offset(void)
+{
+ if (kaslr_enabled()) {
+ mutex_lock(&module_kaslr_mutex);
+ /*
+ * Calculate the module_load_offset the first time this
+ * code is called. Once calculated it stays the same until
+ * reboot.
+ */
+ if (module_load_offset == 0)
+ module_load_offset =
+ (get_random_int() % 1024 + 1) * PAGE_SIZE;
+ mutex_unlock(&module_kaslr_mutex);
+ }
+ return module_load_offset;
+}
+#else
+static unsigned long get_module_load_offset(void)
+{
+ return 0;
+}
+#endif
+
+void *text_alloc(unsigned long size)
+{
+ void *p;
+
+ if (PAGE_ALIGN(size) > MODULES_LEN)
+ return NULL;
+
+ p = __vmalloc_node_range(size, MODULE_ALIGN,
+ MODULES_VADDR + get_module_load_offset(),
+ MODULES_END, GFP_KERNEL,
+ PAGE_KERNEL, 0, NUMA_NO_NODE,
+ __builtin_return_address(0));
+ if (p && (kasan_module_alloc(p, size) < 0)) {
+ vfree(p);
+ return NULL;
+ }
+
+ return p;
+}
+
+void text_free(void *region)
+{
+ /*
+ * This memory may be RO, and freeing RO memory in an interrupt is not
+ * supported by vmalloc.
+ */
+ WARN_ON(in_interrupt());
+
+ vfree(region);
+}
diff --git a/include/linux/text.h b/include/linux/text.h
new file mode 100644
index 000000000000..a27d4a42ecda
--- /dev/null
+++ b/include/linux/text.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _LINUX_TEXT_H
+#define _LINUX_TEXT_H
+
+/*
+ * An allocator used for allocating modules, core sections and init sections.
+ * Returns NULL on failure.
+ */
+void *text_alloc(unsigned long size);
+
+/*
+ * Free memory returned from text_alloc().
+ */
+void text_free(void *region);
+
+#endif /* _LINUX_TEXT_H */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2e97febeef77..fa7687eb0c0e 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -35,6 +35,7 @@
#include <linux/ftrace.h>
#include <linux/cpu.h>
#include <linux/jump_label.h>
+#include <linux/text.h>
#include <asm/sections.h>
#include <asm/cacheflush.h>
@@ -111,12 +112,20 @@ enum kprobe_slot_state {
void __weak *alloc_insn_page(void)
{
+#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
+ return text_alloc(PAGE_SIZE);
+#else
return module_alloc(PAGE_SIZE);
+#endif
}
void __weak free_insn_page(void *page)
{
+#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
+ text_free(page);
+#else
module_memfree(page);
+#endif
}
struct kprobe_insn_cache kprobe_insn_slots = {
@@ -1608,6 +1617,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.
@@ -1618,6 +1628,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
*probed_mod = NULL;
ret = -ENOENT;
}
+#endif
}
out:
preempt_enable();
diff --git a/kernel/module.c b/kernel/module.c
index aa183c9ac0a2..8adeb126b02c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -56,6 +56,7 @@
#include <linux/bsearch.h>
#include <linux/dynamic_debug.h>
#include <linux/audit.h>
+#include <linux/text.h>
#include <uapi/linux/module.h>
#include "module-internal.h"
@@ -2151,7 +2152,12 @@ void __weak module_memfree(void *module_region)
* supported by vmalloc.
*/
WARN_ON(in_interrupt());
+
+#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
+ text_free(module_region);
+#else
vfree(module_region);
+#endif
}
void __weak module_arch_cleanup(struct module *mod)
@@ -2786,9 +2792,13 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
void * __weak module_alloc(unsigned long size)
{
+#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
+ return text_alloc(size);
+#else
return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
NUMA_NO_NODE, __builtin_return_address(0));
+#endif
}
bool __weak module_init_section(const char *name)
--
2.25.1
Add wrappers to take the modules "big lock" in order to encapsulate
conditional compilation (CONFIG_MODULES) inside the wrapper.
Cc: Andi Kleen <[email protected]>
Suggested-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
include/linux/module.h | 15 ++++++++++
kernel/kprobes.c | 4 +--
kernel/livepatch/core.c | 8 ++---
kernel/module.c | 60 ++++++++++++++++++-------------------
kernel/trace/trace_kprobe.c | 4 +--
5 files changed, 53 insertions(+), 38 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 2e6670860d27..857b84bf9e90 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -902,4 +902,19 @@ static inline bool module_sig_ok(struct module *module)
}
#endif /* CONFIG_MODULE_SIG */
+#ifdef CONFIG_MODULES
+static inline void lock_modules(void)
+{
+ mutex_lock(&module_mutex);
+}
+
+static inline void unlock_modules(void)
+{
+ mutex_unlock(&module_mutex);
+}
+#else
+static inline void lock_modules(void) { }
+static inline void unlock_modules(void) { }
+#endif
+
#endif /* _LINUX_MODULE_H */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index fa7687eb0c0e..b4f3c24cd2ef 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -573,7 +573,7 @@ static void kprobe_optimizer(struct work_struct *work)
cpus_read_lock();
mutex_lock(&text_mutex);
/* Lock modules while optimizing kprobes */
- mutex_lock(&module_mutex);
+ lock_modules();
/*
* Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
@@ -598,7 +598,7 @@ static void kprobe_optimizer(struct work_struct *work)
/* Step 4: Free cleaned kprobes after quiesence period */
do_free_cleaned_kprobes();
- mutex_unlock(&module_mutex);
+ unlock_modules();
mutex_unlock(&text_mutex);
cpus_read_unlock();
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index f76fdb925532..d9d9d4973e6b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -57,7 +57,7 @@ static void klp_find_object_module(struct klp_object *obj)
if (!klp_is_module(obj))
return;
- mutex_lock(&module_mutex);
+ lock_modules();
/*
* We do not want to block removal of patched modules and therefore
* we do not take a reference here. The patches are removed by
@@ -74,7 +74,7 @@ static void klp_find_object_module(struct klp_object *obj)
if (mod && mod->klp_alive)
obj->mod = mod;
- mutex_unlock(&module_mutex);
+ unlock_modules();
}
static bool klp_initialized(void)
@@ -163,12 +163,12 @@ static int klp_find_object_symbol(const char *objname, const char *name,
.pos = sympos,
};
- mutex_lock(&module_mutex);
+ lock_modules();
if (objname)
module_kallsyms_on_each_symbol(klp_find_callback, &args);
else
kallsyms_on_each_symbol(klp_find_callback, &args);
- mutex_unlock(&module_mutex);
+ unlock_modules();
/*
* Ensure an address was found. If sympos is 0, ensure symbol is unique;
diff --git a/kernel/module.c b/kernel/module.c
index 8adeb126b02c..3fd8686b1637 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -896,7 +896,7 @@ static void module_unload_free(struct module *mod)
{
struct module_use *use, *tmp;
- mutex_lock(&module_mutex);
+ lock_modules();
list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
struct module *i = use->target;
pr_debug("%s unusing %s\n", mod->name, i->name);
@@ -905,7 +905,7 @@ static void module_unload_free(struct module *mod)
list_del(&use->target_list);
kfree(use);
}
- mutex_unlock(&module_mutex);
+ unlock_modules();
}
#ifdef CONFIG_MODULE_FORCE_UNLOAD
@@ -1025,7 +1025,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
if (ret != 0)
goto out;
- mutex_unlock(&module_mutex);
+ unlock_modules();
/* Final destruction now no one is using it. */
if (mod->exit != NULL)
mod->exit();
@@ -1044,7 +1044,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
wake_up_all(&module_wq);
return 0;
out:
- mutex_unlock(&module_mutex);
+ unlock_modules();
return ret;
}
@@ -1449,7 +1449,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
* in the wait_event_interruptible(), which is harmless.
*/
sched_annotate_sleep();
- mutex_lock(&module_mutex);
+ lock_modules();
sym = find_symbol(name, &owner, &crc,
!(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
if (!sym)
@@ -1476,7 +1476,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
/* We must make copy under the lock if we failed to get ref. */
strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
unlock:
- mutex_unlock(&module_mutex);
+ unlock_modules();
return sym;
}
@@ -1731,10 +1731,10 @@ static void del_usage_links(struct module *mod)
#ifdef CONFIG_MODULE_UNLOAD
struct module_use *use;
- mutex_lock(&module_mutex);
+ lock_modules();
list_for_each_entry(use, &mod->target_list, target_list)
sysfs_remove_link(use->target->holders_dir, mod->name);
- mutex_unlock(&module_mutex);
+ unlock_modules();
#endif
}
@@ -1744,14 +1744,14 @@ static int add_usage_links(struct module *mod)
#ifdef CONFIG_MODULE_UNLOAD
struct module_use *use;
- mutex_lock(&module_mutex);
+ lock_modules();
list_for_each_entry(use, &mod->target_list, target_list) {
ret = sysfs_create_link(use->target->holders_dir,
&mod->mkobj.kobj, mod->name);
if (ret)
break;
}
- mutex_unlock(&module_mutex);
+ unlock_modules();
if (ret)
del_usage_links(mod);
#endif
@@ -2177,9 +2177,9 @@ static void free_module(struct module *mod)
/* We leave it in list to prevent duplicate loads, but make sure
* that noone uses it while it's being deconstructed. */
- mutex_lock(&module_mutex);
+ lock_modules();
mod->state = MODULE_STATE_UNFORMED;
- mutex_unlock(&module_mutex);
+ unlock_modules();
/* Remove dynamic debug info */
ddebug_remove_module(mod->name);
@@ -2197,7 +2197,7 @@ static void free_module(struct module *mod)
free_module_elf(mod);
/* Now we can delete it from the lists */
- mutex_lock(&module_mutex);
+ lock_modules();
/* Unlink carefully: kallsyms could be walking list. */
list_del_rcu(&mod->list);
mod_tree_remove(mod);
@@ -2205,7 +2205,7 @@ static void free_module(struct module *mod)
module_bug_cleanup(mod);
/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
synchronize_rcu();
- mutex_unlock(&module_mutex);
+ unlock_modules();
/* This may be empty, but that's OK */
module_arch_freeing_init(mod);
@@ -3504,10 +3504,10 @@ static bool finished_loading(const char *name)
* in the wait_event_interruptible(), which is harmless.
*/
sched_annotate_sleep();
- mutex_lock(&module_mutex);
+ lock_modules();
mod = find_module_all(name, strlen(name), true);
ret = !mod || mod->state == MODULE_STATE_LIVE;
- mutex_unlock(&module_mutex);
+ unlock_modules();
return ret;
}
@@ -3619,7 +3619,7 @@ static noinline int do_init_module(struct module *mod)
ftrace_free_mem(mod, mod->init_layout.base, mod->init_layout.base +
mod->init_layout.size);
- mutex_lock(&module_mutex);
+ lock_modules();
/* Drop initial reference. */
module_put(mod);
trim_init_extable(mod);
@@ -3651,7 +3651,7 @@ static noinline int do_init_module(struct module *mod)
if (llist_add(&freeinit->node, &init_free_list))
schedule_work(&init_free_wq);
- mutex_unlock(&module_mutex);
+ unlock_modules();
wake_up_all(&module_wq);
return 0;
@@ -3693,12 +3693,12 @@ static int add_unformed_module(struct module *mod)
mod->state = MODULE_STATE_UNFORMED;
again:
- mutex_lock(&module_mutex);
+ lock_modules();
old = find_module_all(mod->name, strlen(mod->name), true);
if (old != NULL) {
if (old->state != MODULE_STATE_LIVE) {
/* Wait in case it fails to load. */
- mutex_unlock(&module_mutex);
+ unlock_modules();
err = wait_event_interruptible(module_wq,
finished_loading(mod->name));
if (err)
@@ -3714,7 +3714,7 @@ static int add_unformed_module(struct module *mod)
err = 0;
out:
- mutex_unlock(&module_mutex);
+ unlock_modules();
out_unlocked:
return err;
}
@@ -3723,7 +3723,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
{
int err;
- mutex_lock(&module_mutex);
+ lock_modules();
/* Find duplicate symbols (must be called under lock). */
err = verify_exported_symbols(mod);
@@ -3740,12 +3740,12 @@ static int complete_formation(struct module *mod, struct load_info *info)
/* Mark state as coming so strong_try_module_get() ignores us,
* but kallsyms etc. can see us. */
mod->state = MODULE_STATE_COMING;
- mutex_unlock(&module_mutex);
+ unlock_modules();
return 0;
out:
- mutex_unlock(&module_mutex);
+ unlock_modules();
return err;
}
@@ -3943,9 +3943,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
klp_module_going(mod);
bug_cleanup:
/* module_bug_cleanup needs module_mutex protection */
- mutex_lock(&module_mutex);
+ lock_modules();
module_bug_cleanup(mod);
- mutex_unlock(&module_mutex);
+ unlock_modules();
ddebug_cleanup:
ftrace_release_mod(mod);
@@ -3959,14 +3959,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
free_unload:
module_unload_free(mod);
unlink_mod:
- mutex_lock(&module_mutex);
+ lock_modules();
/* Unlink carefully: kallsyms could be walking list. */
list_del_rcu(&mod->list);
mod_tree_remove(mod);
wake_up_all(&module_wq);
/* Wait for RCU-sched synchronizing before releasing mod->list. */
synchronize_rcu();
- mutex_unlock(&module_mutex);
+ unlock_modules();
free_module:
/* Free lock-classes; relies on the preceding sync_rcu() */
lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
@@ -4322,7 +4322,7 @@ static char *module_flags(struct module *mod, char *buf)
/* Called by the /proc file system to return a list of modules. */
static void *m_start(struct seq_file *m, loff_t *pos)
{
- mutex_lock(&module_mutex);
+ lock_modules();
return seq_list_start(&modules, *pos);
}
@@ -4333,7 +4333,7 @@ static void *m_next(struct seq_file *m, void *p, loff_t *pos)
static void m_stop(struct seq_file *m, void *p)
{
- mutex_unlock(&module_mutex);
+ unlock_modules();
}
static int m_show(struct seq_file *m, void *p)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index aefb6065b508..710ec6a6aa8f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
if (!p)
return true;
*p = '\0';
- mutex_lock(&module_mutex);
+ lock_modules();
ret = !!find_module(tk->symbol);
- mutex_unlock(&module_mutex);
+ unlock_modules();
*p = ':';
return ret;
--
2.25.1
Remove CONFIG_MODULES dependency by flagging out the dependent code. This
allows to use kprobes in a kernel without support for loadable modules,
which could be useful for a test kernel or perhaps an embedded kernel.
Cc: Andi Kleen <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
include/linux/module.h | 14 +++++++-------
kernel/kprobes.c | 7 +++++++
kernel/trace/trace_kprobe.c | 16 +++++++++++++++-
3 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 857b84bf9e90..eaa8ad02f75c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -290,6 +290,13 @@ extern typeof(name) __mod_##type##__##name##_device_table \
struct notifier_block;
+enum module_state {
+ MODULE_STATE_LIVE, /* Normal state. */
+ MODULE_STATE_COMING, /* Full formed, running module_init. */
+ MODULE_STATE_GOING, /* Going away. */
+ MODULE_STATE_UNFORMED, /* Still setting it up. */
+};
+
#ifdef CONFIG_MODULES
extern int modules_disabled; /* for sysctl */
@@ -305,13 +312,6 @@ struct module_use {
struct module *source, *target;
};
-enum module_state {
- MODULE_STATE_LIVE, /* Normal state. */
- MODULE_STATE_COMING, /* Full formed, running module_init. */
- MODULE_STATE_GOING, /* Going away. */
- MODULE_STATE_UNFORMED, /* Still setting it up. */
-};
-
struct mod_tree_node {
struct module *mod;
struct latch_tree_node node;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b4f3c24cd2ef..3039df13d34e 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2225,6 +2225,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)
{
@@ -2242,6 +2243,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
{
kprobe_remove_area_blacklist(entry, entry + 1);
}
+#endif
int __init __weak arch_populate_kprobe_blacklist(void)
{
@@ -2285,6 +2287,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;
@@ -2330,6 +2333,10 @@ static void remove_module_kprobe_blacklist(struct module *mod)
kprobe_remove_area_blacklist(start, end);
}
}
+#else
+static void add_module_kprobe_blacklist(struct module *mod) {}
+static void remove_module_kprobe_blacklist(struct module *mod) {}
+#endif /* CONFIG_MODULES */
/* Module notifier call back, checking kprobes on the module */
static int kprobes_module_callback(struct notifier_block *nb,
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 710ec6a6aa8f..7fcd1bf2d96e 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -103,8 +103,9 @@ static nokprobe_inline bool trace_kprobe_has_gone(struct trace_kprobe *tk)
return !!(kprobe_gone(&tk->rp.kp));
}
+#ifdef CONFIG_MODULES
static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
- struct module *mod)
+ struct module *mod)
{
int len = strlen(mod->name);
const char *name = trace_kprobe_symbol(tk);
@@ -129,6 +130,17 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
return ret;
}
+#else
+static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
+ struct module *mod)
+{
+ return false;
+}
+static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
+{
+ return false;
+}
+#endif
static bool trace_kprobe_is_busy(struct dyn_event *ev)
{
@@ -685,10 +697,12 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
/* Don't need to check busy - this should have gone. */
__unregister_trace_kprobe(tk);
ret = __register_trace_kprobe(tk);
+#ifdef CONFIG_MODULES
if (ret)
pr_warn("Failed to re-register probe %s on %s: %d\n",
trace_probe_name(&tk->tp),
mod->name, ret);
+#endif
}
}
mutex_unlock(&event_mutex);
--
2.25.1
Hi Jarkko,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.8-rc5]
[cannot apply to tip/x86/core tip/perf/core jeyu/modules-next next-20200714]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jarkko-Sakkinen/kprobes-Remove-MODULES-dependency/20200715-063746
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e9919e11e219eaa5e8041b7b1a196839143e9125
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
arch/x86/kernel/text.c: In function 'get_module_load_offset':
>> arch/x86/kernel/text.c:30:6: error: implicit declaration of function 'get_random_int' [-Werror=implicit-function-declaration]
30 | (get_random_int() % 1024 + 1) * PAGE_SIZE;
| ^~~~~~~~~~~~~~
arch/x86/kernel/text.c: At top level:
arch/x86/kernel/text.c:42:7: warning: no previous prototype for 'text_alloc' [-Wmissing-prototypes]
42 | void *text_alloc(unsigned long size)
| ^~~~~~~~~~
arch/x86/kernel/text.c:62:6: warning: no previous prototype for 'text_free' [-Wmissing-prototypes]
62 | void text_free(void *region)
| ^~~~~~~~~
cc1: some warnings being treated as errors
vim +/get_random_int +30 arch/x86/kernel/text.c
18
19 static unsigned long get_module_load_offset(void)
20 {
21 if (kaslr_enabled()) {
22 mutex_lock(&module_kaslr_mutex);
23 /*
24 * Calculate the module_load_offset the first time this
25 * code is called. Once calculated it stays the same until
26 * reboot.
27 */
28 if (module_load_offset == 0)
29 module_load_offset =
> 30 (get_random_int() % 1024 + 1) * PAGE_SIZE;
31 mutex_unlock(&module_kaslr_mutex);
32 }
33 return module_load_offset;
34 }
35 #else
36 static unsigned long get_module_load_offset(void)
37 {
38 return 0;
39 }
40 #endif
41
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Jarkko,
On Wed, 15 Jul 2020 01:32:27 +0300
Jarkko Sakkinen <[email protected]> wrote:
> Introduce new API for allocating space for code generaed at run-time
> leveraging from module_alloc() and module_memfree() code. Use this to
> perform memory allocations in the kprobes code in order to loose the
> bound between kprobes and modules subsystem.
>
> Initially, use this API only with arch/x86 and define a new config
> flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> new API.
This might need to be extended for the text_alloc() flags as we discuss
in previous thread. (e.g. bpf on arm64)
> Cc: Andi Kleen <[email protected]>
> Suggested-by: Peter Zijlstra <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> arch/Kconfig | 2 +-
> arch/x86/Kconfig | 3 ++
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/module.c | 49 ---------------------------
> arch/x86/kernel/text.c | 71 ++++++++++++++++++++++++++++++++++++++++
I think this would better be arch/x86/mm/text_alloc.c (text.c is too
generic, and this only provides memory allocation.)
> include/linux/text.h | 17 ++++++++++
"text_alloc.h" would be better, or puting the prototype in vmalloc.h
will be easier to use.
> kernel/kprobes.c | 11 +++++++
> kernel/module.c | 10 ++++++
> 8 files changed, 114 insertions(+), 50 deletions(-)
> create mode 100644 arch/x86/kernel/text.c
> create mode 100644 include/linux/text.h
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8cc35dc556c7..e3d6b6868ccb 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -61,7 +61,7 @@ config OPROFILE_NMI_TIMER
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> + depends on MODULES || ARCH_HAS_TEXT_ALLOC
> depends on HAVE_KPROBES
> select KALLSYMS
> help
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0dea7fdd7a00..a4ee5d1300f6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2035,6 +2035,9 @@ config KEXEC_FILE
> config ARCH_HAS_KEXEC_PURGATORY
> def_bool KEXEC_FILE
>
> +config ARCH_HAS_TEXT_ALLOC
> + def_bool y
> +
> config KEXEC_SIG
> bool "Verify kernel signature during kexec_file_load() syscall"
> depends on KEXEC_FILE
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index e77261db2391..2878e4b753a0 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -68,6 +68,7 @@ obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
> obj-y += pci-iommu_table.o
> obj-y += resource.o
> obj-y += irqflags.o
> +obj-y += text.o
>
> obj-y += process.o
> obj-y += fpu/
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index 34b153cbd4ac..261df078f127 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -36,55 +36,6 @@ do { \
> } while (0)
> #endif
>
> -#ifdef CONFIG_RANDOMIZE_BASE
> -static unsigned long module_load_offset;
> -
> -/* Mutex protects the module_load_offset. */
> -static DEFINE_MUTEX(module_kaslr_mutex);
> -
> -static unsigned long int get_module_load_offset(void)
> -{
> - if (kaslr_enabled()) {
> - mutex_lock(&module_kaslr_mutex);
> - /*
> - * Calculate the module_load_offset the first time this
> - * code is called. Once calculated it stays the same until
> - * reboot.
> - */
> - if (module_load_offset == 0)
> - module_load_offset =
> - (get_random_int() % 1024 + 1) * PAGE_SIZE;
> - mutex_unlock(&module_kaslr_mutex);
> - }
> - return module_load_offset;
> -}
> -#else
> -static unsigned long int get_module_load_offset(void)
> -{
> - return 0;
> -}
> -#endif
> -
> -void *module_alloc(unsigned long size)
> -{
> - void *p;
> -
> - if (PAGE_ALIGN(size) > MODULES_LEN)
> - return NULL;
> -
> - p = __vmalloc_node_range(size, MODULE_ALIGN,
> - MODULES_VADDR + get_module_load_offset(),
> - MODULES_END, GFP_KERNEL,
> - PAGE_KERNEL, 0, NUMA_NO_NODE,
> - __builtin_return_address(0));
> - if (p && (kasan_module_alloc(p, size) < 0)) {
> - vfree(p);
> - return NULL;
> - }
> -
> - return p;
> -}
Please don't touch this module_alloc() at all. Then we can
just call __vmalloc_node_range() in the text_alloc().
> -
> #ifdef CONFIG_X86_32
> int apply_relocate(Elf32_Shdr *sechdrs,
> const char *strtab,
> diff --git a/arch/x86/kernel/text.c b/arch/x86/kernel/text.c
> new file mode 100644
> index 000000000000..986b92ff1434
> --- /dev/null
> +++ b/arch/x86/kernel/text.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Kernel module help for x86.
> + * Copyright (C) 2001 Rusty Russell.
> + */
> +
> +#include <linux/kasan.h>
> +#include <linux/mm.h>
> +#include <linux/moduleloader.h>
> +#include <linux/vmalloc.h>
> +#include <asm/setup.h>
> +
> +#ifdef CONFIG_RANDOMIZE_BASE
> +static unsigned long module_load_offset;
> +
> +/* Mutex protects the module_load_offset. */
> +static DEFINE_MUTEX(module_kaslr_mutex);
> +
> +static unsigned long get_module_load_offset(void)
> +{
> + if (kaslr_enabled()) {
> + mutex_lock(&module_kaslr_mutex);
> + /*
> + * Calculate the module_load_offset the first time this
> + * code is called. Once calculated it stays the same until
> + * reboot.
> + */
> + if (module_load_offset == 0)
> + module_load_offset =
> + (get_random_int() % 1024 + 1) * PAGE_SIZE;
> + mutex_unlock(&module_kaslr_mutex);
> + }
> + return module_load_offset;
> +}
> +#else
> +static unsigned long get_module_load_offset(void)
> +{
> + return 0;
> +}
> +#endif
So as I pointed, we can ignore this for kprobes (and other
dynamic allocated trampoline code).
> +
> +void *text_alloc(unsigned long size)
> +{
> + void *p;
> +
> + if (PAGE_ALIGN(size) > MODULES_LEN)
> + return NULL;
> +
> + p = __vmalloc_node_range(size, MODULE_ALIGN,
> + MODULES_VADDR + get_module_load_offset(),
> + MODULES_END, GFP_KERNEL,
> + PAGE_KERNEL, 0, NUMA_NO_NODE,
> + __builtin_return_address(0));
> + if (p && (kasan_module_alloc(p, size) < 0)) {
> + vfree(p);
> + return NULL;
> + }
> +
> + return p;
> +}
> +
> +void text_free(void *region)
> +{
> + /*
> + * This memory may be RO, and freeing RO memory in an interrupt is not
> + * supported by vmalloc.
> + */
> + WARN_ON(in_interrupt());
> +
> + vfree(region);
> +}
> diff --git a/include/linux/text.h b/include/linux/text.h
> new file mode 100644
> index 000000000000..a27d4a42ecda
> --- /dev/null
> +++ b/include/linux/text.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _LINUX_TEXT_H
> +#define _LINUX_TEXT_H
> +
> +/*
> + * An allocator used for allocating modules, core sections and init sections.
> + * Returns NULL on failure.
> + */
> +void *text_alloc(unsigned long size);
> +
> +/*
> + * Free memory returned from text_alloc().
> + */
> +void text_free(void *region);
Hmm, if this is this short, in this version we might better move
these to vmalloc.h.
> +
> +#endif /* _LINUX_TEXT_H */
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2e97febeef77..fa7687eb0c0e 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -35,6 +35,7 @@
> #include <linux/ftrace.h>
> #include <linux/cpu.h>
> #include <linux/jump_label.h>
> +#include <linux/text.h>
>
> #include <asm/sections.h>
> #include <asm/cacheflush.h>
> @@ -111,12 +112,20 @@ enum kprobe_slot_state {
>
> void __weak *alloc_insn_page(void)
> {
> +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> + return text_alloc(PAGE_SIZE);
> +#else
> return module_alloc(PAGE_SIZE);
> +#endif
> }
>
> void __weak free_insn_page(void *page)
> {
> +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> + text_free(page);
> +#else
> module_memfree(page);
> +#endif
> }
>
> struct kprobe_insn_cache kprobe_insn_slots = {
> @@ -1608,6 +1617,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.
> @@ -1618,6 +1628,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> *probed_mod = NULL;
> ret = -ENOENT;
> }
> +#endif
This change is not related to text_alloc(). Please move this to 3/3.
> }
> out:
> preempt_enable();
> diff --git a/kernel/module.c b/kernel/module.c
> index aa183c9ac0a2..8adeb126b02c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -56,6 +56,7 @@
> #include <linux/bsearch.h>
> #include <linux/dynamic_debug.h>
> #include <linux/audit.h>
> +#include <linux/text.h>
> #include <uapi/linux/module.h>
> #include "module-internal.h"
>
> @@ -2151,7 +2152,12 @@ void __weak module_memfree(void *module_region)
> * supported by vmalloc.
> */
> WARN_ON(in_interrupt());
> +
> +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> + text_free(module_region);
> +#else
> vfree(module_region);
> +#endif
> }
>
> void __weak module_arch_cleanup(struct module *mod)
> @@ -2786,9 +2792,13 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
>
> void * __weak module_alloc(unsigned long size)
> {
> +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> + return text_alloc(size);
> +#else
> return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
> NUMA_NO_NODE, __builtin_return_address(0));
> +#endif
> }
Please don't touch kernel/module.c too. This seems to make things complicated.
Thank you,
--
Masami Hiramatsu <[email protected]>
Hi Jarkko,
On Wed, 15 Jul 2020 01:32:29 +0300
Jarkko Sakkinen <[email protected]> wrote:
> Remove CONFIG_MODULES dependency by flagging out the dependent code. This
> allows to use kprobes in a kernel without support for loadable modules,
> which could be useful for a test kernel or perhaps an embedded kernel.
>
OK, looks good, I just have 2 comments below.
> Cc: Andi Kleen <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> include/linux/module.h | 14 +++++++-------
> kernel/kprobes.c | 7 +++++++
> kernel/trace/trace_kprobe.c | 16 +++++++++++++++-
> 3 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 857b84bf9e90..eaa8ad02f75c 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -290,6 +290,13 @@ extern typeof(name) __mod_##type##__##name##_device_table \
>
> struct notifier_block;
>
> +enum module_state {
> + MODULE_STATE_LIVE, /* Normal state. */
> + MODULE_STATE_COMING, /* Full formed, running module_init. */
> + MODULE_STATE_GOING, /* Going away. */
> + MODULE_STATE_UNFORMED, /* Still setting it up. */
> +};
> +
> #ifdef CONFIG_MODULES
>
> extern int modules_disabled; /* for sysctl */
> @@ -305,13 +312,6 @@ struct module_use {
> struct module *source, *target;
> };
>
> -enum module_state {
> - MODULE_STATE_LIVE, /* Normal state. */
> - MODULE_STATE_COMING, /* Full formed, running module_init. */
> - MODULE_STATE_GOING, /* Going away. */
> - MODULE_STATE_UNFORMED, /* Still setting it up. */
> -};
> -
> struct mod_tree_node {
> struct module *mod;
> struct latch_tree_node node;
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index b4f3c24cd2ef..3039df13d34e 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2225,6 +2225,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)
> {
> @@ -2242,6 +2243,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
> {
> kprobe_remove_area_blacklist(entry, entry + 1);
> }
> +#endif
>
> int __init __weak arch_populate_kprobe_blacklist(void)
> {
> @@ -2285,6 +2287,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;
> @@ -2330,6 +2333,10 @@ static void remove_module_kprobe_blacklist(struct module *mod)
> kprobe_remove_area_blacklist(start, end);
> }
> }
> +#else
> +static void add_module_kprobe_blacklist(struct module *mod) {}
> +static void remove_module_kprobe_blacklist(struct module *mod) {}
> +#endif /* CONFIG_MODULES */
Please feel free to move the function. I would like to see;
#ifdef CONFIG_MODULES
/* Remove all symbols in given area from kprobe blacklist */
static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
{
...
}
static void add_module_kprobe_blacklist(struct module *mod)
{
...
}
#else
static void add_module_kprobe_blacklist(struct module *mod) {}
static void remove_module_kprobe_blacklist(struct module *mod) {}
#endif /* CONFIG_MODULES */
Rather than split #ifdefs.
>
> /* Module notifier call back, checking kprobes on the module */
> static int kprobes_module_callback(struct notifier_block *nb,
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 710ec6a6aa8f..7fcd1bf2d96e 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -103,8 +103,9 @@ static nokprobe_inline bool trace_kprobe_has_gone(struct trace_kprobe *tk)
> return !!(kprobe_gone(&tk->rp.kp));
> }
>
> +#ifdef CONFIG_MODULES
> static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> - struct module *mod)
> + struct module *mod)
> {
> int len = strlen(mod->name);
> const char *name = trace_kprobe_symbol(tk);
> @@ -129,6 +130,17 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>
> return ret;
> }
> +#else
> +static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> + struct module *mod)
> +{
> + return false;
> +}
> +static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> +{
> + return false;
> +}
> +#endif
>
> static bool trace_kprobe_is_busy(struct dyn_event *ev)
> {
> @@ -685,10 +697,12 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> /* Don't need to check busy - this should have gone. */
> __unregister_trace_kprobe(tk);
> ret = __register_trace_kprobe(tk);
> +#ifdef CONFIG_MODULES
> if (ret)
> pr_warn("Failed to re-register probe %s on %s: %d\n",
> trace_probe_name(&tk->tp),
> mod->name, ret);
> +#endif
I guess this CONFIG_MODULES is for avoiding build error according to mod->name,
if so, please use module_name(mod) macro instead of this #ifdef.
> }
> }
> mutex_unlock(&event_mutex);
> --
> 2.25.1
>
--
Masami Hiramatsu <[email protected]>
On Wed, 15 Jul 2020 01:32:28 +0300
Jarkko Sakkinen <[email protected]> wrote:
> Add wrappers to take the modules "big lock" in order to encapsulate
> conditional compilation (CONFIG_MODULES) inside the wrapper.
>
> Cc: Andi Kleen <[email protected]>
> Suggested-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> include/linux/module.h | 15 ++++++++++
> kernel/kprobes.c | 4 +--
> kernel/livepatch/core.c | 8 ++---
> kernel/module.c | 60 ++++++++++++++++++-------------------
> kernel/trace/trace_kprobe.c | 4 +--
> 5 files changed, 53 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2e6670860d27..857b84bf9e90 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -902,4 +902,19 @@ static inline bool module_sig_ok(struct module *module)
> }
> #endif /* CONFIG_MODULE_SIG */
>
> +#ifdef CONFIG_MODULES
> +static inline void lock_modules(void)
> +{
> + mutex_lock(&module_mutex);
> +}
> +
> +static inline void unlock_modules(void)
> +{
> + mutex_unlock(&module_mutex);
> +}
> +#else
> +static inline void lock_modules(void) { }
> +static inline void unlock_modules(void) { }
> +#endif
You don't need to add new #ifdefs. There is a room for dummy prototypes
for !CONFIG_MODULES already in modules.h.
-----
struct notifier_block;
#ifdef CONFIG_MODULES
extern int modules_disabled; /* for sysctl */
...
#else /* !CONFIG_MODULES... */
static inline struct module *__module_address(unsigned long addr)
{
-----
So you can just put those inlines in the appropriate places ;)
Thank you,
> +
> #endif /* _LINUX_MODULE_H */
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fa7687eb0c0e..b4f3c24cd2ef 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -573,7 +573,7 @@ static void kprobe_optimizer(struct work_struct *work)
> cpus_read_lock();
> mutex_lock(&text_mutex);
> /* Lock modules while optimizing kprobes */
> - mutex_lock(&module_mutex);
> + lock_modules();
>
> /*
> * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -598,7 +598,7 @@ static void kprobe_optimizer(struct work_struct *work)
> /* Step 4: Free cleaned kprobes after quiesence period */
> do_free_cleaned_kprobes();
>
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> mutex_unlock(&text_mutex);
> cpus_read_unlock();
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f76fdb925532..d9d9d4973e6b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -57,7 +57,7 @@ static void klp_find_object_module(struct klp_object *obj)
> if (!klp_is_module(obj))
> return;
>
> - mutex_lock(&module_mutex);
> + lock_modules();
> /*
> * We do not want to block removal of patched modules and therefore
> * we do not take a reference here. The patches are removed by
> @@ -74,7 +74,7 @@ static void klp_find_object_module(struct klp_object *obj)
> if (mod && mod->klp_alive)
> obj->mod = mod;
>
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> }
>
> static bool klp_initialized(void)
> @@ -163,12 +163,12 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> .pos = sympos,
> };
>
> - mutex_lock(&module_mutex);
> + lock_modules();
> if (objname)
> module_kallsyms_on_each_symbol(klp_find_callback, &args);
> else
> kallsyms_on_each_symbol(klp_find_callback, &args);
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> /*
> * Ensure an address was found. If sympos is 0, ensure symbol is unique;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8adeb126b02c..3fd8686b1637 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -896,7 +896,7 @@ static void module_unload_free(struct module *mod)
> {
> struct module_use *use, *tmp;
>
> - mutex_lock(&module_mutex);
> + lock_modules();
> list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
> struct module *i = use->target;
> pr_debug("%s unusing %s\n", mod->name, i->name);
> @@ -905,7 +905,7 @@ static void module_unload_free(struct module *mod)
> list_del(&use->target_list);
> kfree(use);
> }
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> }
>
> #ifdef CONFIG_MODULE_FORCE_UNLOAD
> @@ -1025,7 +1025,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> if (ret != 0)
> goto out;
>
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> /* Final destruction now no one is using it. */
> if (mod->exit != NULL)
> mod->exit();
> @@ -1044,7 +1044,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> wake_up_all(&module_wq);
> return 0;
> out:
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> return ret;
> }
>
> @@ -1449,7 +1449,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
> * in the wait_event_interruptible(), which is harmless.
> */
> sched_annotate_sleep();
> - mutex_lock(&module_mutex);
> + lock_modules();
> sym = find_symbol(name, &owner, &crc,
> !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
> if (!sym)
> @@ -1476,7 +1476,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
> /* We must make copy under the lock if we failed to get ref. */
> strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
> unlock:
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> return sym;
> }
>
> @@ -1731,10 +1731,10 @@ static void del_usage_links(struct module *mod)
> #ifdef CONFIG_MODULE_UNLOAD
> struct module_use *use;
>
> - mutex_lock(&module_mutex);
> + lock_modules();
> list_for_each_entry(use, &mod->target_list, target_list)
> sysfs_remove_link(use->target->holders_dir, mod->name);
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> #endif
> }
>
> @@ -1744,14 +1744,14 @@ static int add_usage_links(struct module *mod)
> #ifdef CONFIG_MODULE_UNLOAD
> struct module_use *use;
>
> - mutex_lock(&module_mutex);
> + lock_modules();
> list_for_each_entry(use, &mod->target_list, target_list) {
> ret = sysfs_create_link(use->target->holders_dir,
> &mod->mkobj.kobj, mod->name);
> if (ret)
> break;
> }
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> if (ret)
> del_usage_links(mod);
> #endif
> @@ -2177,9 +2177,9 @@ static void free_module(struct module *mod)
>
> /* We leave it in list to prevent duplicate loads, but make sure
> * that noone uses it while it's being deconstructed. */
> - mutex_lock(&module_mutex);
> + lock_modules();
> mod->state = MODULE_STATE_UNFORMED;
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> /* Remove dynamic debug info */
> ddebug_remove_module(mod->name);
> @@ -2197,7 +2197,7 @@ static void free_module(struct module *mod)
> free_module_elf(mod);
>
> /* Now we can delete it from the lists */
> - mutex_lock(&module_mutex);
> + lock_modules();
> /* Unlink carefully: kallsyms could be walking list. */
> list_del_rcu(&mod->list);
> mod_tree_remove(mod);
> @@ -2205,7 +2205,7 @@ static void free_module(struct module *mod)
> module_bug_cleanup(mod);
> /* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
> synchronize_rcu();
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> /* This may be empty, but that's OK */
> module_arch_freeing_init(mod);
> @@ -3504,10 +3504,10 @@ static bool finished_loading(const char *name)
> * in the wait_event_interruptible(), which is harmless.
> */
> sched_annotate_sleep();
> - mutex_lock(&module_mutex);
> + lock_modules();
> mod = find_module_all(name, strlen(name), true);
> ret = !mod || mod->state == MODULE_STATE_LIVE;
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> return ret;
> }
> @@ -3619,7 +3619,7 @@ static noinline int do_init_module(struct module *mod)
>
> ftrace_free_mem(mod, mod->init_layout.base, mod->init_layout.base +
> mod->init_layout.size);
> - mutex_lock(&module_mutex);
> + lock_modules();
> /* Drop initial reference. */
> module_put(mod);
> trim_init_extable(mod);
> @@ -3651,7 +3651,7 @@ static noinline int do_init_module(struct module *mod)
> if (llist_add(&freeinit->node, &init_free_list))
> schedule_work(&init_free_wq);
>
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> wake_up_all(&module_wq);
>
> return 0;
> @@ -3693,12 +3693,12 @@ static int add_unformed_module(struct module *mod)
> mod->state = MODULE_STATE_UNFORMED;
>
> again:
> - mutex_lock(&module_mutex);
> + lock_modules();
> old = find_module_all(mod->name, strlen(mod->name), true);
> if (old != NULL) {
> if (old->state != MODULE_STATE_LIVE) {
> /* Wait in case it fails to load. */
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> err = wait_event_interruptible(module_wq,
> finished_loading(mod->name));
> if (err)
> @@ -3714,7 +3714,7 @@ static int add_unformed_module(struct module *mod)
> err = 0;
>
> out:
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> out_unlocked:
> return err;
> }
> @@ -3723,7 +3723,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
> {
> int err;
>
> - mutex_lock(&module_mutex);
> + lock_modules();
>
> /* Find duplicate symbols (must be called under lock). */
> err = verify_exported_symbols(mod);
> @@ -3740,12 +3740,12 @@ static int complete_formation(struct module *mod, struct load_info *info)
> /* Mark state as coming so strong_try_module_get() ignores us,
> * but kallsyms etc. can see us. */
> mod->state = MODULE_STATE_COMING;
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> return 0;
>
> out:
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> return err;
> }
>
> @@ -3943,9 +3943,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
> klp_module_going(mod);
> bug_cleanup:
> /* module_bug_cleanup needs module_mutex protection */
> - mutex_lock(&module_mutex);
> + lock_modules();
> module_bug_cleanup(mod);
> - mutex_unlock(&module_mutex);
> + unlock_modules();
>
> ddebug_cleanup:
> ftrace_release_mod(mod);
> @@ -3959,14 +3959,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
> free_unload:
> module_unload_free(mod);
> unlink_mod:
> - mutex_lock(&module_mutex);
> + lock_modules();
> /* Unlink carefully: kallsyms could be walking list. */
> list_del_rcu(&mod->list);
> mod_tree_remove(mod);
> wake_up_all(&module_wq);
> /* Wait for RCU-sched synchronizing before releasing mod->list. */
> synchronize_rcu();
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> free_module:
> /* Free lock-classes; relies on the preceding sync_rcu() */
> lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
> @@ -4322,7 +4322,7 @@ static char *module_flags(struct module *mod, char *buf)
> /* Called by the /proc file system to return a list of modules. */
> static void *m_start(struct seq_file *m, loff_t *pos)
> {
> - mutex_lock(&module_mutex);
> + lock_modules();
> return seq_list_start(&modules, *pos);
> }
>
> @@ -4333,7 +4333,7 @@ static void *m_next(struct seq_file *m, void *p, loff_t *pos)
>
> static void m_stop(struct seq_file *m, void *p)
> {
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> }
>
> static int m_show(struct seq_file *m, void *p)
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index aefb6065b508..710ec6a6aa8f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> if (!p)
> return true;
> *p = '\0';
> - mutex_lock(&module_mutex);
> + lock_modules();
> ret = !!find_module(tk->symbol);
> - mutex_unlock(&module_mutex);
> + unlock_modules();
> *p = ':';
>
> return ret;
> --
> 2.25.1
>
--
Masami Hiramatsu <[email protected]>
On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> Introduce new API for allocating space for code generaed at run-time
> leveraging from module_alloc() and module_memfree() code. Use this to
> perform memory allocations in the kprobes code in order to loose the
> bound between kprobes and modules subsystem.
>
> Initially, use this API only with arch/x86 and define a new config
> flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> new API.
> [...]
> diff --git a/include/linux/text.h b/include/linux/text.h
> new file mode 100644
> index 000000000000..a27d4a42ecda
> --- /dev/null
> +++ b/include/linux/text.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _LINUX_TEXT_H
> +#define _LINUX_TEXT_H
> +
> +/*
> + * An allocator used for allocating modules, core sections and init sections.
> + * Returns NULL on failure.
> + */
> +void *text_alloc(unsigned long size);
> +
> +/*
> + * Free memory returned from text_alloc().
> + */
> +void text_free(void *region);
> +
> +#endif /* _LINUX_TEXT_H */
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2e97febeef77..fa7687eb0c0e 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -35,6 +35,7 @@
> #include <linux/ftrace.h>
> #include <linux/cpu.h>
> #include <linux/jump_label.h>
> +#include <linux/text.h>
>
> #include <asm/sections.h>
> #include <asm/cacheflush.h>
> @@ -111,12 +112,20 @@ enum kprobe_slot_state {
>
> void __weak *alloc_insn_page(void)
> {
> +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> + return text_alloc(PAGE_SIZE);
> +#else
> return module_alloc(PAGE_SIZE);
> +#endif
This seems like it shouldn't be needed. I think text_alloc() should
always be visible. In the ARCH_HAS... case it will call the arch
implementation, and without it should just call module_alloc():
For example:
void *text_alloc(unsigned long size)
{
#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
return arch_text_alloc(size);
#else
return module_alloc(size);
#endif
}
--
Kees Cook
On Wed, Jul 15, 2020 at 05:27:32PM +0900, Masami Hiramatsu wrote:
>
> On Wed, 15 Jul 2020 01:32:27 +0300
> Jarkko Sakkinen <[email protected]> wrote:
> > [...]
> > -void *module_alloc(unsigned long size)
> > -{
> > - void *p;
> > -
> > - if (PAGE_ALIGN(size) > MODULES_LEN)
> > - return NULL;
> > -
> > - p = __vmalloc_node_range(size, MODULE_ALIGN,
> > - MODULES_VADDR + get_module_load_offset(),
> > - MODULES_END, GFP_KERNEL,
> > - PAGE_KERNEL, 0, NUMA_NO_NODE,
> > - __builtin_return_address(0));
> > - if (p && (kasan_module_alloc(p, size) < 0)) {
> > - vfree(p);
> > - return NULL;
> > - }
> > -
> > - return p;
> > -}
>
> Please don't touch this module_alloc() at all. Then we can
> just call __vmalloc_node_range() in the text_alloc().
Hm? I thought the requirement was that trampolines needed to stay within
a certain distance of kernel text and that the module_alloc() enforced
that?
--
Kees Cook
On Wed, Jul 15, 2020 at 10:49:33PM +0300, Mike Rapoport wrote:
> On Wed, Jul 15, 2020 at 12:36:02PM -0700, Kees Cook wrote:
> > On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> > > Introduce new API for allocating space for code generaed at run-time
> > > leveraging from module_alloc() and module_memfree() code. Use this to
> > > perform memory allocations in the kprobes code in order to loose the
> > > bound between kprobes and modules subsystem.
> > >
> > > Initially, use this API only with arch/x86 and define a new config
> > > flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> > > new API.
> > > [...]
> > > diff --git a/include/linux/text.h b/include/linux/text.h
> > > new file mode 100644
> > > index 000000000000..a27d4a42ecda
> > > --- /dev/null
> > > +++ b/include/linux/text.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +
> > > +#ifndef _LINUX_TEXT_H
> > > +#define _LINUX_TEXT_H
> > > +
> > > +/*
> > > + * An allocator used for allocating modules, core sections and init sections.
> > > + * Returns NULL on failure.
> > > + */
> > > +void *text_alloc(unsigned long size);
> > > +
> > > +/*
> > > + * Free memory returned from text_alloc().
> > > + */
> > > +void text_free(void *region);
> > > +
> > > +#endif /* _LINUX_TEXT_H */
> > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > index 2e97febeef77..fa7687eb0c0e 100644
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -35,6 +35,7 @@
> > > #include <linux/ftrace.h>
> > > #include <linux/cpu.h>
> > > #include <linux/jump_label.h>
> > > +#include <linux/text.h>
> > >
> > > #include <asm/sections.h>
> > > #include <asm/cacheflush.h>
> > > @@ -111,12 +112,20 @@ enum kprobe_slot_state {
> > >
> > > void __weak *alloc_insn_page(void)
> > > {
> > > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > > + return text_alloc(PAGE_SIZE);
> > > +#else
> > > return module_alloc(PAGE_SIZE);
> > > +#endif
> >
> > This seems like it shouldn't be needed. I think text_alloc() should
> > always be visible. In the ARCH_HAS... case it will call the arch
> > implementation, and without it should just call module_alloc():
> >
> > For example:
> > void *text_alloc(unsigned long size)
> > {
> > #ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > return arch_text_alloc(size);
> > #else
> > return module_alloc(size);
> > #endif
> > }
>
> This inverts the dependcy chain, IMHO, module_alloc() is a special case
> of text_alloc() and not vice versa.
Okay, sure. That's fine too. Whatever the case is, I want to make sure
we keep the KASLR offset though. I don't think that should be unique to
the modules logic.
--
Kees Cook
On Wed, Jul 15, 2020 at 12:36:02PM -0700, Kees Cook wrote:
> On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> > Introduce new API for allocating space for code generaed at run-time
> > leveraging from module_alloc() and module_memfree() code. Use this to
> > perform memory allocations in the kprobes code in order to loose the
> > bound between kprobes and modules subsystem.
> >
> > Initially, use this API only with arch/x86 and define a new config
> > flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> > new API.
> > [...]
> > diff --git a/include/linux/text.h b/include/linux/text.h
> > new file mode 100644
> > index 000000000000..a27d4a42ecda
> > --- /dev/null
> > +++ b/include/linux/text.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _LINUX_TEXT_H
> > +#define _LINUX_TEXT_H
> > +
> > +/*
> > + * An allocator used for allocating modules, core sections and init sections.
> > + * Returns NULL on failure.
> > + */
> > +void *text_alloc(unsigned long size);
> > +
> > +/*
> > + * Free memory returned from text_alloc().
> > + */
> > +void text_free(void *region);
> > +
> > +#endif /* _LINUX_TEXT_H */
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 2e97febeef77..fa7687eb0c0e 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -35,6 +35,7 @@
> > #include <linux/ftrace.h>
> > #include <linux/cpu.h>
> > #include <linux/jump_label.h>
> > +#include <linux/text.h>
> >
> > #include <asm/sections.h>
> > #include <asm/cacheflush.h>
> > @@ -111,12 +112,20 @@ enum kprobe_slot_state {
> >
> > void __weak *alloc_insn_page(void)
> > {
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + return text_alloc(PAGE_SIZE);
> > +#else
> > return module_alloc(PAGE_SIZE);
> > +#endif
>
> This seems like it shouldn't be needed. I think text_alloc() should
> always be visible. In the ARCH_HAS... case it will call the arch
> implementation, and without it should just call module_alloc():
>
> For example:
> void *text_alloc(unsigned long size)
> {
> #ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> return arch_text_alloc(size);
> #else
> return module_alloc(size);
> #endif
> }
This inverts the dependcy chain, IMHO, module_alloc() is a special case
of text_alloc() and not vice versa.
The addition of #ifdefs to alloc_insn_page() is not needed because it is
anyway overriden on x86, so I don't see a point in added #ifdefs until
another arch will enable ARCH_HAS_TEXT_ALLOC.
> --
> Kees Cook
--
Sincerely yours,
Mike.
On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> +void *text_alloc(unsigned long size)
> +{
> + void *p;
> +
> + if (PAGE_ALIGN(size) > MODULES_LEN)
> + return NULL;
> +
> + p = __vmalloc_node_range(size, MODULE_ALIGN,
> + MODULES_VADDR + get_module_load_offset(),
> + MODULES_END, GFP_KERNEL,
> + PAGE_KERNEL, 0, NUMA_NO_NODE,
> + __builtin_return_address(0));
> + if (p && (kasan_module_alloc(p, size) < 0)) {
> + vfree(p);
> + return NULL;
> + }
> +
> + return p;
> +}
> +
> +void text_free(void *region)
> +{
> + /*
> + * This memory may be RO, and freeing RO memory in an interrupt is not
> + * supported by vmalloc.
> + */
> + WARN_ON(in_interrupt());
I think that wants to be:
lockdep_assert_irqs_enabled();
in_interrupt() isn't sufficient, interrupts must also not be disabled
when issuesing TLB invalidations.
> +
> + vfree(region);
> +}
On Wed, Jul 15, 2020 at 05:27:32PM +0900, Masami Hiramatsu wrote:
> Hi Jarkko,
>
> On Wed, 15 Jul 2020 01:32:27 +0300
> Jarkko Sakkinen <[email protected]> wrote:
>
> > Introduce new API for allocating space for code generaed at run-time
> > leveraging from module_alloc() and module_memfree() code. Use this to
> > perform memory allocations in the kprobes code in order to loose the
> > bound between kprobes and modules subsystem.
> >
> > Initially, use this API only with arch/x86 and define a new config
> > flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> > new API.
>
> This might need to be extended for the text_alloc() flags as we discuss
> in previous thread. (e.g. bpf on arm64)
>
> > Cc: Andi Kleen <[email protected]>
> > Suggested-by: Peter Zijlstra <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > arch/Kconfig | 2 +-
> > arch/x86/Kconfig | 3 ++
> > arch/x86/kernel/Makefile | 1 +
> > arch/x86/kernel/module.c | 49 ---------------------------
> > arch/x86/kernel/text.c | 71 ++++++++++++++++++++++++++++++++++++++++
>
> I think this would better be arch/x86/mm/text_alloc.c (text.c is too
> generic, and this only provides memory allocation.)
Agreed.
>
> > include/linux/text.h | 17 ++++++++++
>
> "text_alloc.h" would be better, or puting the prototype in vmalloc.h
> will be easier to use.
>
> > kernel/kprobes.c | 11 +++++++
> > kernel/module.c | 10 ++++++
> > 8 files changed, 114 insertions(+), 50 deletions(-)
> > create mode 100644 arch/x86/kernel/text.c
> > create mode 100644 include/linux/text.h
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 8cc35dc556c7..e3d6b6868ccb 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -61,7 +61,7 @@ config OPROFILE_NMI_TIMER
> >
> > config KPROBES
> > bool "Kprobes"
> > - depends on MODULES
> > + depends on MODULES || ARCH_HAS_TEXT_ALLOC
> > depends on HAVE_KPROBES
> > select KALLSYMS
> > help
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 0dea7fdd7a00..a4ee5d1300f6 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2035,6 +2035,9 @@ config KEXEC_FILE
> > config ARCH_HAS_KEXEC_PURGATORY
> > def_bool KEXEC_FILE
> >
> > +config ARCH_HAS_TEXT_ALLOC
> > + def_bool y
> > +
> > config KEXEC_SIG
> > bool "Verify kernel signature during kexec_file_load() syscall"
> > depends on KEXEC_FILE
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index e77261db2391..2878e4b753a0 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -68,6 +68,7 @@ obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
> > obj-y += pci-iommu_table.o
> > obj-y += resource.o
> > obj-y += irqflags.o
> > +obj-y += text.o
> >
> > obj-y += process.o
> > obj-y += fpu/
> > diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> > index 34b153cbd4ac..261df078f127 100644
> > --- a/arch/x86/kernel/module.c
> > +++ b/arch/x86/kernel/module.c
> > @@ -36,55 +36,6 @@ do { \
> > } while (0)
> > #endif
> >
> > -#ifdef CONFIG_RANDOMIZE_BASE
> > -static unsigned long module_load_offset;
> > -
> > -/* Mutex protects the module_load_offset. */
> > -static DEFINE_MUTEX(module_kaslr_mutex);
> > -
> > -static unsigned long int get_module_load_offset(void)
> > -{
> > - if (kaslr_enabled()) {
> > - mutex_lock(&module_kaslr_mutex);
> > - /*
> > - * Calculate the module_load_offset the first time this
> > - * code is called. Once calculated it stays the same until
> > - * reboot.
> > - */
> > - if (module_load_offset == 0)
> > - module_load_offset =
> > - (get_random_int() % 1024 + 1) * PAGE_SIZE;
> > - mutex_unlock(&module_kaslr_mutex);
> > - }
> > - return module_load_offset;
> > -}
> > -#else
> > -static unsigned long int get_module_load_offset(void)
> > -{
> > - return 0;
> > -}
> > -#endif
> > -
> > -void *module_alloc(unsigned long size)
> > -{
> > - void *p;
> > -
> > - if (PAGE_ALIGN(size) > MODULES_LEN)
> > - return NULL;
> > -
> > - p = __vmalloc_node_range(size, MODULE_ALIGN,
> > - MODULES_VADDR + get_module_load_offset(),
> > - MODULES_END, GFP_KERNEL,
> > - PAGE_KERNEL, 0, NUMA_NO_NODE,
> > - __builtin_return_address(0));
> > - if (p && (kasan_module_alloc(p, size) < 0)) {
> > - vfree(p);
> > - return NULL;
> > - }
> > -
> > - return p;
> > -}
>
> Please don't touch this module_alloc() at all. Then we can
> just call __vmalloc_node_range() in the text_alloc().
This is my mistake, not intentional. Had a fixup that by mistake did not
end up to the final patch set. Sorry about that.
>
> > -
> > #ifdef CONFIG_X86_32
> > int apply_relocate(Elf32_Shdr *sechdrs,
> > const char *strtab,
> > diff --git a/arch/x86/kernel/text.c b/arch/x86/kernel/text.c
> > new file mode 100644
> > index 000000000000..986b92ff1434
> > --- /dev/null
> > +++ b/arch/x86/kernel/text.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Kernel module help for x86.
> > + * Copyright (C) 2001 Rusty Russell.
> > + */
> > +
> > +#include <linux/kasan.h>
> > +#include <linux/mm.h>
> > +#include <linux/moduleloader.h>
> > +#include <linux/vmalloc.h>
> > +#include <asm/setup.h>
> > +
> > +#ifdef CONFIG_RANDOMIZE_BASE
> > +static unsigned long module_load_offset;
> > +
> > +/* Mutex protects the module_load_offset. */
> > +static DEFINE_MUTEX(module_kaslr_mutex);
> > +
> > +static unsigned long get_module_load_offset(void)
> > +{
> > + if (kaslr_enabled()) {
> > + mutex_lock(&module_kaslr_mutex);
> > + /*
> > + * Calculate the module_load_offset the first time this
> > + * code is called. Once calculated it stays the same until
> > + * reboot.
> > + */
> > + if (module_load_offset == 0)
> > + module_load_offset =
> > + (get_random_int() % 1024 + 1) * PAGE_SIZE;
> > + mutex_unlock(&module_kaslr_mutex);
> > + }
> > + return module_load_offset;
> > +}
> > +#else
> > +static unsigned long get_module_load_offset(void)
> > +{
> > + return 0;
> > +}
> > +#endif
>
> So as I pointed, we can ignore this for kprobes (and other
> dynamic allocated trampoline code).
>
> > +
> > +void *text_alloc(unsigned long size)
> > +{
> > + void *p;
> > +
> > + if (PAGE_ALIGN(size) > MODULES_LEN)
> > + return NULL;
> > +
> > + p = __vmalloc_node_range(size, MODULE_ALIGN,
> > + MODULES_VADDR + get_module_load_offset(),
> > + MODULES_END, GFP_KERNEL,
> > + PAGE_KERNEL, 0, NUMA_NO_NODE,
> > + __builtin_return_address(0));
> > + if (p && (kasan_module_alloc(p, size) < 0)) {
> > + vfree(p);
> > + return NULL;
> > + }
> > +
> > + return p;
> > +}
> > +
> > +void text_free(void *region)
> > +{
> > + /*
> > + * This memory may be RO, and freeing RO memory in an interrupt is not
> > + * supported by vmalloc.
> > + */
> > + WARN_ON(in_interrupt());
> > +
> > + vfree(region);
> > +}
> > diff --git a/include/linux/text.h b/include/linux/text.h
> > new file mode 100644
> > index 000000000000..a27d4a42ecda
> > --- /dev/null
> > +++ b/include/linux/text.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _LINUX_TEXT_H
> > +#define _LINUX_TEXT_H
> > +
> > +/*
> > + * An allocator used for allocating modules, core sections and init sections.
> > + * Returns NULL on failure.
> > + */
> > +void *text_alloc(unsigned long size);
> > +
> > +/*
> > + * Free memory returned from text_alloc().
> > + */
> > +void text_free(void *region);
>
> Hmm, if this is this short, in this version we might better move
> these to vmalloc.h.
>
> > +
> > +#endif /* _LINUX_TEXT_H */
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 2e97febeef77..fa7687eb0c0e 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -35,6 +35,7 @@
> > #include <linux/ftrace.h>
> > #include <linux/cpu.h>
> > #include <linux/jump_label.h>
> > +#include <linux/text.h>
> >
> > #include <asm/sections.h>
> > #include <asm/cacheflush.h>
> > @@ -111,12 +112,20 @@ enum kprobe_slot_state {
> >
> > void __weak *alloc_insn_page(void)
> > {
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + return text_alloc(PAGE_SIZE);
> > +#else
> > return module_alloc(PAGE_SIZE);
> > +#endif
> > }
> >
> > void __weak free_insn_page(void *page)
> > {
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + text_free(page);
> > +#else
> > module_memfree(page);
> > +#endif
> > }
> >
> > struct kprobe_insn_cache kprobe_insn_slots = {
> > @@ -1608,6 +1617,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.
> > @@ -1618,6 +1628,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > *probed_mod = NULL;
> > ret = -ENOENT;
> > }
> > +#endif
>
> This change is not related to text_alloc(). Please move this to 3/3.
>
> > }
> > out:
> > preempt_enable();
> > diff --git a/kernel/module.c b/kernel/module.c
> > index aa183c9ac0a2..8adeb126b02c 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -56,6 +56,7 @@
> > #include <linux/bsearch.h>
> > #include <linux/dynamic_debug.h>
> > #include <linux/audit.h>
> > +#include <linux/text.h>
> > #include <uapi/linux/module.h>
> > #include "module-internal.h"
> >
> > @@ -2151,7 +2152,12 @@ void __weak module_memfree(void *module_region)
> > * supported by vmalloc.
> > */
> > WARN_ON(in_interrupt());
> > +
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + text_free(module_region);
> > +#else
> > vfree(module_region);
> > +#endif
> > }
> >
> > void __weak module_arch_cleanup(struct module *mod)
> > @@ -2786,9 +2792,13 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
> >
> > void * __weak module_alloc(unsigned long size)
> > {
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + return text_alloc(size);
> > +#else
> > return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> > GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
> > NUMA_NO_NODE, __builtin_return_address(0));
> > +#endif
> > }
>
> Please don't touch kernel/module.c too. This seems to make things complicated.
>
> Thank you,
>
> --
> Masami Hiramatsu <[email protected]>
Thanks for the review. Agree with your remarks.
/Jarkko
On Wed, Jul 15, 2020 at 05:35:24PM +0900, Masami Hiramatsu wrote:
> Hi Jarkko,
>
> On Wed, 15 Jul 2020 01:32:29 +0300
> Jarkko Sakkinen <[email protected]> wrote:
>
> > Remove CONFIG_MODULES dependency by flagging out the dependent code. This
> > allows to use kprobes in a kernel without support for loadable modules,
> > which could be useful for a test kernel or perhaps an embedded kernel.
> >
>
> OK, looks good, I just have 2 comments below.
>
> > Cc: Andi Kleen <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > include/linux/module.h | 14 +++++++-------
> > kernel/kprobes.c | 7 +++++++
> > kernel/trace/trace_kprobe.c | 16 +++++++++++++++-
> > 3 files changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 857b84bf9e90..eaa8ad02f75c 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -290,6 +290,13 @@ extern typeof(name) __mod_##type##__##name##_device_table \
> >
> > struct notifier_block;
> >
> > +enum module_state {
> > + MODULE_STATE_LIVE, /* Normal state. */
> > + MODULE_STATE_COMING, /* Full formed, running module_init. */
> > + MODULE_STATE_GOING, /* Going away. */
> > + MODULE_STATE_UNFORMED, /* Still setting it up. */
> > +};
> > +
> > #ifdef CONFIG_MODULES
> >
> > extern int modules_disabled; /* for sysctl */
> > @@ -305,13 +312,6 @@ struct module_use {
> > struct module *source, *target;
> > };
> >
> > -enum module_state {
> > - MODULE_STATE_LIVE, /* Normal state. */
> > - MODULE_STATE_COMING, /* Full formed, running module_init. */
> > - MODULE_STATE_GOING, /* Going away. */
> > - MODULE_STATE_UNFORMED, /* Still setting it up. */
> > -};
> > -
> > struct mod_tree_node {
> > struct module *mod;
> > struct latch_tree_node node;
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index b4f3c24cd2ef..3039df13d34e 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2225,6 +2225,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)
> > {
> > @@ -2242,6 +2243,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > {
> > kprobe_remove_area_blacklist(entry, entry + 1);
> > }
> > +#endif
> >
> > int __init __weak arch_populate_kprobe_blacklist(void)
> > {
> > @@ -2285,6 +2287,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;
> > @@ -2330,6 +2333,10 @@ static void remove_module_kprobe_blacklist(struct module *mod)
> > kprobe_remove_area_blacklist(start, end);
> > }
> > }
> > +#else
> > +static void add_module_kprobe_blacklist(struct module *mod) {}
> > +static void remove_module_kprobe_blacklist(struct module *mod) {}
> > +#endif /* CONFIG_MODULES */
>
> Please feel free to move the function. I would like to see;
>
> #ifdef CONFIG_MODULES
> /* Remove all symbols in given area from kprobe blacklist */
> static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> {
> ...
> }
> static void add_module_kprobe_blacklist(struct module *mod)
> {
> ...
> }
> #else
> static void add_module_kprobe_blacklist(struct module *mod) {}
> static void remove_module_kprobe_blacklist(struct module *mod) {}
> #endif /* CONFIG_MODULES */
>
> Rather than split #ifdefs.
>
> >
> > /* Module notifier call back, checking kprobes on the module */
> > static int kprobes_module_callback(struct notifier_block *nb,
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 710ec6a6aa8f..7fcd1bf2d96e 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -103,8 +103,9 @@ static nokprobe_inline bool trace_kprobe_has_gone(struct trace_kprobe *tk)
> > return !!(kprobe_gone(&tk->rp.kp));
> > }
> >
> > +#ifdef CONFIG_MODULES
> > static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > - struct module *mod)
> > + struct module *mod)
> > {
> > int len = strlen(mod->name);
> > const char *name = trace_kprobe_symbol(tk);
> > @@ -129,6 +130,17 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >
> > return ret;
> > }
> > +#else
> > +static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > + struct module *mod)
> > +{
> > + return false;
> > +}
> > +static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > +{
> > + return false;
> > +}
> > +#endif
> >
> > static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > {
> > @@ -685,10 +697,12 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > /* Don't need to check busy - this should have gone. */
> > __unregister_trace_kprobe(tk);
> > ret = __register_trace_kprobe(tk);
> > +#ifdef CONFIG_MODULES
> > if (ret)
> > pr_warn("Failed to re-register probe %s on %s: %d\n",
> > trace_probe_name(&tk->tp),
> > mod->name, ret);
> > +#endif
>
> I guess this CONFIG_MODULES is for avoiding build error according to mod->name,
> if so, please use module_name(mod) macro instead of this #ifdef.
>
> > }
> > }
> > mutex_unlock(&event_mutex);
> > --
> > 2.25.1
> >
>
>
> --
> Masami Hiramatsu <[email protected]>
Thanks, agree with the remarks.
/Jarkko
On Wed, Jul 15, 2020 at 05:39:39PM +0900, Masami Hiramatsu wrote:
> On Wed, 15 Jul 2020 01:32:28 +0300
> Jarkko Sakkinen <[email protected]> wrote:
>
> > Add wrappers to take the modules "big lock" in order to encapsulate
> > conditional compilation (CONFIG_MODULES) inside the wrapper.
> >
> > Cc: Andi Kleen <[email protected]>
> > Suggested-by: Masami Hiramatsu <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > include/linux/module.h | 15 ++++++++++
> > kernel/kprobes.c | 4 +--
> > kernel/livepatch/core.c | 8 ++---
> > kernel/module.c | 60 ++++++++++++++++++-------------------
> > kernel/trace/trace_kprobe.c | 4 +--
> > 5 files changed, 53 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 2e6670860d27..857b84bf9e90 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -902,4 +902,19 @@ static inline bool module_sig_ok(struct module *module)
> > }
> > #endif /* CONFIG_MODULE_SIG */
> >
> > +#ifdef CONFIG_MODULES
> > +static inline void lock_modules(void)
> > +{
> > + mutex_lock(&module_mutex);
> > +}
> > +
> > +static inline void unlock_modules(void)
> > +{
> > + mutex_unlock(&module_mutex);
> > +}
> > +#else
> > +static inline void lock_modules(void) { }
> > +static inline void unlock_modules(void) { }
> > +#endif
>
> You don't need to add new #ifdefs. There is a room for dummy prototypes
> for !CONFIG_MODULES already in modules.h.
>
> -----
> struct notifier_block;
>
> #ifdef CONFIG_MODULES
>
> extern int modules_disabled; /* for sysctl */
>
> ...
> #else /* !CONFIG_MODULES... */
>
> static inline struct module *__module_address(unsigned long addr)
> {
> -----
>
> So you can just put those inlines in the appropriate places ;)
>
> Thank you,
Rrright.
/Jarkko
On Wed, Jul 15, 2020 at 12:36:02PM -0700, Kees Cook wrote:
> On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> > Introduce new API for allocating space for code generaed at run-time
> > leveraging from module_alloc() and module_memfree() code. Use this to
> > perform memory allocations in the kprobes code in order to loose the
> > bound between kprobes and modules subsystem.
> >
> > Initially, use this API only with arch/x86 and define a new config
> > flag CONFIG_ARCH_HAS_TEXT_ALLOC to promote the availability of the
> > new API.
> > [...]
> > diff --git a/include/linux/text.h b/include/linux/text.h
> > new file mode 100644
> > index 000000000000..a27d4a42ecda
> > --- /dev/null
> > +++ b/include/linux/text.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _LINUX_TEXT_H
> > +#define _LINUX_TEXT_H
> > +
> > +/*
> > + * An allocator used for allocating modules, core sections and init sections.
> > + * Returns NULL on failure.
> > + */
> > +void *text_alloc(unsigned long size);
> > +
> > +/*
> > + * Free memory returned from text_alloc().
> > + */
> > +void text_free(void *region);
> > +
> > +#endif /* _LINUX_TEXT_H */
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 2e97febeef77..fa7687eb0c0e 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -35,6 +35,7 @@
> > #include <linux/ftrace.h>
> > #include <linux/cpu.h>
> > #include <linux/jump_label.h>
> > +#include <linux/text.h>
> >
> > #include <asm/sections.h>
> > #include <asm/cacheflush.h>
> > @@ -111,12 +112,20 @@ enum kprobe_slot_state {
> >
> > void __weak *alloc_insn_page(void)
> > {
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > + return text_alloc(PAGE_SIZE);
> > +#else
> > return module_alloc(PAGE_SIZE);
> > +#endif
>
> This seems like it shouldn't be needed. I think text_alloc() should
> always be visible. In the ARCH_HAS... case it will call the arch
> implementation, and without it should just call module_alloc():
>
> For example:
> void *text_alloc(unsigned long size)
> {
> #ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> return arch_text_alloc(size);
> #else
> return module_alloc(size);
> #endif
> }
>
> --
> Kees Cook
I fully agree with your comments and I posted a follow-up series where I
think these should be resolved. Please check it because before I got
this review, It was already posted.
Thank you.
/Jarkko
On Thu, Jul 16, 2020 at 11:02:53AM +0200, Peter Zijlstra wrote:
> On Wed, Jul 15, 2020 at 01:32:27AM +0300, Jarkko Sakkinen wrote:
> > +void *text_alloc(unsigned long size)
> > +{
> > + void *p;
> > +
> > + if (PAGE_ALIGN(size) > MODULES_LEN)
> > + return NULL;
> > +
> > + p = __vmalloc_node_range(size, MODULE_ALIGN,
> > + MODULES_VADDR + get_module_load_offset(),
> > + MODULES_END, GFP_KERNEL,
> > + PAGE_KERNEL, 0, NUMA_NO_NODE,
> > + __builtin_return_address(0));
> > + if (p && (kasan_module_alloc(p, size) < 0)) {
> > + vfree(p);
> > + return NULL;
> > + }
> > +
> > + return p;
> > +}
> > +
> > +void text_free(void *region)
> > +{
> > + /*
> > + * This memory may be RO, and freeing RO memory in an interrupt is not
> > + * supported by vmalloc.
> > + */
> > + WARN_ON(in_interrupt());
>
> I think that wants to be:
>
> lockdep_assert_irqs_enabled();
>
> in_interrupt() isn't sufficient, interrupts must also not be disabled
> when issuesing TLB invalidations.
Shouldn't it be then also fixed in the module_memfree() fallback
implementation (kernel/module.c)?
/Jarkko