Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1e976df3-2925-f6c6-6723-67f127b9e544@gmail.com>
Date:   Fri, 25 Feb 2022 00:27:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: False positive kmemleak report for dtb properties names on
 powerpc
Content-Language: en-US
From:   Ariel Marcovitch <arielmarcovitch@gmail.com>
To:     catalin.marinas@arm.com, akpm@linux-foundation.org,
        mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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