Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220531150823.1004101-1-patrick.wang.shcn@gmail.com>
 <YpZCWbfNE32EzCnz@arm.com> <99faf6b0-30bf-f87c-2620-1eafb4eac1ac@gmail.com> <YpeQNkk31d7JL9g6@arm.com>
In-Reply-To: <YpeQNkk31d7JL9g6@arm.com>
From:   patrick wang <patrick.wang.shcn@gmail.com>
Date:   Thu, 2 Jun 2022 18:22:10 +0800
Message-ID: <CAGcnep_brHvOBv=cM6y4zdefc=kdJyRxG2W8_yaYeBOt0vtQ8Q@mail.gmail.com>
Subject: Re: [PATCH] mm: kmemleak: check boundary of objects allocated with
 physical address when scan
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        linux-kernel@vger.kernel.org, Yee Lee <yee.lee@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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