Received-SPF: pass (google.com: domain of linux-kernel+bounces-31602-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240119040304.3708522-1-yangjihong1@huawei.com>
In-Reply-To: <20240119040304.3708522-1-yangjihong1@huawei.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 19 Jan 2024 14:06:12 -0800
Message-ID: <CAM9d7chheMq=J1kHCTD+2Vxh=iKA5Toe=DyWmvRRX_E6xdXNKA@mail.gmail.com>
Subject: Re: [PATCH 0/3] perf record: Fix segfault with '--timestamp-filename'
 option and pipe mode
To: Yang Jihong <yangjihong1@huawei.com>
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org, 
	mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, 
	irogers@google.com, adrian.hunter@intel.com, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello,

On Thu, Jan 18, 2024 at 8:07=E2=80=AFPM 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=3D0x555ea56197af) at util/stat.c:145
>   145             struct perf_stat_evsel *ps =3D evsel->stats;
>   (gdb) bt
>   #0  evsel__free_stat_priv (evsel=3D0x555ea56197af) at util/stat.c:145
>   #1  evlist__free_stats (evlist=3Devlist@entry=3D0x555bf0dad1b0) at util=
/stat.c:215
>   #2  0x0000555befd425e8 in evlist__delete (evlist=3D0x555bf0dad1b0) at u=
til/evlist.c:175
>   #3  0x0000555befd42785 in evlist__delete (evlist=3D<optimized out>) at =
util/evlist.c:181
>   #4  0x0000555befc7f547 in cmd_record (argc=3D<optimized out>, argv=3D<o=
ptimized out>) at builtin-record.c:4252
>   #5  0x0000555befd27700 in run_builtin (p=3Dp@entry=3D0x555bf05acf88 <co=
mmands+264>, argc=3Dargc@entry=3D4, argv=3Dargv@entry=3D0x7ffc2696a920) at =
perf.c:349
>   #6  0x0000555befc68751 in handle_internal_command (argv=3D0x7ffc2696a92=
0, argc=3D4) at perf.c:402
>   #7  run_argv (argv=3D<synthetic pointer>, argcp=3D<synthetic pointer>) =
at perf.c:446
>   #8  main (argc=3D4, argv=3D0x7ffc2696a920) 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_de=
lete().
>
>
> 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
>