linux-kernel - Re: [PATCH v2 13/13] perf parse-events: Remove ABORT

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: <CAP-5=fWMKjfDYOjDzDZgaNdujz-eMCpXiBvp1=EYfeHzKoiWTw@mail.gmail.com>
Date:   Tue, 11 Jul 2023 22:01:07 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Namhyung Kim <namhyung@...nel.org>
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>,
        Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH v2 13/13] perf parse-events: Remove ABORT_ON

On Sat, Jul 1, 2023 at 11:43 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Fri, Jun 30, 2023 at 8:14 AM Ian Rogers <irogers@...gle.com> wrote:
> >
> > On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <irogers@...gle.com> wrote:
> > > >
> > > > Prefer informative messages rather than none with ABORT_ON. Document
> > > > one failure mode and add an error message for another.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > > ---
> > > >  tools/perf/util/parse-events.y | 22 ++++++++++++++--------
> > > >  1 file changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > > > index 844646752462..454577f7aff6 100644
> > > > --- a/tools/perf/util/parse-events.y
> > > > +++ b/tools/perf/util/parse-events.y
> > > > @@ -22,12 +22,6 @@
> > > >
> > > >  void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
> > > >
> > > > -#define ABORT_ON(val) \
> > > > -do { \
> > > > -       if (val) \
> > > > -               YYABORT; \
> > > > -} while (0)
> > > > -
> > > >  #define PE_ABORT(val) \
> > > >  do { \
> > > >         if (val == -ENOMEM) \
> > > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config
> > > >                 YYNOMEM;
> > > >         errno = 0;
> > > >         num = strtoull($1 + 1, NULL, 16);
> > > > -       ABORT_ON(errno);
> > > > +       /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
> > > > +       if (errno)
> > > > +               YYABORT;
> > > >         free($1);
> > > >         err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
> > > >                                        /*wildcard=*/false);
> > > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > > >  {
> > > >         struct parse_events_array array;
> > > >
> > > > -       ABORT_ON($3 < $1);
> > > > +       if ($3 < $1) {
> > > > +               struct parse_events_state *parse_state = _parse_state;
> > > > +               struct parse_events_error *error = parse_state->error;
> > > > +               char *err_str;
> > > > +
> > > > +               if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
> > >
> > > Isn't it to be "greater-than or equal" ?
> >
> > I think the order is right. From the man page:
> >
> >        When  successful,  these  functions return the number of bytes printed,
> >        just like sprintf(3).  If memory allocation wasn't  possible,  or  some
> >        other error occurs, these functions will return -1, and the contents of
> >        strp are undefined.
> >
> > So here we need to catch -1 and ensure strp (&err_str) is NULL before
> > passing it to parse_events_error__handle.
>
> Oh, I meant the message not the condition in the if statement.
> It seems it aborts if $3 < $1, then it expects $3 >= $1 in the
> normal condition, right?

In the old code with the macro expanded it did:
if ($3 < $1) YYABORT

in the new code it fills in parse_state->error if the same error
condition applies. The change is to get rid of the macro and add an
error message. The asprintf is just added to make the error message
more informative.

Thanks,
Ian

> Thanks,
> Namhyung