Re: False positive kmemleak report for dtb properties names on powerpc

[Ariel Marcovitch <arielmarcovitch@gmail.com>]

Hi Christophe, did you get the chance to look at this?

On 23/03/2022 21:06, Mike Rapoport wrote:
> Hi Catalin,
>
> On Wed, Mar 23, 2022 at 05:22:38PM +0000, Catalin Marinas wrote:
>> Hi Ariel,
>>
>> On Fri, Feb 18, 2022 at 09:45:51PM +0200, Ariel Marcovitch wrote:
>>> I was running a powerpc 32bit kernel (built using
>>> qemu_ppc_mpc8544ds_defconfig
>>> buildroot config, with enabling DEBUGFS+KMEMLEAK+HIGHMEM in the kernel
>>> config)
>>> on qemu and invoked the kmemleak scan (twice. for some reason the first time
>>> wasn't enough).
>> [...]
>>> I got 97 leak reports, all similar to the following:
>> [...]
>>> memblock_alloc lets kmemleak know about the allocated memory using
>>> kmemleak_alloc_phys (in mm/memblock.c:memblock_alloc_range_nid()).
>>>
>>> The problem is with the following code (mm/kmemleak.c):
>>>
>>> ```c
>>>
>>> void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, int min_count,
>>>                                 gfp_t gfp)
>>> {
>>>          if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
>>>                  kmemleak_alloc(__va(phys), size, min_count, gfp);
>>> }
>>>
>>> ```
>>>
>>> When CONFIG_HIGHMEM is enabled, the pfn of the allocated memory is checked
>>> against max_low_pfn, to make sure it is not in the HIGHMEM zone.
>>>
>>> However, when called through unflatten_device_tree(), max_low_pfn is not yet
>>> initialized in powerpc.
>>>
>>> max_low_pfn is initialized (when NUMA is disabled) in
>>> arch/powerpc/mm/mem.c:mem_topology_setup() which is called only after
>>> unflatten_device_tree() is called in the same function (setup_arch()).
>>>
>>> Because max_low_pfn is global it is 0 before initialization, so as far as
>>> kmemleak_alloc_phys() is concerned, every memory is HIGHMEM (: and the
>>> allocated memory is not tracked by kmemleak, causing references to objects
>>> allocated later with kmalloc() to be ignored and these objects are marked as
>>> leaked.
>> Thanks for digging into this. It looks like the logic doesn't work (not
>> sure whether it ever worked).
>>
>>> I actually tried to find out whether this happen on other arches as well,
>>> and it seems like arm64 also have this problem when dtb is used instead of
>>> acpi, although I haven't had the chance to confirm this.
>> arm64 doesn't enable CONFIG_HIGHMEM, so it's not affected.
>>
>>> I don't suppose I can just shuffle the calls in setup_arch() around, so I
>>> wanted to hear your opinions first
>> I think it's better if we change the logic than shuffling the calls.
>> IIUC MEMBLOCK_ALLOC_ACCESSIBLE means that __va() works on the phys
>> address return by memblock, so something like below (untested):
> MEMBLOCK_ALLOC_ACCESSIBLE means "anywhere", see commit e63075a3c937
> ("memblock: Introduce default allocation limit and use it to replace
> explicit ones"), so it won't help to detect high memory.
>
> If I remember correctly, ppc initializes memblock *very* early, so setting
> max_low_pfn along with lowmem_end_addr in
> arch/powerpc/mm/init_32::MMU_init() makes sense to me.
>
> Maybe ppc folks have other ideas...
> I've added Christophe who works on ppc32 these days.
>   
>> -------------8<----------------------
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 7580baa76af1..f3599e857c13 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -1127,8 +1127,7 @@ EXPORT_SYMBOL(kmemleak_no_scan);
>>   void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, int min_count,
>>   			       gfp_t gfp)
>>   {
>> -	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
>> -		kmemleak_alloc(__va(phys), size, min_count, gfp);
>> +	kmemleak_alloc(__va(phys), size, min_count, gfp);
>>   }
>>   EXPORT_SYMBOL(kmemleak_alloc_phys);
>>   
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index b12a364f2766..2515bd4331e8 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -1397,7 +1397,7 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
>>   	 * Skip kmemleak for those places like kasan_init() and
>>   	 * early_pgtable_alloc() due to high volume.
>>   	 */
>> -	if (end != MEMBLOCK_ALLOC_NOLEAKTRACE)
>> +	if (end == MEMBLOCK_ALLOC_ACCESSIBLE)
> This change would enable kmemleak for KASAN on arm and arm64 that AFAIR
> caused OOM in kmemleak and it also will limit tracking only to allocations
> that do not specify 'end' explicitly ;-)
>
>>   		/*
>>   		 * The min_count is set to 0 so that memblock allocated
>>   		 * blocks are never reported as leaks. This is because many
>> -------------8<----------------------
>>
>> But I'm not sure whether we'd now miss some genuine allocations where
>> the memblock limit is different from MEMBLOCK_ALLOC_ACCESSIBLE but still
>> within the lowmem limit. If the above works, we can probably get rid of
>> some other kmemleak callbacks in the kernel.
>>
>> Adding Mike for any input on memblock.
>>
>> -- 
>> Catalin