Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] perf record: Fix coredump with --overwrite and --max-size
To:     Namhyung Kim <namhyung@kernel.org>
CC:     <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>,
        <mark.rutland@arm.com>, <alexander.shishkin@linux.intel.com>,
        <jolsa@kernel.org>, <jiwei.sun@windriver.com>,
        <linux-perf-users@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20221227130701.124278-1-yangjihong1@huawei.com>
 <CAM9d7chDPQ9_J5=XLWbhp2AztiyVwTaRjTf20reCdriseTvX_A@mail.gmail.com>
From:   Yang Jihong <yangjihong1@huawei.com>
Message-ID: <730b1733-8778-8b82-3751-a14905cef114@huawei.com>
Date:   Thu, 29 Dec 2022 20:44:31 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <CAM9d7chDPQ9_J5=XLWbhp2AztiyVwTaRjTf20reCdriseTvX_A@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello,

On 2022/12/28 5:05, Namhyung Kim wrote:
> On Tue, Dec 27, 2022 at 5:10 AM Yang Jihong <yangjihong1@huawei.com> 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 1M -a -- sleep 1
>>    [ perf record: Woken up 1 times to write data ]
>>    perf: Segmentation fault
>>    Obtained 1 stack frames.
>>    [0xc4c67f]
>>    Segmentation fault (core dumped)
>>
>> backtrace of the core file is as follows:
>>
>>    #0  0x0000000000417990 in process_locked_synthesized_event (tool=0x0, event=0x15, sample=0x1de0, machine=0xf8) at builtin-record.c:630
>>    #1  0x000000000057ee53 in perf_event__synthesize_threads (nr_threads_synthesize=21, mmap_data=<optimized out>, needs_mmap=<optimized out>, machine=0x17ad9b0, process=<optimized out>, tool=0x0) at util/synthetic-events.c:1950
>>    #2  __machine__synthesize_threads (nr_threads_synthesize=0, data_mmap=<optimized out>, needs_mmap=<optimized out>, process=<optimized out>, threads=0x8, target=0x8, tool=0x0, machine=0x17ad9b0) at util/synthetic-events.c:1936
>>    #3  machine__synthesize_threads (machine=0x17ad9b0, target=0x8, threads=0x8, needs_mmap=<optimized out>, data_mmap=<optimized out>, nr_threads_synthesize=0) at util/synthetic-events.c:1947
>>    #4  0x000000000040165d in record__synthesize (tail=<optimized out>, rec=0xbe2520 <record>) at builtin-record.c:2010
>>    #5  0x0000000000403989 in __cmd_record (argc=<optimized out>, argv=<optimized out>, rec=0xbe2520 <record>) at builtin-record.c:2810
>>    #6  0x00000000004196ba in record__init_thread_user_masks (rec=0xbe2520 <record>, cpus=0x17a65f0) at builtin-record.c:3837
>>    #7  record__init_thread_masks (rec=0xbe2520 <record>) at builtin-record.c:3938
>>    #8  cmd_record (argc=1, argv=0x7ffdd692dc60) at builtin-record.c:4241
>>    #9  0x00000000004b701d in pager_command_config (var=0x0, value=0x15 <error: Cannot access memory at address 0x15>, data=0x1de0) at perf.c:117
>>    #10 0x00000000004b732b in get_leaf_frame_caller_aarch64 (sample=0xfffffffb, thread=0x0, usr_idx=<optimized out>) at util/arm64-frame-pointer-unwind-support.c:56
>>    #11 0x0000000000406331 in execv_dashed_external (argv=0x7ffdd692d9e8) at perf.c:410
>>    #12 run_argv (argcp=<synthetic pointer>, argv=<synthetic pointer>) at perf.c:431
>>    #13 main (argc=<optimized out>, argv=0x7ffdd692d9e8) at perf.c:562
> 
> I'm not sure this callstack is correct.
This is the backtrace printed by using the gdb to debug the core file, 
which should be normal.

The preceding example should trigger this problem as long as the perf 
file reaches max_size.
> 
>>
>> 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
>>
>> In the overwrite scenario, to synthesize non-sample events,
>> we do not need to check perf size limit.
> 
> Hmm.. I think we should prevent this kind of access after
> record__free_thread_data().  We may set nr_threads to 0
> and save the bytes_written for threads separately.
Ok, will change in next version.
> 
the value of done is 1 here, Therefore, we only need to check the value 
of done first.

Thanks,
Yang.

> Thanks,
> Namhyung
> 
> 
>>
>> Fixes: 6d57581659f7 ("perf record: Add support for limit perf output file size")
>> 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 29dcd454b8e2..c5f169150d63 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -260,7 +260,7 @@ static int record__write(struct record *rec, struct mmap *map __maybe_unused,
>>          else
>>                  rec->bytes_written += size;
>>
>> -       if (record__output_max_size_exceeded(rec) && !done) {
>> +       if (!rec->opts.tail_synthesize && record__output_max_size_exceeded(rec) && !done) {
>>                  fprintf(stderr, "[ perf record: perf size limit reached (%" PRIu64 " KB),"
>>                                  " stopping session ]\n",
>>                                  record__bytes_written(rec) >> 10);
>> --
>> 2.17.1
>>
> .
>