On Wed, Jun 01, 2022 at 06:24:34PM +0800, Patrick Wang wrote: > On 2022/6/1 00:29, Catalin Marinas wrote: > > On Tue, May 31, 2022 at 11:08:23PM +0800, Patrick Wang wrote: > > > + if (kmemleak_enabled && (unsigned long)__va(phys) >= PAGE_OFFSET && > > > + !IS_ERR(__va(phys))) > > > + /* create object with OBJECT_PHYS flag */ > > > + create_object((unsigned long)__va(phys), size, min_count, > > > + gfp, true); > > > > Do we still need to check for __va(phys) >= PAGE_OFFSET? Also I don't > > think IS_ERR(__va(phys)) makes sense, we can't store an error in a > > physical address. The kmemleak_alloc_phys() function is only called on > > successful allocation, so shouldn't bother with error codes. > > In this commit: > 972fa3a7c17c(mm: kmemleak: alloc gray object for reserved > region with direct map) > > The kmemleak_alloc_phys() function is called directly by passing > physical address from devicetree. So I'm concerned that could > __va() => __pa() convert always get the phys back? I thought > check for __va(phys) might help, but it probably dosen't work > and using IS_ERR is indeed inappropriate. > > We might have to store phys in object and convert it via __va() > for normal use like: > > #define object_pointer(obj) \ > (obj->flags & OBJECT_PHYS ? (unsigned long)__va((void *)obj->pointer) \ > : obj->pointer) In the commit you mentioned, the kmemleak callback is skipped if the memory is marked no-map. But you have a point with the va->pa conversion. On 32-bit architectures, the __va() is no longer valid if the pfn is above max_low_pfn. So whatever we add to the rbtree may be entirely bogus, and we can't guarantee that the va->pa conversion back is correct. Storing the phys address in object->pointer only solves the conversion but it doesn't solve the rbtree problem (VA and PA values may overlap, we can't just store the physical address either). And we use the rbtree for searching objects on freeing as well. Given that all the kmemleak_alloc_phys() calls always pass min_count=0 (we should probably get rid of the extra arguments), we don't expect them to leak, so there's no point in adding them to the rbtree. We can instead add a new object_phys_tree_root to store these objects by the physical address for when we need to search (kmemleak_free_part_phys()). This would probably look simpler than recording the callbacks and replaying them. Wherever we use object_tree_root we should check for OBJECT_PHYS and use object_phys_tree_root instead. There aren't many places. -- Catalin
