| 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: <13333844.uLZWGnKmhe@pwmachine>
Date: Thu, 24 Aug 2023 18:09:59 +0200
From: Francis Laniel <flaniel@...ux.microsoft.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC PATCH v2 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols
Le jeudi 24 août 2023, 16:47:21 CEST Masami Hiramatsu a écrit :
> On Thu, 24 Aug 2023 16:31:13 +0200
>
> Francis Laniel <flaniel@...ux.microsoft.com> wrote:
> > Hi.
> >
> > Le jeudi 24 août 2023, 15:02:27 CEST Masami Hiramatsu a écrit :
> > > On Thu, 24 Aug 2023 12:37:34 +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
> > > > call, because the effectively called one has no kprobes.
> > > >
> > > > So, this commit returns EADDRNOTAVAIL when func matches several
> > > > symbols.
> > > > This way, user needs to use addr to remove the ambiguity.
> > >
> > > Thanks for update the patch. I have some comments there.
> > >
> > > > Suggested-by: Masami Hiramatsu <mhiramat@...nel.org>
> > > > Signed-off-by: Francis Laniel <flaniel@...ux.microsoft.com>
> > > > Link:
> > > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@k
> > > > ern
> > > > el.org/ ---
> > > >
> > > > kernel/trace/trace_kprobe.c | 42
> > > > +++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > index 23dba01831f7..0c8dd6ba650b 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 func_name_several_symbols(char *func_name)
> > >
> > > If this returns boolean, please use 'bool' for return type.
> > > Also, I think 'func_name_is_unique()' is more natural.
> >
> > This name sounds better but it means it will check count == 1.
> > I am fine with it, but please see my below comment.
> >
> > > > +{
> > > > + unsigned int count;
> > > > +
> > > > + count = 0;
> > > > + kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> > > > +
> > > > + return count > 1;
> > > > +}
> > > > +
> > > >
> > > > static int __trace_kprobe_create(int argc, const char *argv[])
> > > > {
> > > >
> > > > /*
> > > >
> > > > @@ -836,6 +855,18 @@ static int __trace_kprobe_create(int argc, const
> > > > char
> > > > *argv[])>
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > + /*
> > > > + * If user specifies KSYM, we check it does not correspond to
> > > > several
> > > > + * symbols.
> > > > + * If this is the case, we return EADDRNOTAVAIL to indicate the user
> > > > + * he/she should use ADDR rather than KSYM to remove the ambiguity.
> > > > + */
> > > > + if (symbol && func_name_several_symbols(symbol)) {
> > >
> > > Then, here will be
> > >
> > > if (symbol && !func_name_is_unique(symbol)) {
> >
> > I am fine with the above, but it means if users gives an unknown symbol,
> > we
> > will return EADDRNOTAVAIL instead of currently returning ENOENT.
> > Is it OK?
>
> Ah, good catch! Hm, then what about 'int number_of_same_symbols()' ?
>
>
> if (symbol) {
> num = number_of_same_symbols(symbol);
> if (num > 1)
> return -EADDRNOTAVAIL;
> else if (num == 0)
> return -ENOENT;
> }
Done in the v3 :D!
> Thank you,
>
> > > > + ret = -EADDRNOTAVAIL;
> > > > +
> > > > + goto error;
> > > > + }
> > > > +
> > > >
> > > > trace_probe_log_set_index(0);
> > > > if (event) {
> > > >
> > > > ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > > >
> > > > @@ -1699,6 +1730,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 +1741,16 @@ create_local_trace_kprobe(char *func, void
> > > > *addr,
> > > > unsigned long offs,>
> > > >
> > > > int ret;
> > > > char *event;
> > > >
> > > > + /*
> > > > + * If user specifies func, we check that function name does not
> > > > + * correspond to several symbols.
> > > > + * If this is the case, we return EADDRNOTAVAIL to indicate the user
> > > > + * he/she should use addr and offs rather than func to remove the
> > > > + * ambiguity.
> > > > + */
> > > > + if (func && func_name_several_symbols(func))
> > >
> > > Ditto.
> > >
> > > Thanks!
> > >
> > > > + return ERR_PTR(-EADDRNOTAVAIL);
> > > > +
> > > >
> > > > /*
> > > >
> > > > * 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