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]
Date: Wed, 8 May 2024 09:18:18 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Leo Yan <leo.yan@...ux.dev>
Cc: Leo Yan <leo.yan@....com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Ian Rogers <irogers@...gle.com>, Namhyung Kim <namhyung@...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@...nel.org>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
 James Clark <james.clark@....com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf maps: Process kcore maps in order

On 8/05/24 00:01, Leo Yan wrote:
> Hi Adrian,
> 
> On Mon, May 06, 2024 at 08:43:01AM +0300, Adrian Hunter wrote:
> 
> [...]
> 
>>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>>> index 9ebdb8e13c0b..e15d70845488 100644
>>> --- a/tools/perf/util/symbol.c
>>> +++ b/tools/perf/util/symbol.c
>>> @@ -1266,7 +1266,24 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
>>>  	map__set_end(list_node->map, map__start(list_node->map) + len);
>>>  	map__set_pgoff(list_node->map, pgoff);
>>>  
>>> -	list_add(&list_node->node, &md->maps);
>>> +	/*
>>> +	 * Kcore maps are ordered with:
>>> +	 *   [_text.._end): Kernel text section
>>> +	 *   [VMALLOC_START..VMALLOC_END): vmalloc
>>> +	 *   ...
>>> +	 *
>>> +	 * On Arm64, the '_text' and 'VMALLOC_START' are the same values
>>> +	 * but VMALLOC_END (~124TiB) is much bigger then the text end
>>> +	 * address. So '_text' region is the subset of the vmalloc region.
>>> +	 *
>>> +	 * Afterwards, when dso__load_kcore() adjusts kernel maps, we must
>>> +	 * process the kernel text size prior to handling vmalloc region.
>>> +	 * This can avoid to using any inaccurate kernel text size when
>>> +	 * extending maps with vmalloc region. For this reason, here it
>>> +	 * always adds kcore maps to the tail of list to make sure the
>>> +	 * sequential handling is in order.
>>> +	 */
>>> +	list_add_tail(&list_node->node, &md->maps);
>>
>> This seems reasonable, but I wonder if it might be robust
>> and future proof to also process the main map first
>> e.g. totally untested:
> 
> Makes sense for me, I verified your proposal with a minor improvment,
> please see the comment below.
> 
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 9ebdb8e13c0b..63bce45a5abb 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1365,16 +1365,15 @@ 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;
>>  
>> -	/* Add new maps */
>> +	/* Add replacement_map */
>>  	while (!list_empty(&md.maps)) {
> 
> For the replacement map, as we have located it in the list, here we
> don't need to iterate the whole kcore map list anymore. We can
> directly use the replacement map to update the passed map:
> 
>         /* Update replacement_map */
>         if (replacement_map) {
>                 struct map *map_ref;
> 
>                 list_del_init(&replacement_node->node);
>                 map__set_start(map, map__start(replacement_map));
>                 map__set_end(map, map__end(replacement_map));
>                 map__set_pgoff(map, map__pgoff(replacement_map));
>                 map__set_mapping_type(map, map__mapping_type(replacement_map));
>                 /* Ensure maps are correctly ordered */
>                 map_ref = map__get(map);
>                 maps__remove(kmaps, map_ref);
>                 err = maps__insert(kmaps, map_ref);
>                 map__put(map_ref);
>                 map__put(replacement_map);
>                 if (err)
>                         goto out_err;
>                 free(replacement_node);
>         }
> 
> I also uploaded the verified change to https://termbin.com/rrfo.
> 
> Please let me know if you would like to send a patch for this, or you
> want me to spin a new version. Either is fine for me.

James has a patch that does this also and looks good:

https://lore.kernel.org/linux-perf-users/CAM9d7cjYvMndUmSuwnE1ETwnu_6WrxQ4UzsNHHvo4SVR250L7A@mail.gmail.com/T/#md3d61e4182fc5bc3aee917db9af23a39b617b8ea

However, the "list_add_tail" change still seems worth doing
because it is more logical to process in order rather than
reverse order.  Probably just need to adjust the comment and
commit message.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ