[PATCH] perf test 39 (Session topology) dumps core on s390

[PATCH] perf test 39 (Session topology) dumps core on s390

[Thomas Richter]

perf test 39 fails with core dump on s390. The root cause
is a NULL pointer reference in function check_cpu_topology()
line 76 (or line 82 without -v).
The session->header.env.cpu variable is NULL because on s390
function process_cpu_topology() returns with error:
    socket_id number is too big.
    You may need to upgrade the perf tool.
and releases the env.cpu variable via zfree() and sets it to NULL.

Here is the gdb output:
(gdb) n
76                      pr_debug("CPU %d, core %d, socket %d\n", i,
(gdb) n

Program received signal SIGSEGV, Segmentation fault.
0x00000000010f4d9e in check_cpu_topology (path=0x3ffffffd6c8
	"/tmp/perf-test-J6CHMa", map=0x14a1740) at tests/topology.c:76
76  pr_debug("CPU %d, core %d, socket %d\n", i,
(gdb)

Make sure the env.cpu variable is not used when its NULL.
Test for NULL pointer and return TEST_SKIP if so.

Output before:
[root@p23lp27 perf]# ./perf test -F 39
39: Session topology  :Segmentation fault (core dumped)
[root@p23lp27 perf]#

Output after:
[root@p23lp27 perf]# ./perf test -vF 39
39: Session topology                                      :
--- start ---
templ file: /tmp/perf-test-Ajx59D
socket_id number is too big.You may need to upgrade the perf tool.
---- end ----
Session topology: Skip
[root@p23lp27 perf]#

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
---
 tools/perf/tests/topology.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index 17cb1bb3448c..5f366d650d2b 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -70,6 +70,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
 	session = perf_session__new(&data, false, NULL);
 	TEST_ASSERT_VAL("can't get session", session);
 
+	if (!session->header.env.cpu)
+		return TEST_SKIP;
+
 	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
 		if (!cpu_map__has(map, i))
 			continue;
@@ -95,7 +98,7 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
 {
 	char path[PATH_MAX];
 	struct cpu_map *map;
-	int ret = -1;
+	int ret;
 
 	TEST_ASSERT_VAL("can't get templ file", !get_temp(path));
 
@@ -110,12 +113,9 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
 		goto free_path;
 	}
 
-	if (check_cpu_topology(path, map))
-		goto free_map;
-	ret = 0;
-
-free_map:
+	ret = check_cpu_topology(path, map);
 	cpu_map__put(map);
+
 free_path:
 	unlink(path);
 	return ret;
-- 
2.14.3

Re: [PATCH] perf test 39 (Session topology) dumps core on s390

[Arnaldo Carvalho de Melo]

Em Thu, May 24, 2018 at 03:56:00PM +0200, Thomas Richter escreveu:
> perf test 39 fails with core dump on s390. The root cause
> is a NULL pointer reference in function check_cpu_topology()
> line 76 (or line 82 without -v).
> The session->header.env.cpu variable is NULL because on s390
> function process_cpu_topology() returns with error:
>     socket_id number is too big.
>     You may need to upgrade the perf tool.
> and releases the env.cpu variable via zfree() and sets it to NULL.
> 
> Here is the gdb output:
> (gdb) n
> 76                      pr_debug("CPU %d, core %d, socket %d\n", i,
> (gdb) n
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000000010f4d9e in check_cpu_topology (path=0x3ffffffd6c8
> 	"/tmp/perf-test-J6CHMa", map=0x14a1740) at tests/topology.c:76
> 76  pr_debug("CPU %d, core %d, socket %d\n", i,
> (gdb)
> 
> Make sure the env.cpu variable is not used when its NULL.
> Test for NULL pointer and return TEST_SKIP if so.

Ok, but can you please add a comment just before the added lines:

+     if (!session->header.env.cpu)
+             return TEST_SKIP;
+

Informing that this is so due to some bug, i.e. why is that
process_cpu_topology() returns with that "socked_id is too big" message?

This way people interested in auditing the tests to figure out why some
tests are being skipped can validate that comment and possibly come up
with some fix or better warning message after the "Skip" after that
'perf test' entry?

- Arnaldo
 
> Output before:
> [root@p23lp27 perf]# ./perf test -F 39
> 39: Session topology  :Segmentation fault (core dumped)
> [root@p23lp27 perf]#
> 
> Output after:
> [root@p23lp27 perf]# ./perf test -vF 39
> 39: Session topology                                      :
> --- start ---
> templ file: /tmp/perf-test-Ajx59D
> socket_id number is too big.You may need to upgrade the perf tool.
> ---- end ----
> Session topology: Skip
> [root@p23lp27 perf]#
> 
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
> ---
>  tools/perf/tests/topology.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
> index 17cb1bb3448c..5f366d650d2b 100644
> --- a/tools/perf/tests/topology.c
> +++ b/tools/perf/tests/topology.c
> @@ -70,6 +70,9 @@ static int check_cpu_topology(char *path, struct cpu_map *map)
>  	session = perf_session__new(&data, false, NULL);
>  	TEST_ASSERT_VAL("can't get session", session);
>  
> +	if (!session->header.env.cpu)
> +		return TEST_SKIP;
> +
>  	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
>  		if (!cpu_map__has(map, i))
>  			continue;
> @@ -95,7 +98,7 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
>  {
>  	char path[PATH_MAX];
>  	struct cpu_map *map;
> -	int ret = -1;
> +	int ret;
>  
>  	TEST_ASSERT_VAL("can't get templ file", !get_temp(path));
>  
> @@ -110,12 +113,9 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
>  		goto free_path;
>  	}
>  
> -	if (check_cpu_topology(path, map))
> -		goto free_map;
> -	ret = 0;
> -
> -free_map:
> +	ret = check_cpu_topology(path, map);
>  	cpu_map__put(map);
> +
>  free_path:
>  	unlink(path);
>  	return ret;
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] perf test 39 (Session topology) dumps core on s390

[Ravi Bangoria]

Hi Thomas,

On 05/24/2018 07:26 PM, Thomas Richter wrote:
> @@ -95,7 +98,7 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
>  {
>  	char path[PATH_MAX];
>  	struct cpu_map *map;
> -	int ret = -1;
> +	int ret;

This is failing for me:

tests/topology.c: In function ‘test__session_topology’:
tests/topology.c:101:6: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  int ret;
      ^
cc1: all warnings being treated as errors
  CC       util/intlist.o
mv: cannot stat 'tests/.topology.o.tmp': No such file or directory
/home/ravi/Workspace/linux/tools/build/Makefile.build:96: recipe for target 'tests/topology.o' failed
make[4]: *** [tests/topology.o] Error 1
make[4]: *** Waiting for unfinished jobs....


Thanks,
Ravi

Re: [PATCH] perf test 39 (Session topology) dumps core on s390

[Thomas-Mich Richter]

On 05/28/2018 05:49 AM, Ravi Bangoria wrote:
> Hi Thomas,
> 
> On 05/24/2018 07:26 PM, Thomas Richter wrote:
>> @@ -95,7 +98,7 @@ int test__session_topology(struct test *test __maybe_unused, int subtest __maybe
>>  {
>>  	char path[PATH_MAX];
>>  	struct cpu_map *map;
>> -	int ret = -1;
>> +	int ret;
> 
> This is failing for me:
> 
> tests/topology.c: In function ‘test__session_topology’:
> tests/topology.c:101:6: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   int ret;
>       ^
> cc1: all warnings being treated as errors
>   CC       util/intlist.o
> mv: cannot stat 'tests/.topology.o.tmp': No such file or directory
> /home/ravi/Workspace/linux/tools/build/Makefile.build:96: recipe for target 'tests/topology.o' failed
> make[4]: *** [tests/topology.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> 
> 
> Thanks,
> Ravi
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks for the pointer, will provide V2.

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294