linux-kernel - Re: [PATCH v1 2/4] perf mutex with nsinfo: Updated pthread_mutex

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]

Message-ID: <CAM9d7cgqhFdoHneDe0h-i-c5WM+BLiuSpvfTyczCKGJQk8bwMQ@mail.gmail.com>
Date:   Wed, 27 Jul 2022 15:34:10 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     gpavithrasha@...il.com
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        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@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>
Subject: Re: [PATCH v1 2/4] perf mutex with nsinfo: Updated pthread_mutex_t usage

On Wed, Jul 27, 2022 at 4:20 AM <gpavithrasha@...il.com> wrote:
>
> From: pavithra <gpavithrasha@...il.com>
>
> Updated usage of pthread_mutex_t with nsinfo
> with the new wrapped lock(struct) in mutex.h
> (remove data races).

It doesn't look like you updated the usage with new wrapping.
Instead you introduced new usage.  Then I think you need to
describe why it's needed and what problems it solves.

>
> Signed-off-by: pavithra <gpavithrasha@...il.com>
> ---
>  tools/perf/builtin-inject.c   | 6 +++---
>  tools/perf/util/map.c         | 2 ++
>  tools/perf/util/probe-event.c | 6 +++++-
>  tools/perf/util/symbol.c      | 2 +-
>  4 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 372ecb3e2c06..81eaed8da207 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -388,10 +388,10 @@ static int perf_event__repipe_id_index(struct perf_session *session,
>  static int dso__read_build_id(struct dso *dso)
>  {
>         if (dso->has_build_id)
> -               return 0;
> -
> +               return 0;
> +
>         if (filename__read_build_id(dso->long_name, dso->build_id,
> -                                   sizeof(dso->build_id)) > 0) {
> +                                  sizeof(dso->build_id)) > 0) {

Unrelated whitespace changes.. is it really needed?

Also I'm not sure you are working on acme/perf/core branch.

>                 dso->has_build_id = true;
>                 return 0;
>         }
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 5b83ed1ebbd6..2ef5fe0cc53c 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -214,8 +214,10 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>                         if (!(prot & PROT_EXEC))
>                                 dso__set_loaded(dso);
>                 }
> +               mutex_lock(&dso->lock);
>                 dso->nsinfo = nsi;
>                 dso__put(dso);
> +               mutex_unlock(&dso->lock);
>         }
>         return map;
>  out_delete:
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 91cab5f669d2..e527f2612ba4 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -38,6 +38,7 @@
>  #include "session.h"
>  #include "string2.h"
>  #include "strbuf.h"
> +#include "mutex.h"
>
>  #include <subcmd/pager.h>
>  #include <linux/ctype.h>
> @@ -171,8 +172,11 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
>                 struct map *map;
>
>                 map = dso__new_map(target);
> -               if (map && map->dso)
> +               if (map && map->dso) {
> +                       mutex_lock(&map->dso->lock);
>                         map->dso->nsinfo = nsinfo__get(nsi);
> +                       mutex_unlock(&map->dso->lock);
> +               }
>                 return map;
>         } else {
>                 return kernel_get_module_map(target);
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 342be12cfa1e..4b711b13f915 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1619,7 +1619,7 @@ int dso__load(struct dso *dso, struct map *map)
>         struct nscookie nsc;
>         char newmapname[PATH_MAX];
>         const char *map_path = dso->long_name;
> -
> +       mutex_lock(&dso->lock);

No matching unlock?

Thanks,
Namhyung


>         perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0;
>         if (perfmap) {
>                 if (dso->nsinfo && (dso__find_perf_map(newmapname,
> --
> 2.25.1
>