After thread is added to machine->threads[i].dead in
__machine__remove_thread, the machine->threads[i].dead is freed
when calling free(session) in perf_session__delete(). So it get a
Segmentation fault when accessing it in thread__put().
In this patch, we delay the perf_session__delete until all threads
have been deleted.
This can be reproduced by following steps:
ulimit -c unlimited
export MALLOC_MMAP_THRESHOLD_=0
perf sched record sleep 10
perf sched latency --sort max
Segmentation fault (core dumped)
Signed-off-by: Zhipeng Xie <[email protected]>
Signed-off-by: Wei Li <[email protected]>
---
tools/perf/builtin-sched.c | 63 ++++++++++++++++++++++++++------------
1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 275f2d92a7bf..8a4841fa124c 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1774,7 +1774,8 @@ static int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,
return 0;
}
-static int perf_sched__read_events(struct perf_sched *sched)
+static int __perf_sched__read_events(struct perf_sched *sched,
+ struct perf_session *session)
{
const struct perf_evsel_str_handler handlers[] = {
{ "sched:sched_switch", process_sched_switch_event, },
@@ -1783,30 +1784,17 @@ static int perf_sched__read_events(struct perf_sched *sched)
{ "sched:sched_wakeup_new", process_sched_wakeup_event, },
{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
};
- struct perf_session *session;
- struct perf_data data = {
- .path = input_name,
- .mode = PERF_DATA_MODE_READ,
- .force = sched->force,
- };
- int rc = -1;
-
- session = perf_session__new(&data, false, &sched->tool);
- if (session == NULL) {
- pr_debug("No Memory for session\n");
- return -1;
- }
symbol__init(&session->header.env);
if (perf_session__set_tracepoints_handlers(session, handlers))
- goto out_delete;
+ return -1;
if (perf_session__has_traces(session, "record -R")) {
int err = perf_session__process_events(session);
if (err) {
pr_err("Failed to process events, error %d", err);
- goto out_delete;
+ return -1;
}
sched->nr_events = session->evlist->stats.nr_events[0];
@@ -1814,9 +1802,28 @@ static int perf_sched__read_events(struct perf_sched *sched)
sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
}
- rc = 0;
-out_delete:
+ return 0;
+}
+
+static int perf_sched__read_events(struct perf_sched *sched)
+{
+ struct perf_session *session;
+ struct perf_data data = {
+ .path = input_name,
+ .mode = PERF_DATA_MODE_READ,
+ .force = sched->force,
+ };
+ int rc;
+
+ session = perf_session__new(&data, false, &sched->tool);
+ if (session == NULL) {
+ pr_debug("No Memory for session\n");
+ return -1;
+ }
+
+ rc = __perf_sched__read_events(sched, session);
perf_session__delete(session);
+
return rc;
}
@@ -3130,12 +3137,25 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
static int perf_sched__lat(struct perf_sched *sched)
{
+ struct perf_session *session;
+ struct perf_data data = {
+ .path = input_name,
+ .mode = PERF_DATA_MODE_READ,
+ .force = sched->force,
+ };
struct rb_node *next;
+ int rc = -1;
setup_pager();
- if (perf_sched__read_events(sched))
+ session = perf_session__new(&data, false, &sched->tool);
+ if (session == NULL) {
+ pr_debug("No Memory for session\n");
return -1;
+ }
+
+ if (__perf_sched__read_events(sched, session))
+ goto out_delete;
perf_sched__merge_lat(sched);
perf_sched__sort_lat(sched);
@@ -3164,7 +3184,10 @@ static int perf_sched__lat(struct perf_sched *sched)
print_bad_events(sched);
printf("\n");
- return 0;
+ rc = 0;
+out_delete:
+ perf_session__delete(session);
+ return rc;
}
static int setup_map_cpus(struct perf_sched *sched)
--
2.17.1
On Wed, May 08, 2019 at 10:36:48PM +0800, Wei Li wrote:
> After thread is added to machine->threads[i].dead in
> __machine__remove_thread, the machine->threads[i].dead is freed
> when calling free(session) in perf_session__delete(). So it get a
> Segmentation fault when accessing it in thread__put().
>
> In this patch, we delay the perf_session__delete until all threads
> have been deleted.
>
> This can be reproduced by following steps:
> ulimit -c unlimited
> export MALLOC_MMAP_THRESHOLD_=0
> perf sched record sleep 10
> perf sched latency --sort max
> Segmentation fault (core dumped)
>
> Signed-off-by: Zhipeng Xie <[email protected]>
> Signed-off-by: Wei Li <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
Thahks,
Namhyung
> ---
> tools/perf/builtin-sched.c | 63 ++++++++++++++++++++++++++------------
> 1 file changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 275f2d92a7bf..8a4841fa124c 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1774,7 +1774,8 @@ static int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,
> return 0;
> }
>
> -static int perf_sched__read_events(struct perf_sched *sched)
> +static int __perf_sched__read_events(struct perf_sched *sched,
> + struct perf_session *session)
> {
> const struct perf_evsel_str_handler handlers[] = {
> { "sched:sched_switch", process_sched_switch_event, },
> @@ -1783,30 +1784,17 @@ static int perf_sched__read_events(struct perf_sched *sched)
> { "sched:sched_wakeup_new", process_sched_wakeup_event, },
> { "sched:sched_migrate_task", process_sched_migrate_task_event, },
> };
> - struct perf_session *session;
> - struct perf_data data = {
> - .path = input_name,
> - .mode = PERF_DATA_MODE_READ,
> - .force = sched->force,
> - };
> - int rc = -1;
> -
> - session = perf_session__new(&data, false, &sched->tool);
> - if (session == NULL) {
> - pr_debug("No Memory for session\n");
> - return -1;
> - }
>
> symbol__init(&session->header.env);
>
> if (perf_session__set_tracepoints_handlers(session, handlers))
> - goto out_delete;
> + return -1;
>
> if (perf_session__has_traces(session, "record -R")) {
> int err = perf_session__process_events(session);
> if (err) {
> pr_err("Failed to process events, error %d", err);
> - goto out_delete;
> + return -1;
> }
>
> sched->nr_events = session->evlist->stats.nr_events[0];
> @@ -1814,9 +1802,28 @@ static int perf_sched__read_events(struct perf_sched *sched)
> sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
> }
>
> - rc = 0;
> -out_delete:
> + return 0;
> +}
> +
> +static int perf_sched__read_events(struct perf_sched *sched)
> +{
> + struct perf_session *session;
> + struct perf_data data = {
> + .path = input_name,
> + .mode = PERF_DATA_MODE_READ,
> + .force = sched->force,
> + };
> + int rc;
> +
> + session = perf_session__new(&data, false, &sched->tool);
> + if (session == NULL) {
> + pr_debug("No Memory for session\n");
> + return -1;
> + }
> +
> + rc = __perf_sched__read_events(sched, session);
> perf_session__delete(session);
> +
> return rc;
> }
>
> @@ -3130,12 +3137,25 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
>
> static int perf_sched__lat(struct perf_sched *sched)
> {
> + struct perf_session *session;
> + struct perf_data data = {
> + .path = input_name,
> + .mode = PERF_DATA_MODE_READ,
> + .force = sched->force,
> + };
> struct rb_node *next;
> + int rc = -1;
>
> setup_pager();
>
> - if (perf_sched__read_events(sched))
> + session = perf_session__new(&data, false, &sched->tool);
> + if (session == NULL) {
> + pr_debug("No Memory for session\n");
> return -1;
> + }
> +
> + if (__perf_sched__read_events(sched, session))
> + goto out_delete;
>
> perf_sched__merge_lat(sched);
> perf_sched__sort_lat(sched);
> @@ -3164,7 +3184,10 @@ static int perf_sched__lat(struct perf_sched *sched)
> print_bad_events(sched);
> printf("\n");
>
> - return 0;
> + rc = 0;
> +out_delete:
> + perf_session__delete(session);
> + return rc;
> }
>
> static int setup_map_cpus(struct perf_sched *sched)
> --
> 2.17.1
>
Em Wed, May 22, 2019 at 03:56:10PM +0900, Namhyung Kim escreveu:
> On Wed, May 08, 2019 at 10:36:48PM +0800, Wei Li wrote:
> > After thread is added to machine->threads[i].dead in
> > __machine__remove_thread, the machine->threads[i].dead is freed
> > when calling free(session) in perf_session__delete(). So it get a
> > Segmentation fault when accessing it in thread__put().
> >
> > In this patch, we delay the perf_session__delete until all threads
> > have been deleted.
> >
> > This can be reproduced by following steps:
> > ulimit -c unlimited
> > export MALLOC_MMAP_THRESHOLD_=0
> > perf sched record sleep 10
> > perf sched latency --sort max
> > Segmentation fault (core dumped)
> >
> > Signed-off-by: Zhipeng Xie <[email protected]>
> > Signed-off-by: Wei Li <[email protected]>
>
> Acked-by: Namhyung Kim <[email protected]>
I'll try to analyse this one soon, but my first impression was that we
should just grab reference counts when keeping a pointer to those
threads instead of keeping _all_ threads alive when supposedly we could
trow away unreferenced data structures.
But this is just a first impression from just reading the patch
description, probably I'm missing something.
Thanks for providing instructions on readily triggering the segfault.
- Arnaldo
On Wed, May 22, 2019 at 08:08:23AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 22, 2019 at 03:56:10PM +0900, Namhyung Kim escreveu:
> > On Wed, May 08, 2019 at 10:36:48PM +0800, Wei Li wrote:
> > > After thread is added to machine->threads[i].dead in
> > > __machine__remove_thread, the machine->threads[i].dead is freed
> > > when calling free(session) in perf_session__delete(). So it get a
> > > Segmentation fault when accessing it in thread__put().
> > >
> > > In this patch, we delay the perf_session__delete until all threads
> > > have been deleted.
> > >
> > > This can be reproduced by following steps:
> > > ulimit -c unlimited
> > > export MALLOC_MMAP_THRESHOLD_=0
> > > perf sched record sleep 10
> > > perf sched latency --sort max
> > > Segmentation fault (core dumped)
> > >
> > > Signed-off-by: Zhipeng Xie <[email protected]>
> > > Signed-off-by: Wei Li <[email protected]>
> >
> > Acked-by: Namhyung Kim <[email protected]>
>
> I'll try to analyse this one soon, but my first impression was that we
> should just grab reference counts when keeping a pointer to those
> threads instead of keeping _all_ threads alive when supposedly we could
> trow away unreferenced data structures.
>
> But this is just a first impression from just reading the patch
> description, probably I'm missing something.
No, thread refcounting is fine. We already did it and threads with the
refcount will be accessed only.
But the problem is the head of the list. After using the thread, the
refcount is gone and thread is removed from the list and destroyed.
However the head of list is in a struct machine which was freed with
session already.
Thanks,
Namhyung
>
> Thanks for providing instructions on readily triggering the segfault.
>
> - Arnaldo
Hi Arnaldo,
I found this issue has not been fixed in mainline now, please take a glance at this.
On 2019/5/23 10:50, Namhyung Kim wrote:
> On Wed, May 22, 2019 at 08:08:23AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Wed, May 22, 2019 at 03:56:10PM +0900, Namhyung Kim escreveu:
>>> On Wed, May 08, 2019 at 10:36:48PM +0800, Wei Li wrote:
>>>> After thread is added to machine->threads[i].dead in
>>>> __machine__remove_thread, the machine->threads[i].dead is freed
>>>> when calling free(session) in perf_session__delete(). So it get a
>>>> Segmentation fault when accessing it in thread__put().
>>>>
>>>> In this patch, we delay the perf_session__delete until all threads
>>>> have been deleted.
>>>>
>>>> This can be reproduced by following steps:
>>>> ulimit -c unlimited
>>>> export MALLOC_MMAP_THRESHOLD_=0
>>>> perf sched record sleep 10
>>>> perf sched latency --sort max
>>>> Segmentation fault (core dumped)
>>>>
>>>> Signed-off-by: Zhipeng Xie <[email protected]>
>>>> Signed-off-by: Wei Li <[email protected]>
>>>
>>> Acked-by: Namhyung Kim <[email protected]>
>>
>> I'll try to analyse this one soon, but my first impression was that we
>> should just grab reference counts when keeping a pointer to those
>> threads instead of keeping _all_ threads alive when supposedly we could
>> trow away unreferenced data structures.
>>
>> But this is just a first impression from just reading the patch
>> description, probably I'm missing something.
>
> No, thread refcounting is fine. We already did it and threads with the
> refcount will be accessed only.
>
> But the problem is the head of the list. After using the thread, the
> refcount is gone and thread is removed from the list and destroyed.
> However the head of list is in a struct machine which was freed with
> session already.
>
> Thanks,
> Namhyung
>
>
>>
>> Thanks for providing instructions on readily triggering the segfault.
>>
>> - Arnaldo
>
> .
>
Thanks,
Wei
Em Thu, Jul 04, 2019 at 07:21:28PM +0800, liwei (GF) escreveu:
> Hi Arnaldo,
> I found this issue has not been fixed in mainline now, please take a glance at this.
See below.
> On 2019/5/23 10:50, Namhyung Kim wrote:
> > On Wed, May 22, 2019 at 08:08:23AM -0300, Arnaldo Carvalho de Melo wrote:
> >> I'll try to analyse this one soon, but my first impression was that we
> >> should just grab reference counts when keeping a pointer to those
> >> threads instead of keeping _all_ threads alive when supposedly we could
> >> trow away unreferenced data structures.
> >> But this is just a first impression from just reading the patch
> >> description, probably I'm missing something.
> > No, thread refcounting is fine. We already did it and threads with the
> > refcount will be accessed only.
> > But the problem is the head of the list. After using the thread, the
> > refcount is gone and thread is removed from the list and destroyed.
> > However the head of list is in a struct machine which was freed with
> > session already.
I see, and sorry for the delay, thanks for bringing this up again, how
about the following instead? I tested it with 'perf top' that exercises
this code in a multithreaded fashion as well with the set of steps in
your patch commit log, seems to work.
- Arnaldo
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 86fede2b7507..cf826eca3aaf 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -210,6 +210,18 @@ void machine__exit(struct machine *machine)
for (i = 0; i < THREADS__TABLE_SIZE; i++) {
struct threads *threads = &machine->threads[i];
+ struct thread *thread, *n;
+ /*
+ * Forget about the dead, at this point whatever threads were
+ * left in the dead lists better have a reference count taken
+ * by who is using them, and then, when they drop those references
+ * and it finally hits zero, thread__put() will check and see that
+ * its not in the dead threads list and will not try to remove it
+ * from there, just calling thread__delete() straight away.
+ */
+ list_for_each_entry_safe(thread, n, &threads->dead, node)
+ list_del_init(&thread->node);
+
exit_rwsem(&threads->lock);
}
}
@@ -1759,9 +1771,11 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
if (threads->last_match == th)
threads__set_last_match(threads, NULL);
- BUG_ON(refcount_read(&th->refcnt) == 0);
if (lock)
down_write(&threads->lock);
+
+ BUG_ON(refcount_read(&th->refcnt) == 0);
+
rb_erase_cached(&th->rb_node, &threads->entries);
RB_CLEAR_NODE(&th->rb_node);
--threads->nr;
@@ -1771,9 +1785,16 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
* will be called and we will remove it from the dead_threads list.
*/
list_add_tail(&th->node, &threads->dead);
+
+ /*
+ * We need to do the put here because if this is the last refcount,
+ * then we will be touching the threads->dead head when removing the
+ * thread.
+ */
+ thread__put(th);
+
if (lock)
up_write(&threads->lock);
- thread__put(th);
}
void machine__remove_thread(struct machine *machine, struct thread *th)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 4db8cd2a33ae..873ab505ca80 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -125,10 +125,27 @@ void thread__put(struct thread *thread)
{
if (thread && refcount_dec_and_test(&thread->refcnt)) {
/*
- * Remove it from the dead_threads list, as last reference
- * is gone.
+ * Remove it from the dead threads list, as last reference is
+ * gone, if it is in a dead threads list.
+ *
+ * We may not be there anymore if say, the machine where it was
+ * stored was already deleted, so we already removed it from
+ * the dead threads and some other piece of code still keeps a
+ * reference.
+ *
+ * This is what 'perf sched' does and finally drops it in
+ * perf_sched__lat(), where it calls perf_sched__read_events(),
+ * that processes the events by creating a session and deleting
+ * it, which ends up destroying the list heads for the dead
+ * threads, but before it does that it removes all threads from
+ * it using list_del_init().
+ *
+ * So we need to check here if it is in a dead threads list and
+ * if so, remove it before finally deleting the thread, to avoid
+ * an use after free situation.
*/
- list_del_init(&thread->node);
+ if (!list_empty(&thread->node))
+ list_del_init(&thread->node);
thread__delete(thread);
}
}
Commit-ID: 4c00af0e94cd01b8c5a5e6b3323d34677b04e192
Gitweb: https://git.kernel.org/tip/4c00af0e94cd01b8c5a5e6b3323d34677b04e192
Author: Arnaldo Carvalho de Melo <[email protected]>
AuthorDate: Fri, 5 Jul 2019 12:11:35 -0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Sat, 6 Jul 2019 14:29:32 -0300
perf thread: Allow references to thread objects after machine__exit()
Threads are created when we either synthesize PERF_RECORD_FORK events
for pre-existing threads or when we receive PERF_RECORD_FORK events from
the kernel as new threads get created.
We then keep them in machine->threads[].entries rb trees till when we
receive a PERF_RECORD_EXIT, i.e. that thread terminated.
The thread object has a reference count that is grabbed when, for
instance, we keep that thread referenced in struct hist_entry, in 'perf
report' and 'perf top'.
When we receive a PERF_RECORD_EXIT we remove the thread object from the
rb tree and move it to the corresponding machine->threads[].dead list,
then we do a thread__put(), dropping the reference we had for keeping it
in the rb tree.
In thread__put() we were assuming that when the reference count hit zero
we should remove it from the dead list by simply doing a
list_del_init(&thread->node).
That works well when all the thread lifetime is during the machine that
has the list heads lifetime, since we know that we can do the
list_del_init() and it will update the 'dead' list_head.
But in 'perf sched lat' we were doing:
machine__new() (via perf_session__new)
process events, grabbing refcounts to keep those thread objects
in 'perf sched' local data structures.
machine__exit() (via perf_session__delete) which would delete the
'dead' list heads.
And then doing the final thread__put() for the refcounts 'perf sched'
rightfully obtained for keeping those thread object references.
b00m, since thread__put() would do the list_del_init() touching
a dead dead list head.
Fix it by removing all the dead threads from machine->threads[].dead at
machine__exit(), since whatever is there should have refcounts taken by
things like 'perf sched lat', and make thread__put() check if the thread
is in a linked list before removing it from that list.
Reported-by: Wei Li <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Cc: Adrian Hunter <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Zhipeng Xie <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/machine.c | 25 +++++++++++++++++++++++--
tools/perf/util/thread.c | 23 ++++++++++++++++++++---
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index dc7aafe45a2b..e00dc413652d 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -209,6 +209,18 @@ void machine__exit(struct machine *machine)
for (i = 0; i < THREADS__TABLE_SIZE; i++) {
struct threads *threads = &machine->threads[i];
+ struct thread *thread, *n;
+ /*
+ * Forget about the dead, at this point whatever threads were
+ * left in the dead lists better have a reference count taken
+ * by who is using them, and then, when they drop those references
+ * and it finally hits zero, thread__put() will check and see that
+ * its not in the dead threads list and will not try to remove it
+ * from there, just calling thread__delete() straight away.
+ */
+ list_for_each_entry_safe(thread, n, &threads->dead, node)
+ list_del_init(&thread->node);
+
exit_rwsem(&threads->lock);
}
}
@@ -1758,9 +1770,11 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
if (threads->last_match == th)
threads__set_last_match(threads, NULL);
- BUG_ON(refcount_read(&th->refcnt) == 0);
if (lock)
down_write(&threads->lock);
+
+ BUG_ON(refcount_read(&th->refcnt) == 0);
+
rb_erase_cached(&th->rb_node, &threads->entries);
RB_CLEAR_NODE(&th->rb_node);
--threads->nr;
@@ -1770,9 +1784,16 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
* will be called and we will remove it from the dead_threads list.
*/
list_add_tail(&th->node, &threads->dead);
+
+ /*
+ * We need to do the put here because if this is the last refcount,
+ * then we will be touching the threads->dead head when removing the
+ * thread.
+ */
+ thread__put(th);
+
if (lock)
up_write(&threads->lock);
- thread__put(th);
}
void machine__remove_thread(struct machine *machine, struct thread *th)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index b413ba5b9835..7bfb740d2ede 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -125,10 +125,27 @@ void thread__put(struct thread *thread)
{
if (thread && refcount_dec_and_test(&thread->refcnt)) {
/*
- * Remove it from the dead_threads list, as last reference
- * is gone.
+ * Remove it from the dead threads list, as last reference is
+ * gone, if it is in a dead threads list.
+ *
+ * We may not be there anymore if say, the machine where it was
+ * stored was already deleted, so we already removed it from
+ * the dead threads and some other piece of code still keeps a
+ * reference.
+ *
+ * This is what 'perf sched' does and finally drops it in
+ * perf_sched__lat(), where it calls perf_sched__read_events(),
+ * that processes the events by creating a session and deleting
+ * it, which ends up destroying the list heads for the dead
+ * threads, but before it does that it removes all threads from
+ * it using list_del_init().
+ *
+ * So we need to check here if it is in a dead threads list and
+ * if so, remove it before finally deleting the thread, to avoid
+ * an use after free situation.
*/
- list_del_init(&thread->node);
+ if (!list_empty(&thread->node))
+ list_del_init(&thread->node);
thread__delete(thread);
}
}