| 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: <20150330130940.GB24063@kernel.org>
Date: Mon, 30 Mar 2015 10:09:40 -0300
From: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: David Ahern <dsahern@...il.com>, Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Stephane Eranian <eranian@...gle.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] segfault in perf-top -- thread refcnt
Em Mon, Mar 30, 2015 at 12:22:20PM +0200, Jiri Olsa escreveu:
> On Mon, Mar 30, 2015 at 10:07:37AM +0200, Jiri Olsa wrote:
>
> SNIP
>
> > >
> > > 2 things:
> > > 1. let run for a long time. go about using the server. do lots of builds,
> > > etc. it takes time
> > >
> > > 2. use a box with a LOT of cpus (1024 in my case)
> > >
> > > Make sure ulimit is set to get the core.
> >
> > reproduced under 24 cpu box with kernel build (make -j25)
> > running on background.. will try to look closer
> >
> > perf: Segmentation fault
> > -------- backtrace --------
> > ./perf[0x4fd79b]
> > /lib64/libc.so.6(+0x358f0)[0x7f9cbff528f0]
> > ./perf(thread__put+0x5b)[0x4b1a7b]
> > ./perf(hists__delete_entries+0x70)[0x4c8670]
> > ./perf[0x436a88]
> > ./perf[0x4fa73d]
> > ./perf(perf_evlist__tui_browse_hists+0x97)[0x4fc437]
> > ./perf[0x4381d0]
> > /lib64/libpthread.so.0(+0x7ee5)[0x7f9cc1ff2ee5]
> > /lib64/libc.so.6(clone+0x6d)[0x7f9cc0011b8d]
> > [0x0]
>
> looks like race among __machine__findnew_thread and thread__put
> over the machine->threads rb_tree insert/removal
>
> is there a reason why thread__put does not erase itself from machine->threads?
Thread put should only remove stuff from the rbtree when the refcount
hits zero, and then, it should grab a lock, that is the buggy part, as
this lock isn't there, and remove the entry from the rbtree. Now I see
what Namhyung was talking about...
- Arnaldo
> I'm trying attached patch.. so far so gut ;-)
>
> jirka
>
>
> ---
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e335330..7e6abc7 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -30,6 +30,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
> dsos__init(&machine->kernel_dsos);
>
> machine->threads = RB_ROOT;
> + pthread_mutex_init(&machine->threads_lock, NULL);
> INIT_LIST_HEAD(&machine->dead_threads);
> machine->last_match = NULL;
>
> @@ -380,10 +381,13 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
> if (!create)
> return NULL;
>
> - th = thread__new(pid, tid);
> + th = thread__new(machine, pid, tid);
> if (th != NULL) {
> +
> + pthread_mutex_lock(&machine->threads_lock);
> rb_link_node(&th->rb_node, parent, p);
> rb_insert_color(&th->rb_node, &machine->threads);
> + pthread_mutex_unlock(&machine->threads_lock);
>
> /*
> * We have to initialize map_groups separately
> @@ -394,8 +398,10 @@ static struct thread *__machine__findnew_thread(struct machine *machine,
> * leader and that would screwed the rb tree.
> */
> if (thread__init_map_groups(th, machine)) {
> + pthread_mutex_lock(&machine->threads_lock);
> rb_erase(&th->rb_node, &machine->threads);
> thread__delete(th);
> + pthread_mutex_unlock(&machine->threads_lock);
> return NULL;
> }
> /*
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index e2faf3b..e3468d6 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -30,6 +30,7 @@ struct machine {
> bool comm_exec;
> char *root_dir;
> struct rb_root threads;
> + pthread_mutex_t threads_lock;
> struct list_head dead_threads;
> struct thread *last_match;
> struct vdso_info *vdso_info;
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 1c8fbc9..4592fc4 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -26,7 +26,7 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine)
> return thread->mg ? 0 : -1;
> }
>
> -struct thread *thread__new(pid_t pid, pid_t tid)
> +struct thread *thread__new(struct machine *machine, pid_t pid, pid_t tid)
> {
> char *comm_str;
> struct comm *comm;
> @@ -38,6 +38,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
> thread->ppid = -1;
> thread->cpu = -1;
> INIT_LIST_HEAD(&thread->comm_list);
> + thread->machine = machine;
>
> if (unwind__prepare_access(thread) < 0)
> goto err_thread;
> @@ -91,7 +92,14 @@ struct thread *thread__get(struct thread *thread)
> void thread__put(struct thread *thread)
> {
> if (thread && --thread->refcnt == 0) {
> + struct machine *machine = thread->machine;
> +
> list_del_init(&thread->node);
> +
> + pthread_mutex_lock(&machine->threads_lock);
> + rb_erase(&thread->rb_node, &machine->threads);
> + pthread_mutex_unlock(&machine->threads_lock);
> +
> thread__delete(thread);
> }
> }
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 9b8a54d..df6fb69 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -31,12 +31,13 @@ struct thread {
>
> void *priv;
> struct thread_stack *ts;
> + struct machine *machine;
> };
>
> struct machine;
> struct comm;
>
> -struct thread *thread__new(pid_t pid, pid_t tid);
> +struct thread *thread__new(struct machine *machine, pid_t pid, pid_t tid);
> int thread__init_map_groups(struct thread *thread, struct machine *machine);
> void thread__delete(struct thread *thread);
>
--
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