[PATCH v2] Fix annotate.c use of uninitialized value error
Our local MSAN (Memory Sanitizer) build of perf throws a warning
that comes from the "dso__disassemble_filename" function in
"tools/perf/util/annotate.c" when running perf record.
The warning stems from the call to readlink, in which "build_id_path"
was being read into "linkname". Since readlink does not null terminate,
an uninitialized memory access would later occur when "linkname" is
passed into the strstr function. This is simply fixed by null-terminating
"linkname" after the call to readlink.
To reproduce this warning, build perf by running:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
-fsanitize-memory-track-origins"
(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang)
then running:
tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
-i - --stdio
Please see the cover letter for why false positive warnings may be
generated.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/annotate.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 70de8f6b3aee..e1b075b52dce 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
char *build_id_filename;
char *build_id_path = NULL;
char *pos;
+ int len;
if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
!dso__is_kcore(dso))
@@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
dirname(build_id_path);
- if (dso__is_kcore(dso) ||
- readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
- strstr(linkname, DSO__NAME_KALLSYMS) ||
- access(filename, R_OK)) {
+ if (dso__is_kcore(dso))
+ goto fallback;
+
+ len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
+ if (len < 0)
+ goto fallback;
+
+ linkname[len] = '\0';
+ if (strstr(linkname, DSO__NAME_KALLSYMS) ||
+ access(filename, R_OK)) {
fallback:
/*
* If we don't have build-ids or the build-id file isn't in the
--
2.22.0.709.g102302147b-goog
[tip:perf/urgent] perf header: Fix use of unitialized value warning
Commit-ID: 20f9781f491360e7459c589705a2e4b1f136bee9 Gitweb: https://git.kernel.org/tip/20f9781f491360e7459c589705a2e4b1f136bee9 Author: Numfor Mbiziwo-Tiapo AuthorDate: Wed, 24 Jul 2019 16:44:58 -0700 Committer: Arnaldo Carvalho de Melo CommitDate: Mon, 29 Jul 2019 09:03:43 -0300 perf header: Fix use of unitialized value warning When building our local version of perf with MSAN (Memory Sanitizer) and running the perf record command, MSAN throws a use of uninitialized value warning in "tools/perf/util/util.c:333:6". This warning stems from the "buf" variable being passed into "write". It originated as the variable "ev" with the type union perf_event* defined in the "perf_event__synthesize_attr" function in "tools/perf/util/header.c". In the "perf_event__synthesize_attr" function they allocate space with a malloc call using ev, then go on to only assign some of the member variables before passing "ev" on as a parameter to the "process" function therefore "ev" contains uninitialized memory. Changing the malloc call to zalloc to initialize all the members of "ev" which gets rid of the warning. To reproduce this warning, build perf by running: make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\ -fsanitize-memory-track-origins" (Additionally, llvm might have to be installed and clang might have to be specified as the compiler - export CC=/usr/bin/clang) then running: tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\ -i - --stdio Please see the cover letter for why false positive warnings may be generated. Signed-off-by: Numfor Mbiziwo-Tiapo Cc: Alexander Shishkin Cc: Ian Rogers Cc: Jiri Olsa Cc: Mark Drayton Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Link: http://lkml.kernel.org/r/20190724234500.253358-2-n...@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/header.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index 47877f0f6667..1903d7ec9797 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -3646,7 +3646,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, size += sizeof(struct perf_event_header); size += ids * sizeof(u64); - ev = malloc(size); + ev = zalloc(size); if (ev == NULL) return -ENOMEM;
[PATCH v2] Fix annotate.c use of uninitialized value error
Our local MSAN (Memory Sanitizer) build of perf throws a warning
that comes from the "dso__disassemble_filename" function in
"tools/perf/util/annotate.c" when running perf record.
The warning stems from the call to readlink, in which "build_id_path"
was being read into "linkname". Since readlink does not null terminate,
an uninitialized memory access would later occur when "linkname" is
passed into the strstr function. This is simply fixed by null-terminating
"linkname" after the call to readlink.
To reproduce this warning, build perf by running:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
-fsanitize-memory-track-origins"
(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang)
then running:
tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
-i - --stdio
Please see the cover letter for why false positive warnings may be
generated.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/annotate.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 70de8f6b3aee..e1b075b52dce 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
char *build_id_filename;
char *build_id_path = NULL;
char *pos;
+ int len;
if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
!dso__is_kcore(dso))
@@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
dirname(build_id_path);
- if (dso__is_kcore(dso) ||
- readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
- strstr(linkname, DSO__NAME_KALLSYMS) ||
- access(filename, R_OK)) {
+ if (dso__is_kcore(dso))
+ goto fallback;
+
+ len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
+ if (len < 0)
+ goto fallback;
+
+ linkname[len] = '\0';
+ if (strstr(linkname, DSO__NAME_KALLSYMS) ||
+ access(filename, R_OK)) {
fallback:
/*
* If we don't have build-ids or the build-id file isn't in the
--
2.22.0.709.g102302147b-goog
[PATCH 3/3] Fix sched-messaging.c use of uninitialized value errors
Our local MSAN (Memory Sanitizer) build of perf throws use of
uninitialized value warnings in "tools/perf/bench/sched-messaging.c"
when running perf bench.
The first warning comes from the "ready" function where the "dummy" char
is declared and then passed into "write" without being initialized.
Initializing "dummy" to any character silences the warning.
The second warning comes from the "sender" function where a "write" call
is made to write the contents from the "data" char array when it has not
yet been initialized. Calling memset on "data" silences the warning.
To reproduce this warning, build perf by running:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
-fsanitize-memory-track-origins"
(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang)
then running: tools/perf/perf bench sched all
Please see the cover letter for why false positive warnings may be
generated.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/bench/sched-messaging.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/bench/sched-messaging.c
b/tools/perf/bench/sched-messaging.c
index f9d7641ae833..d22d7b7b591d 100644
--- a/tools/perf/bench/sched-messaging.c
+++ b/tools/perf/bench/sched-messaging.c
@@ -69,7 +69,7 @@ static void fdpair(int fds[2])
/* Block until we're ready to go */
static void ready(int ready_out, int wakefd)
{
- char dummy;
+ char dummy = 'N';
struct pollfd pollfd = { .fd = wakefd, .events = POLLIN };
/* Tell them we're ready. */
@@ -87,6 +87,7 @@ static void *sender(struct sender_context *ctx)
char data[DATASIZE];
unsigned int i, j;
+ memset(data, 'N', DATASIZE);
ready(ctx->ready_out, ctx->wakefd);
/* Now pump to every receiver. */
--
2.22.0.657.g960e92d24f-goog
[PATCH 1/3] Fix util.c use of unitialized value warning
When building our local version of perf with MSAN (Memory Sanitizer) and running the perf record command, MSAN throws a use of uninitialized value warning in "tools/perf/util/util.c:333:6". This warning stems from the "buf" variable being passed into "write". It originated as the variable "ev" with the type union perf_event* defined in the "perf_event__synthesize_attr" function in "tools/perf/util/header.c". In the "perf_event__synthesize_attr" function they allocate space with a malloc call using ev, then go on to only assign some of the member variables before passing "ev" on as a parameter to the "process" function therefore "ev" contains uninitialized memory. Changing the malloc call to calloc initializes all the members of "ev" which gets rid of the warning. To reproduce this warning, build perf by running: make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\ -fsanitize-memory-track-origins" (Additionally, llvm might have to be installed and clang might have to be specified as the compiler - export CC=/usr/bin/clang) then running: tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\ -i - --stdio Please see the cover letter for why false positive warnings may be generated. Signed-off-by: Numfor Mbiziwo-Tiapo --- tools/perf/util/header.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index dec6d218c31c..b9c71fc45ac1 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -3427,7 +3427,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, size += sizeof(struct perf_event_header); size += ids * sizeof(u64); - ev = malloc(size); + ev = calloc(1, size); if (ev == NULL) return -ENOMEM; -- 2.22.0.657.g960e92d24f-goog
[PATCH 0/3] Perf uninitialized value fixes
These patches are all warnings that the MSAN (Memory Sanitizer) build of perf has caught. To build perf with MSAN enabled run: make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\ -fsanitize-memory-track-origins" (The -fsanitizer-memory-track-origins makes the bugs clearer but isn't strictly necessary.) (Additionally, llvm might have to be installed and clang might have to be specified as the compiler - export CC=/usr/bin/clang). The patches "Fix util.c use of uninitialized value warning" and "Fix annotate.c use of uninitialized value error" build on top of each other (the changes in Fix util.c use of uninitialized value warning must be made first). When running the commands provided in the repro instructions, MSAN will generate false positive uninitialized memory errors. This is happening because libc is not MSAN-instrumented. Finding a way to build libc with MSAN will get rid of these false positives and allow the real warnings mentioned in the patches to be shown. Numfor Mbiziwo-Tiapo (3): Fix util.c use of uninitialized value warning Fix annotate.c use of uninitialized value error Fix sched-messaging.c use of uninitialized value errors tools/perf/bench/sched-messaging.c | 3 ++- tools/perf/util/annotate.c | 15 +++ tools/perf/util/header.c | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) -- 2.22.0.657.g960e92d24f-goog
[PATCH 2/3] Fix annotate.c use of uninitialized value error
Our local MSAN (Memory Sanitizer) build of perf throws a warning
that comes from the "dso__disassemble_filename" function in
"tools/perf/util/annotate.c" when running perf record.
The warning stems from the call to readlink, in which "build_id_path"
was being read into "linkname". Since readlink does not null terminate,
an uninitialized memory access would later occur when "linkname" is
passed into the strstr function. This is simply fixed by null-terminating
"linkname" after the call to readlink.
To reproduce this warning, build perf by running:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-fsanitize=memory\
-fsanitize-memory-track-origins"
(Additionally, llvm might have to be installed and clang might have to
be specified as the compiler - export CC=/usr/bin/clang)
then running:
tools/perf/perf record -o - ls / | tools/perf/perf --no-pager annotate\
-i - --stdio
Please see the cover letter for why false positive warnings may be
generated.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/annotate.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 70de8f6b3aee..d8bfb561bc35 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1627,6 +1627,7 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
char *build_id_filename;
char *build_id_path = NULL;
char *pos;
+ int len;
if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS &&
!dso__is_kcore(dso))
@@ -1655,10 +1656,16 @@ static int dso__disassemble_filename(struct dso *dso,
char *filename, size_t fil
if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
dirname(build_id_path);
- if (dso__is_kcore(dso) ||
- readlink(build_id_path, linkname, sizeof(linkname)) < 0 ||
- strstr(linkname, DSO__NAME_KALLSYMS) ||
- access(filename, R_OK)) {
+ if (dso__is_kcore(dso))
+ goto fallback;
+
+ len = readlink(build_id_path, linkname, sizeof(linkname));
+ if (len < 0)
+ goto fallback;
+
+ linkname[len] = '\0';
+ if (strstr(linkname, DSO__NAME_KALLSYMS) ||
+ access(filename, R_OK)) {
fallback:
/*
* If we don't have build-ids or the build-id file isn't in the
--
2.22.0.657.g960e92d24f-goog
[RFC][PATCH 2/2] Fix evsel.c misaligned address errors
The ubsan (undefined behavior sanitizer) build of perf throws
misaligned address erros during 'Sample parsing function' in
perf test.
To reproduce, run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
(see the cover letter for why perf may not build)
then run: tools/perf/perf test 26 -v
Most of the misaligned address errors come from improperly assigning
values to the u64 array in the 'perf_event__synthesize_sample'
function. These are easily fixed by changing the assignments
to use memcpy instead.
In the 'perf_evsel__parse_sample' function, the 'u64* array'
variable has varying numbers of bytes added to it depending on
which if statements it passes. Since this function is called
multiple times under different conditions, the 'array' variable
is sometimes misaligned by 4 bytes and sometimes not. This causes
issues when 'data->branch_stack' is later assigned to an element
in the array.
In the case that the array is misaligned we can add 4 bytes to the
array to realign it. This still causes an incorrect perf data file
(so the test still fails with the ubsan build) but it at least
gets rid of the error.
Comments?
Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/evsel.c | 28
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbc0466db368..a1289fcbbb2d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2288,6 +2288,11 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel,
union perf_event *event,
sizeof(struct branch_entry);
OVERFLOW_CHECK_u64(array);
+ if u64)array) & 7) != 0)
+ array = (void *)array + sizeof(u32);
+
+ assertu64)array) & 7) == 0);
+
data->branch_stack = (struct branch_stack *)array++;
if (data->branch_stack->nr > max_branch_nr)
@@ -2646,7 +2651,8 @@ int perf_event__synthesize_sample(union perf_event
*event, u64 type,
if (type & PERF_SAMPLE_REGS_USER) {
if (sample->user_regs.abi) {
- *array++ = sample->user_regs.abi;
+ memcpy(array++, >user_regs.abi,
+ sizeof(sample->user_regs.abi));
sz = hweight_long(sample->user_regs.mask) * sizeof(u64);
memcpy(array, sample->user_regs.regs, sz);
array = (void *)array + sz;
@@ -2657,32 +2663,31 @@ int perf_event__synthesize_sample(union perf_event
*event, u64 type,
if (type & PERF_SAMPLE_STACK_USER) {
sz = sample->user_stack.size;
- *array++ = sz;
+ memcpy(array++, , sizeof(sample->user_stack.size));
if (sz) {
memcpy(array, sample->user_stack.data, sz);
array = (void *)array + sz;
- *array++ = sz;
+ memcpy(array++, , sizeof(sz));
}
}
if (type & PERF_SAMPLE_WEIGHT) {
- *array = sample->weight;
- array++;
+ memcpy(array++, >weight, sizeof(sample->weight));
}
if (type & PERF_SAMPLE_DATA_SRC) {
- *array = sample->data_src;
- array++;
+ memcpy(array++, >data_src, sizeof(sample->data_src));
}
if (type & PERF_SAMPLE_TRANSACTION) {
- *array = sample->transaction;
- array++;
+ memcpy(array++, >transaction,
+ sizeof(sample->transaction));
}
if (type & PERF_SAMPLE_REGS_INTR) {
if (sample->intr_regs.abi) {
- *array++ = sample->intr_regs.abi;
+ memcpy(array++, >intr_regs.abi,
+ sizeof(sample->intr_regs.abi));
sz = hweight_long(sample->intr_regs.mask) * sizeof(u64);
memcpy(array, sample->intr_regs.regs, sz);
array = (void *)array + sz;
@@ -2692,8 +2697,7 @@ int perf_event__synthesize_sample(union perf_event
*event, u64 type,
}
if (type & PERF_SAMPLE_PHYS_ADDR) {
- *array = sample->phys_addr;
- array++;
+ memcpy(array++, >phys_addr, sizeof(sample->phys_addr));
}
return 0;
--
2.22.0.657.g960e92d24f-goog
[RFC][PATCH 1/2] Fix event.c misaligned address error
The ubsan (undefined behavior sanitizer) build of perf throws an
error when the synthesize "Synthesize cpu map" function from
perf test is run.
This can be reproduced by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
(see cover letter for why perf may not build)
then running: tools/perf/perf test 44 -v
This bug occurs because the cpu_map_data__synthesize function in
event.c calls synthesize_mask, casting the 'data' variable
(of type cpu_map_data*) to a cpu_map_mask*. Since struct
cpu_map_data is 2 byte aligned and struct cpu_map_mask is 8 byte
aligned this causes memory alignment issues.
This is fixed by adding 6 bytes of padding to the struct cpu_map_data,
however, this will break compatibility between perf data files - a file
written by an old perf wouldn't work with a perf with this patch due
to event data alignment changing.
Comments?
Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/event.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index eb95f3384958..82eaf06c2604 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -433,6 +433,7 @@ struct cpu_map_mask {
struct cpu_map_data {
u16 type;
+ u8 pad[6];
chardata[];
};
--
2.22.0.657.g960e92d24f-goog
[RFC][PATCH 0/2] Perf misaligned address fixes
These patches are all errors found by running perf test with the ubsan (undefined behavior sanitizer version of perf). They are solutions to misaligned address errors caught by ubsan but they would break compatibility between perf data files. To build perf with ubsan run: make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined" Perf will throw errors that have been fixed in these patches that have not yet been merged: https://lore.kernel.org/patchwork/patch/1104065/ https://lore.kernel.org/patchwork/patch/1104066/ Please feel free to leave comments. Numfor Mbiziwo-Tiapo (2): Fix event.c misaligned address error Fix evsel.c misaligned address errors tools/perf/util/event.h | 1 + tools/perf/util/evsel.c | 28 2 files changed, 17 insertions(+), 12 deletions(-) -- 2.22.0.657.g960e92d24f-goog
[PATCH 0/3] Perf UBsan Patches
These patches are all errors found by running perf test with the ubsan (undefined behavior sanitizer version of perf). To repro the bug fixed in the "Fix insn.c misaligned address error" patch you must first apply the changes in the Fix backward-ring-buffer.c format-truncation error and Fix ordered-events.c array-bounds error patches since these are necessary to get the ubsan version of perf to build. To build the ubsan version, run: make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined" Numfor Mbiziwo-Tiapo (3): Fix backward-ring-buffer.c format-truncation error Fix ordered-events.c array-bounds error Fix insn.c misaligned address error tools/perf/tests/backward-ring-buffer.c | 2 +- tools/perf/util/intel-pt-decoder/insn.c | 3 ++- tools/perf/util/ordered-events.c| 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) -- 2.22.0.657.g960e92d24f-goog
[PATCH 1/3] Fix backward-ring-buffer.c format-truncation error
Perf does not build with the ubsan (undefined behavior sanitizer)
and there is an error that says:
tests/backward-ring-buffer.c:23:45: error: ‘%d’ directive output
may be truncated writing between 1 and 10 bytes into a region of
size 8 [-Werror=format-truncation=]
snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
This can be reproduced by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
Th error occurs because they are writing to the 10 byte buffer - the
index 'i' of the for loop and the 2 byte hardcoded string. If somehow 'i'
was greater than 8 bytes (10 - 2), then the snprintf function would
truncate the string. Increasing the size of the buffer fixes the error.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/tests/backward-ring-buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/tests/backward-ring-buffer.c
b/tools/perf/tests/backward-ring-buffer.c
index 6d598cc071ae..1a9c3becf5ff 100644
--- a/tools/perf/tests/backward-ring-buffer.c
+++ b/tools/perf/tests/backward-ring-buffer.c
@@ -18,7 +18,7 @@ static void testcase(void)
int i;
for (i = 0; i < NR_ITERS; i++) {
- char proc_name[10];
+ char proc_name[15];
snprintf(proc_name, sizeof(proc_name), "p:%d\n", i);
prctl(PR_SET_NAME, proc_name);
--
2.22.0.657.g960e92d24f-goog
[PATCH 3/3] Fix insn.c misaligned address error
The ubsan (undefined behavior sanitizer) version of perf throws an
error on the 'x86 instruction decoder - new instructions' function
of perf test.
To reproduce this run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
then run: tools/perf/perf test 62 -v
The error occurs in the __get_next macro (line 34) where an int is
read from a potentially unaligned address. Using memcpy instead of
assignment from an unaligned pointer.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/intel-pt-decoder/insn.c
b/tools/perf/util/intel-pt-decoder/insn.c
index ca983e2bea8b..de1944c60aa9 100644
--- a/tools/perf/util/intel-pt-decoder/insn.c
+++ b/tools/perf/util/intel-pt-decoder/insn.c
@@ -31,7 +31,8 @@
((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
#define __get_next(t, insn)\
- ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
+ ({ t r; memcpy(, insn->next_byte, sizeof(t)); \
+ insn->next_byte += sizeof(t); r; })
#define __peek_nbyte_next(t, insn, n) \
({ t r = *(t*)((insn)->next_byte + n); r; })
--
2.22.0.657.g960e92d24f-goog
[PATCH 2/3] Fix ordered-events.c array-bounds error
Perf does not build with the ubsan (undefined behavior sanitizer) and there is an error that says: tools/perf/util/debug.h:38:2: error: array subscript is above array bounds [-Werror=array-bounds] eprintf_time(n, var, t, fmt, ##__VA_ARGS__) ^~~ tools/perf/util/debug.h:40:34: note: in expansion of macro ‘pr_time_N’ #define pr_oe_time(t, fmt, ...) pr_time_N(1, debug_ordered_events, t, pr_fmt(fmt), ##__VA_ARGS__) util/ordered-events.c:329:2: note: in expansion of macro ‘pr_oe_time’ pr_oe_time(oe->next_flush, "next_flush - ordered_events__flush POST %s, nr_events %u\n", This can be reproduced by running (from the tip directory): make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined" The error stems from the 'str' array in the __ordered_events__flush function in tools/perf/util/ordered-events.c. On line 319 of this file, they use values of the variable 'how' (which has the type enum oeflush - defined in ordered-events.h) as indices for the 'str' array. Since 'how' has 5 values and the 'str' array only has 3, when the 4th and 5th values of 'how' (OE_FLUSH__TOP and OE_FLUSH__TIME) are used as indices, this will go out of the bounds of the 'str' array. Adding the matching strings from the enum values into the 'str' array fixes this. Signed-off-by: Numfor Mbiziwo-Tiapo --- tools/perf/util/ordered-events.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c index 897589507d97..c092b0c39d2b 100644 --- a/tools/perf/util/ordered-events.c +++ b/tools/perf/util/ordered-events.c @@ -270,6 +270,8 @@ static int __ordered_events__flush(struct ordered_events *oe, enum oe_flush how, "FINAL", "ROUND", "HALF ", + "TOP", + "TIME", }; int err; bool show_progress = false; -- 2.22.0.657.g960e92d24f-goog
[tip:perf/urgent] perf test mmap-thread-lookup: Initialize variable to suppress memory sanitizer warning
Commit-ID: 4e4cf62b37da5ff45c904a3acf242ab29ed5881d
Gitweb: https://git.kernel.org/tip/4e4cf62b37da5ff45c904a3acf242ab29ed5881d
Author: Numfor Mbiziwo-Tiapo
AuthorDate: Tue, 2 Jul 2019 10:37:15 -0700
Committer: Arnaldo Carvalho de Melo
CommitDate: Tue, 9 Jul 2019 09:33:54 -0300
perf test mmap-thread-lookup: Initialize variable to suppress memory sanitizer
warning
Running the 'perf test' command after building perf with a memory
sanitizer causes a warning that says:
WARNING: MemorySanitizer: use-of-uninitialized-value... in
mmap-thread-lookup.c
Initializing the go variable to 0 silences this harmless warning.
Committer warning:
This was harmless, just a simple test writing whatever was at that
sizeof(int) memory area just to signal another thread blocked reading
that file created with pipe(). Initialize it tho so that we don't get
this warning.
Signed-off-by: Numfor Mbiziwo-Tiapo
Cc: Alexander Shishkin
Cc: Ian Rogers
Cc: Jiri Olsa
Cc: Mark Drayton
Cc: Namhyung Kim
Cc: Peter Zijlstra
Cc: Song Liu
Cc: Stephane Eranian
Link: http://lkml.kernel.org/r/20190702173716.181223-1-n...@google.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/tests/mmap-thread-lookup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/tests/mmap-thread-lookup.c
b/tools/perf/tests/mmap-thread-lookup.c
index ba87e6e8d18c..0a4301a5155c 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -53,7 +53,7 @@ static void *thread_fn(void *arg)
{
struct thread_data *td = arg;
ssize_t ret;
- int go;
+ int go = 0;
if (thread_init(td))
return NULL;
[PATCH] Fix perf stat repeat segfault
When perf stat is called with event groups and the repeat option,
a segfault occurs because the cpu ids are stored on each iteration
of the repeat, when they should only be stored on the first iteration,
which causes a buffer overflow.
This can be replicated by running (from the tip directory):
make -C tools/perf
then running:
tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
Since run_idx keeps track of the current iteration of the repeat,
only storing the cpu ids on the first iteration (when run_idx < 1)
fixes this issue.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/builtin-stat.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 63a3afc7f32b..00a13ce17fd9 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo
__maybe_unused, siginfo_t *inf
workload_exec_errno = info->si_value.sival_int;
}
-static bool perf_evsel__should_store_id(struct perf_evsel *counter)
+static bool perf_evsel__should_store_id(struct perf_evsel *counter, int
run_idx)
{
- return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
+ return (STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID)
+ && run_idx < 1;
}
static bool is_target_alive(struct target *_target,
@@ -503,7 +504,7 @@ static int __run_perf_stat(int argc, const char **argv, int
run_idx)
if (l > stat_config.unit_width)
stat_config.unit_width = l;
- if (perf_evsel__should_store_id(counter) &&
+ if (perf_evsel__should_store_id(counter, run_idx) &&
perf_evsel__store_ids(counter, evsel_list))
return -1;
}
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH] Fix perf test data race for tsan build
The tsan build of perf gives a data race warning in
mmap-thread-lookup.c when perf test is run on our local machines.
This is because there is no lock around the "go_away" variable in
"mmap-thread-lookup.c".
The warning is difficult to reproduce from the tip branch and we
suspect it has something to do with tsan working differently
depending on the clang version.
The warning can be silenced by adding locks around the variable
but it is better practice to make the variable atomic and use
the atomic_set and atomic_read functions. This does not actually
silence the warning because tsan doesn't recognize the atomic
functions as thread safe, but in actuality it should prevent a
data race.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/tests/mmap-thread-lookup.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/tests/mmap-thread-lookup.c
b/tools/perf/tests/mmap-thread-lookup.c
index 5ede9b561d32..f4cbb3d92a46 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -17,7 +17,7 @@
#define THREADS 4
-static int go_away;
+static atomic_t go_away;
struct thread_data {
pthread_t pt;
@@ -64,7 +64,7 @@ static void *thread_fn(void *arg)
return NULL;
}
- while (!go_away) {
+ while (!atomic_read(_away)) {
/* Waiting for main thread to kill us. */
usleep(100);
}
@@ -98,7 +98,7 @@ static int threads_create(void)
struct thread_data *td0 = [0];
int i, err = 0;
- go_away = 0;
+ atomic_set(_away, 0);
/* 0 is main thread */
if (thread_init(td0))
@@ -118,7 +118,7 @@ static int threads_destroy(void)
/* cleanup the main thread */
munmap(td0->map, page_size);
- go_away = 1;
+ atomic_set(_away, 1);
for (i = 1; !err && i < THREADS; i++)
err = pthread_join(threads[i].pt, NULL);
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH] Fix perf stat repeat segfault
When perf stat is called with event groups and the repeat option,
a segfault occurs because the cpu ids are stored on each iteration
of the repeat, when they should only be stored on the first iteration,
which causes a buffer overflow.
This can be replicated by running (from the tip directory):
make -C tools/perf
then running:
tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
Since run_idx keeps track of the current iteration of the repeat,
only storing the cpu ids on the first iteration (when run_idx < 1)
fixes this issue.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/builtin-stat.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 63a3afc7f32b..92d6694367e4 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo
__maybe_unused, siginfo_t *inf
workload_exec_errno = info->si_value.sival_int;
}
-static bool perf_evsel__should_store_id(struct perf_evsel *counter)
+static bool perf_evsel__should_store_id(struct perf_evsel *counter, int
run_idx)
{
- return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
+ return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
+ && run_idx < 1;
}
static bool is_target_alive(struct target *_target,
@@ -503,7 +504,7 @@ static int __run_perf_stat(int argc, const char **argv, int
run_idx)
if (l > stat_config.unit_width)
stat_config.unit_width = l;
- if (perf_evsel__should_store_id(counter) &&
+ if (perf_evsel__should_store_id(counter, run_idx) &&
perf_evsel__store_ids(counter, evsel_list))
return -1;
}
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH] Fix perf-hooks test for sanitizers
The perf-hooks test fails with Address Sanitizer and Memory
Sanitizer builds because it purposefully generates a segfault.
Checking if these sanitizers are active when running this test
will allow the perf-hooks test to pass.
This can be replicated by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=address \
-DADDRESS_SANITIZER=1"
then running tools/perf/perf test 55
Fix past to pass:
The raised signal was changed from SIGSEGV to SIGILL to get the test
to pass on our local machines which use clang 4.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/tests/perf-hooks.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/perf/tests/perf-hooks.c b/tools/perf/tests/perf-hooks.c
index a693bcf017ea..3f5f4b28cf01 100644
--- a/tools/perf/tests/perf-hooks.c
+++ b/tools/perf/tests/perf-hooks.c
@@ -7,7 +7,14 @@
#include "util.h"
#include "perf-hooks.h"
-static void sigsegv_handler(int sig __maybe_unused)
+#if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \
+defined(THREAD_SANITIZER) || defined(SAFESTACK_SANITIZER)
+#define USE_SIGNAL 1
+#else
+#define USE_SIGNAL 0
+#endif
+
+static void signal_handler(int sig __maybe_unused)
{
pr_debug("SIGSEGV is observed as expected, try to recover.\n");
perf_hooks__recover();
@@ -25,6 +32,9 @@ static void the_hook(void *_hook_flags)
*hook_flags = 1234;
/* Generate a segfault, test perf_hooks__recover */
+#if USE_SIGNAL
+ raise(SIGILL);
+#endif
*p = 0;
}
@@ -32,7 +42,7 @@ int test__perf_hooks(struct test *test __maybe_unused, int
subtest __maybe_unuse
{
int hook_flags = 0;
- signal(SIGSEGV, sigsegv_handler);
+ signal(USE_SIGNAL ? SIGILL : SIGSEGV, signal_handler);
perf_hooks__set_hook("test", the_hook, _flags);
perf_hooks__invoke_test();
--
2.22.0.410.gd8fdbe21b5-goog
[tip:perf/core] perf tools: Fix cache.h include directive
Commit-ID: 2d7102a0453769fd37e9f4ce68652e104fbf5c84 Gitweb: https://git.kernel.org/tip/2d7102a0453769fd37e9f4ce68652e104fbf5c84 Author: Numfor Mbiziwo-Tiapo AuthorDate: Thu, 20 Jun 2019 14:54:46 -0700 Committer: Arnaldo Carvalho de Melo CommitDate: Tue, 25 Jun 2019 08:47:09 -0300 perf tools: Fix cache.h include directive Change the include path so that progress.c can find cache.h since it was previously searching in the wrong directory. Committer notes: $ ls -la tools/perf/ui/../cache.h ls: cannot access 'tools/perf/ui/../cache.h': No such file or directory So it really should include ../../util/cache.h, or plain cache.h, since we have -Iutil in INC_FLAGS in tools/perf/Makefile.config Signed-off-by: Numfor Mbiziwo-Tiapo Cc: Jiri Olsa , Cc: Luke Mujica , Cc: Stephane Eranian To: Ian Rogers Link: https://lkml.kernel.org/n/tip-pud8usyutvd2npg2vpsyg...@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/ui/progress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/ui/progress.c b/tools/perf/ui/progress.c index bbfbc91a0fa4..8cd3b64c6893 100644 --- a/tools/perf/ui/progress.c +++ b/tools/perf/ui/progress.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 #include -#include "../cache.h" +#include "../util/cache.h" #include "progress.h" static void null_progress__update(struct ui_progress *p __maybe_unused)
[PATCH 2/2] Fix perf-hooks test
The perf-hooks test fails with Address Sanitizer and Memory Sanitizer builds because it purposefully generates a segfault. Checking if these sanitizers are active when running this test will allow the perf-hooks test to pass. Signed-off-by: Numfor Mbiziwo-Tiapo --- tools/perf/tests/perf-hooks.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tools/perf/tests/perf-hooks.c b/tools/perf/tests/perf-hooks.c index a693bcf017ea..524ecba63615 100644 --- a/tools/perf/tests/perf-hooks.c +++ b/tools/perf/tests/perf-hooks.c @@ -25,7 +25,12 @@ static void the_hook(void *_hook_flags) *hook_flags = 1234; /* Generate a segfault, test perf_hooks__recover */ +#if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \ +defined(THREAD_SANITIZER) || defined(SAFESTACK_SANITIZER) + raise(SIGSEGV); +#else *p = 0; +#endif } int test__perf_hooks(struct test *test __maybe_unused, int subtest __maybe_unused) -- 2.22.0.410.gd8fdbe21b5-goog
[PATCH 1/2] Fix mmap-thread-lookup.c unitialized memory usage
Running the perf test command after building perf with a memory
sanitizer causes a warning that says:
WARNING: MemorySanitizer: use-of-uninitialized-value... in mmap-thread-lookup.c
Initializing the go variable to 0 fixes this change.
Signed-off-by: Numfor Mbiziwo-Tiapo
---
tools/perf/tests/mmap-thread-lookup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/tests/mmap-thread-lookup.c
b/tools/perf/tests/mmap-thread-lookup.c
index 5ede9b561d32..b1abf4752f35 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -52,7 +52,7 @@ static void *thread_fn(void *arg)
{
struct thread_data *td = arg;
ssize_t ret;
- int go;
+ int go = 0;
if (thread_init(td))
return NULL;
--
2.22.0.410.gd8fdbe21b5-goog
