linux-kernel - Re: [PATCH 4/4] kprobes: Remove core dependency on modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <D03UMKC71414.2D6NN1BIWD5TZ@kernel.org>
Date: Tue, 26 Mar 2024 19:13:51 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Mark Rutland" <mark.rutland@....com>, <linux-kernel@...r.kernel.org>
Cc: <agordeev@...ux.ibm.com>, <anil.s.keshavamurthy@...el.com>,
 <aou@...s.berkeley.edu>, <bp@...en8.de>, <catalin.marinas@....com>,
 <dave.hansen@...ux.intel.com>, <davem@...emloft.net>, <gor@...ux.ibm.com>,
 <hca@...ux.ibm.com>, <jcalvinowens@...il.com>,
 <linux-arm-kernel@...ts.infradead.org>, <mhiramat@...nel.org>,
 <mingo@...hat.com>, <mpe@...erman.id.au>, <naveen.n.rao@...ux.ibm.com>,
 <palmer@...belt.com>, <paul.walmsley@...ive.com>, <tglx@...utronix.de>,
 <will@...nel.org>
Subject: Re: [PATCH 4/4] kprobes: Remove core dependency on modules

On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> From: Jarkko Sakkinen <jarkko@...nel.org>
>
> Tracing with kprobes while running a monolithic kernel is currently
> impossible because KPROBES depends on MODULES. While this dependency is
> necessary when KPROBES_USE_MODULE_ALLOC=y, all the other module-specific
> code only exist to handle the case when MODULES=y, and can be hidden
> behind ifdeffery.
>
> Add the necessary ifdeffery, and remove the dependency on MODULES=N when
> KPROBES_USE_MODULE_ALLOC=n.
>
> Currently this allows kprobes to be used when CONFIG_MODULES=n on arm64
> and riscv, and other architectures can enable support by implementing
> their own kprobes_alloc_insn_page() and kprobes_free_insn_page() which
> do not depend on MODULES.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> Link: https://lore.kernel.org/all/20240326012102.27438-1-jarkko@kernel.org/
> [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> Signed-off-by: Mark Rutland <mark.rutland@....com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Jarkko Sakkinen <jarkko@...nel.org>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Naveen N. Rao <naveen.n.rao@...ux.ibm.com>
> ---
>  arch/Kconfig                |  2 +-
>  kernel/kprobes.c            | 12 +++++++++++-
>  kernel/trace/trace_kprobe.c | 15 +++++++++++++--
>  3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..cf43de9ffb5b9 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>  
>  config KPROBES
>  	bool "Kprobes"
> -	depends on MODULES
> +	depends on MODULES || !KPROBES_USE_MODULE_ALLOC
>  	depends on HAVE_KPROBES
>  	select KALLSYMS
>  	select TASKS_RCU if PREEMPTION
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fa2ee4e59eca2..7c2f0b504cdcb 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1582,6 +1582,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  		goto out;
>  	}
>  
> +#ifdef CONFIG_MODULES
>  	/* Check if 'p' is probing a module. */
>  	*probed_mod = __module_text_address((unsigned long) p->addr);
>  	if (*probed_mod) {
> @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  			ret = -ENOENT;
>  		}
>  	}
> +#endif

This can be scoped a bit more (see v7 of my patch set).

> +
>  out:
>  	preempt_enable();
>  	jump_label_unlock();
> @@ -2484,6 +2487,7 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MODULES
>  /* Remove all symbols in given area from kprobe blacklist */
>  static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
>  {
> @@ -2501,6 +2505,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
>  {
>  	kprobe_remove_area_blacklist(entry, entry + 1);
>  }
> +#endif /* CONFIG_MODULES */
>  
>  int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
>  				   char *type, char *sym)
> @@ -2566,6 +2571,7 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
>  	return ret ? : arch_populate_kprobe_blacklist();
>  }
>  
> +#ifdef CONFIG_MODULES
>  static void add_module_kprobe_blacklist(struct module *mod)
>  {
>  	unsigned long start, end;
> @@ -2662,6 +2668,9 @@ static int kprobes_module_callback(struct notifier_block *nb,
>  	mutex_unlock(&kprobe_mutex);
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define kprobes_module_callback	(NULL)
> +#endif /* CONFIG_MODULES */
>  
>  static struct notifier_block kprobe_module_nb = {
>  	.notifier_call = kprobes_module_callback,
> @@ -2726,7 +2735,8 @@ static int __init init_kprobes(void)
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
> -	if (!err)
> +
> +	if (!err && IS_ENABLED(CONFIG_MODULES))
>  		err = register_module_notifier(&kprobe_module_nb);
>  
>  	kprobes_initialized = (err == 0);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 14099cc17fc9e..c509ba776e679 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -111,6 +111,7 @@ static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
>  	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
>  }
>  
> +#ifdef CONFIG_MODULES
>  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  {
>  	char *p;
> @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  
>  	return ret;
>  }
> +#else
> +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> +#endif /* CONFIG_MODULES */
>  
>  static bool trace_kprobe_is_busy(struct dyn_event *ev)
>  {
> @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_MODULES
>  /* Module notifier call back, checking event on the module */
>  static int trace_kprobe_module_callback(struct notifier_block *nb,
>  				       unsigned long val, void *data)
> @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
>  
>  	return NOTIFY_DONE;
>  }
> +#else
> +#define trace_kprobe_module_callback (NULL)
> +#endif /* CONFIG_MODULES */

The last two CONFIG_MODULES sections could be combined. This was also in
v7.

>  
>  static struct notifier_block trace_kprobe_module_nb = {
>  	.notifier_call = trace_kprobe_module_callback,
> @@ -1933,8 +1941,11 @@ static __init int init_kprobe_trace_early(void)
>  	if (ret)
>  		return ret;
>  
> -	if (register_module_notifier(&trace_kprobe_module_nb))
> -		return -EINVAL;
> +	if (IS_ENABLED(CONFIG_MODULES)) {
> +		ret = register_module_notifier(&trace_kprobe_module_nb);
> +		if (ret)
> +			return -EINVAL;
> +	}
>  
>  	return 0;
>  }

Other than lgtm.

BR, Jarkko