Re: [PATCH V6 06/12] perf tools: remove unnecessary callchain validation
On 16/07/13 15:05, Jiri Olsa wrote:
> On Tue, Jul 16, 2013 at 09:38:12AM +0300, Adrian Hunter wrote:
>
> SNIP
>
>> }
>> }
>>
>> -static int perf_session__preprocess_sample(struct perf_session *session,
>> - union perf_event *event, struct
>> perf_sample *sample)
>> -{
>> -if (event->header.type != PERF_RECORD_SAMPLE ||
>> -!sample->callchain)
>> -return 0;
>> -
>> -if (!ip_callchain__valid(sample->callchain, event)) {
>> -pr_debug("call-chain problem with event, skipping it.\n");
>> -++session->stats.nr_invalid_chains;
>> -session->stats.total_invalid_chains += sample->period;
>
> How about the '*invalid_chains' stats here? I dont see
> it incremented in the parsing routine.
>
> Also the current behaviour is to increments stats for invalid
> callchains, but dont fail. With your changes we fail during the
> parsing.
It would fail during parsing sometimes anyway. The code was:
if (type & PERF_SAMPLE_CALLCHAIN) {
if (sample_overlap(event, array, sizeof(data->callchain->nr)))
return -EFAULT;
data->callchain = (struct ip_callchain *)array;
if (sample_overlap(event, array, data->callchain->nr))
return -EFAULT;
array += 1 + data->callchain->nr;
}
But sample overlap did not handle size being effectively negative
i.e. 'offset + size' overflows
static bool sample_overlap(const union perf_event *event,
const void *offset, u64 size)
{
const void *base = event;
if (offset + size > base + event->header.size)
return true;
return false;
}
>
> On the other hand.. maybe we should fail ;-) I think that
> invalid callchain data is serious enough to be overlooked
> by not seeing the nr_invalid_chains got incremented.
>
> let's see other comments and then silently push it :-)
>
> jirka
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 06/12] perf tools: remove unnecessary callchain validation
On 16/07/13 15:05, Jiri Olsa wrote:
On Tue, Jul 16, 2013 at 09:38:12AM +0300, Adrian Hunter wrote:
SNIP
}
}
-static int perf_session__preprocess_sample(struct perf_session *session,
- union perf_event *event, struct
perf_sample *sample)
-{
-if (event-header.type != PERF_RECORD_SAMPLE ||
-!sample-callchain)
-return 0;
-
-if (!ip_callchain__valid(sample-callchain, event)) {
-pr_debug(call-chain problem with event, skipping it.\n);
-++session-stats.nr_invalid_chains;
-session-stats.total_invalid_chains += sample-period;
How about the '*invalid_chains' stats here? I dont see
it incremented in the parsing routine.
Also the current behaviour is to increments stats for invalid
callchains, but dont fail. With your changes we fail during the
parsing.
It would fail during parsing sometimes anyway. The code was:
if (type PERF_SAMPLE_CALLCHAIN) {
if (sample_overlap(event, array, sizeof(data-callchain-nr)))
return -EFAULT;
data-callchain = (struct ip_callchain *)array;
if (sample_overlap(event, array, data-callchain-nr))
return -EFAULT;
array += 1 + data-callchain-nr;
}
But sample overlap did not handle size being effectively negative
i.e. 'offset + size' overflows
static bool sample_overlap(const union perf_event *event,
const void *offset, u64 size)
{
const void *base = event;
if (offset + size base + event-header.size)
return true;
return false;
}
On the other hand.. maybe we should fail ;-) I think that
invalid callchain data is serious enough to be overlooked
by not seeing the nr_invalid_chains got incremented.
let's see other comments and then silently push it :-)
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 06/12] perf tools: remove unnecessary callchain validation
On Tue, Jul 16, 2013 at 09:38:12AM +0300, Adrian Hunter wrote:
SNIP
> }
> }
>
> -static int perf_session__preprocess_sample(struct perf_session *session,
> -union perf_event *event, struct
> perf_sample *sample)
> -{
> - if (event->header.type != PERF_RECORD_SAMPLE ||
> - !sample->callchain)
> - return 0;
> -
> - if (!ip_callchain__valid(sample->callchain, event)) {
> - pr_debug("call-chain problem with event, skipping it.\n");
> - ++session->stats.nr_invalid_chains;
> - session->stats.total_invalid_chains += sample->period;
How about the '*invalid_chains' stats here? I dont see
it incremented in the parsing routine.
Also the current behaviour is to increments stats for invalid
callchains, but dont fail. With your changes we fail during the
parsing.
On the other hand.. maybe we should fail ;-) I think that
invalid callchain data is serious enough to be overlooked
by not seeing the nr_invalid_chains got incremented.
let's see other comments and then silently push it :-)
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V6 06/12] perf tools: remove unnecessary callchain validation
Now that the sample parsing correctly checks data sizes
there is no reason for it to be done again for callchains.
Signed-off-by: Adrian Hunter
---
tools/perf/util/callchain.c | 8
tools/perf/util/callchain.h | 5 -
tools/perf/util/session.c | 20
3 files changed, 33 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 42b6a63..024162a 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -20,14 +20,6 @@
__thread struct callchain_cursor callchain_cursor;
-bool ip_callchain__valid(struct ip_callchain *chain,
-const union perf_event *event)
-{
- unsigned int chain_size = event->header.size;
- chain_size -= (unsigned long)>ip.__more_data - (unsigned
long)event;
- return chain->nr * sizeof(u64) <= chain_size;
-}
-
#define chain_for_each_child(child, parent)\
list_for_each_entry(child, >children, siblings)
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 3ee9f67..988c1aa 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -103,11 +103,6 @@ int callchain_append(struct callchain_root *root,
int callchain_merge(struct callchain_cursor *cursor,
struct callchain_root *dst, struct callchain_root *src);
-struct ip_callchain;
-union perf_event;
-
-bool ip_callchain__valid(struct ip_callchain *chain,
-const union perf_event *event);
/*
* Initialize a cursor before adding entries inside, but keep
* the previously allocated entries as a cache.
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 708c72b..1b07d7a 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -889,22 +889,6 @@ static int perf_session_deliver_event(struct perf_session
*session,
}
}
-static int perf_session__preprocess_sample(struct perf_session *session,
- union perf_event *event, struct
perf_sample *sample)
-{
- if (event->header.type != PERF_RECORD_SAMPLE ||
- !sample->callchain)
- return 0;
-
- if (!ip_callchain__valid(sample->callchain, event)) {
- pr_debug("call-chain problem with event, skipping it.\n");
- ++session->stats.nr_invalid_chains;
- session->stats.total_invalid_chains += sample->period;
- return -EINVAL;
- }
- return 0;
-}
-
static int perf_session__process_user_event(struct perf_session *session,
union perf_event *event,
struct perf_tool *tool, u64
file_offset)
{
@@ -967,10 +951,6 @@ static int perf_session__process_event(struct perf_session
*session,
if (ret)
return ret;
- /* Preprocess sample records - precheck callchains */
- if (perf_session__preprocess_sample(session, event, ))
- return 0;
-
if (tool->ordered_samples) {
ret = perf_session_queue_event(session, event, ,
file_offset);
--
1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V6 06/12] perf tools: remove unnecessary callchain validation
Now that the sample parsing correctly checks data sizes
there is no reason for it to be done again for callchains.
Signed-off-by: Adrian Hunter adrian.hun...@intel.com
---
tools/perf/util/callchain.c | 8
tools/perf/util/callchain.h | 5 -
tools/perf/util/session.c | 20
3 files changed, 33 deletions(-)
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 42b6a63..024162a 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -20,14 +20,6 @@
__thread struct callchain_cursor callchain_cursor;
-bool ip_callchain__valid(struct ip_callchain *chain,
-const union perf_event *event)
-{
- unsigned int chain_size = event-header.size;
- chain_size -= (unsigned long)event-ip.__more_data - (unsigned
long)event;
- return chain-nr * sizeof(u64) = chain_size;
-}
-
#define chain_for_each_child(child, parent)\
list_for_each_entry(child, parent-children, siblings)
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 3ee9f67..988c1aa 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -103,11 +103,6 @@ int callchain_append(struct callchain_root *root,
int callchain_merge(struct callchain_cursor *cursor,
struct callchain_root *dst, struct callchain_root *src);
-struct ip_callchain;
-union perf_event;
-
-bool ip_callchain__valid(struct ip_callchain *chain,
-const union perf_event *event);
/*
* Initialize a cursor before adding entries inside, but keep
* the previously allocated entries as a cache.
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 708c72b..1b07d7a 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -889,22 +889,6 @@ static int perf_session_deliver_event(struct perf_session
*session,
}
}
-static int perf_session__preprocess_sample(struct perf_session *session,
- union perf_event *event, struct
perf_sample *sample)
-{
- if (event-header.type != PERF_RECORD_SAMPLE ||
- !sample-callchain)
- return 0;
-
- if (!ip_callchain__valid(sample-callchain, event)) {
- pr_debug(call-chain problem with event, skipping it.\n);
- ++session-stats.nr_invalid_chains;
- session-stats.total_invalid_chains += sample-period;
- return -EINVAL;
- }
- return 0;
-}
-
static int perf_session__process_user_event(struct perf_session *session,
union perf_event *event,
struct perf_tool *tool, u64
file_offset)
{
@@ -967,10 +951,6 @@ static int perf_session__process_event(struct perf_session
*session,
if (ret)
return ret;
- /* Preprocess sample records - precheck callchains */
- if (perf_session__preprocess_sample(session, event, sample))
- return 0;
-
if (tool-ordered_samples) {
ret = perf_session_queue_event(session, event, sample,
file_offset);
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 06/12] perf tools: remove unnecessary callchain validation
On Tue, Jul 16, 2013 at 09:38:12AM +0300, Adrian Hunter wrote:
SNIP
}
}
-static int perf_session__preprocess_sample(struct perf_session *session,
-union perf_event *event, struct
perf_sample *sample)
-{
- if (event-header.type != PERF_RECORD_SAMPLE ||
- !sample-callchain)
- return 0;
-
- if (!ip_callchain__valid(sample-callchain, event)) {
- pr_debug(call-chain problem with event, skipping it.\n);
- ++session-stats.nr_invalid_chains;
- session-stats.total_invalid_chains += sample-period;
How about the '*invalid_chains' stats here? I dont see
it incremented in the parsing routine.
Also the current behaviour is to increments stats for invalid
callchains, but dont fail. With your changes we fail during the
parsing.
On the other hand.. maybe we should fail ;-) I think that
invalid callchain data is serious enough to be overlooked
by not seeing the nr_invalid_chains got incremented.
let's see other comments and then silently push it :-)
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
