Date:   Fri, 10 Jul 2020 19:32:57 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)),
        "H. Peter Anvin" <hpa@...or.com>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Will Deacon <will@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>, Alexandre Ghiti <alex@...ti.fr>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Peter Collingbourne <pcc@...gle.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH RFC] kprobes: Remove MODULES dependency

Hi Jarkko,

On Fri, 10 Jul 2020 02:45:19 +0300
Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com> wrote:

> Remove MODULES dependency and migrate from module_alloc to vmalloc().
> According to Andi, the history with this dependency is that kprobes
> originally required custom LKM's, which does not hold today anymore.
> 
> 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.

Agreed. Now we have several way to access kprobes via ftrace/perf/bpf
without modules. So it's a good time to remove CONFIG_MODULE dependency.

But I also would like to minimize the changes for code readability.
I made some comments below.

> 
> Cc: Andi Kleen <ak@...ux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> ---
>  arch/Kconfig                   |  1 -
>  arch/x86/kernel/kprobes/core.c |  5 +++--
>  kernel/kprobes.c               | 22 ++++++++++++++++++++--
>  kernel/trace/trace_kprobe.c    | 12 ++++++++++++
>  4 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8cc35dc556c7..bb59cdf335ab 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -61,7 +61,6 @@ config OPROFILE_NMI_TIMER
>  
>  config KPROBES
>  	bool "Kprobes"
> -	depends on MODULES
>  	depends on HAVE_KPROBES
>  	select KALLSYMS
>  	help
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index ada39ddbc922..dc7b8d6fd20d 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -42,6 +42,7 @@
>  #include <linux/moduleloader.h>
>  #include <linux/vmalloc.h>
>  #include <linux/pgtable.h>
> +#include <linux/vmalloc.h>
>  
>  #include <asm/text-patching.h>
>  #include <asm/cacheflush.h>
> @@ -423,7 +424,7 @@ void *alloc_insn_page(void)
>  {
>  	void *page;
>  
> -	page = module_alloc(PAGE_SIZE);
> +	page = vmalloc(PAGE_SIZE);

No, you can not use vmalloc here. The reason why we use module_alloc()
is to allocate the executable memory for trampoline code.
So, you need to use vmalloc_exec() instead.

>  	if (!page)
>  		return NULL;
>  
> @@ -446,7 +447,7 @@ void *alloc_insn_page(void)
>  /* Recover page to RW mode before releasing it */
>  void free_insn_page(void *page)
>  {
> -	module_memfree(page);
> +	vfree(page);
>  }
>  
>  static int arch_copy_kprobe(struct kprobe *p)
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 4a904cc56d68..02680642ea11 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/vmalloc.h>
>  
>  #include <asm/sections.h>
>  #include <asm/cacheflush.h>
> @@ -111,12 +112,12 @@ enum kprobe_slot_state {
>  
>  void __weak *alloc_insn_page(void)
>  {
> -	return module_alloc(PAGE_SIZE);
> +	return vmalloc(PAGE_SIZE);

Ditto.

>  }
>  
>  void __weak free_insn_page(void *page)
>  {
> -	module_memfree(page);
> +	vfree(page);
>  }
>  
>  struct kprobe_insn_cache kprobe_insn_slots = {
> @@ -563,8 +564,11 @@ static void kprobe_optimizer(struct work_struct *work)
>  	mutex_lock(&kprobe_mutex);
>  	cpus_read_lock();
>  	mutex_lock(&text_mutex);
> +
> +#ifdef CONFIG_MODULES
>  	/* Lock modules while optimizing kprobes */
>  	mutex_lock(&module_mutex);
> +#endif

Hmm, can you reduce these "#ifdef CONFIG_MODULE"s ?

e.g. 

#ifdef CONFIG_MODULES
static void lock_modules(void)
{
	mutex_lock(&module_mutex);
}
...
#else
#define lock_modules() do { } while (0)
...
#endif

>  
>  	/*
>  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -589,7 +593,9 @@ static void kprobe_optimizer(struct work_struct *work)
>  	/* Step 4: Free cleaned kprobes after quiesence period */
>  	do_free_cleaned_kprobes();
>  
> +#ifdef CONFIG_MODULES
>  	mutex_unlock(&module_mutex);
> +#endif
>  	mutex_unlock(&text_mutex);
>  	cpus_read_unlock();
>  
> @@ -739,6 +745,7 @@ static int reuse_unused_kprobe(struct kprobe *ap)
>  	return 0;
>  }
>  

> +#ifdef CONFIG_MODULE
>  /* Remove optimized instructions */
>  static void kill_optimized_kprobe(struct kprobe *p)
>  {
> @@ -764,6 +771,7 @@ static void kill_optimized_kprobe(struct kprobe *p)
>  	/* Don't touch the code, because it is already freed. */
>  	arch_remove_optimized_kprobe(op);
>  }
> +#endif

This optprobe related one is OK to keep in this place. 

>  
>  static inline
>  void __prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
> @@ -1608,6 +1616,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			goto out;
>  		}
>  
> +#ifdef CONFIG_MODULE
>  		/*
>  		 * If the module freed .init.text, we couldn't insert
>  		 * kprobes in there.
> @@ -1618,6 +1627,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			*probed_mod = NULL;
>  			ret = -ENOENT;
>  		}
> +#endif
>  	}
>  out:
>  	preempt_enable();
> @@ -2090,6 +2100,7 @@ NOKPROBE_SYMBOL(pre_handler_kretprobe);
>  
>  #endif /* CONFIG_KRETPROBES */
>  
> +#ifdef CONFIG_MODULE
>  /* Set the kprobe gone and remove its instruction buffer. */
>  static void kill_kprobe(struct kprobe *p)
>  {
> @@ -2114,6 +2125,7 @@ static void kill_kprobe(struct kprobe *p)
>  	 */
>  	arch_remove_kprobe(p);
>  }
> +#endif
>  
>  /* Disable one kprobe */
>  int disable_kprobe(struct kprobe *kp)
> @@ -2214,6 +2226,7 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MODULE
>  /* Remove all symbols in given area from kprobe blacklist */
>  static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
>  {
> @@ -2231,6 +2244,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)
>  {
> @@ -2274,6 +2288,7 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
>  	return ret ? : arch_populate_kprobe_blacklist();
>  }
>  
> +#ifdef CONFIG_MODULE
>  static void add_module_kprobe_blacklist(struct module *mod)
>  {
>  	unsigned long start, end;
> @@ -2375,6 +2390,7 @@ static struct notifier_block kprobe_module_nb = {
>  	.notifier_call = kprobes_module_callback,
>  	.priority = 0
>  };
> +#endif /* CONFIG_MODULE */
>  
>  /* Markers of _kprobe_blacklist section */
>  extern unsigned long __start_kprobe_blacklist[];
> @@ -2425,8 +2441,10 @@ static int __init init_kprobes(void)
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
> +#ifdef CONFIG_MODULE
>  	if (!err)
>  		err = register_module_notifier(&kprobe_module_nb);
> +#endif
>  
>  	kprobes_initialized = (err == 0);
>  
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index aefb6065b508..30969b38fce8 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -103,6 +103,7 @@ static nokprobe_inline bool trace_kprobe_has_gone(struct trace_kprobe *tk)
>  	return !!(kprobe_gone(&tk->rp.kp));
>  }
>  
> +#ifdef CONFIG_MODULE
>  static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
>  						 struct module *mod)
>  {
> @@ -110,7 +111,9 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
>  	const char *name = trace_kprobe_symbol(tk);
>  	return strncmp(mod->name, name, len) == 0 && name[len] == ':';
>  }
> +#endif
>  
> +#ifdef CONFIG_MODULE
>  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  {
>  	char *p;
> @@ -129,6 +132,7 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  
>  	return ret;
>  }
> +#endif

Here, too. If you make a dummy function in case of !CONFIG_MODULE,
we don't need to modify caller-side code.

>  
>  static bool trace_kprobe_is_busy(struct dyn_event *ev)
>  {
> @@ -608,10 +612,12 @@ static int append_trace_kprobe(struct trace_kprobe *tk, struct trace_kprobe *to)
>  
>  	/* Register k*probe */
>  	ret = __register_trace_kprobe(tk);
> +#ifdef CONFIG_MODULE
>  	if (ret == -ENOENT && !trace_kprobe_module_exist(tk)) {
>  		pr_warn("This probe might be able to register after target module is loaded. Continue.\n");
>  		ret = 0;
>  	}
> +#endif

So, please make a dummy trace_kprobe_module_exist() and keep this untouched.

>  
>  	if (ret)
>  		trace_probe_unlink(&tk->tp);
> @@ -651,10 +657,12 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>  
>  	/* Register k*probe */
>  	ret = __register_trace_kprobe(tk);
> +#ifdef CONFIG_MODULE
>  	if (ret == -ENOENT && !trace_kprobe_module_exist(tk)) {
>  		pr_warn("This probe might be able to register after target module is loaded. Continue.\n");
>  		ret = 0;
>  	}
> +#endif

Ditto.

>  
>  	if (ret < 0)
>  		unregister_kprobe_event(tk);
> @@ -666,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_MODULE
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -700,6 +709,7 @@ static struct notifier_block trace_kprobe_module_nb = {
>  	.notifier_call = trace_kprobe_module_callback,
>  	.priority = 1	/* Invoked after kprobe module callback */
>  };
> +#endif

Unless it makes any build error, please keep it untouched.

>  
>  /* Convert certain expected symbols into '_' when generating event names */
>  static inline void sanitize_event_name(char *name)
> @@ -1891,8 +1901,10 @@ static __init int init_kprobe_trace_early(void)
>  	if (ret)
>  		return ret;
>  
> +#ifdef CONFIG_MODULE
>  	if (register_module_notifier(&trace_kprobe_module_nb))
>  		return -EINVAL;
> +#endif

Ditto.

Thank you,

>  
>  	return 0;
>  }
> -- 
> 2.25.1
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists