linux-kernel - Re: [PATCH 1/2] perf record: Propagate exit status of a command line workload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 16 Apr 2014 15:34:32 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Stephane Eranian <eranian@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...nel.org>,
	Paul Mackerras <paulus@...ba.org>,
	Namhyung Kim <namhyung.kim@....com>,
	LKML <linux-kernel@...r.kernel.org>,
	David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 1/2] perf record: Propagate exit status of a command line
 workload

On Wed, Apr 16, 2014 at 09:30:43AM +0900, Namhyung Kim wrote:

SNIP

>  
> @@ -356,6 +331,7 @@ static void workload_exec_failed_signal(int signo, siginfo_t *info,
>  static int __cmd_record(struct record *rec, int argc, const char **argv)
>  {
>  	int err;
> +	int status = 0;
>  	unsigned long waking = 0;
>  	const bool forks = argc > 0;
>  	struct machine *machine;
> @@ -367,7 +343,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  
>  	rec->progname = argv[0];
>  
> -	on_exit(record__sig_exit, rec);

so record__sig_exit took care of waiting the child even
in case the record fails before running the workload

after your change all error cases just delete session
and go away, ending up with parent dead and go_pipe
released executing the workload

with attached patch, current perf code:

[jolsa@...va perf]$ ./perf.old record -e cycles -m 10G true
rounding mmap pages size to 17179869184 bytes (4194304 pages)
Permission error mapping pages.
Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
or try again with a smaller value of -m/--mmap_pages.
(current value: 4194304)
true: Terminated

with attached patch, your change:

[jolsa@...va perf]$ ./perf record -e cycles -m 10G true
rounding mmap pages size to 17179869184 bytes (4194304 pages)
Permission error mapping pages.
Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
or try again with a smaller value of -m/--mmap_pages.
(current value: 4194304)
unable to read pipe: No such file or directory


I think that after creating the workload, all error paths
need to release(wait) the child if there's any

thanks,
jirka

---
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 59ef280..185f30a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1072,7 +1072,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist, struct target *tar
 		/*
 		 * Wait until the parent tells us to go.
 		 */
-		if (read(go_pipe[0], &bf, 1) == -1)
+		if (read(go_pipe[0], &bf, 1) != 1)
 			perror("unable to read pipe");
 
 		execvp(argv[0], (char **)argv);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/