linux-kernel - Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 15:54:56 -0500
From:   Rob Herring <robh@...nel.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Jiri Olsa <jolsa@...nel.org>, Leo Yan <leo.yan@...aro.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        James Clark <james.clark@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] perf: Skip and warn on unknown format 'configN' attrs

On Thu, Sep 29, 2022 at 1:54 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Mon, Sep 26, 2022 at 6:32 AM Rob Herring <robh@...nel.org> wrote:
> >
> > On Fri, Sep 16, 2022 at 1:12 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > On Wed, Sep 14, 2022 at 1:09 PM Rob Herring <robh@...nel.org> wrote:
> > > >
> > > > If the kernel exposes a new perf_event_attr field in a format attr, perf
> > > > will return an error stating the specified PMU can't be found. For
> > > > example, a format attr with 'config3:0-63' causes an error as config3 is
> > > > unknown to perf. This causes a compatibility issue between a newer
> > > > kernel with older perf tool.
> > > >
> > > > Before this change with a kernel adding 'config3' I get:
> > > >
> > > > $ perf record -e arm_spe// -- true
> > > > event syntax error: 'arm_spe//'
> > > >                      \___ Cannot find PMU `arm_spe'. Missing kernel support?
> > > > Run 'perf list' for a list of valid events
> > > >
> > > >  Usage: perf record [<options>] [<command>]
> > > >     or: perf record [<options>] -- <command> [<options>]
> > > >
> > > >     -e, --event <event>   event selector. use 'perf list' to list
> > > > available events
> > > >
> > > > After this change, I get:
> > > >
> > > > $ perf record -e arm_spe// -- true
> > > > WARNING: 'arm_spe_0' format 'inv_event_filter' requires 'perf_event_attr::config3' which is not supported by this version of perf!
> > > > [ perf record: Woken up 2 times to write data ]
> > > > [ perf record: Captured and wrote 0.091 MB perf.data ]
> > > >
> > > > To support unknown configN formats, rework the YACC implementation to
> > > > pass any config[0-9]+ format to perf_pmu__new_format() to handle with a
> > > > warning.
> > >
> > > It only handles configN formats but it might add a completely different
> > > name later, right?
> >
> > Right. An unknown configN is a warning. An unknown name is still an
> > error as before. Given that sysfs format attrs are for mapping fields
> > which could be anything to "generic" perf_event_attr fields, how would
> > we ever have anything other than configN?
>
> I'm not sure I'm following.  It could be anything other than configN.

It's possible, yes, but likely or necessary? Probably not.

Let me try again. perf_event_attr:configX fields are pmu specific and
sysfs format files provide the mapping of their specific usage to
configX bits. If we add something to perf_event_attr that's not PMU
specific, but common for perf, then it's going to have a specific name
and no format entry. Right? If we add yet another PMU specific field,
why would we ever have a name other than 'config'? Anything different
has little benefit since format files provide the specific meaning and
it's up to the PMU driver to handle them.

Maybe someone comes up with something, but that seems a lot less
likely than another configN.

> But I don't object to this particular change as it's needed for current
> work.  Later, we can fix the issue if another name comes in.

Is that an Acked/Reviewed-by?

Rob