1. Patchew
  2. linux
  3. View series

[PATCH v3] perf record: Fix segfault with --overwrite and --max-size

Yang Jihong posted 1 patch 2 years, 8 months ago
There is a newer version of this series
tools/perf/builtin-record.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
[PATCH v3] perf record: Fix segfault with --overwrite and --max-size
Posted by Yang Jihong 2 years, 8 months ago 
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>
---

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),"
-- 
2.30.GIT
Re: [PATCH v3] perf record: Fix segfault with --overwrite and --max-size
Posted by Yang Jihong 2 years, 7 months ago 
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>
> ---
> 
> 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),"
>
Re: [PATCH v3] perf record: Fix segfault with --overwrite and --max-size
Posted by Yang Jihong 2 years, 7 months ago 
Hello,

Ping again, please take time to review, thanks.


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>
>> ---
>>
>> 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),"
>>
> 
> .
Re: [PATCH v3] perf record: Fix segfault with --overwrite and --max-size
Posted by Namhyung Kim 2 years, 7 months ago 
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,
> 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),"
> >>
> >
> > .
Re: [PATCH v3] perf record: Fix segfault with --overwrite and --max-size
Posted by Yang Jihong 2 years, 7 months ago 
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),"
>>>>
>>>
>>> .
> 
> .
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.