* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-29 3:10 [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads Masami Hiramatsu (Google)
@ 2023-10-30 13:59 ` Steven Rostedt
2023-10-31 16:56 ` Andrii Nakryiko
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2023-10-30 13:59 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: LKML, Linux trace kernel, Andrii Nakryiko, Francis Laniel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-29 3:10 [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads Masami Hiramatsu (Google)
2023-10-30 13:59 ` Steven Rostedt
@ 2023-10-31 16:56 ` Andrii Nakryiko
2023-10-31 21:24 ` Francis Laniel
2023-11-10 12:31 ` Francis Laniel
3 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2023-10-31 16:56 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: LKML, Linux trace kernel, Steven Rostedt, Andrii Nakryiko,
Francis Laniel
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);
> }
>
> /*
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-29 3:10 [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads Masami Hiramatsu (Google)
2023-10-30 13:59 ` Steven Rostedt
2023-10-31 16:56 ` Andrii Nakryiko
@ 2023-10-31 21:24 ` Francis Laniel
2023-10-31 23:15 ` Masami Hiramatsu
2023-11-10 12:31 ` Francis Laniel
3 siblings, 1 reply; 8+ messages in thread
From: Francis Laniel @ 2023-10-31 21:24 UTC (permalink / raw)
To: LKML, Linux trace kernel, Masami Hiramatsu (Google)
Cc: Masami Hiramatsu, Steven Rostedt, Andrii Nakryiko
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-31 21:24 ` Francis Laniel
@ 2023-10-31 23:15 ` Masami Hiramatsu
2023-11-02 12:57 ` Francis Laniel
0 siblings, 1 reply; 8+ messages in thread
From: Masami Hiramatsu @ 2023-10-31 23:15 UTC (permalink / raw)
To: Francis Laniel; +Cc: LKML, Linux trace kernel, Steven Rostedt, Andrii Nakryiko
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,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-31 23:15 ` Masami Hiramatsu
@ 2023-11-02 12:57 ` Francis Laniel
2023-11-03 3:27 ` Masami Hiramatsu
0 siblings, 1 reply; 8+ messages in thread
From: Francis Laniel @ 2023-11-02 12:57 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: LKML, Linux trace kernel, Steven Rostedt, Andrii Nakryiko
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-11-02 12:57 ` Francis Laniel
@ 2023-11-03 3:27 ` Masami Hiramatsu
0 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2023-11-03 3:27 UTC (permalink / raw)
To: Francis Laniel; +Cc: LKML, Linux trace kernel, Steven Rostedt, Andrii Nakryiko
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.
>
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads
2023-10-29 3:10 [PATCH for-next] tracing/kprobes: Add symbol counting check when module loads Masami Hiramatsu (Google)
` (2 preceding siblings ...)
2023-10-31 21:24 ` Francis Laniel
@ 2023-11-10 12:31 ` Francis Laniel
3 siblings, 0 replies; 8+ messages in thread
From: Francis Laniel @ 2023-11-10 12:31 UTC (permalink / raw)
To: LKML, Linux trace kernel, Masami Hiramatsu (Google)
Cc: Masami Hiramatsu, Steven Rostedt, Andrii Nakryiko
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.
^ permalink raw reply [flat|nested] 8+ messages in thread