Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220208211637.2221872-1-eranian@google.com> <20220208211637.2221872-11-eranian@google.com>
 <CABPqkBSzc167wNbJpWu7msfZ-KrGu-nMtFdw-naN+M7Q0py-vA@mail.gmail.com> <5609a43a-afbe-7629-b84d-75ace62da4fe@amd.com>
In-Reply-To: <5609a43a-afbe-7629-b84d-75ace62da4fe@amd.com>
From:   Stephane Eranian <eranian@google.com>
Date:   Tue, 15 Mar 2022 00:12:46 -0700
Message-ID: <CABPqkBSnuMkT6t8SG-sFZ1-VN-CWLN4wNnS0zg40OHaUJB+99A@mail.gmail.com>
Subject: Re: [PATCH v6 10/12] perf tools: Improve IBS error handling
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     peterz@infradead.org, kim.phillips@amd.com, acme@redhat.com,
        jolsa@redhat.com, songliubraving@fb.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Mar 14, 2022 at 11:23 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> >> +static bool is_amd(const char *arch, const char *cpuid)
> >> +{
> >> +       return arch && !strcmp("x86", arch) && cpuid && strstarts(cpuid, "AuthenticAMD");
> >> +}
> >> +
> >> +static bool is_amd_ibs(struct evsel *evsel)
> >> +{
> >> +       return evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name, "ibs", 3);
> >> +}
> >> +
> >>  int evsel__open_strerror(struct evsel *evsel, struct target *target,
> >>                          int err, char *msg, size_t size)
> >>  {
> >> +       struct perf_env *env = evsel__env(evsel);
> >> +       const char *arch = perf_env__arch(env);
> >> +       const char *cpuid = perf_env__cpuid(env);
> >
> >
> > This code dies for me on the latest tip.git because env = NULL and
> > perf_env_cpuid() is broken for NULL argument.
> > I don't quite know where this env global variable is set but I hope
> > there is a better way of doing this, maybe using
> > the evsel__env() function in the same util/evsel.c file.
> >
> > Similarly, the is_amd_ibs() suffers from a NULL pointer dereference
> > because evsel->pmu_name maybe NULL:
> >
> > $ perf record -e rc2 .....
> >
> > causes a NULL pmu_name.
>
> This should fix the issue:
>
> @@ -2965,7 +2989,7 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>
>  struct perf_env *evsel__env(struct evsel *evsel)
>  {
> -       if (evsel && evsel->evlist)
> +       if (evsel && evsel->evlist && evsel->evlist->env)
>                 return evsel->evlist->env;
>         return &perf_env;
>  }
>
I will check with that change. Similarly, the IBS helper needs to have
the following change:
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2854,13 +2854,18 @@ static bool is_amd(const char *arch, const char *cpuid)

 static bool is_amd_ibs(struct evsel *evsel)
 {
-       return evsel->core.attr.precise_ip ||
!strncmp(evsel->pmu_name, "ibs", 3);
+       return evsel->core.attr.precise_ip
+           || (evsel->pmu_name && !strncmp(evsel->pmu_name, "ibs", 3));
 }