Re: [PATCH v3 1/3] perf maps: Sort kcore maps

[Adrian Hunter]

On 28/05/24 18:00, James Clark wrote:
> 
> 
> On 25/05/2024 10:29, Leo Yan wrote:
>> Hi Adrian,
>>
>> On 5/22/2024 11:31 AM, Adrian Hunter wrote:
>>> On 20/05/24 12:06, Leo Yan wrote:
>>>> When merging kcore maps into the kernel maps, it has an implicit
>>>> requirement for the kcore maps ordering, otherwise, some sections
>>>> delivered by the kcore maps will be ignored.
>>>
>>> perf treats the kernel text map as a special case.  The problem
>>> is that the kcore loading logic did not cater for there being 2
>>> maps that covered the kernel mapping.
>>>
>>> The workaround was to choose the smaller mapping, but then that
>>> still only worked if that was the first.
>>
>> You could see below are Kcore maps dumped on Arm64:
>>
>> kore map start: ffff000000000000 end: ffff00001ac00000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff00001ad88000 end: ffff000032000000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff000032101000 end: ffff00003e000000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff000040000000 end: ffff000089b80000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff000089cc0000 end: ffff0000b9ab0000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff0000b9ad0000 end: ffff0000b9bb0000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff0000b9c50000 end: ffff0000b9d50000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff0000ba114000 end: ffff0000bf130000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff0000bf180000 end: ffff0000e0000000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff000200000000 end: ffff000220000000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff800000000000 end: ffff800080000000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff800080000000 end: ffff8000822f0000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: ffff800080000000 end: fffffdffbf800000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc0000000 end: fffffdffc06b0000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc06b6000 end: fffffdffc0c80000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc0c84000 end: fffffdffc0f80000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc1000000 end: fffffdffc226e000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc2273000 end: fffffdffc2e6b000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc2e6b000 end: fffffdffc2e6f000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc2e71000 end: fffffdffc2e76000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc2e84000 end: fffffdffc2fc5000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc2fc6000 end: fffffdffc3800000 name: [kernel.kallsyms]
>> refcnt: 1
>> kore map start: fffffdffc8000000 end: fffffdffc8800000 name: [kernel.kallsyms]
>> refcnt: 1
>>
>> You could see it's much more complex rather than only for kernel text section
>> and vmalloc region. We cannot only handle the case for the overlapping between
>> the text section and vmalloc region, it is possible for other maps to be
>> overlapping with each other.

That should not matter because maps__merge_in() deals with overlaps. Just
need the replacement map to cover the kernel text.

There would be a problem if the mappings were inconsistent i.e. the overlapped
part pointed to a different part of the file.  Not sure how much sense there is
in general in overlapping program headers in an ELF file, but if they exist,
they *must* have:

	vaddr_1 - offset_1 == vaddr_2 - offset_2

We could add a check for that, but that would be fatal i.e. fail to load kcore
at all.

>>
>> And different arches have their own definition for the Kcore maps. This is why
>> I want to sort maps in this patch, it can allow us to find a reliable way to
>> append the kcore maps.
>>
>>> James essentially fixed that by ensuring the kernel "replacement"
>>> map is inserted first.
>>
>> Yeah, I agreed James' patch has fixed the kernel "replacement" map. But as I
>> elaborated above, there still have other maps might overlap with each other,
>> my understanding is we don't handle all cases.
>>> In other respects, the ordering of the maps does not matter, so
>>> I am not sure it is worth this extra processing.
>>
>> To sell my patch, I have another point for why we need sorting Kcore maps.
>>
>> Now Perf verifies the map in the function check_invariants(), this function
>> reports the broken issue, but we still have no clue how the broken issue happens.
>>
>> If we can sort the Kcore maps in kcore_mapfn(), then we have chance to detect
>> the overlapping within maps, and then it can reports the overlapping in the
>> first place and this would be helpful for debugging and locating the failures.
>>
> 
> +1 for this point. If check_invariants() insists on sorted maps, then
> keeping them sorted as early as possible is better.
> 
> Debugging future issues will be easier if the assert is closer to the
> source of the problem.

The issue there is with using maps__insert().  It should be
maps__fixup_overlap_and_insert() instead.  Probably most callers
of maps__insert() should be using maps__fixup_overlap_and_insert().
Probably they should be renamed:

  maps__insert			  -> maps__insert_unsafe
  maps__fixup_overlap_and_insert  -> maps__insert