[PATCH 0/4] perf maps/symbols: Various assert fixes

[PATCH 0/4] perf maps/symbols: Various assert fixes

[James Clark]

A few different asserts are hit when running perf report on minimal
Arm systems when kcore is used, or the .debug/ info can't be loaded or
/boot isn't mounted etc.

These result in some less common paths being hit for resolving symbols
and things are done in an order that breaks some assumptions. I'm not
sure if we could do something to make the tests pick this up, but maybe
not easily if it would involve mocking the filesystem or even a specific
kernel. I tried a few different variations of --kcore and --vmlinux
arguments but ultimately I could only reproduce these issues by running
on specific kernels and root filesystems.

James Clark (4):
  perf symbols: Remove map from list before updating addresses
  perf maps: Re-use __maps__free_maps_by_name()
  perf symbols: Update kcore map before merging in remaining symbols
  perf symbols: Fix ownership of string in dso__load_vmlinux()

 tools/perf/util/maps.c   | 14 ++++++------
 tools/perf/util/symbol.c | 49 ++++++++++++++++++++++++----------------
 2 files changed, 36 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH 1/4] perf symbols: Remove map from list before updating addresses

[James Clark]

Make the order of operations remove, update, add. Updating addresses
before the map is removed causes the ordering check to fail when the map
is removed. This can be reproduced when running Perf on an Arm system
with a static kernel and Perf uses kcore rather than other sources:

  $ perf record -- ls
  $ perf report

  util/maps.c:96: check_invariants: Assertion `map__end(prev) <=
    map__start(map) || map__start(prev) == map__start(map)' failed

Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/symbol.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 7772a4d3e66c..2d95f22d713d 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1377,13 +1377,15 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 		if (RC_CHK_EQUAL(new_map, replacement_map)) {
 			struct map *map_ref;
 
-			map__set_start(map, map__start(new_map));
-			map__set_end(map, map__end(new_map));
-			map__set_pgoff(map, map__pgoff(new_map));
-			map__set_mapping_type(map, map__mapping_type(new_map));
 			/* Ensure maps are correctly ordered */
 			map_ref = map__get(map);
 			maps__remove(kmaps, map_ref);
+
+			map__set_start(map_ref, map__start(new_map));
+			map__set_end(map_ref, map__end(new_map));
+			map__set_pgoff(map_ref, map__pgoff(new_map));
+			map__set_mapping_type(map_ref, map__mapping_type(new_map));
+
 			err = maps__insert(kmaps, map_ref);
 			map__put(map_ref);
 			map__put(new_map);
-- 
2.34.1

[PATCH 2/4] perf maps: Re-use __maps__free_maps_by_name()

[James Clark]

maps__merge_in() hard codes the steps to free the maps_by_name list. It
seems to not map__put() each element before freeing, and it sets
maps_by_name_sorted to true after freeing, which may be harmless but
is inconsistent with maps__init() and other functions.

maps__maps_by_name_addr() is also quite hard to read because we already
have maps__maps_by_name() and maps__maps_by_address(), but the function
is only used in that place so delete it.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/maps.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 61eb742d91e3..16b39db594f4 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -124,11 +124,6 @@ static void maps__set_maps_by_address(struct maps *maps, struct map **new)
 
 }
 
-static struct map ***maps__maps_by_name_addr(struct maps *maps)
-{
-	return &RC_CHK_ACCESS(maps)->maps_by_name;
-}
-
 static void maps__set_nr_maps_allocated(struct maps *maps, unsigned int nr_maps_allocated)
 {
 	RC_CHK_ACCESS(maps)->nr_maps_allocated = nr_maps_allocated;
@@ -284,6 +279,9 @@ void maps__put(struct maps *maps)
 
 static void __maps__free_maps_by_name(struct maps *maps)
 {
+	if (!maps__maps_by_name(maps))
+		return;
+
 	/*
 	 * Free everything to try to do it from the rbtree in the next search
 	 */
@@ -291,6 +289,9 @@ static void __maps__free_maps_by_name(struct maps *maps)
 		map__put(maps__maps_by_name(maps)[i]);
 
 	zfree(&RC_CHK_ACCESS(maps)->maps_by_name);
+
+	/* Consistent with maps__init(). When maps_by_name == NULL, maps_by_name_sorted == false */
+	maps__set_maps_by_name_sorted(maps, false);
 }
 
 static int map__start_cmp(const void *a, const void *b)
@@ -1167,8 +1168,7 @@ int maps__merge_in(struct maps *kmaps, struct map *new_map)
 	}
 	maps__set_maps_by_address(kmaps, merged_maps_by_address);
 	maps__set_maps_by_address_sorted(kmaps, true);
-	zfree(maps__maps_by_name_addr(kmaps));
-	maps__set_maps_by_name_sorted(kmaps, true);
+	__maps__free_maps_by_name(kmaps);
 	maps__set_nr_maps_allocated(kmaps, merged_nr_maps_allocated);
 
 	/* Copy entries before the new_map that can't overlap. */
-- 
2.34.1

Re: [PATCH 2/4] perf maps: Re-use __maps__free_maps_by_name()

[Namhyung Kim]

On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>
> maps__merge_in() hard codes the steps to free the maps_by_name list. It
> seems to not map__put() each element before freeing, and it sets
> maps_by_name_sorted to true after freeing, which may be harmless but
> is inconsistent with maps__init() and other functions.
>
> maps__maps_by_name_addr() is also quite hard to read because we already
> have maps__maps_by_name() and maps__maps_by_address(), but the function
> is only used in that place so delete it.

Agreed, I feel like we need some more cleanup here.

Thanks,
Namhyung


>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/maps.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 61eb742d91e3..16b39db594f4 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -124,11 +124,6 @@ static void maps__set_maps_by_address(struct maps *maps, struct map **new)
>
>  }
>
> -static struct map ***maps__maps_by_name_addr(struct maps *maps)
> -{
> -       return &RC_CHK_ACCESS(maps)->maps_by_name;
> -}
> -
>  static void maps__set_nr_maps_allocated(struct maps *maps, unsigned int nr_maps_allocated)
>  {
>         RC_CHK_ACCESS(maps)->nr_maps_allocated = nr_maps_allocated;
> @@ -284,6 +279,9 @@ void maps__put(struct maps *maps)
>
>  static void __maps__free_maps_by_name(struct maps *maps)
>  {
> +       if (!maps__maps_by_name(maps))
> +               return;
> +
>         /*
>          * Free everything to try to do it from the rbtree in the next search
>          */
> @@ -291,6 +289,9 @@ static void __maps__free_maps_by_name(struct maps *maps)
>                 map__put(maps__maps_by_name(maps)[i]);
>
>         zfree(&RC_CHK_ACCESS(maps)->maps_by_name);
> +
> +       /* Consistent with maps__init(). When maps_by_name == NULL, maps_by_name_sorted == false */
> +       maps__set_maps_by_name_sorted(maps, false);
>  }
>
>  static int map__start_cmp(const void *a, const void *b)
> @@ -1167,8 +1168,7 @@ int maps__merge_in(struct maps *kmaps, struct map *new_map)
>         }
>         maps__set_maps_by_address(kmaps, merged_maps_by_address);
>         maps__set_maps_by_address_sorted(kmaps, true);
> -       zfree(maps__maps_by_name_addr(kmaps));
> -       maps__set_maps_by_name_sorted(kmaps, true);
> +       __maps__free_maps_by_name(kmaps);
>         maps__set_nr_maps_allocated(kmaps, merged_nr_maps_allocated);
>
>         /* Copy entries before the new_map that can't overlap. */
> --
> 2.34.1
>

Re: [PATCH 2/4] perf maps: Re-use __maps__free_maps_by_name()

[Ian Rogers]

On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>
> maps__merge_in() hard codes the steps to free the maps_by_name list. It
> seems to not map__put() each element before freeing, and it sets
> maps_by_name_sorted to true after freeing, which may be harmless but
> is inconsistent with maps__init() and other functions.
>
> maps__maps_by_name_addr() is also quite hard to read because we already
> have maps__maps_by_name() and maps__maps_by_address(), but the function
> is only used in that place so delete it.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/maps.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 61eb742d91e3..16b39db594f4 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -124,11 +124,6 @@ static void maps__set_maps_by_address(struct maps *maps, struct map **new)
>
>  }
>
> -static struct map ***maps__maps_by_name_addr(struct maps *maps)
> -{
> -       return &RC_CHK_ACCESS(maps)->maps_by_name;
> -}
> -
>  static void maps__set_nr_maps_allocated(struct maps *maps, unsigned int nr_maps_allocated)
>  {
>         RC_CHK_ACCESS(maps)->nr_maps_allocated = nr_maps_allocated;
> @@ -284,6 +279,9 @@ void maps__put(struct maps *maps)
>
>  static void __maps__free_maps_by_name(struct maps *maps)
>  {
> +       if (!maps__maps_by_name(maps))
> +               return;
> +
>         /*
>          * Free everything to try to do it from the rbtree in the next search

nit: this comment is stale, it should be maps_by_address rather than the rbtree.

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

>          */
> @@ -291,6 +289,9 @@ static void __maps__free_maps_by_name(struct maps *maps)
>                 map__put(maps__maps_by_name(maps)[i]);
>
>         zfree(&RC_CHK_ACCESS(maps)->maps_by_name);
> +
> +       /* Consistent with maps__init(). When maps_by_name == NULL, maps_by_name_sorted == false */
> +       maps__set_maps_by_name_sorted(maps, false);
>  }
>
>  static int map__start_cmp(const void *a, const void *b)
> @@ -1167,8 +1168,7 @@ int maps__merge_in(struct maps *kmaps, struct map *new_map)
>         }
>         maps__set_maps_by_address(kmaps, merged_maps_by_address);
>         maps__set_maps_by_address_sorted(kmaps, true);
> -       zfree(maps__maps_by_name_addr(kmaps));
> -       maps__set_maps_by_name_sorted(kmaps, true);
> +       __maps__free_maps_by_name(kmaps);
>         maps__set_nr_maps_allocated(kmaps, merged_nr_maps_allocated);
>
>         /* Copy entries before the new_map that can't overlap. */
> --
> 2.34.1
>

[PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols

[James Clark]

When loading kcore, the main vmlinux map is updated in the same loop
that merges the remaining maps. If a map that overlaps is merged in
before kcore, the list can become unsortable when the main map addresses
are updated. This will later trigger the check_invariants() assert:

  $ perf record
  $ perf report

  util/maps.c:96: check_invariants: Assertion `map__end(prev) <=
    map__start(map) || map__start(prev) == map__start(map)' failed.
  Aborted

Fix it by moving the main map update prior to the loop so that
maps__merge_in() can split it if necessary.

Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/symbol.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 2d95f22d713d..e98dfe766da3 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1289,7 +1289,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 {
 	struct maps *kmaps = map__kmaps(map);
 	struct kcore_mapfn_data md;
-	struct map *replacement_map = NULL;
+	struct map *map_ref, *replacement_map = NULL;
 	struct machine *machine;
 	bool is_64_bit;
 	int err, fd;
@@ -1367,6 +1367,24 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	if (!replacement_map)
 		replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;
 
+	/*
+	 * Update addresses of vmlinux map. Re-insert it to ensure maps are
+	 * correctly ordered. Do this before using maps__merge_in() for the
+	 * remaining maps so vmlinux gets split if necessary.
+	 */
+	map_ref = map__get(map);
+	maps__remove(kmaps, map_ref);
+
+	map__set_start(map_ref, map__start(replacement_map));
+	map__set_end(map_ref, map__end(replacement_map));
+	map__set_pgoff(map_ref, map__pgoff(replacement_map));
+	map__set_mapping_type(map_ref, map__mapping_type(replacement_map));
+
+	err = maps__insert(kmaps, map_ref);
+	map__put(map_ref);
+	if (err)
+		goto out_err;
+
 	/* Add new maps */
 	while (!list_empty(&md.maps)) {
 		struct map_list_node *new_node = list_entry(md.maps.next, struct map_list_node, node);
@@ -1374,24 +1392,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 
 		list_del_init(&new_node->node);
 
-		if (RC_CHK_EQUAL(new_map, replacement_map)) {
-			struct map *map_ref;
-
-			/* Ensure maps are correctly ordered */
-			map_ref = map__get(map);
-			maps__remove(kmaps, map_ref);
-
-			map__set_start(map_ref, map__start(new_map));
-			map__set_end(map_ref, map__end(new_map));
-			map__set_pgoff(map_ref, map__pgoff(new_map));
-			map__set_mapping_type(map_ref, map__mapping_type(new_map));
-
-			err = maps__insert(kmaps, map_ref);
-			map__put(map_ref);
-			map__put(new_map);
-			if (err)
-				goto out_err;
-		} else {
+		/* skip if replacement_map, already inserted above */
+		if (!RC_CHK_EQUAL(new_map, replacement_map)) {
 			/*
 			 * Merge kcore map into existing maps,
 			 * and ensure that current maps (eBPF)
-- 
2.34.1

Re: [PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols

[Namhyung Kim]

On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>
> When loading kcore, the main vmlinux map is updated in the same loop
> that merges the remaining maps. If a map that overlaps is merged in
> before kcore, the list can become unsortable when the main map addresses
> are updated. This will later trigger the check_invariants() assert:
>
>   $ perf record
>   $ perf report
>
>   util/maps.c:96: check_invariants: Assertion `map__end(prev) <=
>     map__start(map) || map__start(prev) == map__start(map)' failed.
>   Aborted
>
> Fix it by moving the main map update prior to the loop so that
> maps__merge_in() can split it if necessary.

Looks like you and Leo are working on the same problem.

https://lore.kernel.org/r/20240505202805.583253-1-leo.yan@arm.com/

>
> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/symbol.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 2d95f22d713d..e98dfe766da3 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1289,7 +1289,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  {
>         struct maps *kmaps = map__kmaps(map);
>         struct kcore_mapfn_data md;
> -       struct map *replacement_map = NULL;
> +       struct map *map_ref, *replacement_map = NULL;
>         struct machine *machine;
>         bool is_64_bit;
>         int err, fd;
> @@ -1367,6 +1367,24 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>         if (!replacement_map)
>                 replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;
>
> +       /*
> +        * Update addresses of vmlinux map. Re-insert it to ensure maps are
> +        * correctly ordered. Do this before using maps__merge_in() for the
> +        * remaining maps so vmlinux gets split if necessary.
> +        */
> +       map_ref = map__get(map);
> +       maps__remove(kmaps, map_ref);

A nitpick.  It'd be natural to use 'map' instead of 'map_ref'
(even if they are the same) since IIUC we want to remove
the old 'map' and update 'map_ref' then add it back.

> +
> +       map__set_start(map_ref, map__start(replacement_map));
> +       map__set_end(map_ref, map__end(replacement_map));
> +       map__set_pgoff(map_ref, map__pgoff(replacement_map));
> +       map__set_mapping_type(map_ref, map__mapping_type(replacement_map));

So here, replacement_map should not be NULL right?

Thanks,
Namhyung

> +
> +       err = maps__insert(kmaps, map_ref);
> +       map__put(map_ref);
> +       if (err)
> +               goto out_err;
> +
>         /* Add new maps */
>         while (!list_empty(&md.maps)) {
>                 struct map_list_node *new_node = list_entry(md.maps.next, struct map_list_node, node);
> @@ -1374,24 +1392,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>
>                 list_del_init(&new_node->node);
>
> -               if (RC_CHK_EQUAL(new_map, replacement_map)) {
> -                       struct map *map_ref;
> -
> -                       /* Ensure maps are correctly ordered */
> -                       map_ref = map__get(map);
> -                       maps__remove(kmaps, map_ref);
> -
> -                       map__set_start(map_ref, map__start(new_map));
> -                       map__set_end(map_ref, map__end(new_map));
> -                       map__set_pgoff(map_ref, map__pgoff(new_map));
> -                       map__set_mapping_type(map_ref, map__mapping_type(new_map));
> -
> -                       err = maps__insert(kmaps, map_ref);
> -                       map__put(map_ref);
> -                       map__put(new_map);
> -                       if (err)
> -                               goto out_err;
> -               } else {
> +               /* skip if replacement_map, already inserted above */
> +               if (!RC_CHK_EQUAL(new_map, replacement_map)) {
>                         /*
>                          * Merge kcore map into existing maps,
>                          * and ensure that current maps (eBPF)
> --
> 2.34.1
>

Re: [PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols

[James Clark]

On 08/05/2024 05:10, Namhyung Kim wrote:
> On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>>
>> When loading kcore, the main vmlinux map is updated in the same loop
>> that merges the remaining maps. If a map that overlaps is merged in
>> before kcore, the list can become unsortable when the main map addresses
>> are updated. This will later trigger the check_invariants() assert:
>>
>>   $ perf record
>>   $ perf report
>>
>>   util/maps.c:96: check_invariants: Assertion `map__end(prev) <=
>>     map__start(map) || map__start(prev) == map__start(map)' failed.
>>   Aborted
>>
>> Fix it by moving the main map update prior to the loop so that
>> maps__merge_in() can split it if necessary.
> 
> Looks like you and Leo are working on the same problem.
> 
> https://lore.kernel.org/r/20240505202805.583253-1-leo.yan@arm.com/
> 

Oops I should have checked the list. It looks like we can still take his
fix as well though, with an updated comment.

>>
>> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/util/symbol.c | 40 +++++++++++++++++++++-------------------
>>  1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 2d95f22d713d..e98dfe766da3 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1289,7 +1289,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>  {
>>         struct maps *kmaps = map__kmaps(map);
>>         struct kcore_mapfn_data md;
>> -       struct map *replacement_map = NULL;
>> +       struct map *map_ref, *replacement_map = NULL;
>>         struct machine *machine;
>>         bool is_64_bit;
>>         int err, fd;
>> @@ -1367,6 +1367,24 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>         if (!replacement_map)
>>                 replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;
>>
>> +       /*
>> +        * Update addresses of vmlinux map. Re-insert it to ensure maps are
>> +        * correctly ordered. Do this before using maps__merge_in() for the
>> +        * remaining maps so vmlinux gets split if necessary.
>> +        */
>> +       map_ref = map__get(map);
>> +       maps__remove(kmaps, map_ref);
> 
> A nitpick.  It'd be natural to use 'map' instead of 'map_ref'
> (even if they are the same) since IIUC we want to remove
> the old 'map' and update 'map_ref' then add it back.
> 

Using map makes sense, I can update that.

>> +
>> +       map__set_start(map_ref, map__start(replacement_map));
>> +       map__set_end(map_ref, map__end(replacement_map));
>> +       map__set_pgoff(map_ref, map__pgoff(replacement_map));
>> +       map__set_mapping_type(map_ref, map__mapping_type(replacement_map));
> 
> So here, replacement_map should not be NULL right?
> 

Yes it shouldn't be. It would only be NULL if md.maps is empty, but
there's already an exit condition for that above.

Some of the other code also assumes node->map is always set, so it can't
be NULL that way either.

> Thanks,
> Namhyung
> 
>> +
>> +       err = maps__insert(kmaps, map_ref);
>> +       map__put(map_ref);
>> +       if (err)
>> +               goto out_err;
>> +
>>         /* Add new maps */
>>         while (!list_empty(&md.maps)) {
>>                 struct map_list_node *new_node = list_entry(md.maps.next, struct map_list_node, node);
>> @@ -1374,24 +1392,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>
>>                 list_del_init(&new_node->node);
>>
>> -               if (RC_CHK_EQUAL(new_map, replacement_map)) {
>> -                       struct map *map_ref;
>> -
>> -                       /* Ensure maps are correctly ordered */
>> -                       map_ref = map__get(map);
>> -                       maps__remove(kmaps, map_ref);
>> -
>> -                       map__set_start(map_ref, map__start(new_map));
>> -                       map__set_end(map_ref, map__end(new_map));
>> -                       map__set_pgoff(map_ref, map__pgoff(new_map));
>> -                       map__set_mapping_type(map_ref, map__mapping_type(new_map));
>> -
>> -                       err = maps__insert(kmaps, map_ref);
>> -                       map__put(map_ref);
>> -                       map__put(new_map);
>> -                       if (err)
>> -                               goto out_err;
>> -               } else {
>> +               /* skip if replacement_map, already inserted above */
>> +               if (!RC_CHK_EQUAL(new_map, replacement_map)) {
>>                         /*
>>                          * Merge kcore map into existing maps,
>>                          * and ensure that current maps (eBPF)
>> --
>> 2.34.1
>>

Re: [PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols

[Leo Yan]

On 5/8/2024 10:14 AM, James Clark wrote:

[...]

>> Looks like you and Leo are working on the same problem.
>>
>> https://lore.kernel.org/r/20240505202805.583253-1-leo.yan@arm.com/
>>
> 
> Oops I should have checked the list. It looks like we can still take his
> fix as well though, with an updated comment.

Sorry for duplicate work. I will resend my patch separately with refined
comment, as suggested by Adrian.

[...]

>>> @@ -1289,7 +1289,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>>  {
>>>         struct maps *kmaps = map__kmaps(map);
>>>         struct kcore_mapfn_data md;
>>> -       struct map *replacement_map = NULL;
>>> +       struct map *map_ref, *replacement_map = NULL;
>>>         struct machine *machine;
>>>         bool is_64_bit;
>>>         int err, fd;
>>> @@ -1367,6 +1367,24 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>>         if (!replacement_map)
>>>                 replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;

As the 'replacement' map is mainly used to adjust the kernel's sections
between '_stext' and '_end', some arches might don't share the same issue with
Arm64.  So it is a bit redundant for assignment 'replacement_map' if it is
NULL, we can consider to remove the above two lines.

>>>
>>> +       /*
>>> +        * Update addresses of vmlinux map. Re-insert it to ensure maps are
>>> +        * correctly ordered. Do this before using maps__merge_in() for the
>>> +        * remaining maps so vmlinux gets split if necessary.
>>> +        */
>>> +       map_ref = map__get(map);
>>> +       maps__remove(kmaps, map_ref);
>>
>> A nitpick.  It'd be natural to use 'map' instead of 'map_ref'
>> (even if they are the same) since IIUC we want to remove
>> the old 'map' and update 'map_ref' then add it back.
>>
> 
> Using map makes sense, I can update that.
> 
>>> +
>>> +       map__set_start(map_ref, map__start(replacement_map));
>>> +       map__set_end(map_ref, map__end(replacement_map));
>>> +       map__set_pgoff(map_ref, map__pgoff(replacement_map));
>>> +       map__set_mapping_type(map_ref, map__mapping_type(replacement_map));
>>
>> So here, replacement_map should not be NULL right?
>>
> 
> Yes it shouldn't be. It would only be NULL if md.maps is empty, but
> there's already an exit condition for that above.
> 
> Some of the other code also assumes node->map is always set, so it can't
> be NULL that way either.

Thus, we can consider to check condition for 'replacement' map is NULL or not.

    if (replacement_map) {
	list_del_init(&new_node->node);

	map_ref = map__get(map);
	maps__remove(kmaps, map_ref);
	...
	map__put(new_map);
	if (err)
		goto out_err;
	free(new_node);
    }

[...]

>>> @@ -1374,24 +1392,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>>
>>>                 list_del_init(&new_node->node);
>>>
>>> -               if (RC_CHK_EQUAL(new_map, replacement_map)) {
>>> -                       struct map *map_ref;
>>> -
>>> -                       /* Ensure maps are correctly ordered */
>>> -                       map_ref = map__get(map);
>>> -                       maps__remove(kmaps, map_ref);
>>> -
>>> -                       map__set_start(map_ref, map__start(new_map));
>>> -                       map__set_end(map_ref, map__end(new_map));
>>> -                       map__set_pgoff(map_ref, map__pgoff(new_map));
>>> -                       map__set_mapping_type(map_ref, map__mapping_type(new_map));
>>> -
>>> -                       err = maps__insert(kmaps, map_ref);
>>> -                       map__put(map_ref);
>>> -                       map__put(new_map);
>>> -                       if (err)
>>> -                               goto out_err;
>>> -               } else {
>>> +               /* skip if replacement_map, already inserted above */
>>> +               if (!RC_CHK_EQUAL(new_map, replacement_map)) {

With above change, we don't need check 'replacement_map' at here.

Just extend a bit for considering a more clean fixing, we need to sort all
ranges in 'md.maps', this would be benefit for two things:

- We can fix up any map regions, not only limit to the 'replacement_map'. With
  sorting maps in 'md.maps', we can totally remove the code for
  'replacement_map'.
- We can report the potential issue caused by overlapping in the first place
  rather than the assert log in check_invariants(). This is easier for later
  debugging.

But current patch is good enough for me, I don't have strong opinion for this.

Thanks,
Leo

>>>                         /*
>>>                          * Merge kcore map into existing maps,
>>>                          * and ensure that current maps (eBPF)
>>> --
>>> 2.34.1
>>>

[PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux()

[James Clark]

The linked commit updated dso__load_vmlinux() to call
dso__set_long_name() before loading the symbols. Loading the symbols may
not succeed but dso__set_long_name() takes ownership of the string. The
two callers of this function free the string themselves on failure
cases, resulting in the following error:

  $ perf record -- ls
  $ perf report

  free(): double free detected in tcache 2

Fix it by always taking ownership of the string, even on failure. This
means the string is either freed at the very first early exit condition,
or later when the dso is deleted or the long name is replaced. Now no
special return value is needed to signify that the caller needs to
free the string.

Fixes: e59fea47f83e ("perf symbols: Fix DSO kernel load and symbol process to correctly map DSO to its long_name, type and adjust_symbols")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/symbol.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e98dfe766da3..6a0900dcdfd3 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1977,6 +1977,10 @@ int dso__load(struct dso *dso, struct map *map)
 	return ret;
 }
 
+/*
+ * Always takes ownership of vmlinux when vmlinux_allocated == true, even if
+ * it returns an error.
+ */
 int dso__load_vmlinux(struct dso *dso, struct map *map,
 		      const char *vmlinux, bool vmlinux_allocated)
 {
@@ -1995,8 +1999,11 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
 	else
 		symtab_type = DSO_BINARY_TYPE__VMLINUX;
 
-	if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type))
+	if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type)) {
+		if (vmlinux_allocated)
+			free((char *) vmlinux);
 		return -1;
+	}
 
 	/*
 	 * dso__load_sym() may copy 'dso' which will result in the copies having
@@ -2039,7 +2046,6 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
 		err = dso__load_vmlinux(dso, map, filename, true);
 		if (err > 0)
 			goto out;
-		free(filename);
 	}
 out:
 	return err;
@@ -2191,7 +2197,6 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
 		err = dso__load_vmlinux(dso, map, filename, true);
 		if (err > 0)
 			return err;
-		free(filename);
 	}
 
 	if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
-- 
2.34.1

Re: [PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux()

[Ian Rogers]

On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>
> The linked commit updated dso__load_vmlinux() to call
> dso__set_long_name() before loading the symbols. Loading the symbols may
> not succeed but dso__set_long_name() takes ownership of the string. The
> two callers of this function free the string themselves on failure
> cases, resulting in the following error:
>
>   $ perf record -- ls
>   $ perf report
>
>   free(): double free detected in tcache 2
>
> Fix it by always taking ownership of the string, even on failure. This
> means the string is either freed at the very first early exit condition,
> or later when the dso is deleted or the long name is replaced. Now no
> special return value is needed to signify that the caller needs to
> free the string.
>
> Fixes: e59fea47f83e ("perf symbols: Fix DSO kernel load and symbol process to correctly map DSO to its long_name, type and adjust_symbols")
> Signed-off-by: James Clark <james.clark@arm.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/symbol.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e98dfe766da3..6a0900dcdfd3 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1977,6 +1977,10 @@ int dso__load(struct dso *dso, struct map *map)
>         return ret;
>  }
>
> +/*
> + * Always takes ownership of vmlinux when vmlinux_allocated == true, even if
> + * it returns an error.
> + */
>  int dso__load_vmlinux(struct dso *dso, struct map *map,
>                       const char *vmlinux, bool vmlinux_allocated)
>  {
> @@ -1995,8 +1999,11 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>         else
>                 symtab_type = DSO_BINARY_TYPE__VMLINUX;
>
> -       if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type))
> +       if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type)) {
> +               if (vmlinux_allocated)
> +                       free((char *) vmlinux);
>                 return -1;
> +       }
>
>         /*
>          * dso__load_sym() may copy 'dso' which will result in the copies having
> @@ -2039,7 +2046,6 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
>                 err = dso__load_vmlinux(dso, map, filename, true);
>                 if (err > 0)
>                         goto out;
> -               free(filename);
>         }
>  out:
>         return err;
> @@ -2191,7 +2197,6 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
>                 err = dso__load_vmlinux(dso, map, filename, true);
>                 if (err > 0)
>                         return err;
> -               free(filename);
>         }
>
>         if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
> --
> 2.34.1
>

Re: [PATCH 0/4] perf maps/symbols: Various assert fixes

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 03:12:04PM +0100, James Clark wrote:
> A few different asserts are hit when running perf report on minimal
> Arm systems when kcore is used, or the .debug/ info can't be loaded or
> /boot isn't mounted etc.
> 
> These result in some less common paths being hit for resolving symbols
> and things are done in an order that breaks some assumptions. I'm not
> sure if we could do something to make the tests pick this up, but maybe
> not easily if it would involve mocking the filesystem or even a specific
> kernel. I tried a few different variations of --kcore and --vmlinux
> arguments but ultimately I could only reproduce these issues by running
> on specific kernels and root filesystems.

Please consider adding Fixes tags so that we can help the work of
backporters/stable?

- Arnaldo
 
> James Clark (4):
>   perf symbols: Remove map from list before updating addresses
>   perf maps: Re-use __maps__free_maps_by_name()
>   perf symbols: Update kcore map before merging in remaining symbols
>   perf symbols: Fix ownership of string in dso__load_vmlinux()
> 
>  tools/perf/util/maps.c   | 14 ++++++------
>  tools/perf/util/symbol.c | 49 ++++++++++++++++++++++++----------------
>  2 files changed, 36 insertions(+), 27 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH 0/4] perf maps/symbols: Various assert fixes

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 12:11:42PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 03:12:04PM +0100, James Clark wrote:
> > A few different asserts are hit when running perf report on minimal
> > Arm systems when kcore is used, or the .debug/ info can't be loaded or
> > /boot isn't mounted etc.
> > 
> > These result in some less common paths being hit for resolving symbols
> > and things are done in an order that breaks some assumptions. I'm not
> > sure if we could do something to make the tests pick this up, but maybe
> > not easily if it would involve mocking the filesystem or even a specific
> > kernel. I tried a few different variations of --kcore and --vmlinux
> > arguments but ultimately I could only reproduce these issues by running
> > on specific kernels and root filesystems.
> 
> Please consider adding Fixes tags so that we can help the work of
> backporters/stable?

Sorry, you already did it for the last two patches in the series, so you
couldn't find easily what were the csets that introduced the problems in
the first two patches?

- Arnaldo
>  
> > James Clark (4):
> >   perf symbols: Remove map from list before updating addresses
> >   perf maps: Re-use __maps__free_maps_by_name()
> >   perf symbols: Update kcore map before merging in remaining symbols
> >   perf symbols: Fix ownership of string in dso__load_vmlinux()
> > 
> >  tools/perf/util/maps.c   | 14 ++++++------
> >  tools/perf/util/symbol.c | 49 ++++++++++++++++++++++++----------------
> >  2 files changed, 36 insertions(+), 27 deletions(-)
> > 
> > -- 
> > 2.34.1

Re: [PATCH 0/4] perf maps/symbols: Various assert fixes

[James Clark]

On 07/05/2024 16:12, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 12:11:42PM -0300, Arnaldo Carvalho de Melo wrote:
>> On Tue, May 07, 2024 at 03:12:04PM +0100, James Clark wrote:
>>> A few different asserts are hit when running perf report on minimal
>>> Arm systems when kcore is used, or the .debug/ info can't be loaded or
>>> /boot isn't mounted etc.
>>>
>>> These result in some less common paths being hit for resolving symbols
>>> and things are done in an order that breaks some assumptions. I'm not
>>> sure if we could do something to make the tests pick this up, but maybe
>>> not easily if it would involve mocking the filesystem or even a specific
>>> kernel. I tried a few different variations of --kcore and --vmlinux
>>> arguments but ultimately I could only reproduce these issues by running
>>> on specific kernels and root filesystems.
>>
>> Please consider adding Fixes tags so that we can help the work of
>> backporters/stable?
> 
> Sorry, you already did it for the last two patches in the series, so you
> couldn't find easily what were the csets that introduced the problems in
> the first two patches?
> 
> - Arnaldo

1,3 and 4 are all fixes and have tags. 2 is just a tidyup so I didn't
add the tag. I probably should have explained that in the cover letter.

>>  
>>> James Clark (4):
>>>   perf symbols: Remove map from list before updating addresses
>>>   perf maps: Re-use __maps__free_maps_by_name()
>>>   perf symbols: Update kcore map before merging in remaining symbols
>>>   perf symbols: Fix ownership of string in dso__load_vmlinux()
>>>
>>>  tools/perf/util/maps.c   | 14 ++++++------
>>>  tools/perf/util/symbol.c | 49 ++++++++++++++++++++++++----------------
>>>  2 files changed, 36 insertions(+), 27 deletions(-)
>>>
>>> -- 
>>> 2.34.1
>