| 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
| ||
|
Date: Thu, 06 May 2010 19:13:33 -0400
From: Masami Hiramatsu <mhiramat@...hat.com>
To: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
Mel Gorman <mel@....ul.ie>,
Randy Dunlap <rdunlap@...otime.net>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Roland McGrath <roland@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Ananth N Mavinakayanahalli <ananth@...ibm.com>,
Oleg Nesterov <oleg@...hat.com>,
Mark Wielaard <mjw@...hat.com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
LKML <linux-kernel@...r.kernel.org>,
Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
Frederic Weisbecker <fweisbec@...il.com>,
"Frank Ch. Eigler" <fche@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH v3 10/10] perf: perf interface for uprobes.
Srikar Dronamraju wrote:
> perf-events.patch
>
> From: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
>
> This patch enhances perf probe to accept pid and user vaddr.
> This patch provides very basic support for uprobes.
Good work! Thank you!
> TODO:
> Update perf-probes.txt.
> Global tracing.
Could you also rebase your patch on the latest tip tree? :)
(and remove die() too)
> Here is a terminal snapshot of placing, using and removing a probe on a
> process with pid 3329 (corresponding to zsh)
>
> [ Probing a function in the executable using function name ]
> -------------------------------------------------------------
> [root@...D]# perf probe -p 3329 zfree@zsh
Hmm, I'm not sure why we have to specify both of PID and exec name.
Is that possible to give a symbol as below? (omit exec name)
# perf probe -p 3329 zfree
> Added new event:
> probe:p_3329_zfree (on 0x446420)
Hmm, the event name should be simpler, as like as zfree_3329.
or, we might better make a new event group for each process, as like as
'probe_3329:zfree'
> You can now use it on all perf tools, such as:
>
> perf record -e probe:p_3329_zfree -a sleep 1
> [root@...D]# perf probe --list
> probe:p_3329_zfree (on 3329:0x0000000000446420)
> [root@...D]# cat /sys/kernel/debug/tracing/uprobe_events
> p:probe/p_3329_zfree 3329:0x0000000000446420
> [root@...D]# perf record -f -e probe:p_3329_zfree -a sleep 10
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.039 MB perf.data (~1716 samples) ]
> [root@...D]# perf probe -p 3329 --del probe:p_3329_zfree
> Remove event: probe:p_3329_zfree
> [root@...D]# perf report
> # Samples: 447
> #
> # Overhead Command Shared Object Symbol
> # ........ ............... ............. ......
> #
> 100.00% zsh zsh [.] zfree
>
>
> #
> # (For a higher level overview, try: perf report --sort comm,dso)
> #
>
> [ Probing a function + offset ]
> -------------------------------
Looks great! :D
[...]
>
> Signed-off-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
> ---
>
> tools/perf/builtin-probe.c | 34 +++++--
> tools/perf/builtin-top.c | 20 ----
> tools/perf/util/event.c | 20 ++++
> tools/perf/util/event.h | 1
> tools/perf/util/probe-event.c | 207 ++++++++++++++++++++++++++++++++--------
> tools/perf/util/probe-event.h | 9 +-
> tools/perf/util/probe-finder.h | 1
> tools/perf/util/symbol.c | 6 +
> 8 files changed, 224 insertions(+), 74 deletions(-)
Some comments on the patch.
[...]
>
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 152d6c9..9f867ad 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -55,6 +55,7 @@ static struct {
> bool force_add;
> bool show_lines;
> int nr_probe;
> + pid_t pid;
> struct probe_point probes[MAX_PROBES];
> struct strlist *dellist;
> struct map_groups kmap_groups;
> @@ -73,7 +74,7 @@ static void parse_probe_event(const char *str)
> die("Too many probes (> %d) are specified.", MAX_PROBES);
>
> /* Parse perf-probe event into probe_point */
> - parse_perf_probe_event(str, pp, &session.need_dwarf);
> + parse_perf_probe_event(str, pp, &session.need_dwarf, session.pid);
>
> pr_debug("%d arguments\n", pp->nr_args);
> }
> @@ -203,6 +204,8 @@ static const struct option options[] = {
> "FUNC[:RLN[+NUM|:RLN2]]|SRC:ALN[+NUM|:ALN2]",
> "Show source code lines.", opt_show_lines),
> #endif
> + OPT_INTEGER('p', "pid", &session.pid,
> + "specify a pid for a uprobes based probe"),
> OPT_END()
> };
>
> @@ -263,7 +266,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
> }
>
> #ifndef NO_DWARF_SUPPORT
> - if (session.show_lines) {
> + if (session.show_lines && !session.pid) {
> if (session.nr_probe != 0 || session.dellist) {
> pr_warning(" Error: Don't use --line with"
> " --add/--del.\n");
Hmm, if user gave --list with -p, what happened?
We need to check a bad combination and warn it.
> @@ -283,12 +286,19 @@ int cmd_probe(int argc, const char **argv, const char *prefix __used)
> #endif
>
> if (session.dellist) {
> - del_trace_kprobe_events(session.dellist);
> + if (session.pid)
> + del_trace_uprobe_events(session.dellist);
> + else
> + del_trace_kprobe_events(session.dellist);
> +
> strlist__delete(session.dellist);
> if (session.nr_probe == 0)
> return 0;
> }
>
> + if (session.pid)
> + goto end_dwarf;
> +
Again, it must be checked that the combination of -p option and dwarf requirement.
The latest code has perf_probe_event_need_dwarf(), so you can check it easier.
[...]
> @@ -112,6 +113,64 @@ static bool check_event_name(const char *name)
> return true;
> }
>
> +/*
> + * uprobe_events only accepts address:
> + * Convert function and any offset to address
> + */
> +static void convert_name_to_addr(struct probe_point *pp)
> +{
> + struct perf_session *session;
> + struct thread *thread;
> + struct symbol *sym;
> + struct map *map;
> + char *name = pp->file;
> + unsigned long long vaddr;
> +
> + /* check if user has specifed a virtual address */
> + vaddr = strtoul(pp->function, NULL, 0);
> + if (vaddr)
> + return;
> +
> + session = perf_session__new(NULL, O_WRONLY, false);
> + DIE_IF(session == NULL);
> + symbol_conf.sort_by_name = true;
> + symbol_conf.try_vmlinux_path = false;
> + if (symbol__init() < 0)
> + semantic_error("Cannot initialize symbols.");
> +
> + event__synthesize_thread(pp->upid, event__process, session);
> +
> + thread = perf_session__findnew(session, pp->upid);
> + DIE_IF(thread == NULL);
> +
> + if (!name)
> + /* Lets find the function in the executable. */
> + name = thread->comm;
> + DIE_IF(name == NULL);
> +
> + map = map_groups__find_by_name(&thread->mg, MAP__FUNCTION, name);
> + if (!map)
> + semantic_error("Cannot find appropriate DSO.");
> +
> + sym = map__find_symbol_by_name(map, pp->function, NULL);
> + if (!sym)
> + semantic_error("Cannot find appropriate Symbol.");
> +
> + if (map->start > sym->start)
> + vaddr = map->start;
> + vaddr += sym->start + pp->offset + map->pgoff;
> + pp->offset = 0;
> +
> + if (!pp->event)
> + pp->event = pp->function;
> + else
> + free(pp->function);
> + pp->function = zalloc(sizeof(char *) * 20);
> + if (!pp->function)
> + die("Failed to allocate memory by zalloc.");
> + e_snprintf(pp->function, 20, "0x%llx", vaddr);
> +}
Well, after rebasing, you'll need to remove die()s from here and
make it returns errors.
[...]
> @@ -383,7 +450,11 @@ int synthesize_trace_kprobe_event(struct probe_point *pp)
> pp->probes[0] = buf = zalloc(MAX_CMDLEN);
> if (!buf)
> die("Failed to allocate memory by zalloc.");
> - ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, pp->offset);
> + if (pp->offset)
> + ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function,
> + pp->offset);
> + else
> + ret = e_snprintf(buf, MAX_CMDLEN, "%s", pp->function);
> if (ret <= 0)
> goto error;
> len = ret;
Isn't it a cleanup ?
[...]
> @@ -595,22 +697,25 @@ static void get_new_event_name(char *buf, size_t len, const char *base,
> die("Too many events are on the same function.");
> }
>
> -void add_trace_kprobe_events(struct probe_point *probes, int nr_probes,
> - bool force_add)
> +static void add_trace_probe_events(int fd, struct probe_point *probes,
> + int nr_probes, bool force_add, pid_t pid)
> {
> - int i, j, fd;
> + int i, j;
> struct probe_point *pp;
> char buf[MAX_CMDLEN];
> + char tempbuf[MAX_CMDLEN];
> char event[64];
> struct strlist *namelist;
> bool allow_suffix;
>
> - fd = open_kprobe_events(O_RDWR, O_APPEND);
> /* Get current event names */
> namelist = get_perf_event_names(fd, false);
>
> for (j = 0; j < nr_probes; j++) {
> pp = probes + j;
> + pp->upid = pid;
> + if (pid)
> + snprintf(tempbuf, MAX_CMDLEN, "%d:", pid);
> if (!pp->event)
> pp->event = strdup(pp->function);
> if (!pp->group)
> @@ -621,12 +726,13 @@ void add_trace_kprobe_events(struct probe_point *probes, int nr_probes,
> for (i = 0; i < pp->found; i++) {
> /* Get an unused new event name */
> get_new_event_name(event, 64, pp->event, namelist,
> - allow_suffix);
> - snprintf(buf, MAX_CMDLEN, "%c:%s/%s %s\n",
> + allow_suffix, pid);
> + snprintf(buf, MAX_CMDLEN, "%c:%s/%s %s%s\n",
> pp->retprobe ? 'r' : 'p',
> pp->group, event,
> + pp->upid ? tempbuf : " ",
> pp->probes[i]);
> - write_trace_kprobe_event(fd, buf);
> + write_trace_probe_event(fd, buf);
> printf("Added new event:\n");
> /* Get the first parameter (probe-point) */
> sscanf(pp->probes[i], "%s", buf);
> @@ -650,7 +756,21 @@ void add_trace_kprobe_events(struct probe_point *probes, int nr_probes,
> close(fd);
> }
>
> -static void __del_trace_kprobe_event(int fd, struct str_node *ent)
> +void add_trace_kprobe_events(struct probe_point *probes, int nr_probes,
> + bool force_add)
> +{
> + int fd = open_kprobe_events(O_RDWR, O_APPEND);
> + add_trace_probe_events(fd, probes, nr_probes, force_add, 0);
> +}
> +
> +void add_trace_uprobe_events(struct probe_point *probes, int nr_probes,
> + bool force_add, pid_t pid)
> +{
> + int fd = open_uprobe_events(O_RDWR, O_APPEND);
> + add_trace_probe_events(fd, probes, nr_probes, force_add, pid);
> +}
Please close fd in the function which opened it.
> @@ -696,16 +816,13 @@ static void del_trace_kprobe_event(int fd, const char *group,
> pr_info("Info: event \"%s\" does not exist, could not remove it.\n", buf);
> }
>
> -void del_trace_kprobe_events(struct strlist *dellist)
> +static void del_trace_probe_events(int fd, struct strlist *dellist)
> {
> - int fd;
> const char *group, *event;
> char *p, *str;
> struct str_node *ent;
> struct strlist *namelist;
>
> - fd = open_kprobe_events(O_RDWR, O_APPEND);
> - /* Get current event names */
> namelist = get_perf_event_names(fd, true);
>
> strlist__for_each(ent, dellist) {
> @@ -723,13 +840,25 @@ void del_trace_kprobe_events(struct strlist *dellist)
> event = str;
> }
> pr_debug("Group: %s, Event: %s\n", group, event);
> - del_trace_kprobe_event(fd, group, event, namelist);
> + del_trace_probe_event(fd, group, event, namelist);
> free(str);
> }
> strlist__delete(namelist);
> close(fd);
> }
>
> +void del_trace_kprobe_events(struct strlist *dellist)
> +{
> + int fd = open_kprobe_events(O_RDWR, O_APPEND);
> + del_trace_probe_events(fd, dellist);
> +}
> +
> +void del_trace_uprobe_events(struct strlist *dellist)
> +{
> + int fd = open_uprobe_events(O_RDWR, O_APPEND);
> + del_trace_probe_events(fd, dellist);
> +}
Ditto.
[...]
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index c458c4a..7267050 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -958,12 +958,14 @@ static int dso__load_sym(struct dso *self, struct map *map, const char *name,
> nr_syms = shdr.sh_size / shdr.sh_entsize;
>
> memset(&sym, 0, sizeof(sym));
> - if (!self->kernel) {
> + if (self->kernel || symbol_conf.sort_by_name)
> + self->adjust_symbols = 0;
> + else {
> self->adjust_symbols = (ehdr.e_type == ET_EXEC ||
> elf_section_by_name(elf, &ehdr, &shdr,
> ".gnu.prelink_undo",
> NULL) != NULL);
> - } else self->adjust_symbols = 0;
> + }
>
> elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
> struct symbol *f;
This one changes the symbol.c, so I think you'd better make a
separate patch for this change.
Thank you,
--
Masami Hiramatsu
e-mail: mhiramat@...hat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists