Re: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'
On Tue, Dec 15, 2015 at 04:18:44AM +, Wang Nan wrote: > Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist > testcases when kptr_restrict is on') solves a double free problem when > 'perf test hist' calling setup_fake_machine(). However, the result is > still incorrect. For example: > > $ ./perf test -v 'filtering hist entries' > 25: Test filtering hist entries : > --- start --- > test child forked, pid 4186 > Cannot create kernel maps > test child finished with 0 > end > Test filtering hist entries: Ok > > In this case the body of this test is not get executed at all, but the > result is 'Ok'. I think 'Skip' should be better. > > The reason is that *ALL* hists test cases forget to reset err after > using it to hold an error code. > > This patch ensure err is reset. > > In case when kptr_restrict prevent normal user get kernel address, this > test should be skipped, not fail. This patch uses linux/err.h to store > error code in return value of setup_fake_machine(), and let 'EACCES' to > indicate this problem. > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Cc: Masami Hiramatsu > Cc: Namhyung Kim Acked-by: Jiri Olsa thanks, 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] perf tests: Fix false TEST_OK result for 'perf test hist'
On Tue, Dec 15, 2015 at 04:18:44AM +, Wang Nan wrote: > Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist > testcases when kptr_restrict is on') solves a double free problem when > 'perf test hist' calling setup_fake_machine(). However, the result is > still incorrect. For example: > > $ ./perf test -v 'filtering hist entries' > 25: Test filtering hist entries : > --- start --- > test child forked, pid 4186 > Cannot create kernel maps > test child finished with 0 > end > Test filtering hist entries: Ok > > In this case the body of this test is not get executed at all, but the > result is 'Ok'. I think 'Skip' should be better. > > The reason is that *ALL* hists test cases forget to reset err after > using it to hold an error code. > > This patch ensure err is reset. > > In case when kptr_restrict prevent normal user get kernel address, this > test should be skipped, not fail. This patch uses linux/err.h to store > error code in return value of setup_fake_machine(), and let 'EACCES' to > indicate this problem. > > Signed-off-by: Wang Nan> Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Cc: Masami Hiramatsu > Cc: Namhyung Kim Acked-by: Jiri Olsa thanks, 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] perf tests: Fix false TEST_OK result for 'perf test hist'
Hi, On Tue, Dec 15, 2015 at 04:18:44AM +, Wang Nan wrote: > Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist > testcases when kptr_restrict is on') solves a double free problem when > 'perf test hist' calling setup_fake_machine(). However, the result is > still incorrect. For example: > > $ ./perf test -v 'filtering hist entries' > 25: Test filtering hist entries : > --- start --- > test child forked, pid 4186 > Cannot create kernel maps > test child finished with 0 > end > Test filtering hist entries: Ok > > In this case the body of this test is not get executed at all, but the > result is 'Ok'. I think 'Skip' should be better. Right, I missed this part. But on second thought, I think it should not create real kernel maps. What setup_fake_machine() does is to create/insert fake dso/mmap/symbols for testing hist functions. So it has to use the predefined info not real ones. > > The reason is that *ALL* hists test cases forget to reset err after > using it to hold an error code. > > This patch ensure err is reset. > > In case when kptr_restrict prevent normal user get kernel address, this > test should be skipped, not fail. This patch uses linux/err.h to store > error code in return value of setup_fake_machine(), and let 'EACCES' to > indicate this problem. Please just remove the machine__create_kernel_maps() in setup_fake_machine() and simply return TEST_FAIL if there's an error. Thanks, Namhyung > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Cc: Masami Hiramatsu > Cc: Namhyung Kim > --- > tools/perf/tests/hists_common.c | 12 +--- > tools/perf/tests/hists_common.h | 1 + > tools/perf/tests/hists_cumulate.c | 6 +- > tools/perf/tests/hists_filter.c | 6 +- > tools/perf/tests/hists_link.c | 6 +- > tools/perf/tests/hists_output.c | 6 +- > 6 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c > index bcfd081..2eca1b2 100644 > --- a/tools/perf/tests/hists_common.c > +++ b/tools/perf/tests/hists_common.c > @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines > *machines) > { > struct machine *machine = machines__find(machines, HOST_KERNEL_ID); > size_t i; > + int err = -ENOMEM; > > if (machine == NULL) { > pr_debug("Not enough memory for machine setup\n"); > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > if (machine__create_kernel_maps(machine)) { > pr_debug("Cannot create kernel maps\n"); > - return NULL; > + if (symbol_conf.kptr_restrict) { > + pr_debug("Hint: Check > /proc/sys/kernel/kptr_restrict.\n"); > + err = -EACCES; > + } > + return ERR_PTR(err); > } > > for (i = 0; i < ARRAY_SIZE(fake_threads); i++) { > @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines > *machines) > out: > pr_debug("Not enough memory for machine setup\n"); > machine__delete_threads(machine); > - return NULL; > + machine__exit(machine); > + return ERR_PTR(err); > } > > void print_hists_in(struct hists *hists) > diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h > index 888254e..0252eae 100644 > --- a/tools/perf/tests/hists_common.h > +++ b/tools/perf/tests/hists_common.h > @@ -1,5 +1,6 @@ > #ifndef __PERF_TESTS__HISTS_COMMON_H__ > #define __PERF_TESTS__HISTS_COMMON_H__ > +#include > > struct machine; > struct machines; > diff --git a/tools/perf/tests/hists_cumulate.c > b/tools/perf/tests/hists_cumulate.c > index 8292948..c211075 100644 > --- a/tools/perf/tests/hists_cumulate.c > +++ b/tools/perf/tests/hists_cumulate.c > @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused) > err = parse_events(evlist, "cpu-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > machines__init(); > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c > index ccb5b49..795a04d 100644 > --- a/tools/perf/tests/hists_filter.c > +++ b/tools/perf/tests/hists_filter.c > @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused) > err = parse_events(evlist, "task-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > /* default sort order (comm,dso,sym) will be used */ > if (setup_sorting() < 0) > @@ -129,8 +130,11 @@
[PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'
Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist testcases when kptr_restrict is on') solves a double free problem when 'perf test hist' calling setup_fake_machine(). However, the result is still incorrect. For example: $ ./perf test -v 'filtering hist entries' 25: Test filtering hist entries : --- start --- test child forked, pid 4186 Cannot create kernel maps test child finished with 0 end Test filtering hist entries: Ok In this case the body of this test is not get executed at all, but the result is 'Ok'. I think 'Skip' should be better. The reason is that *ALL* hists test cases forget to reset err after using it to hold an error code. This patch ensure err is reset. In case when kptr_restrict prevent normal user get kernel address, this test should be skipped, not fail. This patch uses linux/err.h to store error code in return value of setup_fake_machine(), and let 'EACCES' to indicate this problem. Signed-off-by: Wang Nan Cc: Arnaldo Carvalho de Melo Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim --- tools/perf/tests/hists_common.c | 12 +--- tools/perf/tests/hists_common.h | 1 + tools/perf/tests/hists_cumulate.c | 6 +- tools/perf/tests/hists_filter.c | 6 +- tools/perf/tests/hists_link.c | 6 +- tools/perf/tests/hists_output.c | 6 +- 6 files changed, 30 insertions(+), 7 deletions(-) diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c index bcfd081..2eca1b2 100644 --- a/tools/perf/tests/hists_common.c +++ b/tools/perf/tests/hists_common.c @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines *machines) { struct machine *machine = machines__find(machines, HOST_KERNEL_ID); size_t i; + int err = -ENOMEM; if (machine == NULL) { pr_debug("Not enough memory for machine setup\n"); - return NULL; + return ERR_PTR(-ENOMEM); } if (machine__create_kernel_maps(machine)) { pr_debug("Cannot create kernel maps\n"); - return NULL; + if (symbol_conf.kptr_restrict) { + pr_debug("Hint: Check /proc/sys/kernel/kptr_restrict.\n"); + err = -EACCES; + } + return ERR_PTR(err); } for (i = 0; i < ARRAY_SIZE(fake_threads); i++) { @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines *machines) out: pr_debug("Not enough memory for machine setup\n"); machine__delete_threads(machine); - return NULL; + machine__exit(machine); + return ERR_PTR(err); } void print_hists_in(struct hists *hists) diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h index 888254e..0252eae 100644 --- a/tools/perf/tests/hists_common.h +++ b/tools/perf/tests/hists_common.h @@ -1,5 +1,6 @@ #ifndef __PERF_TESTS__HISTS_COMMON_H__ #define __PERF_TESTS__HISTS_COMMON_H__ +#include struct machine; struct machines; diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c index 8292948..c211075 100644 --- a/tools/perf/tests/hists_cumulate.c +++ b/tools/perf/tests/hists_cumulate.c @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused) err = parse_events(evlist, "cpu-clock", NULL); if (err) goto out; + err = TEST_FAIL; machines__init(); /* setup threads/dso/map/symbols also */ machine = setup_fake_machine(); - if (!machine) + if (IS_ERR(machine)) { + if (PTR_ERR(machine) == -EACCES) + err = TEST_SKIP; goto out; + } if (verbose > 1) machine__fprintf(machine, stderr); diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c index ccb5b49..795a04d 100644 --- a/tools/perf/tests/hists_filter.c +++ b/tools/perf/tests/hists_filter.c @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused) err = parse_events(evlist, "task-clock", NULL); if (err) goto out; + err = TEST_FAIL; /* default sort order (comm,dso,sym) will be used */ if (setup_sorting() < 0) @@ -129,8 +130,11 @@ int test__hists_filter(int subtest __maybe_unused) /* setup threads/dso/map/symbols also */ machine = setup_fake_machine(); - if (!machine) + if (IS_ERR(machine)) { + if (PTR_ERR(machine) == -EACCES) + err = TEST_SKIP; goto out; + } if (verbose > 1) machine__fprintf(machine, stderr); diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c index 6243e2b..f5f37ef9 100644 --- a/tools/perf/tests/hists_link.c +++ b/tools/perf/tests/hists_link.c @@ -293,6 +293,7 @@ int test__hists_link(int subtest
Re: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'
Hi, On Tue, Dec 15, 2015 at 04:18:44AM +, Wang Nan wrote: > Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist > testcases when kptr_restrict is on') solves a double free problem when > 'perf test hist' calling setup_fake_machine(). However, the result is > still incorrect. For example: > > $ ./perf test -v 'filtering hist entries' > 25: Test filtering hist entries : > --- start --- > test child forked, pid 4186 > Cannot create kernel maps > test child finished with 0 > end > Test filtering hist entries: Ok > > In this case the body of this test is not get executed at all, but the > result is 'Ok'. I think 'Skip' should be better. Right, I missed this part. But on second thought, I think it should not create real kernel maps. What setup_fake_machine() does is to create/insert fake dso/mmap/symbols for testing hist functions. So it has to use the predefined info not real ones. > > The reason is that *ALL* hists test cases forget to reset err after > using it to hold an error code. > > This patch ensure err is reset. > > In case when kptr_restrict prevent normal user get kernel address, this > test should be skipped, not fail. This patch uses linux/err.h to store > error code in return value of setup_fake_machine(), and let 'EACCES' to > indicate this problem. Please just remove the machine__create_kernel_maps() in setup_fake_machine() and simply return TEST_FAIL if there's an error. Thanks, Namhyung > > Signed-off-by: Wang Nan> Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Cc: Masami Hiramatsu > Cc: Namhyung Kim > --- > tools/perf/tests/hists_common.c | 12 +--- > tools/perf/tests/hists_common.h | 1 + > tools/perf/tests/hists_cumulate.c | 6 +- > tools/perf/tests/hists_filter.c | 6 +- > tools/perf/tests/hists_link.c | 6 +- > tools/perf/tests/hists_output.c | 6 +- > 6 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c > index bcfd081..2eca1b2 100644 > --- a/tools/perf/tests/hists_common.c > +++ b/tools/perf/tests/hists_common.c > @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines > *machines) > { > struct machine *machine = machines__find(machines, HOST_KERNEL_ID); > size_t i; > + int err = -ENOMEM; > > if (machine == NULL) { > pr_debug("Not enough memory for machine setup\n"); > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > if (machine__create_kernel_maps(machine)) { > pr_debug("Cannot create kernel maps\n"); > - return NULL; > + if (symbol_conf.kptr_restrict) { > + pr_debug("Hint: Check > /proc/sys/kernel/kptr_restrict.\n"); > + err = -EACCES; > + } > + return ERR_PTR(err); > } > > for (i = 0; i < ARRAY_SIZE(fake_threads); i++) { > @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines > *machines) > out: > pr_debug("Not enough memory for machine setup\n"); > machine__delete_threads(machine); > - return NULL; > + machine__exit(machine); > + return ERR_PTR(err); > } > > void print_hists_in(struct hists *hists) > diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h > index 888254e..0252eae 100644 > --- a/tools/perf/tests/hists_common.h > +++ b/tools/perf/tests/hists_common.h > @@ -1,5 +1,6 @@ > #ifndef __PERF_TESTS__HISTS_COMMON_H__ > #define __PERF_TESTS__HISTS_COMMON_H__ > +#include > > struct machine; > struct machines; > diff --git a/tools/perf/tests/hists_cumulate.c > b/tools/perf/tests/hists_cumulate.c > index 8292948..c211075 100644 > --- a/tools/perf/tests/hists_cumulate.c > +++ b/tools/perf/tests/hists_cumulate.c > @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused) > err = parse_events(evlist, "cpu-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > machines__init(); > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c > index ccb5b49..795a04d 100644 > --- a/tools/perf/tests/hists_filter.c > +++ b/tools/perf/tests/hists_filter.c > @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused) > err = parse_events(evlist, "task-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > >
[PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'
Commit 71d6de64feddd4b45326fba2111b3006d9e0 ('perf test: Fix hist testcases when kptr_restrict is on') solves a double free problem when 'perf test hist' calling setup_fake_machine(). However, the result is still incorrect. For example: $ ./perf test -v 'filtering hist entries' 25: Test filtering hist entries : --- start --- test child forked, pid 4186 Cannot create kernel maps test child finished with 0 end Test filtering hist entries: Ok In this case the body of this test is not get executed at all, but the result is 'Ok'. I think 'Skip' should be better. The reason is that *ALL* hists test cases forget to reset err after using it to hold an error code. This patch ensure err is reset. In case when kptr_restrict prevent normal user get kernel address, this test should be skipped, not fail. This patch uses linux/err.h to store error code in return value of setup_fake_machine(), and let 'EACCES' to indicate this problem. Signed-off-by: Wang NanCc: Arnaldo Carvalho de Melo Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim --- tools/perf/tests/hists_common.c | 12 +--- tools/perf/tests/hists_common.h | 1 + tools/perf/tests/hists_cumulate.c | 6 +- tools/perf/tests/hists_filter.c | 6 +- tools/perf/tests/hists_link.c | 6 +- tools/perf/tests/hists_output.c | 6 +- 6 files changed, 30 insertions(+), 7 deletions(-) diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c index bcfd081..2eca1b2 100644 --- a/tools/perf/tests/hists_common.c +++ b/tools/perf/tests/hists_common.c @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines *machines) { struct machine *machine = machines__find(machines, HOST_KERNEL_ID); size_t i; + int err = -ENOMEM; if (machine == NULL) { pr_debug("Not enough memory for machine setup\n"); - return NULL; + return ERR_PTR(-ENOMEM); } if (machine__create_kernel_maps(machine)) { pr_debug("Cannot create kernel maps\n"); - return NULL; + if (symbol_conf.kptr_restrict) { + pr_debug("Hint: Check /proc/sys/kernel/kptr_restrict.\n"); + err = -EACCES; + } + return ERR_PTR(err); } for (i = 0; i < ARRAY_SIZE(fake_threads); i++) { @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines *machines) out: pr_debug("Not enough memory for machine setup\n"); machine__delete_threads(machine); - return NULL; + machine__exit(machine); + return ERR_PTR(err); } void print_hists_in(struct hists *hists) diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h index 888254e..0252eae 100644 --- a/tools/perf/tests/hists_common.h +++ b/tools/perf/tests/hists_common.h @@ -1,5 +1,6 @@ #ifndef __PERF_TESTS__HISTS_COMMON_H__ #define __PERF_TESTS__HISTS_COMMON_H__ +#include struct machine; struct machines; diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c index 8292948..c211075 100644 --- a/tools/perf/tests/hists_cumulate.c +++ b/tools/perf/tests/hists_cumulate.c @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused) err = parse_events(evlist, "cpu-clock", NULL); if (err) goto out; + err = TEST_FAIL; machines__init(); /* setup threads/dso/map/symbols also */ machine = setup_fake_machine(); - if (!machine) + if (IS_ERR(machine)) { + if (PTR_ERR(machine) == -EACCES) + err = TEST_SKIP; goto out; + } if (verbose > 1) machine__fprintf(machine, stderr); diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c index ccb5b49..795a04d 100644 --- a/tools/perf/tests/hists_filter.c +++ b/tools/perf/tests/hists_filter.c @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused) err = parse_events(evlist, "task-clock", NULL); if (err) goto out; + err = TEST_FAIL; /* default sort order (comm,dso,sym) will be used */ if (setup_sorting() < 0) @@ -129,8 +130,11 @@ int test__hists_filter(int subtest __maybe_unused) /* setup threads/dso/map/symbols also */ machine = setup_fake_machine(); - if (!machine) + if (IS_ERR(machine)) { + if (PTR_ERR(machine) == -EACCES) + err = TEST_SKIP; goto out; + } if (verbose > 1) machine__fprintf(machine, stderr); diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c index 6243e2b..f5f37ef9 100644 ---