| 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: <87285133-a06b-da22-2737-4cc6bc0cdade@us.ibm.com>
Date: Mon, 10 Apr 2017 09:53:48 -0500
From: Paul Clarke <pc@...ibm.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
David Ahern <dsahern@...il.com>,
"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
On 04/10/2017 08:11 AM, Masami Hiramatsu wrote:
> On Wed, 5 Apr 2017 22:30:03 -0500
> Paul Clarke <pc@...ibm.com> wrote:
>> Symbol versioning, as in glibc, results in symbols being defined as:
>> <real symbol>@[@]<version>
>> (Note that "@@" identifies a default symbol, if the symbol name
>> is repeated.)
>>
>> perf is currently unable to deal with this, and is unable to create
>> user probes at such symbols:
>> --
>> $ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
>> 0000000000008d30 t __pthread_create_2_1
>> 0000000000008d30 T pthread_create@@GLIBC_2.17
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> probe-definition(0): pthread_create
>> symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
>> 0 arguments
>> Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
>> Try to find probe point from debuginfo.
>> Probe point 'pthread_create' not found.
>> Error: Failed to add events. Reason: No such file or directory (Code: -2)
>> --
>>
>> One is not able to specify the fully versioned symbol, either, due to
>> syntactic conflicts with other uses of "@" by perf:
>> --
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
>> probe-definition(0): pthread_create@@GLIBC_2.17
>> Semantic error :SRC@SRC is not allowed.
>> 0 arguments
>> Error: Command Parse Error. Reason: Invalid argument (Code: -22)
>> --
>>
>> This patch ignores the versioning tag for default symbols, thus
>> allowing probes to be created for these symbols:
>> --
>> $ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> Added new event:
>> probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
>>
>> You can now use it in all perf tools, such as:
>>
>> perf record -e probe_libpthread:pthread_create -aR sleep 1
>>
>> $ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
>> $ /usr/bin/sudo ./perf script
>> test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
>> test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
>> $ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
>> Removed event: probe_libpthread:pthread_create
>> --
>>
>> Signed-off-by: Paul A. Clarke <pc@...ibm.com>
>> ---
>> v4:
>> - rebased to acme/perf/core
>> - moved changes from "map" namespace to "symbol" namespace,
>> - rewrote symbol__compare (now *match) to avoid need for strdup
>> - new symbol__match_symbol_name to support versioned symbols, ignoring default
>> tags
>> - new arch__compare_symbol_names_n to map to strncmp
>> - dso__find_symbol_by_name: now tries exact match (as before), then tries
>> also adding symbols tagged as default (@@)
>> - symbols__find_by_name: new parameter to support finding with or without default
>> tags
>> - does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
>>
>> v3:
>> - code style fixes per David Ahern
>>
>> v2:
>> - move logic from arch__compare_symbol_names to new map__compare_symbol_names
>> - call through from map__compare_symbol_names to arch__compare_symbol_names
>> - redirect uses of arch__compare_symbol_names
>> - send patch to LKML
>> tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
>> tools/perf/util/map.c | 5 ---
>> tools/perf/util/map.h | 5 ++-
>> tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
>> tools/perf/util/symbol.h | 9 +++++
>> 5 files changed, 73 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
>> index 39dbe51..0d40e17 100644
>> --- a/tools/perf/arch/powerpc/util/sym-handling.c
>> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
>> @@ -52,6 +52,18 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
>>
>> return strcmp(namea, nameb);
>> }
>> +
>> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n)
>> +{
>> + /* Skip over initial dot */
>> + if (*namea == '.')
>> + namea++;
>> + if (*nameb == '.')
>> + nameb++;
>> +
>> + return strncmp(namea, nameb, n);
>> +}
>> #endif
>>
>> #if defined(_CALL_ELF) && _CALL_ELF == 2
>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>> index c1870ac..f4d8272 100644
>> --- a/tools/perf/util/map.c
>> +++ b/tools/perf/util/map.c
>> @@ -325,11 +325,6 @@ int map__load(struct map *map)
>> return 0;
>> }
>>
>> -int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>> -{
>> - return strcmp(namea, nameb);
>> -}
>> -
>> struct symbol *map__find_symbol(struct map *map, u64 addr)
>> {
>> if (map__load(map) < 0)
>> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
>> index c8a5a64..325bbc8 100644
>> --- a/tools/perf/util/map.h
>> +++ b/tools/perf/util/map.h
>> @@ -130,13 +130,14 @@ struct thread;
>> */
>> #define __map__for_each_symbol_by_name(map, sym_name, pos) \
>> for (pos = map__find_symbol_by_name(map, sym_name); \
>> - pos && arch__compare_symbol_names(pos->name, sym_name) == 0; \
>> + pos && \
>> + symbol__match_symbol_name(pos->name, sym_name, \
>> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY) == 0; \
>> pos = symbol__next_by_name(pos))
>>
>> #define map__for_each_symbol_by_name(map, sym_name, pos) \
>> __map__for_each_symbol_by_name(map, sym_name, (pos))
>>
>> -int arch__compare_symbol_names(const char *namea, const char *nameb);
>> void map__init(struct map *map, enum map_type type,
>> u64 start, u64 end, u64 pgoff, struct dso *dso);
>> struct map *map__new(struct machine *machine, u64 start, u64 len,
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 9b4d8ba..c29440d 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
>> return tail - str;
>> }
>>
>> +int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>> +{
>> + return strcmp(namea, nameb);
>> +}
>> +
>> +int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n)
>> +{
>> + return strncmp(namea, nameb, n);
>> +}
>> +
>> int __weak arch__choose_best_symbol(struct symbol *syma,
>> struct symbol *symb __maybe_unused)
>> {
>> @@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
>> }
>> }
>>
>> +int symbol__match_symbol_name(const char *name, const char *str,
>> + int including_tags)
>
> Hmm, why wouldn't you use bool for including_tags?
This was to allow for the possibility of returning multiple matches in the future.
>> +{
>> + const char *versioning;
>> +
>> + if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
>> + (versioning = strstr(name,"@@"))) {
>> +
>> + unsigned int len = strlen(str);
>> + if (len < versioning - name)
>> + len = versioning - name;
>> +
>> + return arch__compare_symbol_names_n(name, str, len);
>> + } else
>> + return arch__compare_symbol_names(name, str);
>> +}
>> +
>> static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> - const char *name)
>> + const char *name,
>> + unsigned int including_tags)
>
> And here, you are using "unsigned int".
That's my mistake.
>> {
>> struct rb_node *n;
>> struct symbol_name_rb_node *s = NULL;
>> @@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> int cmp;
>>
>> s = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> - cmp = arch__compare_symbol_names(name, s->sym.name);
>> + cmp = symbol__match_symbol_name(s->sym.name, name,
>> + including_tags);
>>
>> - if (cmp < 0)
>> + if (cmp > 0)
>> n = n->rb_left;
>> - else if (cmp > 0)
>> + else if (cmp < 0)
>> n = n->rb_right;
>> else
>> break;
>> @@ -424,16 +454,17 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> if (n == NULL)
>> return NULL;
>>
>> - /* return first symbol that has same name (if any) */
>> - for (n = rb_prev(n); n; n = rb_prev(n)) {
>> - struct symbol_name_rb_node *tmp;
>> + if (including_tags != SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY)
>> + /* return first symbol that has same name (if any) */
>> + for (n = rb_prev(n); n; n = rb_prev(n)) {
>> + struct symbol_name_rb_node *tmp;
>>
>> - tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> - if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
>> - break;
>> + tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> + if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
>> + break;
>>
>> - s = tmp;
>> - }
>> + s = tmp;
>> + }
>>
>> return &s->sym;
>> }
>> @@ -500,7 +531,12 @@ struct symbol *symbol__next_by_name(struct symbol *sym)
>> struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>> const char *name)
>> {
>> - return symbols__find_by_name(&dso->symbol_names[type], name);
>> + struct symbol *s = symbols__find_by_name(&dso->symbol_names[type], name,
>> + SYMBOLS__TAG_INCLUDE_NONE);
>> + if (!s)
>> + s = symbols__find_by_name(&dso->symbol_names[type], name,
>> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY);
>> + return s;
>> }
>>
>> void dso__sort_by_name(struct dso *dso, enum map_type type)
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 5245d2f..bde2100 100644
>> --- a/tools/perf/util/symbol.h
>> +++ b/tools/perf/util/symbol.h
>> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
>> #define SYMBOL_A 0
>> #define SYMBOL_B 1
>>
>> +int arch__compare_symbol_names(const char *namea, const char *nameb);
>> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n);
>> int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
>>
>> +#define SYMBOLS__TAG_INCLUDE_NONE 0
>> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
>
> Hmm, even if you like to pass an obvious flag, these should be
> defined by enum, and please choose simpler and clear flag name.
>
> E.g.
> enum {
> SYMBOLS__COMP_WITH_TAG,
> SYMBOLS__COMP_WITHOUT_TAG,
> };
Earlier in this thread, we discussed supporting non-default tagged symbols (those tagged with "@version"). To allow for that, I was thinking of these three cases:
- SYMBOLS__TAG_INCLUDE_NONE: ignore all tagged symbols (the current behavior)
- SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY: match tagged symbols when tagged as default ("@@"; added with this patch)
- SYMBOLS__TAG_INCLUDE_ALL: match all tagged symbols ("@" and "@@"; future)
For the names of the enums, I chose "SYMBOLS__TAG..." to put it first in the SYMBOLS namespace, and second in a "TAG" sub-namespace.
I'm happy to fix the discrepancy with signed/unsigned by changing to enum.
Let me know what enum names are preferred, given the above discussion, and I'll post a new patch.
Thanks for the review!
PC
Powered by blists - more mailing lists