On Wed, Jan 18, 2023 at 05:16:46PM +0000, Catalin Marinas wrote: > Hi Isaac, > > Please cc me on kmemleak patches. I only noticed when Andrew picket them > up. Will do, sorry about that. > > What I don't understand is why kmemleak scans such CMA regions. The only > reason for a kmemleak_ignore_phys() call in cma_declare_contiguous_nid() > is because the kmemleak_alloc_phys() hook was called on the > memblock_alloc_range_nid() path, so we don't want this scanned. The reason is because kmemleak_ignore_phys() is only called within cma_declare_contiguous_nid(), which is not called for every CMA region. For instance, CMA regions which are specified through the devicetree and not constrained to a fixed address are allocated through early_init_dt_alloc_reserved_memory_arch(), which eventually calls kmemleak_alloc_phys() through memblock_phys_alloc_range(). When the CMA region is constrained to a particular address, it is allocated through early_init_dt_reserve_memory(), which is followed up by a call to kmemleak_alloc_phys() due to this commit: https://lore.kernel.org/all/20211123090641.3654006-1-calvinzhang.cool@xxxxxxxxx/T/#u I'm not sure if that commit is appropriate, given that reserved regions that still have their direct mappings intact may be used for DMA, which isn't appropriate for kmemleak scanning. In any one of these cases, the kmemleak object is never removed, nor is kmemleak told to ignore it, so it ends up scanning it later. > Do you have a backtrace? Yes: [ 61.155981][ T97] Unable to handle kernel paging request at virtual address ... [ 61.156241][ T97] Hardware name: Oriole EVT 1.1 (DT) [ 61.156243][ T97] pstate: 204000c5 (nzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 61.156246][ T97] pc : scan_block+0xbc/0x3c8 [ 61.156253][ T97] lr : scan_block+0xac/0x3c8 [ 61.156291][ T97] Call trace: [ 61.156293][ T97] scan_block+0xbc/0x3c8 [ 61.156296][ T97] scan_gray_list+0x130/0x23c [ 61.156299][ T97] kmemleak_scan+0x408/0x71c [ 61.156302][ T97] kmemleak_scan_thread+0xc0/0xf0 [ 61.156306][ T97] kthread+0x114/0x15c [ 61.156311][ T97] ret_from_fork+0x10/0x20 when I looked at the PTE from the page table walk, I saw that the virtual address mapped to the physical address within a CMA region, and that the page was unmapped (because of CONFIG_DEBUG_PAGEALLOC). > kmemleak would only scan such objects if it knows about them. So I think > it's only the case where CMA does a memblock allocation. The > kmemleak_ignore_phys() should tell kmemleak not to touch this region but > it's probably better to just free it altogether (i.e. replace the ignore > with the free kmemleak callback). Would this be sufficient for your > scenario? I agree that freeing the kmemleak object is a better strategy. However, replacing the call to kmemleak_ignore_phys() wouldn't be sufficient, as there are other scenarios that would still leave behind kmemleak objects to be scanned. That's why I ended up freeing the kmemleak object in a path that is common for all CMA areas. > I may be missing something but I don't get why kmemleak needs to be > informed only to tell kmemleak shortly after to remove them from its > list of objects. That's a good point, and I agree that it's pointless; ideally whatever way the CMA region is allocated would not inform kmemleak, and we wouldn't have to deal with kmemleak. We could use a flag like MEMBLOCK_ALLOC_NOLEAKTRACE to tell memblock to skip the call to kmemleak_alloc_phys(), but that wouldn't work as is, since MEMBLOCK_ALLOC_NOLEAKTRACE is meant to be used as the "end" parameter for memblock_alloc() and friends. For CMA regions "end" can be used (e.g. the case where the CMA region is supposed to be within a certain range). Perhaps we should change memblock to take MEMBLOCK_ALLOC_NOLEAKTRACE as a flag under a "flags" argument instead? --Isaac
