Messages in this thread |  | | Date | Sat, 9 Apr 2022 16:47:38 +0300 | Subject | Re: False positive kmemleak report for dtb properties names on powerpc | From | 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
|  |