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

Commit Message

Mark Rutland March 26, 2024, 4:36 p.m. UTC

From: Jarkko Sakkinen <[e-mail]>

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 <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]/
[Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
Signed-off-by: Mark Rutland <[e-mail]>
Cc: Anil S Keshavamurthy <[e-mail]>
Cc: David S. Miller <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Masami Hiramatsu <[e-mail]>
Cc: Naveen N. Rao <[e-mail]>
---
 arch/Kconfig                |  2 +-
 kernel/kprobes.c            | 12 +++++++++++-
 kernel/trace/trace_kprobe.c | 15 +++++++++++++--
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Jarkko Sakkinen March 26, 2024, 5:13 p.m. UTC | #1

On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> From: Jarkko Sakkinen <[e-mail]>
>
> 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 <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/
> [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> Signed-off-by: Mark Rutland <[e-mail]>
> Cc: Anil S Keshavamurthy <[e-mail]>
> Cc: David S. Miller <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Masami Hiramatsu <[e-mail]>
> Cc: Naveen N. Rao <[e-mail]>
> ---
>  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

Mark Rutland March 26, 2024, 5:38 p.m. UTC | #2

On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:

> > +#ifdef CONFIG_MODULES
> >  	/* Check if 'p' is probing a module. */
> >  	*probed_mod = __module_text_address((unsigned long) p->addr);
> >  	if (*probed_mod) {
> > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> >  			ret = -ENOENT;
> >  		}
> >  	}
> > +#endif
> 
> This can be scoped a bit more (see v7 of my patch set).

> > +#ifdef CONFIG_MODULES
> >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >  {
> >  	char *p;
> > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >  
> >  	return ret;
> >  }
> > +#else
> > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > +#endif /* CONFIG_MODULES */
> >  
> >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> >  {
> > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> >  	return ret;
> >  }
> >  
> > +#ifdef CONFIG_MODULES
> >  /* Module notifier call back, checking event on the module */
> >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> >  				       unsigned long val, void *data)
> > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> >  
> >  	return NOTIFY_DONE;
> >  }
> > +#else
> > +#define trace_kprobe_module_callback (NULL)
> > +#endif /* CONFIG_MODULES */
> 
> The last two CONFIG_MODULES sections could be combined. This was also in
> v7.

> Other than lgtm.

Great! I've folded your v7 changes in, and pushed that out to:

  https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules

I'll hold off sending that out to the list until other folk have had a chance
to comment.

Mark.

Masami Hiramatsu (Google) March 27, 2024, 12:01 a.m. UTC | #3

On Tue, 26 Mar 2024 17:38:18 +0000
Mark Rutland <[e-mail]> wrote:

> On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> 
> > > +#ifdef CONFIG_MODULES
> > >  	/* Check if 'p' is probing a module. */
> > >  	*probed_mod = __module_text_address((unsigned long) p->addr);
> > >  	if (*probed_mod) {
> > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > >  			ret = -ENOENT;
> > >  		}
> > >  	}
> > > +#endif
> > 
> > This can be scoped a bit more (see v7 of my patch set).
> 
> > > +#ifdef CONFIG_MODULES
> > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > >  {
> > >  	char *p;
> > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > >  
> > >  	return ret;
> > >  }
> > > +#else
> > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > +#endif /* CONFIG_MODULES */
> > >  
> > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > >  {
> > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > >  	return ret;
> > >  }
> > >  
> > > +#ifdef CONFIG_MODULES
> > >  /* Module notifier call back, checking event on the module */
> > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > >  				       unsigned long val, void *data)
> > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > >  
> > >  	return NOTIFY_DONE;
> > >  }
> > > +#else
> > > +#define trace_kprobe_module_callback (NULL)
> > > +#endif /* CONFIG_MODULES */
> > 
> > The last two CONFIG_MODULES sections could be combined. This was also in
> > v7.
> 
> > Other than lgtm.
> 
> Great! I've folded your v7 changes in, and pushed that out to:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> 
> I'll hold off sending that out to the list until other folk have had a chance
> to comment.

Yeah, the updated one looks good to me too.

Thanks!

> 
> Mark.

Jarkko Sakkinen March 27, 2024, 1:23 p.m. UTC | #4

On Wed, 2024-03-27 at 09:01 +0900, Masami Hiramatsu wrote:
> On Tue, 26 Mar 2024 17:38:18 +0000
> Mark Rutland <[e-mail]> wrote:
> 
> > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > 
> > > > +#ifdef CONFIG_MODULES
> > > >  	/* Check if 'p' is probing a module. */
> > > >  	*probed_mod = __module_text_address((unsigned long) p-
> > > > >addr);
> > > >  	if (*probed_mod) {
> > > > @@ -1605,6 +1606,8 @@ static int
> > > > check_kprobe_address_safe(struct kprobe *p,
> > > >  			ret = -ENOENT;
> > > >  		}
> > > >  	}
> > > > +#endif
> > > 
> > > This can be scoped a bit more (see v7 of my patch set).
> > 
> > > > +#ifdef CONFIG_MODULES
> > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct
> > > > trace_kprobe *tk)
> > > >  {
> > > >  	char *p;
> > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool
> > > > trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_exist(tk) false /* aka a module
> > > > never exists */
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > >  {
> > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct
> > > > trace_kprobe *tk)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  /* Module notifier call back, checking event on the module */
> > > >  static int trace_kprobe_module_callback(struct notifier_block
> > > > *nb,
> > > >  				       unsigned long val, void
> > > > *data)
> > > > @@ -699,6 +704,9 @@ static int
> > > > trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_callback (NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > 
> > > The last two CONFIG_MODULES sections could be combined. This was
> > > also in
> > > v7.
> > 
> > > Other than lgtm.
> > 
> > Great! I've folded your v7 changes in, and pushed that out to:
> > 
> >  
> > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > 
> > I'll hold off sending that out to the list until other folk have
> > had a chance
> > to comment.
> 
> Yeah, the updated one looks good to me too.
> 
> Thanks!

Yeah, I'm also planning to test this with x86 instrumenting sgx_* calls
as I need to test the cgroups support for it so can help with the
coverage both RISC-V and x86 (as I find a good time slot).

BR, Jarkko

Jarkko Sakkinen March 27, 2024, 5:46 p.m. UTC | #5

On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> On Tue, 26 Mar 2024 17:38:18 +0000
> Mark Rutland <[e-mail]> wrote:
>
> > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > 
> > > > +#ifdef CONFIG_MODULES
> > > >  	/* Check if 'p' is probing a module. */
> > > >  	*probed_mod = __module_text_address((unsigned long) p->addr);
> > > >  	if (*probed_mod) {
> > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > >  			ret = -ENOENT;
> > > >  		}
> > > >  	}
> > > > +#endif
> > > 
> > > This can be scoped a bit more (see v7 of my patch set).
> > 
> > > > +#ifdef CONFIG_MODULES
> > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  {
> > > >  	char *p;
> > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > +#endif /* CONFIG_MODULES */
> > > >  
> > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > >  {
> > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > >  /* Module notifier call back, checking event on the module */
> > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  				       unsigned long val, void *data)
> > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > >  
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > > +#else
> > > > +#define trace_kprobe_module_callback (NULL)
> > > > +#endif /* CONFIG_MODULES */
> > > 
> > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > v7.
> > 
> > > Other than lgtm.
> > 
> > Great! I've folded your v7 changes in, and pushed that out to:
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > 
> > I'll hold off sending that out to the list until other folk have had a chance
> > to comment.
>
> Yeah, the updated one looks good to me too.
>
> Thanks!

As for RISC-V:

Tested-by: Jarkko Sakkinen <[e-mail]> # arch/riscv

I'm fine with adding to all patches because it would be hard
to place tested-by to any specific patch (e.g. if this was a
syscall I would give tested-by just for that patch).

Just adding disclaimer because depending on subsystem people
are more or less strict with this tag :-)

BR, Jarkko

Masami Hiramatsu (Google) March 27, 2024, 11:47 p.m. UTC | #6

On Wed, 27 Mar 2024 19:46:50 +0200
"Jarkko Sakkinen" <[e-mail]> wrote:

> On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> > On Tue, 26 Mar 2024 17:38:18 +0000
> > Mark Rutland <[e-mail]> wrote:
> >
> > > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > > 
> > > > > +#ifdef CONFIG_MODULES
> > > > >  	/* Check if 'p' is probing a module. */
> > > > >  	*probed_mod = __module_text_address((unsigned long) p->addr);
> > > > >  	if (*probed_mod) {
> > > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > >  			ret = -ENOENT;
> > > > >  		}
> > > > >  	}
> > > > > +#endif
> > > > 
> > > > This can be scoped a bit more (see v7 of my patch set).
> > > 
> > > > > +#ifdef CONFIG_MODULES
> > > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  {
> > > > >  	char *p;
> > > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > >  
> > > > >  	return ret;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > > +#endif /* CONFIG_MODULES */
> > > > >  
> > > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > > >  {
> > > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_MODULES
> > > > >  /* Module notifier call back, checking event on the module */
> > > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  				       unsigned long val, void *data)
> > > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > >  
> > > > >  	return NOTIFY_DONE;
> > > > >  }
> > > > > +#else
> > > > > +#define trace_kprobe_module_callback (NULL)
> > > > > +#endif /* CONFIG_MODULES */
> > > > 
> > > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > > v7.
> > > 
> > > > Other than lgtm.
> > > 
> > > Great! I've folded your v7 changes in, and pushed that out to:
> > > 
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > > 
> > > I'll hold off sending that out to the list until other folk have had a chance
> > > to comment.
> >
> > Yeah, the updated one looks good to me too.
> >
> > Thanks!
> 
> As for RISC-V:
> 
> Tested-by: Jarkko Sakkinen <[e-mail]> # arch/riscv

Thank you for testing!

> 
> I'm fine with adding to all patches because it would be hard
> to place tested-by to any specific patch (e.g. if this was a
> syscall I would give tested-by just for that patch).

Except for the 1st patch because that is for arm64, right? :)

> 
> Just adding disclaimer because depending on subsystem people
> are more or less strict with this tag :-)
> 
> BR, Jarkko

Thanks,

Jarkko Sakkinen March 30, 2024, 11:32 a.m. UTC | #7

On Thu Mar 28, 2024 at 1:47 AM EET, Masami Hiramatsu (Google) wrote:
> On Wed, 27 Mar 2024 19:46:50 +0200
> "Jarkko Sakkinen" <[e-mail]> wrote:
>
> > On Wed Mar 27, 2024 at 2:01 AM EET, Masami Hiramatsu (Google) wrote:
> > > On Tue, 26 Mar 2024 17:38:18 +0000
> > > Mark Rutland <[e-mail]> wrote:
> > >
> > > > On Tue, Mar 26, 2024 at 07:13:51PM +0200, Jarkko Sakkinen wrote:
> > > > > On Tue Mar 26, 2024 at 6:36 PM EET, Mark Rutland wrote:
> > > > 
> > > > > > +#ifdef CONFIG_MODULES
> > > > > >  	/* Check if 'p' is probing a module. */
> > > > > >  	*probed_mod = __module_text_address((unsigned long) p->addr);
> > > > > >  	if (*probed_mod) {
> > > > > > @@ -1605,6 +1606,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > > > > >  			ret = -ENOENT;
> > > > > >  		}
> > > > > >  	}
> > > > > > +#endif
> > > > > 
> > > > > This can be scoped a bit more (see v7 of my patch set).
> > > > 
> > > > > > +#ifdef CONFIG_MODULES
> > > > > >  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > > >  {
> > > > > >  	char *p;
> > > > > > @@ -129,6 +130,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > > > > >  
> > > > > >  	return ret;
> > > > > >  }
> > > > > > +#else
> > > > > > +#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
> > > > > > +#endif /* CONFIG_MODULES */
> > > > > >  
> > > > > >  static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > > > > >  {
> > > > > > @@ -670,6 +674,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > > > > >  	return ret;
> > > > > >  }
> > > > > >  
> > > > > > +#ifdef CONFIG_MODULES
> > > > > >  /* Module notifier call back, checking event on the module */
> > > > > >  static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > > >  				       unsigned long val, void *data)
> > > > > > @@ -699,6 +704,9 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> > > > > >  
> > > > > >  	return NOTIFY_DONE;
> > > > > >  }
> > > > > > +#else
> > > > > > +#define trace_kprobe_module_callback (NULL)
> > > > > > +#endif /* CONFIG_MODULES */
> > > > > 
> > > > > The last two CONFIG_MODULES sections could be combined. This was also in
> > > > > v7.
> > > > 
> > > > > Other than lgtm.
> > > > 
> > > > Great! I've folded your v7 changes in, and pushed that out to:
> > > > 
> > > >   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=kprobes/without-modules
> > > > 
> > > > I'll hold off sending that out to the list until other folk have had a chance
> > > > to comment.
> > >
> > > Yeah, the updated one looks good to me too.
> > >
> > > Thanks!
> > 
> > As for RISC-V:
> > 
> > Tested-by: Jarkko Sakkinen <[e-mail]> # arch/riscv
>
> Thank you for testing!
>
> > 
> > I'm fine with adding to all patches because it would be hard
> > to place tested-by to any specific patch (e.g. if this was a
> > syscall I would give tested-by just for that patch).
>
> Except for the 1st patch because that is for arm64, right? :)

Right! For that not required :-)

>
> > 
> > Just adding disclaimer because depending on subsystem people
> > are more or less strict with this tag :-)
> > 
> > BR, Jarkko
>
> Thanks,

BR, Jarkko

Mark Rutland April 3, 2024, 11:20 a.m. UTC | #8

On Tue, Mar 26, 2024 at 04:36:24PM +0000, Mark Rutland wrote:
> From: Jarkko Sakkinen <[e-mail]>
> 
> 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 <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/
> [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> Signed-off-by: Mark Rutland <[e-mail]>
> Cc: Anil S Keshavamurthy <[e-mail]>
> Cc: David S. Miller <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Masami Hiramatsu <[e-mail]>
> Cc: Naveen N. Rao <[e-mail]>
> ---
>  arch/Kconfig                |  2 +-
>  kernel/kprobes.c            | 12 +++++++++++-
>  kernel/trace/trace_kprobe.c | 15 +++++++++++++--
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 85bb59f7b8c07..cf43de9ffb5b9 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,7 +52,7 @@ config GENERIC_ENTRY
>  
>  config KPROBES
>  	bool "Kprobes"
> -	depends on MODULES
> +	depends on MODULES || !KPROBES_USE_MODULE_ALLOC

Whoops; that should be:

	depends on MODULES || HAVE_KPROBES_ALLOC

... with similar fixups in the commit message to describe HAVE_KPROBES_ALLOC
rather than KPROBES_USE_MODULE_ALLOC (which does not exist in any version of
the series that got sent to the list).

I'll send a v2 with that fixed (and the other changes from Jarkko's v7 base
patch) once I've locally tested that for architectures with and without
HAVE_KPROBES_ALLOC.

Mark.

Jarkko Sakkinen April 3, 2024, 4:10 p.m. UTC | #9

On Wed Apr 3, 2024 at 2:20 PM EEST, Mark Rutland wrote:
> On Tue, Mar 26, 2024 at 04:36:24PM +0000, Mark Rutland wrote:
> > From: Jarkko Sakkinen <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Link: https://lore.kernel.org/all/[e-mail]/
> > [Mark: Remove execmem changes, depend on !KPROBES_USE_MODULE_ALLOC]
> > Signed-off-by: Mark Rutland <[e-mail]>
> > Cc: Anil S Keshavamurthy <[e-mail]>
> > Cc: David S. Miller <[e-mail]>
> > Cc: Jarkko Sakkinen <[e-mail]>
> > Cc: Masami Hiramatsu <[e-mail]>
> > Cc: Naveen N. Rao <[e-mail]>
> > ---
> >  arch/Kconfig                |  2 +-
> >  kernel/kprobes.c            | 12 +++++++++++-
> >  kernel/trace/trace_kprobe.c | 15 +++++++++++++--
> >  3 files changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 85bb59f7b8c07..cf43de9ffb5b9 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -52,7 +52,7 @@ config GENERIC_ENTRY
> >  
> >  config KPROBES
> >  	bool "Kprobes"
> > -	depends on MODULES
> > +	depends on MODULES || !KPROBES_USE_MODULE_ALLOC
>
> Whoops; that should be:
>
> 	depends on MODULES || HAVE_KPROBES_ALLOC
>
> ... with similar fixups in the commit message to describe HAVE_KPROBES_ALLOC
> rather than KPROBES_USE_MODULE_ALLOC (which does not exist in any version of
> the series that got sent to the list).
>
> I'll send a v2 with that fixed (and the other changes from Jarkko's v7 base
> patch) once I've locally tested that for architectures with and without
> HAVE_KPROBES_ALLOC.

OK, please put to me to the CC list as I'm not ATM subscribed
to the tracing list.

BR, Jarkko

Patch

diff --git a/arch/Kconfig b/arch/Kconfig
index 85bb59f7b8c07..cf43de9ffb5b9 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -52,7 +52,7 @@  config GENERIC_ENTRY
 
 config KPROBES
 	bool "Kprobes"
-	depends on MODULES
+	depends on MODULES || !KPROBES_USE_MODULE_ALLOC
 	depends on HAVE_KPROBES
 	select KALLSYMS
 	select TASKS_RCU if PREEMPTION
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index fa2ee4e59eca2..7c2f0b504cdcb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1582,6 +1582,7 @@  static int check_kprobe_address_safe(struct kprobe *p,
 		goto out;
 	}
 
+#ifdef CONFIG_MODULES
 	/* Check if 'p' is probing a module. */
 	*probed_mod = __module_text_address((unsigned long) p->addr);
 	if (*probed_mod) {
@@ -1605,6 +1606,8 @@  static int check_kprobe_address_safe(struct kprobe *p,
 			ret = -ENOENT;
 		}
 	}
+#endif
+
 out:
 	preempt_enable();
 	jump_label_unlock();
@@ -2484,6 +2487,7 @@  int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
 	return 0;
 }
 
+#ifdef CONFIG_MODULES
 /* Remove all symbols in given area from kprobe blacklist */
 static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
 {
@@ -2501,6 +2505,7 @@  static void kprobe_remove_ksym_blacklist(unsigned long entry)
 {
 	kprobe_remove_area_blacklist(entry, entry + 1);
 }
+#endif /* CONFIG_MODULES */
 
 int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
 				   char *type, char *sym)
@@ -2566,6 +2571,7 @@  static int __init populate_kprobe_blacklist(unsigned long *start,
 	return ret ? : arch_populate_kprobe_blacklist();
 }
 
+#ifdef CONFIG_MODULES
 static void add_module_kprobe_blacklist(struct module *mod)
 {
 	unsigned long start, end;
@@ -2662,6 +2668,9 @@  static int kprobes_module_callback(struct notifier_block *nb,
 	mutex_unlock(&kprobe_mutex);
 	return NOTIFY_DONE;
 }
+#else
+#define kprobes_module_callback	(NULL)
+#endif /* CONFIG_MODULES */
 
 static struct notifier_block kprobe_module_nb = {
 	.notifier_call = kprobes_module_callback,
@@ -2726,7 +2735,8 @@  static int __init init_kprobes(void)
 	err = arch_init_kprobes();
 	if (!err)
 		err = register_die_notifier(&kprobe_exceptions_nb);
-	if (!err)
+
+	if (!err && IS_ENABLED(CONFIG_MODULES))
 		err = register_module_notifier(&kprobe_module_nb);
 
 	kprobes_initialized = (err == 0);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 14099cc17fc9e..c509ba776e679 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -111,6 +111,7 @@  static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
 	return strncmp(module_name(mod), name, len) == 0 && name[len] == ':';
 }
 
+#ifdef CONFIG_MODULES
 static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 {
 	char *p;
@@ -129,6 +130,9 @@  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 
 	return ret;
 }
+#else
+#define trace_kprobe_module_exist(tk) false /* aka a module never exists */
+#endif /* CONFIG_MODULES */
 
 static bool trace_kprobe_is_busy(struct dyn_event *ev)
 {
@@ -670,6 +674,7 @@  static int register_trace_kprobe(struct trace_kprobe *tk)
 	return ret;
 }
 
+#ifdef CONFIG_MODULES
 /* Module notifier call back, checking event on the module */
 static int trace_kprobe_module_callback(struct notifier_block *nb,
 				       unsigned long val, void *data)
@@ -699,6 +704,9 @@  static int trace_kprobe_module_callback(struct notifier_block *nb,
 
 	return NOTIFY_DONE;
 }
+#else
+#define trace_kprobe_module_callback (NULL)
+#endif /* CONFIG_MODULES */
 
 static struct notifier_block trace_kprobe_module_nb = {
 	.notifier_call = trace_kprobe_module_callback,
@@ -1933,8 +1941,11 @@  static __init int init_kprobe_trace_early(void)
 	if (ret)
 		return ret;
 
-	if (register_module_notifier(&trace_kprobe_module_nb))
-		return -EINVAL;
+	if (IS_ENABLED(CONFIG_MODULES)) {
+		ret = register_module_notifier(&trace_kprobe_module_nb);
+		if (ret)
+			return -EINVAL;
+	}
 
 	return 0;
 }