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;
Date:   Wed, 06 Jul 2022 18:01:29 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH] perf trace: Fix SIGSEGV when processing augmented args
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     cclaudio@linux.ibm.com, Jiri Olsa <jolsa@kernel.org>,
        Linux =?iso-8859-1?q?Kernel=0A?= Mailing List 
        <linux-kernel@vger.kernel.org>, Namhyung Kim <namhyung@kernel.org>
References: <20220310104741.209834-1-naveen.n.rao@linux.vnet.ibm.com>
        <Yi+9G1nK1shEIXVN@kernel.org> <1647364864.3xrhklc7kl.naveen@linux.ibm.com>
        <YjDSRb1wwswKpJNJ@kernel.org> <YjJKaSanKI7LQdEk@kernel.org>
In-Reply-To: <YjJKaSanKI7LQdEk@kernel.org>
MIME-Version: 1.0
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1657110053.93m1349h4k.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Arnaldo,

Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 15, 2022 at 02:52:05PM -0300, Arnaldo Carvalho de Melo escrev=
eu:
>> Em Tue, Mar 15, 2022 at 10:57:57PM +0530, Naveen N. Rao escreveu:
>> > Yes, it looks like the current check in 'perf' isn't working. The belo=
w
>> > patch also resolves the crash we are seeing:
>> =20
>> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
>> > index 2f1d20553a0aa3..86b459f4ebdd61 100644
>> > --- a/tools/perf/builtin-trace.c
>> > +++ b/tools/perf/builtin-trace.c
>> > @@ -2326,7 +2326,7 @@ static int trace__sys_enter(struct trace *trace,=
 struct evsel *evsel,
>> >         * thinking that the extra 2 u64 args are the augmented filenam=
e, so just check
>> >         * here and avoid using augmented syscalls when the evsel is th=
e raw_syscalls one.
>> >         */
>> > -       if (evsel !=3D trace->syscalls.events.sys_enter)
>> > +       if (strcmp(evsel__name(evsel), "raw_syscalls:sys_enter"))
>> >                augmented_args =3D syscall__augmented_args(sc, sample, =
&augmented_args_size, trace->raw_augmented_syscalls_args_size);
>> >        ttrace->entry_time =3D sample->time;
>> >        msg =3D ttrace->entry_str;
>>=20
>> Interesting, that should be equivalent :-\ humm, not really, understood,
>> when processing perf.data files we don't setup
>> trace->syscalls.events.sys_enter...
>>=20
>> switching from strcmp() to something cheaper but equivalent should be
>> the fix for now.
>=20
> I'll add a trace->use_augmented_args boolean that will do this test
> once, and then use it in this case and will audit to check if this
> should be used in other places.

Does something like the below look reasonable?

I know this isn't quite what you proposed, but it fixes the problem for=20
me while avoiding the need for a string comparison. I also think this=20
addresses all uses in 'perf trace', though I didn't audit the need for a=20
similar fix elsewhere in 'perf'.


Thanks,
Naveen

---
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d2de2a4073e7eb..352b88a51dec2d 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4291,6 +4292,8 @@ static int trace__replay(struct trace *trace)
                goto out;
        }
=20
+       trace->syscalls.events.sys_enter =3D evsel;
+
        evsel =3D evlist__find_tracepoint_by_name(session->evlist, "raw_sys=
calls:sys_exit");
        if (evsel =3D=3D NULL)
                evsel =3D evlist__find_tracepoint_by_name(session->evlist, =
"syscalls:sys_exit");
@@ -4301,6 +4304,8 @@ static int trace__replay(struct trace *trace)
                goto out;
        }
=20
+       trace->syscalls.events.sys_exit  =3D evsel;
+
        evlist__for_each_entry(session->evlist, evsel) {
                if (evsel->core.attr.type =3D=3D PERF_TYPE_SOFTWARE &&
                    (evsel->core.attr.config =3D=3D PERF_COUNT_SW_PAGE_FAUL=
TS_MAJ ||