| Message ID | 20240411160051.2093261-15-rppt@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm: jit/text allocator | expand |
Hi Mike, On Thu, 11 Apr 2024 19:00:50 +0300 Mike Rapoport <rppt@kernel.org> wrote: > From: "Mike Rapoport (IBM)" <rppt@kernel.org> > > 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) <rppt@kernel.org> > --- > 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, &kprobe_blacklist, list) { > - if (ent->start_addr < start || ent->start_addr >= end) > - continue; > - list_del(&ent->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, &kprobe_blacklist, list) { > + if (ent->start_addr < start || ent->start_addr >= end) > + continue; > + list_del(&ent->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(&kprobe_exceptions_nb); > +#ifdef CONFIG_MODULES > if (!err) > err = register_module_notifier(&kprobe_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 trace_kprobe_module_exist(struct trace_kprobe *tk) > > return ret; > } > +#else > +static inline bool trace_kprobe_module_exist(struct trace_kprobe *tk) > +{ > + return false; > +} > +#endif > > static bool trace_kprobe_is_busy(struct dyn_event *ev) > { > @@ -670,6 +677,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) > @@ -704,6 +712,7 @@ static struct notifier_block trace_kprobe_module_nb = { > .notifier_call = trace_kprobe_module_callback, > .priority = 1 /* Invoked after kprobe module callback */ > }; > +#endif > > static int count_symbols(void *data, unsigned long unused) > { > @@ -1933,8 +1942,10 @@ static __init int init_kprobe_trace_early(void) > if (ret) > return ret; > > +#ifdef CONFIG_MODULES > if (register_module_notifier(&trace_kprobe_module_nb)) > return -EINVAL; > +#endif > > return 0; > } > -- > 2.43.0 > >
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 <rppt@kernel.org> wrote: > > > From: "Mike Rapoport (IBM)" <rppt@kernel.org> > > > > 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) <rppt@kernel.org> > > --- > > arch/Kconfig | 2 +- > > kernel/kprobes.c | 43 +++++++++++++++++++++---------------- > > kernel/trace/trace_kprobe.c | 11 ++++++++++ > > 3 files changed, 37 insertions(+), 19 deletions(-) > > > > -- > Masami Hiramatsu
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 <rppt@kernel.org> wrote: > > > From: "Mike Rapoport (IBM)" <rppt@kernel.org> > > > > 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
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 <rppt@kernel.org> wrote: >> >>> From: "Mike Rapoport (IBM)" <rppt@kernel.org> >>> >>> 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 >
On Fri, Apr 19, 2024 at 03:59:40PM +0000, 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 <rppt@kernel.org> wrote: > >> > >>> From: "Mike Rapoport (IBM)" <rppt@kernel.org> > >>> > >>> 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 > >
On Sat, 20 Apr 2024 10:33:38 +0300 Mike Rapoport <rppt@kernel.org> wrote: > On Fri, Apr 19, 2024 at 03:59:40PM +0000, 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 <rppt@kernel.org> wrote: > > >> > > >>> From: "Mike Rapoport (IBM)" <rppt@kernel.org> > > >>> > > >>> 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. >
On Sat, Apr 20, 2024 at 06:15:00PM +0900, Masami Hiramatsu wrote: > On Sat, 20 Apr 2024 10:33:38 +0300 > Mike Rapoport <rppt@kernel.org> wrote: > > > On Fri, Apr 19, 2024 at 03:59:40PM +0000, 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 <rppt@kernel.org> wrote: > > > >> > > > >>> From: "Mike Rapoport (IBM)" <rppt@kernel.org> > > > >>> > > > >>> 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) <mhiramat@kernel.org>
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, &kprobe_blacklist, list) { - if (ent->start_addr < start || ent->start_addr >= end) - continue; - list_del(&ent->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, &kprobe_blacklist, list) { + if (ent->start_addr < start || ent->start_addr >= end) + continue; + list_del(&ent->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(&kprobe_exceptions_nb); +#ifdef CONFIG_MODULES if (!err) err = register_module_notifier(&kprobe_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 trace_kprobe_module_exist(struct trace_kprobe *tk) return ret; } +#else +static inline bool trace_kprobe_module_exist(struct trace_kprobe *tk) +{ + return false; +} +#endif static bool trace_kprobe_is_busy(struct dyn_event *ev) { @@ -670,6 +677,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) @@ -704,6 +712,7 @@ static struct notifier_block trace_kprobe_module_nb = { .notifier_call = trace_kprobe_module_callback, .priority = 1 /* Invoked after kprobe module callback */ }; +#endif static int count_symbols(void *data, unsigned long unused) { @@ -1933,8 +1942,10 @@ static __init int init_kprobe_trace_early(void) if (ret) return ret; +#ifdef CONFIG_MODULES if (register_module_notifier(&trace_kprobe_module_nb)) return -EINVAL; +#endif return 0; }