Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Sat, Apr 20, 2024 at 06:15:00PM +0900, Masami Hiramatsu wrote:
> On Sat, 20 Apr 2024 10:33:38 +0300
> Mike Rapoport wrote:
>
> > On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
> > >
> > >
> > > Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > > > Hi Masami,
> > > >
> > > > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> > > >> Hi Mike,
> > > >>
> > > >> On Thu, 11 Apr 2024 19:00:50 +0300
> > > >> Mike Rapoport wrote:
> > > >>
> > > >>> From: "Mike Rapoport (IBM)"
> > > >>>
> > > >>> kprobes depended on CONFIG_MODULES because it has to allocate memory
> > > >>> for
> > > >>> code.
> > > >>>
> > > >>> Since code allocations are now implemented with execmem, kprobes can
> > > >>> be
> > > >>> enabled in non-modular kernels.
> > > >>>
> > > >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes
> > > >>> inside
> > > >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> > > >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> > > >>
> > > >> Thanks for this work, but this conflicts with the latest fix in
> > > >> v6.9-rc4.
> > > >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> > > >> function body? We have enough dummy functions for that, so it should
> > > >> not make a problem.
> > > >
> > > > The code in check_kprobe_address_safe() that gets the module and checks
> > > > for
> > > > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > > > I can pull it out to a helper or leave #ifdef in the function body,
> > > > whichever you prefer.
> > >
> > > As far as I can see, the only problem is MODULE_STATE_COMING.
> > > Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h
> > > ?
> >
> > There's dereference of 'struct module' there:
> >
> > (*probed_mod)->state != MODULE_STATE_COMING) {
> > ...
> > }
> >
> > so moving out 'enum module_state' won't be enough.
>
> Hmm, this part should be inline functions like;
>
> #ifdef CONFIG_MODULES
> static inline bool module_is_coming(struct module *mod)
> {
> return mod->state == MODULE_STATE_COMING;
> }
> #else
> #define module_is_coming(mod) (false)
I'd prefer
static inline module_is_coming(struct module *mod)
{
return false;
}
> #endif
>
> Then we don't need the enum.
> Thank you,
>
> --
> Masami Hiramatsu (Google)
--
Sincerely yours,
Mike.
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Sat, 20 Apr 2024 10:33:38 +0300
Mike Rapoport wrote:
> On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
> >
> >
> > Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > > Hi Masami,
> > >
> > > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> > >> Hi Mike,
> > >>
> > >> On Thu, 11 Apr 2024 19:00:50 +0300
> > >> Mike Rapoport wrote:
> > >>
> > >>> From: "Mike Rapoport (IBM)"
> > >>>
> > >>> kprobes depended on CONFIG_MODULES because it has to allocate memory for
> > >>> code.
> > >>>
> > >>> Since code allocations are now implemented with execmem, kprobes can be
> > >>> enabled in non-modular kernels.
> > >>>
> > >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside
> > >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> > >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> > >>
> > >> Thanks for this work, but this conflicts with the latest fix in v6.9-rc4.
> > >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> > >> function body? We have enough dummy functions for that, so it should
> > >> not make a problem.
> > >
> > > The code in check_kprobe_address_safe() that gets the module and checks
> > > for
> > > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > > I can pull it out to a helper or leave #ifdef in the function body,
> > > whichever you prefer.
> >
> > As far as I can see, the only problem is MODULE_STATE_COMING.
> > Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h ?
>
> There's dereference of 'struct module' there:
>
> (*probed_mod)->state != MODULE_STATE_COMING) {
> ...
> }
>
> so moving out 'enum module_state' won't be enough.
Hmm, this part should be inline functions like;
#ifdef CONFIG_MODULES
static inline bool module_is_coming(struct module *mod)
{
return mod->state == MODULE_STATE_COMING;
}
#else
#define module_is_coming(mod) (false)
#endif
Then we don't need the enum.
Thank you,
>
> > >
> > >> --
> > >> Masami Hiramatsu
> > >
>
> --
> Sincerely yours,
> Mike.
>
--
Masami Hiramatsu (Google)
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
>
>
> Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > Hi Masami,
> >
> > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> >> Hi Mike,
> >>
> >> On Thu, 11 Apr 2024 19:00:50 +0300
> >> Mike Rapoport wrote:
> >>
> >>> From: "Mike Rapoport (IBM)"
> >>>
> >>> kprobes depended on CONFIG_MODULES because it has to allocate memory for
> >>> code.
> >>>
> >>> Since code allocations are now implemented with execmem, kprobes can be
> >>> enabled in non-modular kernels.
> >>>
> >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside
> >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> >>
> >> Thanks for this work, but this conflicts with the latest fix in v6.9-rc4.
> >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> >> function body? We have enough dummy functions for that, so it should
> >> not make a problem.
> >
> > The code in check_kprobe_address_safe() that gets the module and checks for
> > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > I can pull it out to a helper or leave #ifdef in the function body,
> > whichever you prefer.
>
> As far as I can see, the only problem is MODULE_STATE_COMING.
> Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h ?
There's dereference of 'struct module' there:
(*probed_mod)->state != MODULE_STATE_COMING) {
...
}
so moving out 'enum module_state' won't be enough.
> >
> >> --
> >> Masami Hiramatsu
> >
--
Sincerely yours,
Mike.
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
Le 19/04/2024 à 17:49, Mike Rapoport a écrit : > Hi Masami, > > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote: >> Hi Mike, >> >> On Thu, 11 Apr 2024 19:00:50 +0300 >> Mike Rapoport wrote: >> >>> From: "Mike Rapoport (IBM)" >>> >>> kprobes depended on CONFIG_MODULES because it has to allocate memory for >>> code. >>> >>> Since code allocations are now implemented with execmem, kprobes can be >>> enabled in non-modular kernels. >>> >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the >>> dependency of CONFIG_KPROBES on CONFIG_MODULES. >> >> Thanks for this work, but this conflicts with the latest fix in v6.9-rc4. >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in >> function body? We have enough dummy functions for that, so it should >> not make a problem. > > The code in check_kprobe_address_safe() that gets the module and checks for > __init functions does not compile with IS_ENABLED(CONFIG_MODULES). > I can pull it out to a helper or leave #ifdef in the function body, > whichever you prefer. As far as I can see, the only problem is MODULE_STATE_COMING. Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h ? > >> -- >> Masami Hiramatsu >
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
Hi Masami, On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote: > Hi Mike, > > On Thu, 11 Apr 2024 19:00:50 +0300 > Mike Rapoport wrote: > > > From: "Mike Rapoport (IBM)" > > > > kprobes depended on CONFIG_MODULES because it has to allocate memory for > > code. > > > > Since code allocations are now implemented with execmem, kprobes can be > > enabled in non-modular kernels. > > > > Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside > > modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the > > dependency of CONFIG_KPROBES on CONFIG_MODULES. > > Thanks for this work, but this conflicts with the latest fix in v6.9-rc4. > Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in > function body? We have enough dummy functions for that, so it should > not make a problem. The code in check_kprobe_address_safe() that gets the module and checks for __init functions does not compile with IS_ENABLED(CONFIG_MODULES). I can pull it out to a helper or leave #ifdef in the function body, whichever you prefer. > -- > Masami Hiramatsu -- Sincerely yours, Mike.
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
Hi Masami, On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote: > Hi Mike, > > On Thu, 11 Apr 2024 19:00:50 +0300 > Mike Rapoport wrote: > > > From: "Mike Rapoport (IBM)" > > > > kprobes depended on CONFIG_MODULES because it has to allocate memory for > > code. > > > > Since code allocations are now implemented with execmem, kprobes can be > > enabled in non-modular kernels. > > > > Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside > > modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the > > dependency of CONFIG_KPROBES on CONFIG_MODULES. > > Thanks for this work, but this conflicts with the latest fix in v6.9-rc4. > Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in > function body? We have enough dummy functions for that, so it should > not make a problem. I'll rebase and will try to reduce ifdefery where possible. > Thank you, > > > > > Signed-off-by: Mike Rapoport (IBM) > > --- > > arch/Kconfig| 2 +- > > kernel/kprobes.c| 43 + > > kernel/trace/trace_kprobe.c | 11 ++ > > 3 files changed, 37 insertions(+), 19 deletions(-) > > > > -- > Masami Hiramatsu -- Sincerely yours, Mike.
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
Hi Mike,
On Thu, 11 Apr 2024 19:00:50 +0300
Mike Rapoport wrote:
> From: "Mike Rapoport (IBM)"
>
> kprobes depended on CONFIG_MODULES because it has to allocate memory for
> code.
>
> Since code allocations are now implemented with execmem, kprobes can be
> enabled in non-modular kernels.
>
> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside
> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> dependency of CONFIG_KPROBES on CONFIG_MODULES.
Thanks for this work, but this conflicts with the latest fix in v6.9-rc4.
Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
function body? We have enough dummy functions for that, so it should
not make a problem.
Thank you,
>
> Signed-off-by: Mike Rapoport (IBM)
> ---
> arch/Kconfig| 2 +-
> kernel/kprobes.c| 43 +
> kernel/trace/trace_kprobe.c | 11 ++
> 3 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index bc9e8e5dccd5..68177adf61a0 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,9 +52,9 @@ config GENERIC_ENTRY
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> depends on HAVE_KPROBES
> select KALLSYMS
> + select EXECMEM
> select TASKS_RCU if PREEMPTION
> help
> Kprobes allows you to trap at almost any kernel address and
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 047ca629ce49..90c056853e6f 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1580,6 +1580,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) {
> @@ -1603,6 +1604,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> ret = -ENOENT;
> }
> }
> +#endif
> +
> out:
> preempt_enable();
> jump_label_unlock();
> @@ -2482,24 +2485,6 @@ int kprobe_add_area_blacklist(unsigned long start,
> unsigned long end)
> return 0;
> }
>
> -/* Remove all symbols in given area from kprobe blacklist */
> -static void kprobe_remove_area_blacklist(unsigned long start, unsigned long
> end)
> -{
> - struct kprobe_blacklist_entry *ent, *n;
> -
> - list_for_each_entry_safe(ent, n, _blacklist, list) {
> - if (ent->start_addr < start || ent->start_addr >= end)
> - continue;
> - list_del(>list);
> - kfree(ent);
> - }
> -}
> -
> -static void kprobe_remove_ksym_blacklist(unsigned long entry)
> -{
> - kprobe_remove_area_blacklist(entry, entry + 1);
> -}
> -
> int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long
> *value,
> char *type, char *sym)
> {
> @@ -2564,6 +2549,25 @@ static int __init populate_kprobe_blacklist(unsigned
> long *start,
> return ret ? : arch_populate_kprobe_blacklist();
> }
>
> +#ifdef CONFIG_MODULES
> +/* Remove all symbols in given area from kprobe blacklist */
> +static void kprobe_remove_area_blacklist(unsigned long start, unsigned long
> end)
> +{
> + struct kprobe_blacklist_entry *ent, *n;
> +
> + list_for_each_entry_safe(ent, n, _blacklist, list) {
> + if (ent->start_addr < start || ent->start_addr >= end)
> + continue;
> + list_del(>list);
> + kfree(ent);
> + }
> +}
> +
> +static void kprobe_remove_ksym_blacklist(unsigned long entry)
> +{
> + kprobe_remove_area_blacklist(entry, entry + 1);
> +}
> +
> static void add_module_kprobe_blacklist(struct module *mod)
> {
> unsigned long start, end;
> @@ -2665,6 +2669,7 @@ static struct notifier_block kprobe_module_nb = {
> .notifier_call = kprobes_module_callback,
> .priority = 0
> };
> +#endif
>
> void kprobe_free_init_mem(void)
> {
> @@ -2724,8 +2729,10 @@ static int __init init_kprobes(void)
> err = arch_init_kprobes();
> if (!err)
> err = register_die_notifier(_exceptions_nb);
> +#ifdef CONFIG_MODULES
> if (!err)
> err = register_module_notifier(_module_nb);
> +#endif
>
> kprobes_initialized = (err == 0);
> kprobe_sysctls_init();
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 14099cc17fc9..f0610137d6a3 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,12 @@ static nokprobe_inline bool
>
