| 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, 24 Oct 2013 10:30:39 +0100
From: Frederic Weisbecker <fweisbec@...il.com>
To: David Ahern <dsahern@...il.com>
Cc: Arnaldo Melo <acme@...stprotocols.net>,
LKML <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...hat.com>,
Mike Galbraith <efault@....de>,
Namhyung Kim <namhyung@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH] perf session: Add option to copy events when queueing
2013/9/14 Frederic Weisbecker <fweisbec@...il.com>:
> On Fri, Sep 06, 2013 at 01:37:01PM -0600, David Ahern wrote:
>> When processing events the session code has an ordered samples queue which is
>> used to time-sort events coming in across multiple mmaps. At a later point in
>> time samples on the queue are flushed up to some timestamp at which point the
>> event is actually processed.
>>
>> When analyzing events live (ie., record/analysis path in the same command)
>> there is a race that leads to corrupted events and parse errors which cause
>> perf to terminate. The problem is that when the event is placed in the ordered
>> samples queue it is only a reference to the event which is really sitting in
>> the mmap buffer. Even though the event is queued for later processing the mmap
>> tail pointer is updated which indicates to the kernel that the event has been
>> processed. The race is flushing the event from the queue before it gets
>> overwritten by some other event. For commands trying to process events live
>> (versus just writing to a file) and processing a high rate of events this leads
>> to parse failures and perf terminates.
>>
>> Examples hitting this problem are 'perf kvm stat live', especially with nested
>> VMs which generate 100,000+ traces per second, and a command processing
>> scheduling events with a high rate of context switching -- e.g., running
>> 'perf bench sched pipe'.
>>
>> This patch offers live commands an option to copy the event when it is placed in
>> the ordered samples queue.
>>
>> Signed-off-by: David Ahern <dsahern@...il.com>
>> Cc: Frederic Weisbecker <fweisbec@...il.com>
>> Cc: Ingo Molnar <mingo@...nel.org>
>> Cc: Jiri Olsa <jolsa@...hat.com>
>> Cc: Mike Galbraith <efault@....de>
>> Cc: Namhyung Kim <namhyung@...nel.org>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Stephane Eranian <eranian@...gle.com>
>> ---
>> tools/perf/util/session.c | 17 ++++++++++++++---
>> tools/perf/util/session.h | 1 +
>> 2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 1b185ca..71f16db 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -483,6 +483,8 @@ static void perf_session_free_sample_buffers(struct perf_session *session)
>>
>> sq = list_entry(os->to_free.next, struct sample_queue, list);
>> list_del(&sq->list);
>> + if (session->copy_on_queue)
>> + free(sq->event);
>> free(sq);
>> }
>> }
>> @@ -513,11 +515,15 @@ static int flush_sample_queue(struct perf_session *s,
>> break;
>>
>> ret = perf_evlist__parse_sample(s->evlist, iter->event, &sample);
>> - if (ret)
>> + if (ret) {
>> pr_err("Can't parse sample, err = %d\n", ret);
>> - else {
>> + if (s->copy_on_queue)
>> + free(iter->event);
>> + } else {
>> ret = perf_session_deliver_event(s, iter->event, &sample, tool,
>> iter->file_offset);
>> + if (s->copy_on_queue)
>> + free(iter->event);
>> if (ret)
>> return ret;
>> }
>> @@ -676,7 +682,12 @@ int perf_session_queue_event(struct perf_session *s, union perf_event *event,
>>
>> new->timestamp = timestamp;
>> new->file_offset = file_offset;
>> - new->event = event;
>> +
>> + if (s->copy_on_queue) {
>> + new->event = malloc(event->header.size);
>> + memcpy(new->event, event, event->header.size);
>> + } else
>> + new->event = event;
>>
>> __queue_event(new, s);
>>
>> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
>> index 3aa75fb..4adfcbb 100644
>> --- a/tools/perf/util/session.h
>> +++ b/tools/perf/util/session.h
>> @@ -38,6 +38,7 @@ struct perf_session {
>> bool fd_pipe;
>> bool repipe;
>> struct ordered_samples ordered_samples;
>> + bool copy_on_queue;
>
> So do you think it should stay optional? This looks like a global problem, I mean
> the event can be unmapped anytime for any builtin tool mapping it, right?
>
> Also we already allocate the sample list node (struct sample_queue) from os->sample
> buffer. ie: we have our own allocator there.
>
> Probably we should reuse that and include the copied event space in "struct sample_queue"?
>
> Also looking at it now, it seems we have a bug on the existing code:
>
>
> if (!list_empty(sc)) {
> new = list_entry(sc->next, struct sample_queue, list);
> list_del(&new->list);
> } else if (os->sample_buffer) {
> new = os->sample_buffer + os->sample_buffer_idx;
> if (++os->sample_buffer_idx == MAX_SAMPLE_BUFFER)
> os->sample_buffer = NULL;
> } else {
> os->sample_buffer = malloc(MAX_SAMPLE_BUFFER * sizeof(*new));
> if (!os->sample_buffer)
> return -ENOMEM;
> list_add(&os->sample_buffer->list, &os->to_free);
> os->sample_buffer_idx = 2;
> new = os->sample_buffer + 1;
> }
>
> If we actually run out of buffer rooms, we should realloc right after and not
> wait for the next entry, otherwise we loose an event:
Bah, checking that again, there don't seem to be a bug there. Actually
the sample buffer is reset after we pick the last entry. So it looks
all fine. I got confused as usual. Nevermind.
--
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