Messages in this thread | | | Subject | Re: [PATCH v3] perf record: Fix segfault with --overwrite and --max-size | From | Yang Jihong <> | Date | Mon, 6 Feb 2023 11:40:32 +0800 |
| |
Hello,
On 2023/2/6 10:44, Namhyung Kim wrote: > Hello, > > On Sun, Feb 5, 2023 at 5:29 PM Yang Jihong <yangjihong1@huawei.com> wrote: >> >> Hello, >> >> Ping again, please take time to review, thanks. > > Sorry for the late reply. Now it looks good to me. Thanks for the review.
Thanks, Yang. > >> >> >> Thanks, >> Yang >> >> >> On 2023/1/28 9:27, Yang Jihong wrote: >>> Hello, >>> >>> The data written by the thread (data.X file) has been saved as >>> recommended in v2 patch. Please check whether it is feasible. Thank you. >>> >>> Thanks, >>> Yang >>> >>> On 2023/1/13 15:38, Yang Jihong wrote: >>>> When --overwrite and --max-size options of perf record are used together, >>>> a segmentation fault occurs. The following is an example: >>>> >>>> # perf record -e sched:sched* --overwrite --max-size 1K -a -- sleep 1 >>>> [ perf record: Woken up 1 times to write data ] >>>> perf: Segmentation fault >>>> Obtained 12 stack frames. >>>> ./perf/perf(+0x197673) [0x55f99710b673] >>>> /lib/x86_64-linux-gnu/libc.so.6(+0x3ef0f) [0x7fa45f3cff0f] >>>> ./perf/perf(+0x8eb40) [0x55f997002b40] >>>> ./perf/perf(+0x1f6882) [0x55f99716a882] >>>> ./perf/perf(+0x794c2) [0x55f996fed4c2] >>>> ./perf/perf(+0x7b7c7) [0x55f996fef7c7] >>>> ./perf/perf(+0x9074b) [0x55f99700474b] >>>> ./perf/perf(+0x12e23c) [0x55f9970a223c] >>>> ./perf/perf(+0x12e54a) [0x55f9970a254a] >>>> ./perf/perf(+0x7db60) [0x55f996ff1b60] >>>> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe6) >>>> [0x7fa45f3b2c86] >>>> ./perf/perf(+0x7dfe9) [0x55f996ff1fe9] >>>> Segmentation fault (core dumped) >>>> >>>> backtrace of the core file is as follows: >>>> >>>> (gdb) bt >>>> #0 record__bytes_written (rec=0x55f99755a200 <record>) at >>>> builtin-record.c:234 >>>> #1 record__output_max_size_exceeded (rec=0x55f99755a200 <record>) >>>> at builtin-record.c:242 >>>> #2 record__write (map=0x0, size=12816, bf=0x55f9978da2e0, >>>> rec=0x55f99755a200 <record>) at builtin-record.c:263 >>>> #3 process_synthesized_event (tool=tool@entry=0x55f99755a200 >>>> <record>, event=event@entry=0x55f9978da2e0, sample=sample@entry=0x0, >>>> machine=machine@entry=0x55f997893658) at builtin-record.c:618 >>>> #4 0x000055f99716a883 in __perf_event__synthesize_id_index >>>> (tool=tool@entry=0x55f99755a200 <record>, >>>> process=process@entry=0x55f997002aa0 <process_synthesized_event>, >>>> evlist=0x55f9978928b0, machine=machine@entry=0x55f997893658, >>>> from=from@entry=0) at util/synthetic-events.c:1895 >>>> #5 0x000055f99716a91f in perf_event__synthesize_id_index >>>> (tool=tool@entry=0x55f99755a200 <record>, >>>> process=process@entry=0x55f997002aa0 <process_synthesized_event>, >>>> evlist=<optimized out>, machine=machine@entry=0x55f997893658) >>>> at util/synthetic-events.c:1905 >>>> #6 0x000055f996fed4c3 in record__synthesize (tail=tail@entry=true, >>>> rec=0x55f99755a200 <record>) at builtin-record.c:1997 >>>> #7 0x000055f996fef7c8 in __cmd_record (argc=argc@entry=2, >>>> argv=argv@entry=0x7ffc67551260, rec=0x55f99755a200 <record>) at >>>> builtin-record.c:2802 >>>> #8 0x000055f99700474c in cmd_record (argc=<optimized out>, >>>> argv=0x7ffc67551260) at builtin-record.c:4258 >>>> #9 0x000055f9970a223d in run_builtin (p=0x55f997564d88 >>>> <commands+264>, argc=10, argv=0x7ffc67551260) at perf.c:330 >>>> #10 0x000055f9970a254b in handle_internal_command (argc=10, >>>> argv=0x7ffc67551260) at perf.c:384 >>>> #11 0x000055f996ff1b61 in run_argv (argcp=<synthetic pointer>, >>>> argv=<synthetic pointer>) at perf.c:428 >>>> #12 main (argc=<optimized out>, argv=0x7ffc67551260) at perf.c:562 >>>> >>>> The reason is that record__bytes_written accesses the freed memory >>>> rec->thread_data, >>>> The process is as follows: >>>> __cmd_record >>>> -> record__free_thread_data >>>> -> zfree(&rec->thread_data) // free rec->thread_data >>>> -> record__synthesize >>>> -> perf_event__synthesize_id_index >>>> -> process_synthesized_event >>>> -> record__write >>>> -> record__bytes_written // access rec->thread_data >>>> >>>> We add a member variable "thread_bytes_written" in the struct "record" >>>> to save the data size written by the threads. >>>> >>>> Fixes: 6d57581659f7 ("perf record: Add support for limit perf output >>>> file size") >>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> > > Acked-by: Namhyung Kim <namhyung@kernel.org> > > Thanks, > Namhyung > > >>>> --- >>>> >>>> Changes since v2: >>>> - Save data size written by threads to calculate the correct total >>>> data size. >>>> - Update commit message. >>>> >>>> Changes since v1: >>>> - Add variable check in record__bytes_written for code hardening. >>>> - Save bytes_written separately to reduce one calculation. >>>> - Remove rec->opts.tail_synthesize check. >>>> >>>> tools/perf/builtin-record.c | 16 ++++++---------- >>>> 1 file changed, 6 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>>> index 29dcd454b8e2..8374117e66f6 100644 >>>> --- a/tools/perf/builtin-record.c >>>> +++ b/tools/perf/builtin-record.c >>>> @@ -154,6 +154,7 @@ struct record { >>>> struct perf_tool tool; >>>> struct record_opts opts; >>>> u64 bytes_written; >>>> + u64 thread_bytes_written; >>>> struct perf_data data; >>>> struct auxtrace_record *itr; >>>> struct evlist *evlist; >>>> @@ -226,14 +227,7 @@ static bool switch_output_time(struct record *rec) >>>> static u64 record__bytes_written(struct record *rec) >>>> { >>>> - int t; >>>> - u64 bytes_written = rec->bytes_written; >>>> - struct record_thread *thread_data = rec->thread_data; >>>> - >>>> - for (t = 0; t < rec->nr_threads; t++) >>>> - bytes_written += thread_data[t].bytes_written; >>>> - >>>> - return bytes_written; >>>> + return rec->bytes_written + rec->thread_bytes_written; >>>> } >>>> static bool record__output_max_size_exceeded(struct record *rec) >>>> @@ -255,10 +249,12 @@ static int record__write(struct record *rec, >>>> struct mmap *map __maybe_unused, >>>> return -1; >>>> } >>>> - if (map && map->file) >>>> + if (map && map->file) { >>>> thread->bytes_written += size; >>>> - else >>>> + rec->thread_bytes_written += size; >>>> + } else { >>>> rec->bytes_written += size; >>>> + } >>>> if (record__output_max_size_exceeded(rec) && !done) { >>>> fprintf(stderr, "[ perf record: perf size limit reached (%" >>>> PRIu64 " KB)," >>>> >>> >>> . > > . >
| |