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;
MIME-Version: 1.0
References: <20230626161059.324046-1-james.clark@arm.com> <20230626161059.324046-3-james.clark@arm.com>
 <ZJonE3ZZ2cBUq0U8@google.com>
In-Reply-To: <ZJonE3ZZ2cBUq0U8@google.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 27 Jun 2023 09:42:58 -0700
Message-ID: <CAP-5=fX+FDAkFoMhQY27_+fiRp_UDruP8qWTdjvqU5-uE0SdWA@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf report: Don't add to histogram when there is no
 thread found
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     James Clark <james.clark@arm.com>,
        linux-perf-users@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        John Garry <john.g.garry@oracle.com>,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 26, 2023 at 5:02=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> > thread__find_map() chooses to exit without assigning a thread to the
> > addr_location in some scenarios, for example when there are samples fro=
m
> > a guest and perf_guest =3D=3D false. This results in a segfault when ad=
ding
> > to the histogram because it uses unguarded accesses to the thread membe=
r
> > of the addr_location.
>
> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> init/exit/copy functions") that introduced the change, I'm not sure if
> it's the intend behavior.
>
> It might change maps and map, but not thread.  Then I think no reason
> to not set the al->thread at the beginning.
>
> How about this?  Ian?
> (I guess we can get rid of the duplicate 'al->map =3D NULL' part)

It seemed strange that we were failing to find a map (the function's
purpose) but then populating the address_location. The change below
brings back that somewhat odd behavior. I'm okay with reverting to the
old behavior, clearly there were users relying on it. We should
probably also copy maps and not just thread, as that was the previous
behavior.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> ---8<---
>
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 3860b0c74829..4cbb092e0684 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -581,15 +581,14 @@ struct map *thread__find_map(struct thread *thread,=
 u8 cpumode, u64 addr,
>         maps__zput(al->maps);
>         map__zput(al->map);
>         thread__zput(al->thread);
> +       al->thread =3D thread__get(thread);
>
>         al->addr =3D addr;
>         al->cpumode =3D cpumode;
>         al->filtered =3D 0;
>
> -       if (machine =3D=3D NULL) {
> -               al->map =3D NULL;
> +       if (machine =3D=3D NULL)
>                 return NULL;
> -       }
>
>         if (cpumode =3D=3D PERF_RECORD_MISC_KERNEL && perf_host) {
>                 al->level =3D 'k';
> @@ -605,7 +604,6 @@ struct map *thread__find_map(struct thread *thread, u=
8 cpumode, u64 addr,
>                 al->level =3D 'u';
>         } else {
>                 al->level =3D 'H';
> -               al->map =3D NULL;
>
>                 if ((cpumode =3D=3D PERF_RECORD_MISC_GUEST_USER ||
>                         cpumode =3D=3D PERF_RECORD_MISC_GUEST_KERNEL) &&
> @@ -619,7 +617,6 @@ struct map *thread__find_map(struct thread *thread, u=
8 cpumode, u64 addr,
>                 return NULL;
>         }
>         al->maps =3D maps__get(maps);
> -       al->thread =3D thread__get(thread);
>         al->map =3D map__get(maps__find(maps, al->addr));
>         if (al->map !=3D NULL) {
>                 /*