| Message ID | 169854904604.132316.12500381416261460174.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | [for-next] tracing/kprobes: Add symbol counting check when module loads | expand |
On Sun, 29 Oct 2023 12:10:46 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Check the number of probe target symbols in the target module when > the module is loaded. If the probe is not on the unique name symbols > in the module, it will be rejected at that point. > > Note that the symbol which has a unique name in the target module, > it will be accepted even if there are same-name symbols in the > kernel or other modules, > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 112 ++++++++++++++++++++++++++----------------- > 1 file changed, 68 insertions(+), 44 deletions(-) Reviewed-by: Steven Rosted (Google) <rostedt@goodmis.org> -- Steve
On Sat, Oct 28, 2023 at 8:10 PM Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Check the number of probe target symbols in the target module when > the module is loaded. If the probe is not on the unique name symbols > in the module, it will be rejected at that point. > > Note that the symbol which has a unique name in the target module, > it will be accepted even if there are same-name symbols in the > kernel or other modules, > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 112 ++++++++++++++++++++++++++----------------- > 1 file changed, 68 insertions(+), 44 deletions(-) > LGTM. Acked-by: Andrii Nakryiko <andrii@kernel.org> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index e834f149695b..90cf2219adb4 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk) > return ret; > } > > +static int validate_module_probe_symbol(const char *modname, const char *symbol); > + > +static int register_module_trace_kprobe(struct module *mod, struct trace_kprobe *tk) > +{ > + const char *p; > + int ret = 0; > + > + p = strchr(trace_kprobe_symbol(tk), ':'); > + if (p) > + ret = validate_module_probe_symbol(module_name(mod), p++); > + if (!ret) > + ret = register_trace_kprobe(tk); > + return ret; > +} > + > /* Module notifier call back, checking event on the module */ > static int trace_kprobe_module_callback(struct notifier_block *nb, > unsigned long val, void *data) > @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb, > if (trace_kprobe_within_module(tk, mod)) { > /* Don't need to check busy - this should have gone. */ > __unregister_trace_kprobe(tk); > - ret = __register_trace_kprobe(tk); > + ret = register_module_trace_kprobe(mod, tk); > if (ret) > pr_warn("Failed to re-register probe %s on %s: %d\n", > trace_probe_name(&tk->tp), > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char *name, unsigned long unused) > return 0; > } > > -static unsigned int number_of_same_symbols(char *func_name) > +static unsigned int number_of_same_symbols(const char *mod, const char *func_name) > { > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > + if (!mod) > + kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); > > return ctx.count; > } > > +static int validate_module_probe_symbol(const char *modname, const char *symbol) > +{ > + unsigned int count = number_of_same_symbols(modname, symbol); > + > + if (count > 1) { > + /* > + * Users should use ADDR to remove the ambiguity of > + * using KSYM only. > + */ > + return -EADDRNOTAVAIL; > + } else if (count == 0) { > + /* > + * We can return ENOENT earlier than when register the > + * kprobe. > + */ > + return -ENOENT; > + } > + return 0; > +} > + > +static int validate_probe_symbol(char *symbol) > +{ > + char *mod = NULL, *p; > + int ret; > + > + p = strchr(symbol, ':'); > + if (p) { > + mod = symbol; > + symbol = p + 1; > + *p = '\0'; > + } > + ret = validate_module_probe_symbol(mod, symbol); > + if (p) > + *p = ':'; > + return ret; > +} > + > static int __trace_kprobe_create(int argc, const char *argv[]) > { > /* > @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > trace_probe_log_err(0, BAD_PROBE_ADDR); > goto parse_error; > } > + ret = validate_probe_symbol(symbol); > + if (ret) { > + if (ret == -EADDRNOTAVAIL) > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > + else > + trace_probe_log_err(0, BAD_PROBE_ADDR); > + goto parse_error; > + } > if (is_return) > ctx.flags |= TPARG_FL_RETURN; > ret = kprobe_on_func_entry(NULL, symbol, offset); > @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > } > } > > - if (symbol && !strchr(symbol, ':')) { > - unsigned int count; > - > - count = number_of_same_symbols(symbol); > - if (count > 1) { > - /* > - * Users should use ADDR to remove the ambiguity of > - * using KSYM only. > - */ > - trace_probe_log_err(0, NON_UNIQ_SYMBOL); > - ret = -EADDRNOTAVAIL; > - > - goto error; > - } else if (count == 0) { > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - trace_probe_log_err(0, BAD_PROBE_ADDR); > - ret = -ENOENT; > - > - goto error; > - } > - } > - > trace_probe_log_set_index(0); > if (event) { > ret = traceprobe_parse_event_name(&event, &group, gbuf, > @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > char *event; > > if (func) { > - unsigned int count; > - > - count = number_of_same_symbols(func); > - if (count > 1) > - /* > - * Users should use addr to remove the ambiguity of > - * using func only. > - */ > - return ERR_PTR(-EADDRNOTAVAIL); > - else if (count == 0) > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - return ERR_PTR(-ENOENT); > + ret = validate_probe_symbol(func); > + if (ret) > + return ERR_PTR(ret); > } > > /* >
Hi! Le dimanche 29 octobre 2023, 05:10:46 EET Masami Hiramatsu (Google) a écrit : > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Check the number of probe target symbols in the target module when > the module is loaded. If the probe is not on the unique name symbols > in the module, it will be rejected at that point. > > Note that the symbol which has a unique name in the target module, > it will be accepted even if there are same-name symbols in the > kernel or other modules, I am wondering about symbols which are only part of one specific module. I wrote a comment about it below, please let me know what you think about it. > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 112 > ++++++++++++++++++++++++++----------------- 1 file changed, 68 > insertions(+), 44 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index e834f149695b..90cf2219adb4 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe > *tk) return ret; > } > > +static int validate_module_probe_symbol(const char *modname, const char > *symbol); + > +static int register_module_trace_kprobe(struct module *mod, struct > trace_kprobe *tk) +{ > + const char *p; > + int ret = 0; > + > + p = strchr(trace_kprobe_symbol(tk), ':'); > + if (p) > + ret = validate_module_probe_symbol(module_name(mod), p++); > + if (!ret) > + ret = register_trace_kprobe(tk); > + return ret; > +} > + > /* Module notifier call back, checking event on the module */ > static int trace_kprobe_module_callback(struct notifier_block *nb, > unsigned long val, void *data) > @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct > notifier_block *nb, if (trace_kprobe_within_module(tk, mod)) { > /* Don't need to check busy - this should have gone. */ > __unregister_trace_kprobe(tk); > - ret = __register_trace_kprobe(tk); > + ret = register_module_trace_kprobe(mod, tk); > if (ret) > pr_warn("Failed to re-register probe %s on %s: %d\n", > trace_probe_name(&tk->tp), > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char > *name, unsigned long unused) return 0; > } > > -static unsigned int number_of_same_symbols(char *func_name) > +static unsigned int number_of_same_symbols(const char *mod, const char > *func_name) { > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > + if (!mod) > + kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); I may be missing something here or reviewing too quickly. Wouldn't this function return count to be 0 if func_name is only part of the module named mod? Indeed, if the function is not in kernel symbol, kallsyms_on_each_match_symbol() will not loop. And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding module will be skipped, so count_mob_symbols() would not be called. Hence, we would have 0 as count, which would lead to ENOENT later. > return ctx.count; > } > > +static int validate_module_probe_symbol(const char *modname, const char > *symbol) +{ > + unsigned int count = number_of_same_symbols(modname, symbol); > + > + if (count > 1) { > + /* > + * Users should use ADDR to remove the ambiguity of > + * using KSYM only. > + */ > + return -EADDRNOTAVAIL; > + } else if (count == 0) { > + /* > + * We can return ENOENT earlier than when register the > + * kprobe. > + */ > + return -ENOENT; > + } > + return 0; > +} > + > +static int validate_probe_symbol(char *symbol) > +{ > + char *mod = NULL, *p; > + int ret; > + > + p = strchr(symbol, ':'); > + if (p) { > + mod = symbol; > + symbol = p + 1; > + *p = '\0'; > + } > + ret = validate_module_probe_symbol(mod, symbol); > + if (p) > + *p = ':'; > + return ret; > +} > + > static int __trace_kprobe_create(int argc, const char *argv[]) > { > /* > @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) trace_probe_log_err(0, BAD_PROBE_ADDR); > goto parse_error; > } > + ret = validate_probe_symbol(symbol); > + if (ret) { > + if (ret == -EADDRNOTAVAIL) > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > + else > + trace_probe_log_err(0, BAD_PROBE_ADDR); > + goto parse_error; > + } > if (is_return) > ctx.flags |= TPARG_FL_RETURN; > ret = kprobe_on_func_entry(NULL, symbol, offset); > @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) } > } > > - if (symbol && !strchr(symbol, ':')) { > - unsigned int count; > - > - count = number_of_same_symbols(symbol); > - if (count > 1) { > - /* > - * Users should use ADDR to remove the ambiguity of > - * using KSYM only. > - */ > - trace_probe_log_err(0, NON_UNIQ_SYMBOL); > - ret = -EADDRNOTAVAIL; > - > - goto error; > - } else if (count == 0) { > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - trace_probe_log_err(0, BAD_PROBE_ADDR); > - ret = -ENOENT; > - > - goto error; > - } > - } > - > trace_probe_log_set_index(0); > if (event) { > ret = traceprobe_parse_event_name(&event, &group, gbuf, > @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, > unsigned long offs, char *event; > > if (func) { > - unsigned int count; > - > - count = number_of_same_symbols(func); > - if (count > 1) > - /* > - * Users should use addr to remove the ambiguity of > - * using func only. > - */ > - return ERR_PTR(-EADDRNOTAVAIL); > - else if (count == 0) > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - return ERR_PTR(-ENOENT); > + ret = validate_probe_symbol(func); > + if (ret) > + return ERR_PTR(ret); > } > > /* Best regards.
Hi, On Tue, 31 Oct 2023 23:24:43 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char > > *name, unsigned long unused) return 0; > > } > > > > -static unsigned int number_of_same_symbols(char *func_name) > > +static unsigned int number_of_same_symbols(const char *mod, const char > > *func_name) { > > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > + if (!mod) > > + kallsyms_on_each_match_symbol(count_symbols, func_name, > &ctx.count); > > > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); > > I may be missing something here or reviewing too quickly. > Wouldn't this function return count to be 0 if func_name is only part of the > module named mod? No, please read below. > Indeed, if the function is not in kernel symbol, > kallsyms_on_each_match_symbol() will not loop. > And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding > module will be skipped, so count_mob_symbols() would not be called. > Hence, we would have 0 as count, which would lead to ENOENT later. Would you mean the case func_name is on the specific module? If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on symbols in the module names 'mod'. int module_kallsyms_on_each_symbol(const char *modname, int (*fn)(void *, const char *, unsigned long), void *data) { struct module *mod; unsigned int i; int ret = 0; mutex_lock(&module_mutex); list_for_each_entry(mod, &modules, list) { struct mod_kallsyms *kallsyms; if (mod->state == MODULE_STATE_UNFORMED) continue; if (modname && strcmp(modname, mod->name)) continue; ... So with above change, 'if mod is not specified, search the symbols in kernel and all modules. If mod is sepecified, search the symbol on the specific module'. Thus, "if func_name is only part of the module named mod", the module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module correctly. Thank you, Thank you,
Hi! Le mercredi 1 novembre 2023, 01:15:09 EET Masami Hiramatsu a écrit : > Hi, > > On Tue, 31 Oct 2023 23:24:43 +0200 > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const > > > char > > > *name, unsigned long unused) return 0; > > > > > > } > > > > > > -static unsigned int number_of_same_symbols(char *func_name) > > > +static unsigned int number_of_same_symbols(const char *mod, const char > > > *func_name) { > > > > > > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > > > > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > > + if (!mod) > > > + kallsyms_on_each_match_symbol(count_symbols, func_name, > > > > &ctx.count); > > > > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > > > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); > > > > I may be missing something here or reviewing too quickly. > > Wouldn't this function return count to be 0 if func_name is only part of > > the module named mod? > > No, please read below. > > > Indeed, if the function is not in kernel symbol, > > kallsyms_on_each_match_symbol() will not loop. > > And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding > > module will be skipped, so count_mob_symbols() would not be called. > > Hence, we would have 0 as count, which would lead to ENOENT later. > > Would you mean the case func_name is on the specific module? > If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on > symbols in the module names 'mod'. > > int module_kallsyms_on_each_symbol(const char *modname, > int (*fn)(void *, const char *, unsigned > long), void *data) > { > struct module *mod; > unsigned int i; > int ret = 0; > > mutex_lock(&module_mutex); > list_for_each_entry(mod, &modules, list) { > struct mod_kallsyms *kallsyms; > > if (mod->state == MODULE_STATE_UNFORMED) > continue; > > if (modname && strcmp(modname, mod->name)) > continue; > ... > > So with above change, 'if mod is not specified, search the symbols in kernel > and all modules. If mod is sepecified, search the symbol on the specific > module'. > > Thus, "if func_name is only part of the module named mod", the > module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module > correctly. Sorry, I looked to quickly and forgot about the return value of strcmp()... From the code, everything seems OK! If I have some time, I will test it and potentially come back with a "Tested- by" tag but without any warranty. > Thank you, > > > Thank you, Best regards.
On Thu, 02 Nov 2023 14:57:12 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > Hi! > > Le mercredi 1 novembre 2023, 01:15:09 EET Masami Hiramatsu a écrit : > > Hi, > > > > On Tue, 31 Oct 2023 23:24:43 +0200 > > > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > > > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const > > > > char > > > > *name, unsigned long unused) return 0; > > > > > > > > } > > > > > > > > -static unsigned int number_of_same_symbols(char *func_name) > > > > +static unsigned int number_of_same_symbols(const char *mod, const char > > > > *func_name) { > > > > > > > > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > > > > > > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > > > + if (!mod) > > > > + kallsyms_on_each_match_symbol(count_symbols, func_name, > > > > > > &ctx.count); > > > > > > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > > > > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); > > > > > > I may be missing something here or reviewing too quickly. > > > Wouldn't this function return count to be 0 if func_name is only part of > > > the module named mod? > > > > No, please read below. > > > > > Indeed, if the function is not in kernel symbol, > > > kallsyms_on_each_match_symbol() will not loop. > > > And, by giving mod to module_kallsyms_on_each_symbol(), the corresponding > > > module will be skipped, so count_mob_symbols() would not be called. > > > Hence, we would have 0 as count, which would lead to ENOENT later. > > > > Would you mean the case func_name is on the specific module? > > If 'mod' is specified, module_kallsyms_on_each_symbol() only loops on > > symbols in the module names 'mod'. > > > > int module_kallsyms_on_each_symbol(const char *modname, > > int (*fn)(void *, const char *, unsigned > > long), void *data) > > { > > struct module *mod; > > unsigned int i; > > int ret = 0; > > > > mutex_lock(&module_mutex); > > list_for_each_entry(mod, &modules, list) { > > struct mod_kallsyms *kallsyms; > > > > if (mod->state == MODULE_STATE_UNFORMED) > > continue; > > > > if (modname && strcmp(modname, mod->name)) > > continue; > > ... > > > > So with above change, 'if mod is not specified, search the symbols in kernel > > and all modules. If mod is sepecified, search the symbol on the specific > > module'. > > > > Thus, "if func_name is only part of the module named mod", the > > module_kallsyms_on_each_symbol() will count the 'func_name' in 'mod' module > > correctly. > > Sorry, I looked to quickly and forgot about the return value of strcmp()... No problem, strcmp() always traps us :) > > From the code, everything seems OK! > If I have some time, I will test it and potentially come back with a "Tested- > by" tag but without any warranty. Thank you! > > > Thank you, > > > > > > Thank you, > > Best regards. > >
Hi! Le dimanche 29 octobre 2023, 05:10:46 EET Masami Hiramatsu (Google) a écrit : > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Check the number of probe target symbols in the target module when > the module is loaded. If the probe is not on the unique name symbols > in the module, it will be rejected at that point. > > Note that the symbol which has a unique name in the target module, > it will be accepted even if there are same-name symbols in the > kernel or other modules, Sorry for the delay in my review, I just remember I should review your patch. I tested it on top of latest master and it return EINVAL instead of EADDRNOTAVAIL: # Behavior without the patch: root@vm-amd64:~# uname -r 6.6.0-15859-g89cdf9d55601 root@vm-amd64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/ kprobe_events -bash: echo: write error: Cannot assign requested address # With the patch: root@vm-amd64:~# uname -r 6.6.0-15860-gd6a7e0f39ec5 root@vm-amd64:~# echo 'p:myprobe name_show' > /sys/kernel/tracing/ kprobe_events -bash: echo: write error: Invalid argument I did not GDB to track down from where it comes (it is planned nonetheless), but is this behavior wanted? > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 112 > ++++++++++++++++++++++++++----------------- 1 file changed, 68 > insertions(+), 44 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index e834f149695b..90cf2219adb4 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe > *tk) return ret; > } > > +static int validate_module_probe_symbol(const char *modname, const char > *symbol); + > +static int register_module_trace_kprobe(struct module *mod, struct > trace_kprobe *tk) +{ > + const char *p; > + int ret = 0; > + > + p = strchr(trace_kprobe_symbol(tk), ':'); > + if (p) > + ret = validate_module_probe_symbol(module_name(mod), p++); > + if (!ret) > + ret = register_trace_kprobe(tk); > + return ret; > +} > + > /* Module notifier call back, checking event on the module */ > static int trace_kprobe_module_callback(struct notifier_block *nb, > unsigned long val, void *data) > @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct > notifier_block *nb, if (trace_kprobe_within_module(tk, mod)) { > /* Don't need to check busy - this should have gone. */ > __unregister_trace_kprobe(tk); > - ret = __register_trace_kprobe(tk); > + ret = register_module_trace_kprobe(mod, tk); > if (ret) > pr_warn("Failed to re-register probe %s on %s: %d\n", > trace_probe_name(&tk->tp), > @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char > *name, unsigned long unused) return 0; > } > > -static unsigned int number_of_same_symbols(char *func_name) > +static unsigned int number_of_same_symbols(const char *mod, const char > *func_name) { > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > + if (!mod) > + kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > > - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); > + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); > > return ctx.count; > } > > +static int validate_module_probe_symbol(const char *modname, const char > *symbol) +{ > + unsigned int count = number_of_same_symbols(modname, symbol); > + > + if (count > 1) { > + /* > + * Users should use ADDR to remove the ambiguity of > + * using KSYM only. > + */ > + return -EADDRNOTAVAIL; > + } else if (count == 0) { > + /* > + * We can return ENOENT earlier than when register the > + * kprobe. > + */ > + return -ENOENT; > + } > + return 0; > +} > + > +static int validate_probe_symbol(char *symbol) > +{ > + char *mod = NULL, *p; > + int ret; > + > + p = strchr(symbol, ':'); > + if (p) { > + mod = symbol; > + symbol = p + 1; > + *p = '\0'; > + } > + ret = validate_module_probe_symbol(mod, symbol); > + if (p) > + *p = ':'; > + return ret; > +} > + > static int __trace_kprobe_create(int argc, const char *argv[]) > { > /* > @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) trace_probe_log_err(0, BAD_PROBE_ADDR); > goto parse_error; > } > + ret = validate_probe_symbol(symbol); > + if (ret) { > + if (ret == -EADDRNOTAVAIL) > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > + else > + trace_probe_log_err(0, BAD_PROBE_ADDR); > + goto parse_error; > + } > if (is_return) > ctx.flags |= TPARG_FL_RETURN; > ret = kprobe_on_func_entry(NULL, symbol, offset); > @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) } > } > > - if (symbol && !strchr(symbol, ':')) { > - unsigned int count; > - > - count = number_of_same_symbols(symbol); > - if (count > 1) { > - /* > - * Users should use ADDR to remove the ambiguity of > - * using KSYM only. > - */ > - trace_probe_log_err(0, NON_UNIQ_SYMBOL); > - ret = -EADDRNOTAVAIL; > - > - goto error; > - } else if (count == 0) { > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - trace_probe_log_err(0, BAD_PROBE_ADDR); > - ret = -ENOENT; > - > - goto error; > - } > - } > - > trace_probe_log_set_index(0); > if (event) { > ret = traceprobe_parse_event_name(&event, &group, gbuf, > @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, > unsigned long offs, char *event; > > if (func) { > - unsigned int count; > - > - count = number_of_same_symbols(func); > - if (count > 1) > - /* > - * Users should use addr to remove the ambiguity of > - * using func only. > - */ > - return ERR_PTR(-EADDRNOTAVAIL); > - else if (count == 0) > - /* > - * We can return ENOENT earlier than when register the > - * kprobe. > - */ > - return ERR_PTR(-ENOENT); > + ret = validate_probe_symbol(func); > + if (ret) > + return ERR_PTR(ret); > } > > /* Best regards.
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index e834f149695b..90cf2219adb4 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -670,6 +670,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk) return ret; } +static int validate_module_probe_symbol(const char *modname, const char *symbol); + +static int register_module_trace_kprobe(struct module *mod, struct trace_kprobe *tk) +{ + const char *p; + int ret = 0; + + p = strchr(trace_kprobe_symbol(tk), ':'); + if (p) + ret = validate_module_probe_symbol(module_name(mod), p++); + if (!ret) + ret = register_trace_kprobe(tk); + return ret; +} + /* Module notifier call back, checking event on the module */ static int trace_kprobe_module_callback(struct notifier_block *nb, unsigned long val, void *data) @@ -688,7 +703,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb, if (trace_kprobe_within_module(tk, mod)) { /* Don't need to check busy - this should have gone. */ __unregister_trace_kprobe(tk); - ret = __register_trace_kprobe(tk); + ret = register_module_trace_kprobe(mod, tk); if (ret) pr_warn("Failed to re-register probe %s on %s: %d\n", trace_probe_name(&tk->tp), @@ -729,17 +744,55 @@ static int count_mod_symbols(void *data, const char *name, unsigned long unused) return 0; } -static unsigned int number_of_same_symbols(char *func_name) +static unsigned int number_of_same_symbols(const char *mod, const char *func_name) { struct sym_count_ctx ctx = { .count = 0, .name = func_name }; - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); + if (!mod) + kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx); + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); return ctx.count; } +static int validate_module_probe_symbol(const char *modname, const char *symbol) +{ + unsigned int count = number_of_same_symbols(modname, symbol); + + if (count > 1) { + /* + * Users should use ADDR to remove the ambiguity of + * using KSYM only. + */ + return -EADDRNOTAVAIL; + } else if (count == 0) { + /* + * We can return ENOENT earlier than when register the + * kprobe. + */ + return -ENOENT; + } + return 0; +} + +static int validate_probe_symbol(char *symbol) +{ + char *mod = NULL, *p; + int ret; + + p = strchr(symbol, ':'); + if (p) { + mod = symbol; + symbol = p + 1; + *p = '\0'; + } + ret = validate_module_probe_symbol(mod, symbol); + if (p) + *p = ':'; + return ret; +} + static int __trace_kprobe_create(int argc, const char *argv[]) { /* @@ -859,6 +912,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) trace_probe_log_err(0, BAD_PROBE_ADDR); goto parse_error; } + ret = validate_probe_symbol(symbol); + if (ret) { + if (ret == -EADDRNOTAVAIL) + trace_probe_log_err(0, NON_UNIQ_SYMBOL); + else + trace_probe_log_err(0, BAD_PROBE_ADDR); + goto parse_error; + } if (is_return) ctx.flags |= TPARG_FL_RETURN; ret = kprobe_on_func_entry(NULL, symbol, offset); @@ -871,31 +932,6 @@ static int __trace_kprobe_create(int argc, const char *argv[]) } } - if (symbol && !strchr(symbol, ':')) { - unsigned int count; - - count = number_of_same_symbols(symbol); - if (count > 1) { - /* - * Users should use ADDR to remove the ambiguity of - * using KSYM only. - */ - trace_probe_log_err(0, NON_UNIQ_SYMBOL); - ret = -EADDRNOTAVAIL; - - goto error; - } else if (count == 0) { - /* - * We can return ENOENT earlier than when register the - * kprobe. - */ - trace_probe_log_err(0, BAD_PROBE_ADDR); - ret = -ENOENT; - - goto error; - } - } - trace_probe_log_set_index(0); if (event) { ret = traceprobe_parse_event_name(&event, &group, gbuf, @@ -1767,21 +1803,9 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs, char *event; if (func) { - unsigned int count; - - count = number_of_same_symbols(func); - if (count > 1) - /* - * Users should use addr to remove the ambiguity of - * using func only. - */ - return ERR_PTR(-EADDRNOTAVAIL); - else if (count == 0) - /* - * We can return ENOENT earlier than when register the - * kprobe. - */ - return ERR_PTR(-ENOENT); + ret = validate_probe_symbol(func); + if (ret) + return ERR_PTR(ret); } /*