linux-kernel - Re: [PATCH 1/6] perf maps: Introduce map__set

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=fUqLyCt4=4XgoCjTkmeO4yrHTvEU1Hw48-+S3Szi20x5g@mail.gmail.com>
Date: Thu, 27 Feb 2025 15:27:19 -0800
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Namhyung Kim <namhyung@...nel.org>, Ingo Molnar <mingo@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, James Clark <james.clark@...aro.org>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Kan Liang <kan.liang@...ux.intel.com>, Clark Williams <williams@...hat.com>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	Arnaldo Carvalho de Melo <acme@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 1/6] perf maps: Introduce map__set_kmap() for kernel maps

On Thu, Feb 27, 2025 at 1:55 PM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@...hat.com>
>
> We need to set it in other places than __maps__insert(), so that we can
> have access to the 'struct kmap' from a kernel 'struct map'.
>
> When building perf with 'DEBUG=1' we can notice it failing a consistency
> check done in the check_invariants() function:
>
>   root@...ber:~# perf record -- perf test -w offcpu
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.040 MB perf.data (23 samples) ]
>   perf: util/maps.c:95: check_invariants: Assertion `map__end(prev) <= map__end(map)' failed.
>   Aborted (core dumped)
>   root@...ber:~#
>
> The investigation on that was happening bisected to 876e80cf83d10585
> ("perf tools: Fixup end address of modules"), and the following patches
> will plug the problems found, this patch is just legwork on that
> direction.
>
> Acked-by: Namhyung Kim <namhyung@...nel.org>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Ian Rogers <irogers@...gle.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: James Clark <james.clark@...aro.org>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Kan Liang <kan.liang@...ux.intel.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Stephane Eranian <eranian@...gle.com>
> Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
> Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> ---
>  tools/perf/util/maps.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 09c9cc326c08d435..e21d29f5df01c6f7 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -428,11 +428,29 @@ static unsigned int maps__by_name_index(const struct maps *maps, const struct ma
>         return -1;
>  }
>
> +static void map__set_kmap(struct map *map, struct maps *maps)

I'm not sure this is the right function name. The kmap is created when
the map is created, so perhaps `map__set_kmap_maps(..)` is more
accurate.

Fwiw, kmap is:
```
#define KMAP_NAME_LEN 256

struct kmap {
struct ref_reloc_sym *ref_reloc_sym;
struct maps      *kmaps;
char      name[KMAP_NAME_LEN];
};
```

The name is pretty chunky, perhaps it should be a strdup-ed pointer.
ref_reloc_sym is used in the context of a machine, so perhaps there
should be a hashmap from map to ref_reloc_sym.
The kmaps variable seems to largely be unnecessary, things like
addr_location and map_symbol carry around a map and the associated
maps, so when kmaps is needed it was probably already one of the
arguments to the calling function and could have just been passed
through.
So I think there is a cleanup to:
1) remove kmaps - just pass down an associated maps
2) make ref_reloc_sym and name things looked up from a hashmap
3) get rid of kmap altogether so that invariants don't need to be
maintained and issues like this shouldn't repeat.

Thanks,
Ian

> +{
> +       struct dso *dso;
> +
> +       if (map == NULL)
> +               return;
> +
> +       dso = map__dso(map);
> +
> +       if (dso && dso__kernel(dso)) {
> +                struct kmap *kmap = map__kmap(map);
> +
> +                if (kmap)
> +                        kmap->kmaps = maps;
> +                else
> +                        pr_err("Internal error: kernel dso with non kernel map\n");
> +        }
> +}
> +
>  static int __maps__insert(struct maps *maps, struct map *new)
>  {
>         struct map **maps_by_address = maps__maps_by_address(maps);
>         struct map **maps_by_name = maps__maps_by_name(maps);
> -       const struct dso *dso = map__dso(new);
>         unsigned int nr_maps = maps__nr_maps(maps);
>         unsigned int nr_allocate = RC_CHK_ACCESS(maps)->nr_maps_allocated;
>
> @@ -483,14 +501,9 @@ static int __maps__insert(struct maps *maps, struct map *new)
>         }
>         if (map__end(new) < map__start(new))
>                 RC_CHK_ACCESS(maps)->ends_broken = true;
> -       if (dso && dso__kernel(dso)) {
> -               struct kmap *kmap = map__kmap(new);
>
> -               if (kmap)
> -                       kmap->kmaps = maps;
> -               else
> -                       pr_err("Internal error: kernel dso with non kernel map\n");
> -       }
> +       map__set_kmap(new, maps);
> +
>         return 0;
>  }
>
> --
> 2.47.0
>