linux-kernel - Re: [PATCH v1] perf test: Ignore security failures in all PMU test

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] [day] [month] [year] [list]

Message-ID: <ZvWlICnVELB0WEto@google.com>
Date: Thu, 26 Sep 2024 11:17:04 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	Colin Ian King <colin.i.king@...il.com>,
	Athira Jajeev <atrajeev@...ux.vnet.ibm.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf test: Ignore security failures in all PMU test

On Thu, Sep 26, 2024 at 08:24:30AM -0700, Ian Rogers wrote:
> On Wed, Sep 25, 2024 at 9:52 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > On Wed, Sep 25, 2024 at 10:30:13AM -0700, Ian Rogers wrote:
> > > Refactor code to have some more error diagnosis on traps, etc. and to
> > > do less work on each line. Add an ignore situation for security failures.
> > >
> > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > ---
> > >  tools/perf/tests/shell/stat_all_pmu.sh | 52 ++++++++++++++++++++------
> > >  1 file changed, 40 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/shell/stat_all_pmu.sh b/tools/perf/tests/shell/stat_all_pmu.sh
> > > index d2a3506e0d19..42456d89c5da 100755
> > > --- a/tools/perf/tests/shell/stat_all_pmu.sh
> > > +++ b/tools/perf/tests/shell/stat_all_pmu.sh
> > > @@ -1,23 +1,51 @@
> > > -#!/bin/sh
> > > +#!/bin/bash
> > >  # perf all PMU test
> > >  # SPDX-License-Identifier: GPL-2.0
> > >
> > >  set -e
> > > +err=0
> > > +result=""
> > > +
> > > +trap_cleanup() {
> > > +  echo "Unexpected signal in ${FUNCNAME[1]}"
> > > +  echo "$result"
> > > +  exit 1
> > > +}
> > > +trap trap_cleanup EXIT TERM INT
> > >
> > >  # Test all PMU events; however exclude parameterized ones (name contains '?')
> > > -for p in $(perf list --raw-dump pmu | sed 's/[[:graph:]]\+?[[:graph:]]\+[[:space:]]//g'); do
> > > +for p in $(perf list --raw-dump pmu | sed 's/[[:graph:]]\+?[[:graph:]]\+[[:space:]]//g')
> > > +do
> > >    echo "Testing $p"
> > >    result=$(perf stat -e "$p" true 2>&1)
> > > -  if ! echo "$result" | grep -q "$p" && ! echo "$result" | grep -q "<not supported>" ; then
> > > -    # We failed to see the event and it is supported. Possibly the workload was
> > > -    # too small so retry with something longer.
> > > -    result=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> > > -    if ! echo "$result" | grep -q "$p" ; then
> > > -      echo "Event '$p' not printed in:"
> > > -      echo "$result"
> > > -      exit 1
> > > -    fi
> > > +  if echo "$result" | grep -q "$p"
> > > +  then
> > > +    # Event seen in output.
> > > +    continue
> > > +  fi
> > > +  if echo "$result" | grep -q "<not supported>"
> > > +  then
> > > +    # Event not supported, so ignore.
> > > +    continue
> >
> > I'm curious about this case.  It'll show up even if it's not supported
> > right?  Then the first condition would match and it doesn't reach here.
> >
> >   $ perf stat -e LLC-loads true
> >
> >    Performance counter stats for 'true':
> >
> >      <not supported>      LLC-loads
> >
> >          0.001213558 seconds time elapsed
> >
> >          0.001373000 seconds user
> >          0.000000000 seconds sys
> 
> We carry this change at Google for an issue on older series kernels. I
> was resending the patch as a courtesy in the migration of development
> work to:
> https://github.com/googleprodkernel/linux-perf
> I'm okay with this being a Google only change.

I'm not sure if it's a kernel issue.  IIUC you want to run perf stat
again if some events are not counted, right?

I understand your patch just converted the existing logic and added the
check for security related failures.  So I'll pick it up for now but we
may want to revisit this later.

Thanks,
Namhyung