Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 1 Jun 2022 17:13:42 +0100
From:   Catalin Marinas <catalin.marinas@arm.com>
To:     Patrick Wang <patrick.wang.shcn@gmail.com>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, yee.lee@mediatek.com
Subject: Re: [PATCH] mm: kmemleak: check boundary of objects allocated with
 physical address when scan
Message-ID: <YpeQNkk31d7JL9g6@arm.com>
References: <20220531150823.1004101-1-patrick.wang.shcn@gmail.com>
 <YpZCWbfNE32EzCnz@arm.com>
 <99faf6b0-30bf-f87c-2620-1eafb4eac1ac@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <99faf6b0-30bf-f87c-2620-1eafb4eac1ac@gmail.com>
Precedence: bulk

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