| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5704161.DvuYhMxLoT@pwmachine>
Date: Fri, 25 Aug 2023 14:34:49 +0200
From: Francis Laniel <flaniel@...ux.microsoft.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: linux-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...nel.org>,
linux-trace-kernel@...r.kernel.org,
Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols
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@...ux.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@...nel.org>
> > Signed-off-by: Francis Laniel <flaniel@...ux.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.
Powered by blists - more mailing lists