| Message ID | 20230824160859.66113-2-flaniel@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Return EADDRNOTAVAIL when func matches several symbols during kprobe creation | expand |
On Thu, 24 Aug 2023 18:08:59 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > Previously to this commit, if func matches several symbols, a kprobe, being > either sysfs or PMU, would only be installed for the first matching address. > This could lead to some misunderstanding when some BPF code was never called > because it was attached to a function which was indeed not called, because > the effectively called one has no kprobes attached. > > So, this commit returns EADDRNOTAVAIL when func matches several symbols. > This way, user needs to use address to remove the ambiguity. > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> > Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/ Looks good to me! Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you! > --- > kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 23dba01831f7..2f393739e8cf 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = { > .priority = 1 /* Invoked after kprobe module callback */ > }; > > +static int count_symbols(void *data, unsigned long unused) > +{ > + unsigned int *count = data; > + > + (*count)++; > + > + return 0; > +} > + > +static unsigned int number_of_same_symbols(char *func_name) > +{ > + unsigned int count; > + > + count = 0; > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); > + > + return count; > +} > + > static int __trace_kprobe_create(int argc, const char *argv[]) > { > /* > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > } > } > > + if (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. > + */ > + ret = -EADDRNOTAVAIL; > + > + goto error; > + } else if (count == 0) { > + /* > + * We can return ENOENT earlier than when register the > + * kprobe. > + */ > + ret = -ENOENT; > + > + goto error; > + } > + } > + > trace_probe_log_set_index(0); > if (event) { > ret = traceprobe_parse_event_name(&event, &group, gbuf, > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk) > } > > #ifdef CONFIG_PERF_EVENTS > + > /* create a trace_kprobe, but don't add it to global lists */ > struct trace_event_call * > create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > int ret; > 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); > + } > + > /* > * local trace_kprobes are not added to dyn_event, so they are never > * searched in find_trace_kprobe(). Therefore, there is no concern of > -- > 2.34.1 >
On Thu, 24 Aug 2023 18:08:59 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > Previously to this commit, if func matches several symbols, a kprobe, being > either sysfs or PMU, would only be installed for the first matching address. > This could lead to some misunderstanding when some BPF code was never called > because it was attached to a function which was indeed not called, because > the effectively called one has no kprobes attached. > > So, this commit returns EADDRNOTAVAIL when func matches several symbols. > This way, user needs to use address to remove the ambiguity. > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> > Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/ > --- Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) fails. # tail 60-kprobe_module.tc-log.vsOHnF ... + : + : 'Add an event on a module function without specifying event name' + : + echo 'p trace_printk:trace_printk_irq_work' sh: write error: No such file or directory Ah, the function on non-exist module should be checked too. # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ + + printfwc '%s' -c 'p ' + pos=2 + printf+ '%s'tr 'p ^non_exist_func' -d ^ + command='p non_exist_func' + echo 'Test command: p non_exist_func' Test command: p non_exist_func + echo + grep 'trace_kprobe: error:' -A 3 error_log Also, this doesn't leave a syntax error message. So, the below changes are needed. diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 8ab46a2a446d..1e57bc896952 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char *argv[]) } } - if (symbol) { + if (symbol && !strchr(symbol, ':')) { unsigned int count; count = number_of_same_symbols(symbol); @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char *argv[]) * Users should use ADDR to remove the ambiguity of * using KSYM only. */ + trace_probe_log_err(0, NON_UNIQ_SYMBOL); ret = -EADDRNOTAVAIL; goto error; @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char *argv[]) * We can return ENOENT earlier than when register the * kprobe. */ + trace_probe_log_err(0, BAD_PROBE_ADDR); ret = -ENOENT; goto error; diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h index 7f929482e8d4..a4f478448eef 100644 --- a/kernel/trace/trace_probe.h +++ b/kernel/trace/trace_probe.h @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive number"), \ C(MAXACT_TOO_BIG, "Maxactive is too big"), \ C(BAD_PROBE_ADDR, "Invalid probed address or symbol"), \ + C(NON_UNIQ_SYMBOL, "The symbol is not unique"), \ C(BAD_RETPROBE, "Retprobe address must be an function entry"), \ C(NO_TRACEPOINT, "Tracepoint is not found"), \ C(BAD_ADDR_SUFFIX, "Invalid probed address suffix"), \ Thank you, > kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 23dba01831f7..2f393739e8cf 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = { > .priority = 1 /* Invoked after kprobe module callback */ > }; > > +static int count_symbols(void *data, unsigned long unused) > +{ > + unsigned int *count = data; > + > + (*count)++; > + > + return 0; > +} > + > +static unsigned int number_of_same_symbols(char *func_name) > +{ > + unsigned int count; > + > + count = 0; > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); > + > + return count; > +} > + > static int __trace_kprobe_create(int argc, const char *argv[]) > { > /* > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > } > } > > + if (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. > + */ > + ret = -EADDRNOTAVAIL; > + > + goto error; > + } else if (count == 0) { > + /* > + * We can return ENOENT earlier than when register the > + * kprobe. > + */ > + ret = -ENOENT; > + > + goto error; > + } > + } > + > trace_probe_log_set_index(0); > if (event) { > ret = traceprobe_parse_event_name(&event, &group, gbuf, > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk) > } > > #ifdef CONFIG_PERF_EVENTS > + > /* create a trace_kprobe, but don't add it to global lists */ > struct trace_event_call * > create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > int ret; > 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); > + } > + > /* > * local trace_kprobes are not added to dyn_event, so they are never > * searched in find_trace_kprobe(). Therefore, there is no concern of > -- > 2.34.1 >
Hi. Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit : > On Thu, 24 Aug 2023 18:08:59 +0200 > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > Previously to this commit, if func matches several symbols, a kprobe, > > being > > either sysfs or PMU, would only be installed for the first matching > > address. This could lead to some misunderstanding when some BPF code was > > never called because it was attached to a function which was indeed not > > called, because the effectively called one has no kprobes attached. > > > > So, this commit returns EADDRNOTAVAIL when func matches several symbols. > > This way, user needs to use address to remove the ambiguity. > > > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> > > Link: > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern > > el.org/ --- > > Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) > fails. > > # tail 60-kprobe_module.tc-log.vsOHnF > ... > + : > + : 'Add an event on a module function without specifying event name' > + : > + echo 'p trace_printk:trace_printk_irq_work' > sh: write error: No such file or directory > > Ah, the function on non-exist module should be checked too. > > # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ > + + printfwc '%s' -c > 'p ' > + pos=2 > + printf+ '%s'tr 'p ^non_exist_func' > -d ^ > + command='p non_exist_func' > + echo 'Test command: p non_exist_func' > Test command: p non_exist_func > + echo > + grep 'trace_kprobe: error:' -A 3 error_log > > Also, this doesn't leave a syntax error message. > > So, the below changes are needed. Excellent catch! Thank you, I will apply this patch and send v4 right after. Regarding test, do you think I can add a test for the EADDRNOTAVAIL case? Maybe it should go inside LTP? As this would need having a kernel compiled with a name pointing to several symbols? Also, should some man pages somewhere be updated to reflect the case kprobe can return EADDRNOTAVAIL? > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 8ab46a2a446d..1e57bc896952 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) } > } > > - if (symbol) { > + if (symbol && !strchr(symbol, ':')) { > unsigned int count; > > count = number_of_same_symbols(symbol); > @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) * Users should use ADDR to remove the ambiguity of > * using KSYM only. > */ > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > ret = -EADDRNOTAVAIL; > > goto error; > @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char > *argv[]) * We can return ENOENT earlier than when register the > * kprobe. > */ > + trace_probe_log_err(0, BAD_PROBE_ADDR); > ret = -ENOENT; > > goto error; > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > index 7f929482e8d4..a4f478448eef 100644 > --- a/kernel/trace/trace_probe.h > +++ b/kernel/trace/trace_probe.h > @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct > trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive > number"), \ > C(MAXACT_TOO_BIG, "Maxactive is too big"), \ > C(BAD_PROBE_ADDR, "Invalid probed address or symbol"), \ > + C(NON_UNIQ_SYMBOL, "The symbol is not unique"), \ > C(BAD_RETPROBE, "Retprobe address must be an function entry"), \ > C(NO_TRACEPOINT, "Tracepoint is not found"), \ > C(BAD_ADDR_SUFFIX, "Invalid probed address suffix"), \ > > Thank you, > > > kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 23dba01831f7..2f393739e8cf 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = > > {> > > .priority = 1 /* Invoked after kprobe module callback */ > > > > }; > > > > +static int count_symbols(void *data, unsigned long unused) > > +{ > > + unsigned int *count = data; > > + > > + (*count)++; > > + > > + return 0; > > +} > > + > > +static unsigned int number_of_same_symbols(char *func_name) > > +{ > > + unsigned int count; > > + > > + count = 0; > > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); > > + > > + return count; > > +} > > + > > > > static int __trace_kprobe_create(int argc, const char *argv[]) > > { > > > > /* > > > > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char > > *argv[])> > > } > > > > } > > > > + if (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. > > + */ > > > > > > > > + ret = -EADDRNOTAVAIL; > > + > > + goto error; > > + } else if (count == 0) { > > + /* > > + * We can return ENOENT earlier than when register the > > + * kprobe. > > + */ > > + ret = -ENOENT; > > + > > + goto error; > > + } > > + } > > + > > > > trace_probe_log_set_index(0); > > if (event) { > > > > ret = traceprobe_parse_event_name(&event, &group, gbuf, > > > > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct > > trace_kprobe *tk)> > > } > > > > #ifdef CONFIG_PERF_EVENTS > > > > + > > > > /* create a trace_kprobe, but don't add it to global lists */ > > struct trace_event_call * > > create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > > > > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, > > unsigned long offs,> > > int ret; > > 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); > > + } > > + > > > > /* > > > > * local trace_kprobes are not added to dyn_event, so they are never > > * searched in find_trace_kprobe(). Therefore, there is no concern of Best regards.
On Fri, 25 Aug 2023 14:34:49 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > Hi. > > Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit : > > On Thu, 24 Aug 2023 18:08:59 +0200 > > > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > > Previously to this commit, if func matches several symbols, a kprobe, > > > being > > > either sysfs or PMU, would only be installed for the first matching > > > address. This could lead to some misunderstanding when some BPF code was > > > never called because it was attached to a function which was indeed not > > > called, because the effectively called one has no kprobes attached. > > > > > > So, this commit returns EADDRNOTAVAIL when func matches several symbols. > > > This way, user needs to use address to remove the ambiguity. > > > > > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > > > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> > > > Link: > > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern > > > el.org/ --- > > > > Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) > > fails. > > > > # tail 60-kprobe_module.tc-log.vsOHnF > > ... > > + : > > + : 'Add an event on a module function without specifying event name' > > + : > > + echo 'p trace_printk:trace_printk_irq_work' > > sh: write error: No such file or directory > > > > Ah, the function on non-exist module should be checked too. > > > > # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ > > + + printfwc '%s' -c > > 'p ' > > + pos=2 > > + printf+ '%s'tr 'p ^non_exist_func' > > -d ^ > > + command='p non_exist_func' > > + echo 'Test command: p non_exist_func' > > Test command: p non_exist_func > > + echo > > + grep 'trace_kprobe: error:' -A 3 error_log > > > > Also, this doesn't leave a syntax error message. > > > > So, the below changes are needed. > > Excellent catch! Thank you, I will apply this patch and send v4 right after. > Regarding test, do you think I can add a test for the EADDRNOTAVAIL case? Hmm, in that case, you need to change something in tracefs/README so that we can identify the kernel has different behavior. Or we have to change this is a "Fix" for backporting. > Maybe it should go inside LTP? As this would need having a kernel compiled > with a name pointing to several symbols? For this tracing feature, I rather like to use tools/testing/selftests/ftrace to test it. And it is used on all stable kernel, that is why we need to add some change on tracefs/README or something. But I would like to wait for Alessandro's work. After his work, in this time we need to probe all the same-name symbols as your original patch does. This is because 1:n mapping can happen as Alessandro pointed in https://lore.kernel.org/all/CAPp5cGQsRdB0+KHR1wX2bDDdc5sTzSNPA417PNJb0ypmV=yS6w@mail.gmail.com/ But if his feature is configurable (and maybe so), we need to keep this version... We have many options. - this normal kallsyms: the same-name symbols should not be used. - enhanced kallsyms (if 1:n symbol has the same suffix): the same name symbols should be probed at once. - enhanced kallsyms (if 1:n symbol has different suffix): the same-name symbol must not exist. > > Also, should some man pages somewhere be updated to reflect the case kprobe can > return EADDRNOTAVAIL? No, it is a tracefs interface and we don't have man pages yet. Thank you, > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 8ab46a2a446d..1e57bc896952 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char > > *argv[]) } > > } > > > > - if (symbol) { > > + if (symbol && !strchr(symbol, ':')) { > > unsigned int count; > > > > count = number_of_same_symbols(symbol); > > @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char > > *argv[]) * Users should use ADDR to remove the ambiguity of > > * using KSYM only. > > */ > > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > > ret = -EADDRNOTAVAIL; > > > > goto error; > > @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char > > *argv[]) * We can return ENOENT earlier than when register the > > * kprobe. > > */ > > + trace_probe_log_err(0, BAD_PROBE_ADDR); > > ret = -ENOENT; > > > > goto error; > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > index 7f929482e8d4..a4f478448eef 100644 > > --- a/kernel/trace/trace_probe.h > > +++ b/kernel/trace/trace_probe.h > > @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct > > trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive > > number"), \ > > C(MAXACT_TOO_BIG, "Maxactive is too big"), \ > > C(BAD_PROBE_ADDR, "Invalid probed address or symbol"), \ > > + C(NON_UNIQ_SYMBOL, "The symbol is not unique"), \ > > C(BAD_RETPROBE, "Retprobe address must be an function > entry"), \ > > C(NO_TRACEPOINT, "Tracepoint is not found"), \ > > C(BAD_ADDR_SUFFIX, "Invalid probed address suffix"), \ > > > > Thank you, > > > > > kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > > index 23dba01831f7..2f393739e8cf 100644 > > > --- a/kernel/trace/trace_kprobe.c > > > +++ b/kernel/trace/trace_kprobe.c > > > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = > > > {> > > > .priority = 1 /* Invoked after kprobe module callback */ > > > > > > }; > > > > > > +static int count_symbols(void *data, unsigned long unused) > > > +{ > > > + unsigned int *count = data; > > > + > > > + (*count)++; > > > + > > > + return 0; > > > +} > > > + > > > +static unsigned int number_of_same_symbols(char *func_name) > > > +{ > > > + unsigned int count; > > > + > > > + count = 0; > > > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); > > > + > > > + return count; > > > +} > > > + > > > > > > static int __trace_kprobe_create(int argc, const char *argv[]) > > > { > > > > > > /* > > > > > > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char > > > *argv[])> > > > } > > > > > > } > > > > > > + if (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. > > > + */ > > > > > > > > > > > > + ret = -EADDRNOTAVAIL; > > > + > > > + goto error; > > > + } else if (count == 0) { > > > + /* > > > + * We can return ENOENT earlier than when register the > > > + * kprobe. > > > + */ > > > + ret = -ENOENT; > > > + > > > + goto error; > > > + } > > > + } > > > + > > > > > > trace_probe_log_set_index(0); > > > if (event) { > > > > > > ret = traceprobe_parse_event_name(&event, &group, gbuf, > > > > > > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct > > > trace_kprobe *tk)> > > > } > > > > > > #ifdef CONFIG_PERF_EVENTS > > > > > > + > > > > > > /* create a trace_kprobe, but don't add it to global lists */ > > > struct trace_event_call * > > > create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > > > > > > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, > > > unsigned long offs,> > > > int ret; > > > 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); > > > + } > > > + > > > > > > /* > > > > > > * local trace_kprobes are not added to dyn_event, so they are never > > > * searched in find_trace_kprobe(). Therefore, there is no concern of > > Best regards. > >
Le vendredi 25 août 2023, 15:13:21 CEST Masami Hiramatsu a écrit : > On Fri, 25 Aug 2023 14:34:49 +0200 > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > Hi. > > > > Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit : > > > On Thu, 24 Aug 2023 18:08:59 +0200 > > > > > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > > > Previously to this commit, if func matches several symbols, a kprobe, > > > > being > > > > either sysfs or PMU, would only be installed for the first matching > > > > address. This could lead to some misunderstanding when some BPF code > > > > was > > > > never called because it was attached to a function which was indeed > > > > not > > > > called, because the effectively called one has no kprobes attached. > > > > > > > > So, this commit returns EADDRNOTAVAIL when func matches several > > > > symbols. > > > > This way, user needs to use address to remove the ambiguity. > > > > > > > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > > > > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> > > > > Link: > > > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@k > > > > ern > > > > el.org/ --- > > > > > > Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) > > > fails. > > > > > > # tail 60-kprobe_module.tc-log.vsOHnF > > > > > > ... > > > + : > > > + : 'Add an event on a module function without specifying event name' > > > + : > > > + echo 'p trace_printk:trace_printk_irq_work' > > > sh: write error: No such file or directory > > > > > > Ah, the function on non-exist module should be checked too. > > > > > > # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ > > > + + printfwc '%s' -c > > > > > > 'p ' > > > > > > + pos=2 > > > + printf+ '%s'tr 'p ^non_exist_func' > > > > > > -d ^ > > > > > > + command='p non_exist_func' > > > + echo 'Test command: p non_exist_func' > > > Test command: p non_exist_func > > > + echo > > > + grep 'trace_kprobe: error:' -A 3 error_log > > > > > > Also, this doesn't leave a syntax error message. > > > > > > So, the below changes are needed. > > > > Excellent catch! Thank you, I will apply this patch and send v4 right > > after. Regarding test, do you think I can add a test for the > > EADDRNOTAVAIL case? > Hmm, in that case, you need to change something in tracefs/README so that > we can identify the kernel has different behavior. Or we have to change > this is a "Fix" for backporting. Oops, sorry I sent the v4 with a test but as a separated commit, so we can just ignore it for the moment. > > Maybe it should go inside LTP? As this would need having a kernel compiled > > with a name pointing to several symbols? > > For this tracing feature, I rather like to use > tools/testing/selftests/ftrace to test it. And it is used on all stable > kernel, that is why we need to add some change on tracefs/README or > something. > > But I would like to wait for Alessandro's work. After his work, in this time > we need to probe all the same-name symbols as your original patch does. > This is because 1:n mapping can happen as Alessandro pointed in > > https://lore.kernel.org/all/CAPp5cGQsRdB0+KHR1wX2bDDdc5sTzSNPA417PNJb0ypmV=y > S6w@mail.gmail.com/ > > But if his feature is configurable (and maybe so), we need to keep this > version... We have many options. > > - this normal kallsyms: the same-name symbols should not be used. > - enhanced kallsyms (if 1:n symbol has the same suffix): the same name > symbols should be probed at once. > - enhanced kallsyms (if 1:n symbol has different suffix): the same-name > symbol must not exist. I understand! In future case, we could still have a test and change its behavior (i.e. potentially skipping it) when KALLSYMS_ALIAS is set. > > Also, should some man pages somewhere be updated to reflect the case > > kprobe can return EADDRNOTAVAIL? > > No, it is a tracefs interface and we don't have man pages yet. I was more thinking to the PMU counterpart as it is created through perf_event_open()? > Thank you, > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > > index 8ab46a2a446d..1e57bc896952 100644 > > > --- a/kernel/trace/trace_kprobe.c > > > +++ b/kernel/trace/trace_kprobe.c > > > @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const > > > char > > > *argv[]) } > > > > > > } > > > > > > - if (symbol) { > > > + if (symbol && !strchr(symbol, ':')) { > > > > > > unsigned int count; > > > > > > count = number_of_same_symbols(symbol); > > > > > > @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const > > > char > > > *argv[]) * Users should use ADDR to remove the ambiguity of > > > > > > * using KSYM only. > > > */ > > > > > > + trace_probe_log_err(0, NON_UNIQ_SYMBOL); > > > > > > ret = -EADDRNOTAVAIL; > > > > > > goto error; > > > > > > @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const > > > char > > > *argv[]) * We can return ENOENT earlier than when register the > > > > > > * kprobe. > > > */ > > > > > > + trace_probe_log_err(0, BAD_PROBE_ADDR); > > > > > > ret = -ENOENT; > > > > > > goto error; > > > > > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > > index 7f929482e8d4..a4f478448eef 100644 > > > --- a/kernel/trace/trace_probe.h > > > +++ b/kernel/trace/trace_probe.h > > > @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct > > > trace_event_call *event_call, C(BAD_MAXACT, "Invalid maxactive > > > number"), \ > > > > > > C(MAXACT_TOO_BIG, "Maxactive is too big"), \ > > > C(BAD_PROBE_ADDR, "Invalid probed address or symbol"), \ > > > > > > + C(NON_UNIQ_SYMBOL, "The symbol is not unique"), \ > > > > > > C(BAD_RETPROBE, "Retprobe address must be an function > > > > entry"), \ > > > > > C(NO_TRACEPOINT, "Tracepoint is not found"), \ > > > C(BAD_ADDR_SUFFIX, "Invalid probed address suffix"), \ > > > > > > Thank you, > > > > > > > kernel/trace/trace_kprobe.c | 61 > > > > +++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 61 insertions(+) > > > > > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > > > index 23dba01831f7..2f393739e8cf 100644 > > > > --- a/kernel/trace/trace_kprobe.c > > > > +++ b/kernel/trace/trace_kprobe.c > > > > @@ -705,6 +705,25 @@ static struct notifier_block > > > > trace_kprobe_module_nb = > > > > {> > > > > > > > > .priority = 1 /* Invoked after kprobe module callback */ > > > > > > > > }; > > > > > > > > +static int count_symbols(void *data, unsigned long unused) > > > > +{ > > > > + unsigned int *count = data; > > > > + > > > > + (*count)++; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static unsigned int number_of_same_symbols(char *func_name) > > > > +{ > > > > + unsigned int count; > > > > + > > > > + count = 0; > > > > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); > > > > + > > > > + return count; > > > > +} > > > > + > > > > > > > > static int __trace_kprobe_create(int argc, const char *argv[]) > > > > { > > > > > > > > /* > > > > > > > > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const > > > > char > > > > *argv[])> > > > > > > > > } > > > > > > > > } > > > > > > > > + if (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. > > > > + */ > > > > > > > > > > > > > > > > + ret = -EADDRNOTAVAIL; > > > > + > > > > + goto error; > > > > + } else if (count == 0) { > > > > + /* > > > > + * We can return ENOENT earlier than when register the > > > > + * kprobe. > > > > + */ > > > > + ret = -ENOENT; > > > > + > > > > + goto error; > > > > + } > > > > + } > > > > + > > > > > > > > trace_probe_log_set_index(0); > > > > if (event) { > > > > > > > > ret = traceprobe_parse_event_name(&event, &group, gbuf, > > > > > > > > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct > > > > trace_kprobe *tk)> > > > > > > > > } > > > > > > > > #ifdef CONFIG_PERF_EVENTS > > > > > > > > + > > > > > > > > /* create a trace_kprobe, but don't add it to global lists */ > > > > struct trace_event_call * > > > > create_local_trace_kprobe(char *func, void *addr, unsigned long offs, > > > > > > > > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void > > > > *addr, > > > > unsigned long offs,> > > > > > > > > int ret; > > > > 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); > > > > + } > > > > + > > > > > > > > /* > > > > > > > > * local trace_kprobes are not added to dyn_event, so they are never > > > > * searched in find_trace_kprobe(). Therefore, there is no concern > > > > of > > > > Best regards.
On Fri, 25 Aug 2023 22:13:21 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > Excellent catch! Thank you, I will apply this patch and send v4 right after. > > Regarding test, do you think I can add a test for the EADDRNOTAVAIL case? > > Hmm, in that case, you need to change something in tracefs/README so that > we can identify the kernel has different behavior. Or we have to change > this is a "Fix" for backporting. I prefer this to be a Fix and backported. Thanks, -- Steve
Hi. Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit : > On Fri, 25 Aug 2023 22:13:21 +0900 > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > Excellent catch! Thank you, I will apply this patch and send v4 right > > > after. Regarding test, do you think I can add a test for the > > > EADDRNOTAVAIL case?> > > Hmm, in that case, you need to change something in tracefs/README so that > > we can identify the kernel has different behavior. Or we have to change > > this is a "Fix" for backporting. > > I prefer this to be a Fix and backported. This makes sense, I will send v5 to stable mailing list too! > Thanks, > > -- Steve Best regards.
Hi Francis, On Thu, 31 Aug 2023 09:14:55 +0200 Francis Laniel <flaniel@linux.microsoft.com> wrote: > Hi. > > Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit : > > On Fri, 25 Aug 2023 22:13:21 +0900 > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > Excellent catch! Thank you, I will apply this patch and send v4 right > > > > after. Regarding test, do you think I can add a test for the > > > > EADDRNOTAVAIL case?> > > > Hmm, in that case, you need to change something in tracefs/README so that > > > we can identify the kernel has different behavior. Or we have to change > > > this is a "Fix" for backporting. > > > > I prefer this to be a Fix and backported. > > This makes sense, I will send v5 to stable mailing list too! I missed this a while. did you send v5 ? I could not find in my mbox. Thank you, > > > Thanks, > > > > -- Steve > > Best regards. > >
Hi! Le mercredi 18 octobre 2023, 09:30:20 EEST Masami Hiramatsu a écrit : > Hi Francis, > > On Thu, 31 Aug 2023 09:14:55 +0200 > > Francis Laniel <flaniel@linux.microsoft.com> wrote: > > Hi. > > > > Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit : > > > On Fri, 25 Aug 2023 22:13:21 +0900 > > > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > Excellent catch! Thank you, I will apply this patch and send v4 > > > > > right > > > > > after. Regarding test, do you think I can add a test for the > > > > > EADDRNOTAVAIL case?> > > > > > > > > Hmm, in that case, you need to change something in tracefs/README so > > > > that > > > > we can identify the kernel has different behavior. Or we have to > > > > change > > > > this is a "Fix" for backporting. > > > > > > I prefer this to be a Fix and backported. > > > > This makes sense, I will send v5 to stable mailing list too! > > I missed this a while. did you send v5 ? I could not find in my mbox. Sorry, I took a bit of time before sending the v5 as I wanted to wait for Alessandro patchset to be merged first. As it seems more work is needed on his contribution I think we can go with this fix first. > Thank you, > > > > Thanks, > > > > > > -- Steve > > > > Best regards. Best regards.
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 23dba01831f7..2f393739e8cf 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = { .priority = 1 /* Invoked after kprobe module callback */ }; +static int count_symbols(void *data, unsigned long unused) +{ + unsigned int *count = data; + + (*count)++; + + return 0; +} + +static unsigned int number_of_same_symbols(char *func_name) +{ + unsigned int count; + + count = 0; + kallsyms_on_each_match_symbol(count_symbols, func_name, &count); + + return count; +} + static int __trace_kprobe_create(int argc, const char *argv[]) { /* @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[]) } } + if (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. + */ + ret = -EADDRNOTAVAIL; + + goto error; + } else if (count == 0) { + /* + * We can return ENOENT earlier than when register the + * kprobe. + */ + ret = -ENOENT; + + goto error; + } + } + trace_probe_log_set_index(0); if (event) { ret = traceprobe_parse_event_name(&event, &group, gbuf, @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk) } #ifdef CONFIG_PERF_EVENTS + /* create a trace_kprobe, but don't add it to global lists */ struct trace_event_call * create_local_trace_kprobe(char *func, void *addr, unsigned long offs, @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs, int ret; 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); + } + /* * local trace_kprobes are not added to dyn_event, so they are never * searched in find_trace_kprobe(). Therefore, there is no concern of
Previously to this commit, if func matches several symbols, a kprobe, being either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached. So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity. Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/ --- kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)