linux-kernel - Re: [PATCH v3 03/22] perf dso: Make lock error check and add BUG

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fWZTmbQpmoXRKt9HdMU-E_B2uo5b4BHOW6RC6w5VkQ0AQ@mail.gmail.com>
Date:   Fri, 11 Feb 2022 09:43:19 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Darren Hart <dvhart@...radead.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        André Almeida <andrealmeid@...labora.com>,
        James Clark <james.clark@....com>,
        John Garry <john.garry@...wei.com>,
        Riccardo Mancini <rickyman7@...il.com>,
        Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Leo Yan <leo.yan@...aro.org>, Andi Kleen <ak@...ux.intel.com>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Shunsuke Nakamura <nakamura.shun@...itsu.com>,
        Song Liu <song@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Miaoqian Lin <linmq006@...il.com>,
        Stephen Brennan <stephen.s.brennan@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
        German Gomez <german.gomez@....com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Hao Luo <haoluo@...gle.com>, eranian@...gle.com
Subject: Re: [PATCH v3 03/22] perf dso: Make lock error check and add BUG_ONs

On Fri, Feb 11, 2022 at 9:13 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> Em Fri, Feb 11, 2022 at 02:33:56AM -0800, Ian Rogers escreveu:
> > Make the pthread mutex on dso use the error check type. This allows
> > deadlock checking via the return type. Assert the returned value from
> > mutex lock is always 0.
>
> I think this is too blunt/pervasive source code wise, perhaps we should
> wrap this like its done with rwsem in tools/perf/util/rwsem.h to get
> away from pthreads primitives and make the source code look more like
> a kernel one and then, taking advantage of the so far ideologic
> needless indirection, add this BUG_ON if we build with "DEBUG=1" or
> something, wdyt?

My concern with semaphores is that they are a concurrency primitive
that has more flexibility and power than a mutex. I like a mutex as it
is quite obvious what is going on and that is good from a tooling
point of view. A deadlock with two mutexes is easy to understand. On a
semaphore, were we using it like a condition variable? There's more to
figure out. I also like the idea of compiling the perf command with
emscripten, we could then generate say perf annotate output in your
web browser. Emscripten has implementations of standard posix
libraries including pthreads, but we may need to have two approaches
in the perf code if we want to compile with emscripten and use
semaphores when targeting linux.

Where this change comes from is that I worried that extending the
locked regions to cover the race that'd been found would then expose
the kind of recursive deadlock that pthread mutexes all too willingly
allow. With this code we at least see the bug and don't just hang. I
don't think we need the change to the mutexes for this change, but we
do need to extend the regions to fix the data race.

Let me know how you prefer it and I can roll it into a v4 version.

Thanks,
Ian

> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/util/dso.c    | 12 +++++++++---
> >  tools/perf/util/symbol.c |  2 +-
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 9cc8a1772b4b..6beccffeef7b 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -784,7 +784,7 @@ dso_cache__free(struct dso *dso)
> >       struct rb_root *root = &dso->data.cache;
> >       struct rb_node *next = rb_first(root);
> >
> > -     pthread_mutex_lock(&dso->lock);
> > +     BUG_ON(pthread_mutex_lock(&dso->lock) != 0);
> >       while (next) {
> >               struct dso_cache *cache;
> >
> > @@ -830,7 +830,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
> >       struct dso_cache *cache;
> >       u64 offset = new->offset;
> >
> > -     pthread_mutex_lock(&dso->lock);
> > +     BUG_ON(pthread_mutex_lock(&dso->lock) != 0);
> >       while (*p != NULL) {
> >               u64 end;
> >
> > @@ -1259,6 +1259,8 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
> >       struct dso *dso = calloc(1, sizeof(*dso) + strlen(name) + 1);
> >
> >       if (dso != NULL) {
> > +             pthread_mutexattr_t lock_attr;
> > +
> >               strcpy(dso->name, name);
> >               if (id)
> >                       dso->id = *id;
> > @@ -1286,8 +1288,12 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
> >               dso->root = NULL;
> >               INIT_LIST_HEAD(&dso->node);
> >               INIT_LIST_HEAD(&dso->data.open_entry);
> > -             pthread_mutex_init(&dso->lock, NULL);
> > +             pthread_mutexattr_init(&lock_attr);
> > +             pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
> > +             pthread_mutex_init(&dso->lock, &lock_attr);
> > +             pthread_mutexattr_destroy(&lock_attr);
> >               refcount_set(&dso->refcnt, 1);
> > +
> >       }
> >
> >       return dso;
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index b2ed3140a1fa..43f47532696f 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1783,7 +1783,7 @@ int dso__load(struct dso *dso, struct map *map)
> >       }
> >
> >       nsinfo__mountns_enter(dso->nsinfo, &nsc);
> > -     pthread_mutex_lock(&dso->lock);
> > +     BUG_ON(pthread_mutex_lock(&dso->lock) != 0);
> >
> >       /* check again under the dso->lock */
> >       if (dso__loaded(dso)) {
> > --
> > 2.35.1.265.g69c8d7142f-goog
>
> --
>
> - Arnaldo