False positive kmemleak report for dtb properties names on powerpc

False positive kmemleak report for dtb properties names on powerpc

[Ariel Marcovitch]

Hello!

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).

(Actually the problem will probably reproduce on every ppc kernel with
HIGHMEM enabled, but I only checked this config)

I got 97 leak reports, all similar to the following:

```

unreferenced object 0xc1803840 (size 16):
   comm "swapper", pid 1, jiffies 4294892303 (age 39.320s)
   hex dump (first 16 bytes):
     64 65 76 69 63 65 5f 74 79 70 65 00 00 00 00 00 device_type.....
   backtrace:
     [<(ptrval)>] kstrdup+0x40/0x98
     [<(ptrval)>] __of_add_property_sysfs+0xa4/0x10c
     [<(ptrval)>] __of_attach_node_sysfs+0xc0/0x110
     [<(ptrval)>] of_core_init+0xa8/0x15c
     [<(ptrval)>] driver_init+0x24/0x3c
     [<(ptrval)>] kernel_init_freeable+0xb8/0x23c
     [<(ptrval)>] kernel_init+0x24/0x14c
     [<(ptrval)>] ret_from_kernel_thread+0x5c/0x64
```

The objects in the reports are the names of the sysfs files created for 
the dtb
nodes and properties.

These are definitely not leaked, as they are even visible to the user as 
the sysfs file names.

These strings (for dtb properties, in the case of the shown report, but 
the case with dtb nodes is very similar) are created in 
__of_add_property_sysfs() and the pointer to them is stored in 
pp->attr.attr.name (so, actually stored in the memory pointed by pp)

pp is one of the dtb property objects which are allocated in 
early_init_dt_alloc_memory_arch() in of/fdt.c using memblock_alloc. This 
happens very early, in setup_arch()->unflatten_device_tree().

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.

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.

I don't suppose I can just shuffle the calls in setup_arch() around, so 
I wanted to hear your opinions first

Thanks!

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

[Catalin Marinas]

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):

-------------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)
 		/*
 		 * 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

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

[Mike Rapoport]

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

-- 
Sincerely yours,
Mike.

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

[Ariel Marcovitch]

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

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

[Christophe Leroy]

Hi Ariel

Le 09/04/2022 à 15:47, Ariel Marcovitch a écrit :
> Hi Christophe, did you get the chance to look at this?

I tested something this morning, it works for me, see below

> 
> 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)

...

>>>> 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.

I think memblock is already available at the end of MMU_init() on PPC32 
and at the end of early_setup() on PPC64. It means it is ready when we 
enter setup_arch().

I tested the change below, it works for me, I don't get any kmemleak 
report anymore.

diff --git a/arch/powerpc/kernel/setup-common.c 
b/arch/powerpc/kernel/setup-common.c
index 518ae5aa9410..9f4e50b176c9 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
  	/* Set a half-reasonable default so udelay does something sensible */
  	loops_per_jiffy = 500000000 / HZ;

+	/* Parse memory topology */
+	mem_topology_setup();
+
  	/* Unflatten the device-tree passed by prom_init or kexec */
  	unflatten_device_tree();

@@ -882,9 +885,6 @@ void __init setup_arch(char **cmdline_p)
  	/* Check the SMT related command line arguments (ppc64). */
  	check_smt_enabled();

-	/* Parse memory topology */
-	mem_topology_setup();
-
  	/*
  	 * Release secondary cpus out of their spinloops at 0x60 now that
  	 * we can map physical -> logical CPU ids.
---


Christophe

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

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Hi Ariel
>
> Le 09/04/2022 à 15:47, Ariel Marcovitch a écrit :
>> Hi Christophe, did you get the chance to look at this?
>
> I tested something this morning, it works for me, see below
>
>> 
>> 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)
>
> ...
>
>>>>> 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.
>
> I think memblock is already available at the end of MMU_init() on PPC32 
> and at the end of early_setup() on PPC64. It means it is ready when we 
> enter setup_arch().
>
> I tested the change below, it works for me, I don't get any kmemleak 
> report anymore.
>
> diff --git a/arch/powerpc/kernel/setup-common.c 
> b/arch/powerpc/kernel/setup-common.c
> index 518ae5aa9410..9f4e50b176c9 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
>   	/* Set a half-reasonable default so udelay does something sensible */
>   	loops_per_jiffy = 500000000 / HZ;
>
> +	/* Parse memory topology */
> +	mem_topology_setup();
> +
>   	/* Unflatten the device-tree passed by prom_init or kexec */
>   	unflatten_device_tree();

The 64-bit/NUMA version of mem_topology_setup() requires the device tree
to be unflattened, so I don't think that can work.

Setting max_low_pfn etc in MMU_init() as Mike suggested seems more
likely to work.

But we might need to set it again in mem_topology_setup() though, so
that things that change memblock_end_of_DRAM() are reflected, eg. memory
limit or crash dump?

cheers

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

[Mike Rapoport]

On Tue, Apr 12, 2022 at 04:47:47PM +1000, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > Hi Ariel
> >
> > Le 09/04/2022 à 15:47, Ariel Marcovitch a écrit :
> >> Hi Christophe, did you get the chance to look at this?
> >
> > I tested something this morning, it works for me, see below
> >
> >> 
> >> 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)
> >
> > ...
> >
> >>>>> 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.
> >
> > I think memblock is already available at the end of MMU_init() on PPC32 
> > and at the end of early_setup() on PPC64. It means it is ready when we 
> > enter setup_arch().
> >
> > I tested the change below, it works for me, I don't get any kmemleak 
> > report anymore.
> >
> > diff --git a/arch/powerpc/kernel/setup-common.c 
> > b/arch/powerpc/kernel/setup-common.c
> > index 518ae5aa9410..9f4e50b176c9 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
> >   	/* Set a half-reasonable default so udelay does something sensible */
> >   	loops_per_jiffy = 500000000 / HZ;
> >
> > +	/* Parse memory topology */
> > +	mem_topology_setup();
> > +
> >   	/* Unflatten the device-tree passed by prom_init or kexec */
> >   	unflatten_device_tree();
> 
> The 64-bit/NUMA version of mem_topology_setup() requires the device tree
> to be unflattened, so I don't think that can work.
> 
> Setting max_low_pfn etc in MMU_init() as Mike suggested seems more
> likely to work.
> 
> But we might need to set it again in mem_topology_setup() though, so
> that things that change memblock_end_of_DRAM() are reflected, eg. memory
> limit or crash dump?

I don't think this can cause issues for kmemleak Ariel reported. The
kmemleak checks if there is a linear mapping for a PFN or that PFN is only
accessible via HIGHMEM. Memory limit or crash dump won't change the split,
or am I missing something?
 
> cheers

-- 
Sincerely yours,
Mike.

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

[Ariel Marcovitch]

Ping :)

On 18/02/2022 21:45, Ariel Marcovitch wrote:
> Hello!
>
> 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).
>
> (Actually the problem will probably reproduce on every ppc kernel with
> HIGHMEM enabled, but I only checked this config)
>
> I got 97 leak reports, all similar to the following:
>
> ```
>
> unreferenced object 0xc1803840 (size 16):
>   comm "swapper", pid 1, jiffies 4294892303 (age 39.320s)
>   hex dump (first 16 bytes):
>     64 65 76 69 63 65 5f 74 79 70 65 00 00 00 00 00 device_type.....
>   backtrace:
>     [<(ptrval)>] kstrdup+0x40/0x98
>     [<(ptrval)>] __of_add_property_sysfs+0xa4/0x10c
>     [<(ptrval)>] __of_attach_node_sysfs+0xc0/0x110
>     [<(ptrval)>] of_core_init+0xa8/0x15c
>     [<(ptrval)>] driver_init+0x24/0x3c
>     [<(ptrval)>] kernel_init_freeable+0xb8/0x23c
>     [<(ptrval)>] kernel_init+0x24/0x14c
>     [<(ptrval)>] ret_from_kernel_thread+0x5c/0x64
> ```
>
> The objects in the reports are the names of the sysfs files created 
> for the dtb
> nodes and properties.
>
> These are definitely not leaked, as they are even visible to the user 
> as the sysfs file names.
>
> These strings (for dtb properties, in the case of the shown report, 
> but the case with dtb nodes is very similar) are created in 
> __of_add_property_sysfs() and the pointer to them is stored in 
> pp->attr.attr.name (so, actually stored in the memory pointed by pp)
>
> pp is one of the dtb property objects which are allocated in 
> early_init_dt_alloc_memory_arch() in of/fdt.c using memblock_alloc. 
> This happens very early, in setup_arch()->unflatten_device_tree().
>
> 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.
>
> 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.
>
> I don't suppose I can just shuffle the calls in setup_arch() around, 
> so I wanted to hear your opinions first
>
> Thanks!
>

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

[Ariel Marcovitch]

Pinging again :)

On 25/02/2022 0:27, Ariel Marcovitch wrote:
> Ping :)
>
> On 18/02/2022 21:45, Ariel Marcovitch wrote:
>> Hello!
>>
>> 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).
>>
>> (Actually the problem will probably reproduce on every ppc kernel with
>> HIGHMEM enabled, but I only checked this config)
>>
>> I got 97 leak reports, all similar to the following:
>>
>> ```
>>
>> unreferenced object 0xc1803840 (size 16):
>>   comm "swapper", pid 1, jiffies 4294892303 (age 39.320s)
>>   hex dump (first 16 bytes):
>>     64 65 76 69 63 65 5f 74 79 70 65 00 00 00 00 00 device_type.....
>>   backtrace:
>>     [<(ptrval)>] kstrdup+0x40/0x98
>>     [<(ptrval)>] __of_add_property_sysfs+0xa4/0x10c
>>     [<(ptrval)>] __of_attach_node_sysfs+0xc0/0x110
>>     [<(ptrval)>] of_core_init+0xa8/0x15c
>>     [<(ptrval)>] driver_init+0x24/0x3c
>>     [<(ptrval)>] kernel_init_freeable+0xb8/0x23c
>>     [<(ptrval)>] kernel_init+0x24/0x14c
>>     [<(ptrval)>] ret_from_kernel_thread+0x5c/0x64
>> ```
>>
>> The objects in the reports are the names of the sysfs files created 
>> for the dtb
>> nodes and properties.
>>
>> These are definitely not leaked, as they are even visible to the user 
>> as the sysfs file names.
>>
>> These strings (for dtb properties, in the case of the shown report, 
>> but the case with dtb nodes is very similar) are created in 
>> __of_add_property_sysfs() and the pointer to them is stored in 
>> pp->attr.attr.name (so, actually stored in the memory pointed by pp)
>>
>> pp is one of the dtb property objects which are allocated in 
>> early_init_dt_alloc_memory_arch() in of/fdt.c using memblock_alloc. 
>> This happens very early, in setup_arch()->unflatten_device_tree().
>>
>> 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.
>>
>> 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.
>>
>> I don't suppose I can just shuffle the calls in setup_arch() around, 
>> so I wanted to hear your opinions first
>>
>> Thanks!
>>