Re: [PATCH] mm: kmemleak: check boundary of objects allocated with physical address when scan

[patrick wang <patrick.wang.shcn@gmail.com>]

On Thu, Jun 2, 2022 at 12:13 AM Catalin Marinas <catalin.marinas@arm.com> 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