The test case 'Read backward ring buffer' failed on 32-bit architectures
which were found by LKFT perf testing. The test failed on arm32 x15
device, qemu_arm32, qemu_i386, and found intermittent failure on i386;
the failure log is as below:
50: Read backward ring buffer :
--- start ---
test child forked, pid 510
Using CPUID GenuineIntel-6-9E-9
mmap size 1052672B
mmap size 8192B
Finished reading overwrite ring buffer: rewind
free(): invalid next size (fast)
test child interrupted
---- end ----
Read backward ring buffer: FAILED!
The log hints there have issue for memory usage, thus free() reports
error 'invalid next size' and directly exit for the case. Finally, this
issue is root caused as out of bounds memory access for the data array
'evsel->id'.
The backward ring buffer test invokes do_test() twice. 'evsel->id' is
allocated at the first call with the flow:
test__backward_ring_buffer()
`-> do_test()
`-> evlist__mmap()
`-> evlist__mmap_ex()
`-> perf_evsel__alloc_id()
So 'evsel->id' is allocated with one item, and it will be used in
function perf_evlist__id_add():
evsel->id[0] = id
evsel->ids = 1
At the second call for do_test(), it skips to initialize 'evsel->id'
and reuses the array which is allocated in the first call. But
'evsel->ids' contains the stale value. Thus:
evsel->id[1] = id -> out of bound access
evsel->ids = 2
To fix this issue, we will use evlist__open() and evlist__close() pair
functions to prepare and cleanup context for evlist; so 'evsel->id' and
'evsel->ids' can be initialized properly when invoke do_test() and avoid
the out of bounds memory access.
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/tests/backward-ring-buffer.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
index 338cd9faa835..5128f727c0ef 100644
--- a/tools/perf/tests/backward-ring-buffer.c
+++ b/tools/perf/tests/backward-ring-buffer.c
@@ -147,6 +147,15 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m
goto out_delete_evlist;
}
+ evlist__close(evlist);
+
+ err = evlist__open(evlist);
+ if (err < 0) {
+ pr_debug("perf_evlist__open: %s\n",
+ str_error_r(errno, sbuf, sizeof(sbuf)));
+ goto out_delete_evlist;
+ }
+
err = do_test(evlist, 1, &sample_count, &comm_count);
if (err != TEST_OK)
goto out_delete_evlist;
--
2.17.1
On Thu, Nov 07, 2019 at 10:02:44AM +0800, Leo Yan wrote:
> The test case 'Read backward ring buffer' failed on 32-bit architectures
> which were found by LKFT perf testing. The test failed on arm32 x15
> device, qemu_arm32, qemu_i386, and found intermittent failure on i386;
> the failure log is as below:
>
> 50: Read backward ring buffer :
> --- start ---
> test child forked, pid 510
> Using CPUID GenuineIntel-6-9E-9
> mmap size 1052672B
> mmap size 8192B
> Finished reading overwrite ring buffer: rewind
> free(): invalid next size (fast)
> test child interrupted
> ---- end ----
> Read backward ring buffer: FAILED!
>
> The log hints there have issue for memory usage, thus free() reports
> error 'invalid next size' and directly exit for the case. Finally, this
> issue is root caused as out of bounds memory access for the data array
> 'evsel->id'.
>
> The backward ring buffer test invokes do_test() twice. 'evsel->id' is
> allocated at the first call with the flow:
>
> test__backward_ring_buffer()
> `-> do_test()
> `-> evlist__mmap()
> `-> evlist__mmap_ex()
> `-> perf_evsel__alloc_id()
>
> So 'evsel->id' is allocated with one item, and it will be used in
> function perf_evlist__id_add():
>
> evsel->id[0] = id
> evsel->ids = 1
>
> At the second call for do_test(), it skips to initialize 'evsel->id'
> and reuses the array which is allocated in the first call. But
> 'evsel->ids' contains the stale value. Thus:
>
> evsel->id[1] = id -> out of bound access
> evsel->ids = 2
>
> To fix this issue, we will use evlist__open() and evlist__close() pair
> functions to prepare and cleanup context for evlist; so 'evsel->id' and
> 'evsel->ids' can be initialized properly when invoke do_test() and avoid
> the out of bounds memory access.
right, we need to solve this on libperf level, so it's possible
to call mmap/munmap multiple time without close/open.. I'll try
to send something, but meanwhile this is good workaround
Reviewed-by: Jiri Olsa <[email protected]>
thanks,
jirka
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/tests/backward-ring-buffer.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index 338cd9faa835..5128f727c0ef 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -147,6 +147,15 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m
> goto out_delete_evlist;
> }
>
> + evlist__close(evlist);
> +
> + err = evlist__open(evlist);
> + if (err < 0) {
> + pr_debug("perf_evlist__open: %s\n",
> + str_error_r(errno, sbuf, sizeof(sbuf)));
> + goto out_delete_evlist;
> + }
> +
> err = do_test(evlist, 1, &sample_count, &comm_count);
> if (err != TEST_OK)
> goto out_delete_evlist;
> --
> 2.17.1
>
On Thu, Nov 07, 2019 at 10:42:26AM +0100, Jiri Olsa wrote:
[...]
> > To fix this issue, we will use evlist__open() and evlist__close() pair
> > functions to prepare and cleanup context for evlist; so 'evsel->id' and
> > 'evsel->ids' can be initialized properly when invoke do_test() and avoid
> > the out of bounds memory access.
>
> right, we need to solve this on libperf level, so it's possible
> to call mmap/munmap multiple time without close/open.. I'll try
> to send something, but meanwhile this is good workaround
>
> Reviewed-by: Jiri Olsa <[email protected]>
Thanks for reviewing, Jiri.
You are welcome to send us the fixing patches, I am glad to test it on
qemu_arm.
Thanks,
Leo Yan
Em Thu, Nov 07, 2019 at 06:20:29PM +0800, Leo Yan escreveu:
> On Thu, Nov 07, 2019 at 10:42:26AM +0100, Jiri Olsa wrote:
> [...]
> > > To fix this issue, we will use evlist__open() and evlist__close() pair
> > > functions to prepare and cleanup context for evlist; so 'evsel->id' and
> > > 'evsel->ids' can be initialized properly when invoke do_test() and avoid
> > > the out of bounds memory access.
> > right, we need to solve this on libperf level, so it's possible
> > to call mmap/munmap multiple time without close/open.. I'll try
> > to send something, but meanwhile this is good workaround
> >
> > Reviewed-by: Jiri Olsa <[email protected]>
> Thanks for reviewing, Jiri.
> You are welcome to send us the fixing patches, I am glad to test it on
> qemu_arm.
Thanks, applied after adding:
Fixes: ee74701ed8ad ("perf tests: Add test to check backward ring buffer")
Cc: [email protected] # v4.10+
Please consider doing it next time,
- Arnaldo
On Thu, Nov 07, 2019 at 09:06:43AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 07, 2019 at 06:20:29PM +0800, Leo Yan escreveu:
> > On Thu, Nov 07, 2019 at 10:42:26AM +0100, Jiri Olsa wrote:
>
> > [...]
>
> > > > To fix this issue, we will use evlist__open() and evlist__close() pair
> > > > functions to prepare and cleanup context for evlist; so 'evsel->id' and
> > > > 'evsel->ids' can be initialized properly when invoke do_test() and avoid
> > > > the out of bounds memory access.
>
> > > right, we need to solve this on libperf level, so it's possible
> > > to call mmap/munmap multiple time without close/open.. I'll try
> > > to send something, but meanwhile this is good workaround
> > >
> > > Reviewed-by: Jiri Olsa <[email protected]>
>
> > Thanks for reviewing, Jiri.
>
> > You are welcome to send us the fixing patches, I am glad to test it on
> > qemu_arm.
>
> Thanks, applied after adding:
>
> Fixes: ee74701ed8ad ("perf tests: Add test to check backward ring buffer")
> Cc: [email protected] # v4.10+
>
> Please consider doing it next time,
Thanks a lot for helping to add 'Fixes' tag, Arnaldo.
Will note this for later patches.
Thanks,
Leo Yan
The following commit has been merged into the perf/core branch of tip:
Commit-ID: af8490eb2b33684e26a0a927a9d93ae43cd08890
Gitweb: https://git.kernel.org/tip/af8490eb2b33684e26a0a927a9d93ae43cd08890
Author: Leo Yan <[email protected]>
AuthorDate: Thu, 07 Nov 2019 10:02:44 +08:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Thu, 07 Nov 2019 09:04:22 -03:00
perf tests: Fix out of bounds memory access
The test case 'Read backward ring buffer' failed on 32-bit architectures
which were found by LKFT perf testing. The test failed on arm32 x15
device, qemu_arm32, qemu_i386, and found intermittent failure on i386;
the failure log is as below:
50: Read backward ring buffer :
--- start ---
test child forked, pid 510
Using CPUID GenuineIntel-6-9E-9
mmap size 1052672B
mmap size 8192B
Finished reading overwrite ring buffer: rewind
free(): invalid next size (fast)
test child interrupted
---- end ----
Read backward ring buffer: FAILED!
The log hints there have issue for memory usage, thus free() reports
error 'invalid next size' and directly exit for the case. Finally, this
issue is root caused as out of bounds memory access for the data array
'evsel->id'.
The backward ring buffer test invokes do_test() twice. 'evsel->id' is
allocated at the first call with the flow:
test__backward_ring_buffer()
`-> do_test()
`-> evlist__mmap()
`-> evlist__mmap_ex()
`-> perf_evsel__alloc_id()
So 'evsel->id' is allocated with one item, and it will be used in
function perf_evlist__id_add():
evsel->id[0] = id
evsel->ids = 1
At the second call for do_test(), it skips to initialize 'evsel->id'
and reuses the array which is allocated in the first call. But
'evsel->ids' contains the stale value. Thus:
evsel->id[1] = id -> out of bound access
evsel->ids = 2
To fix this issue, we will use evlist__open() and evlist__close() pair
functions to prepare and cleanup context for evlist; so 'evsel->id' and
'evsel->ids' can be initialized properly when invoke do_test() and avoid
the out of bounds memory access.
Fixes: ee74701ed8ad ("perf tests: Add test to check backward ring buffer")
Signed-off-by: Leo Yan <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Naresh Kamboju <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Wang Nan <[email protected]>
Cc: [email protected] # v4.10+
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/tests/backward-ring-buffer.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
index a4cd30c..15cea51 100644
--- a/tools/perf/tests/backward-ring-buffer.c
+++ b/tools/perf/tests/backward-ring-buffer.c
@@ -148,6 +148,15 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m
goto out_delete_evlist;
}
+ evlist__close(evlist);
+
+ err = evlist__open(evlist);
+ if (err < 0) {
+ pr_debug("perf_evlist__open: %s\n",
+ str_error_r(errno, sbuf, sizeof(sbuf)));
+ goto out_delete_evlist;
+ }
+
err = do_test(evlist, 1, &sample_count, &comm_count);
if (err != TEST_OK)
goto out_delete_evlist;