On Tue, 9 Jul 2024 09:04:36 +0900 "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote: > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > Currently, kprobe event checks whether the target symbol name is unique > or not, so that it does not put a probe on an unexpected place. But this > skips the check if the target is on a module because the module may not > be loaded. > > To fix this issue, this patch checks the number of probe target symbols > in a 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@xxxxxxxxxx> Instead of version up, let me send a fix. Thanks, > --- > Changes in v4: > - Hide find_module() in try_module_get_by_name(). > - Add bpf ML. > Changes in v3: > - Update the patch description. > - Update for latest probe/for-next > Updated from last October post, which was dropped by test failure: > https://lore.kernel.org/linux-trace-kernel/169854904604.132316.12500381416261460174.stgit@devnote2/ > Changes in v2: > - Fix to skip checking uniqueness if the target module is not loaded. > - Fix register_module_trace_kprobe() to pass correct symbol name. > - Fix to call __register_trace_kprobe() from module callback. > --- > kernel/trace/trace_kprobe.c | 138 +++++++++++++++++++++++++++++-------------- > 1 file changed, 94 insertions(+), 44 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 7fd0f8576e4c..61a6da808203 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -678,6 +678,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk) > } > > #ifdef CONFIG_MODULES > +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 + 1); > + 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) > @@ -696,7 +711,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), > @@ -747,17 +762,81 @@ 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; > +} > + > +#ifdef CONFIG_MODULES > +/* Return NULL if the module is not loaded or under unloading. */ > +static struct module *try_module_get_by_name(const char *name) > +{ > + struct module *mod; > + > + rcu_read_lock_sched(); > + mod = find_module(name); > + if (mod && !try_module_get(mod)) > + mod = NULL; > + rcu_read_unlock_sched(); > + > + return mod; > +} > +#else > +#define try_module_get_by_name(name) (NULL) > +#endif > + > +static int validate_probe_symbol(char *symbol) > +{ > + struct module *mod = NULL; > + char *modname = NULL, *p; > + int ret = 0; > + > + p = strchr(symbol, ':'); > + if (p) { > + modname = symbol; > + symbol = p + 1; > + *p = '\0'; > + mod = try_module_get_by_name(modname); > + if (!mod) > + goto out; > + } > + > + ret = validate_module_probe_symbol(modname, symbol); > +out: > + if (p) > + *p = ':'; > + if (mod) > + module_put(mod); > + return ret; > +} > + > static int trace_kprobe_entry_handler(struct kretprobe_instance *ri, > struct pt_regs *regs); > > @@ -881,6 +960,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); > @@ -893,31 +980,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, > @@ -1835,21 +1897,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); > } > > /* > -- Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
