* [PATCH 1/3] perf record: Fix possible incorrect free in record__switch_output()
2024-01-19 4:03 [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Yang Jihong
@ 2024-01-19 4:03 ` Yang Jihong
2024-01-19 4:03 ` [PATCH 2/3] perf record: Check conflict between '--timestamp-filename' option and pipe mode before recording Yang Jihong
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Yang Jihong @ 2024-01-19 4:03 UTC (permalink / raw)
To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
jolsa, irogers, adrian.hunter, linux-perf-users, linux-kernel
Cc: yangjihong1
perf_data__switch() may not assign a legal value to 'new_filename'.
In this case, 'new_filename' uses the on-stack value, which may cause a
incorrect free and unexpected result.
Fixes: 03724b2e9c45 ("perf record: Allow to limit number of reported perf.data files")
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
tools/perf/builtin-record.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 91e6828c38cc..fb8d4067b76c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1830,8 +1830,8 @@ static int
record__switch_output(struct record *rec, bool at_exit)
{
struct perf_data *data = &rec->data;
+ char *new_filename = NULL;
int fd, err;
- char *new_filename;
/* Same Size: "2015122520103046"*/
char timestamp[] = "InvalidTimestamp";
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/3] perf record: Check conflict between '--timestamp-filename' option and pipe mode before recording
2024-01-19 4:03 [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Yang Jihong
2024-01-19 4:03 ` [PATCH 1/3] perf record: Fix possible incorrect free in record__switch_output() Yang Jihong
@ 2024-01-19 4:03 ` Yang Jihong
2024-01-19 4:03 ` [PATCH 3/3] perf data: Minor code style alignment cleanup Yang Jihong
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Yang Jihong @ 2024-01-19 4:03 UTC (permalink / raw)
To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
jolsa, irogers, adrian.hunter, linux-perf-users, linux-kernel
Cc: yangjihong1
In pipe mode, no need to switch perf data output, therefore,
'--timestamp-filename' option should not take effect.
Check the conflict before recording and output WARNING.
In this case, the check pipe mode in perf_data__switch() can be removed.
Before:
# perf record --timestamp-filename -o- perf test -w noploop | perf report -i- --percent-limit=1
# To display the perf.data header info, please use --header/--header-only options.
#
[ perf record: Woken up 1 times to write data ]
[ perf record: Dump -.2024011812110182 ]
#
# Total Lost Samples: 0
#
# Samples: 4K of event 'cycles:P'
# Event count (approx.): 2176784359
#
# Overhead Command Shared Object Symbol
# ........ ....... .................... ......................................
#
97.83% perf perf [.] noploop
#
# (Tip: Print event counts in CSV format with: perf stat -x,)
#
After:
# perf record --timestamp-filename -o- perf test -w noploop | perf report -i- --percent-limit=1
WARNING: --timestamp-filename option is not available in pipe mode.
# To display the perf.data header info, please use --header/--header-only options.
#
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.000 MB - ]
#
# Total Lost Samples: 0
#
# Samples: 4K of event 'cycles:P'
# Event count (approx.): 2185575421
#
# Overhead Command Shared Object Symbol
# ........ ....... ..................... .............................................
#
97.75% perf perf [.] noploop
#
# (Tip: Profiling branch (mis)predictions with: perf record -b / perf report)
#
Fixes: ecfd7a9c044e ("perf record: Add '--timestamp-filename' option to append timestamp to output file name")
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
tools/perf/builtin-record.c | 5 +++++
tools/perf/util/data.c | 2 --
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index fb8d4067b76c..5e3ea5cf1429 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2472,6 +2472,11 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
if (data->is_pipe && rec->evlist->core.nr_entries == 1)
rec->opts.sample_id = true;
+ if (rec->timestamp_filename && perf_data__is_pipe(data)) {
+ rec->timestamp_filename = false;
+ pr_warning("WARNING: --timestamp-filename option is not available in pipe mode.\n");
+ }
+
evlist__uniquify_name(rec->evlist);
/* Debug message used by test scripts */
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index c29d8a382b19..550675ce0b78 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -430,8 +430,6 @@ int perf_data__switch(struct perf_data *data,
{
int ret;
- if (check_pipe(data))
- return -EINVAL;
if (perf_data__is_read(data))
return -EINVAL;
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 3/3] perf data: Minor code style alignment cleanup
2024-01-19 4:03 [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Yang Jihong
2024-01-19 4:03 ` [PATCH 1/3] perf record: Fix possible incorrect free in record__switch_output() Yang Jihong
2024-01-19 4:03 ` [PATCH 2/3] perf record: Check conflict between '--timestamp-filename' option and pipe mode before recording Yang Jihong
@ 2024-01-19 4:03 ` Yang Jihong
2024-01-19 22:06 ` [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Namhyung Kim
2024-01-22 20:38 ` Namhyung Kim
4 siblings, 0 replies; 6+ messages in thread
From: Yang Jihong @ 2024-01-19 4:03 UTC (permalink / raw)
To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
jolsa, irogers, adrian.hunter, linux-perf-users, linux-kernel
Cc: yangjihong1
Minor code style alignment cleanup for perf_data__switch() and
perf_data__write().
No functional change.
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
tools/perf/builtin-record.c | 7 ++++---
tools/perf/util/data.c | 8 ++++----
tools/perf/util/data.h | 6 +++---
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 5e3ea5cf1429..0b6f29fa0064 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1853,16 +1853,17 @@ record__switch_output(struct record *rec, bool at_exit)
}
fd = perf_data__switch(data, timestamp,
- rec->session->header.data_offset,
- at_exit, &new_filename);
+ rec->session->header.data_offset,
+ at_exit, &new_filename);
if (fd >= 0 && !at_exit) {
rec->bytes_written = 0;
rec->session->header.data_size = 0;
}
- if (!quiet)
+ if (!quiet) {
fprintf(stderr, "[ perf record: Dump %s.%s ]\n",
data->path, timestamp);
+ }
if (rec->switch_output.num_files) {
int n = rec->switch_output.cur_file + 1;
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 550675ce0b78..08c4bfbd817f 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -413,7 +413,7 @@ ssize_t perf_data_file__write(struct perf_data_file *file,
}
ssize_t perf_data__write(struct perf_data *data,
- void *buf, size_t size)
+ void *buf, size_t size)
{
if (data->use_stdio) {
if (fwrite(buf, size, 1, data->file.fptr) == 1)
@@ -424,9 +424,9 @@ ssize_t perf_data__write(struct perf_data *data,
}
int perf_data__switch(struct perf_data *data,
- const char *postfix,
- size_t pos, bool at_exit,
- char **new_filepath)
+ const char *postfix,
+ size_t pos, bool at_exit,
+ char **new_filepath)
{
int ret;
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index effcc195d7e9..110f3ebde30f 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -80,7 +80,7 @@ int perf_data__open(struct perf_data *data);
void perf_data__close(struct perf_data *data);
ssize_t perf_data__read(struct perf_data *data, void *buf, size_t size);
ssize_t perf_data__write(struct perf_data *data,
- void *buf, size_t size);
+ void *buf, size_t size);
ssize_t perf_data_file__write(struct perf_data_file *file,
void *buf, size_t size);
/*
@@ -91,8 +91,8 @@ ssize_t perf_data_file__write(struct perf_data_file *file,
* Return value is fd of new output.
*/
int perf_data__switch(struct perf_data *data,
- const char *postfix,
- size_t pos, bool at_exit, char **new_filepath);
+ const char *postfix,
+ size_t pos, bool at_exit, char **new_filepath);
int perf_data__create_dir(struct perf_data *data, int nr);
int perf_data__open_dir(struct perf_data *data);
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode
2024-01-19 4:03 [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Yang Jihong
` (2 preceding siblings ...)
2024-01-19 4:03 ` [PATCH 3/3] perf data: Minor code style alignment cleanup Yang Jihong
@ 2024-01-19 22:06 ` Namhyung Kim
2024-01-22 20:38 ` Namhyung Kim
4 siblings, 0 replies; 6+ messages in thread
From: Namhyung Kim @ 2024-01-19 22:06 UTC (permalink / raw)
To: Yang Jihong
Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
irogers, adrian.hunter, linux-perf-users, linux-kernel
Hello,
On Thu, Jan 18, 2024 at 8:07 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>
> When perf record uses '--timestamp-filename' option and pipe mode,
> will occur segfault:
>
> # perf record --timestamp-filename -o- true 1>/dev/null
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Dump -.2024011813361681 ]
> perf: Segmentation fault
> Segmentation fault (core dumped)
>
> Debug the core file by using the gdb:
>
> # gdb perf core.3706841
> <SNIP>
> [New LWP 3706841]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> Core was generated by `perf record --timestamp-filename -o- true'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 evsel__free_stat_priv (evsel=0x555ea56197af) at util/stat.c:145
> 145 struct perf_stat_evsel *ps = evsel->stats;
> (gdb) bt
> #0 evsel__free_stat_priv (evsel=0x555ea56197af) at util/stat.c:145
> #1 evlist__free_stats (evlist=evlist@entry=0x555bf0dad1b0) at util/stat.c:215
> #2 0x0000555befd425e8 in evlist__delete (evlist=0x555bf0dad1b0) at util/evlist.c:175
> #3 0x0000555befd42785 in evlist__delete (evlist=<optimized out>) at util/evlist.c:181
> #4 0x0000555befc7f547 in cmd_record (argc=<optimized out>, argv=<optimized out>) at builtin-record.c:4252
> #5 0x0000555befd27700 in run_builtin (p=p@entry=0x555bf05acf88 <commands+264>, argc=argc@entry=4, argv=argv@entry=0x7ffc2696a920) at perf.c:349
> #6 0x0000555befc68751 in handle_internal_command (argv=0x7ffc2696a920, argc=4) at perf.c:402
> #7 run_argv (argv=<synthetic pointer>, argcp=<synthetic pointer>) at perf.c:446
> #8 main (argc=4, argv=0x7ffc2696a920) at perf.c:562
>
> It is found that the memory of the evsel is modified.
> The reason is that perf_data__switch() not initialized 'new_filename',
> as a result, it uses the on-stack variable. It happens that the evsel
> address is stored here. 'new_filename' is freed later when uses
> '--timestamp-filename' option. Therefore, segfault occurs in the evsel_delete().
>
>
> 1. patch1 fixes this problem.
> 2. patch2 to optimize the process, check conflict between
> '--timestamp-filename' option and the pipe mode before recording to
> avoid switch perf data.
> 3. patch3 fixes the code style problem by the way.
>
>
> Yang Jihong (3):
> perf record: Fix possible incorrect free in record__switch_output()
> perf record: Check conflict between '--timestamp-filename' option and
> pipe mode before recording
> perf data: Minor code style alignment cleanup
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
>
> tools/perf/builtin-record.c | 14 ++++++++++----
> tools/perf/util/data.c | 10 ++++------
> tools/perf/util/data.h | 6 +++---
> 3 files changed, 17 insertions(+), 13 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode
2024-01-19 4:03 [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Yang Jihong
` (3 preceding siblings ...)
2024-01-19 22:06 ` [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename' option and pipe mode Namhyung Kim
@ 2024-01-22 20:38 ` Namhyung Kim
4 siblings, 0 replies; 6+ messages in thread
From: Namhyung Kim @ 2024-01-22 20:38 UTC (permalink / raw)
To: peterz, mingo, linux-kernel, mark.rutland, linux-perf-users,
irogers, alexander.shishkin, jolsa, Yang Jihong, adrian.hunter,
acme
On Fri, 19 Jan 2024 04:03:01 +0000, Yang Jihong wrote:
> When perf record uses '--timestamp-filename' option and pipe mode,
> will occur segfault:
>
> # perf record --timestamp-filename -o- true 1>/dev/null
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Dump -.2024011813361681 ]
> perf: Segmentation fault
> Segmentation fault (core dumped)
>
> [...]
Applied to perf-tools-next, thanks!
Namhyung
^ permalink raw reply [flat|nested] 6+ messages in thread