On Thu, Jun 2, 2022 at 12:13 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote: > > 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. Considering the usage of objects with OBJECT_PHYS, storing the phys address and giving their own rbtree should solve the phys problem. I will post a v2 ASAP. Thanks, Patrick
