Date:   Wed, 25 Jan 2023 12:08:58 +0000
From:   Catalin Marinas <catalin.marinas@arm.com>
To:     Isaac Manjarres <isaacmanjarres@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Saravana Kannan <saravanak@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        kernel-team@android.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH v1 0/2] Fixes for kmemleak tracking with CMA regions
Message-ID: <Y9Eb2k3COoUXJ0CB@arm.com>
References: <20230109221624.592315-1-isaacmanjarres@google.com>
 <Y8gpfgEXtialPVLk@arm.com>
 <Y8neaPB2y689WKOf@google.com>
 <Y8/96eIu47UfqsWO@arm.com>
 <Y9BLfUz5GY69zmjn@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9BLfUz5GY69zmjn@google.com>
Precedence: bulk

On Tue, Jan 24, 2023 at 01:19:57PM -0800, Isaac Manjarres wrote:
> On Tue, Jan 24, 2023 at 03:48:57PM +0000, Catalin Marinas wrote:
> > Thanks for digging this out. This patch shouldn't have ended up upstream
> > (commit 972fa3a7c17c "mm: kmemleak: alloc gray object for reserved
> > region with direct map"). I thought both Calvin Zhang and I agreed that
> > it's not the correct approach (not even sure there was a real problem to
> > fix).
> > 
> > Do you still get the any faults with the above commit reverted? I'd
> > prefer this if it works rather than adding unnecessary
> > kmemleak_alloc/free callbacks that pretty much cancel each-other.
> 
> Yes, I still see the same problem after reverting that commit. The problem
> still persists because there are CMA areas that are allocated through
> memblock_phys_alloc_range(), which invokes kmemleak_alloc_phys(). The
> allocation call stack is along the lines of:
> 
> kmemleak_alloc_phys()
> memblock_alloc_range_nid()
> memblock_phys_alloc_range()
> __reserved_mem_alloc_size()
> fdt_init_reserved_mem()

Ah, that's another place that kmemleak shouldn't care about.

> I also followed up on my suggestion about adding a flags parameter to
> the memblock allocation functions to be able to use
> MEMBLOCK_ALLOC_NOLEAKTRACE in this particular scenario, but that would
> involve changing many call-sites, which doesn't make much sense given
> that there are only 4 call-sites that actually use this flag.

Yeah, the current way of passing MEMBLOCK_ALLOC_NOLEAKTRACE is not
ideal. We did it more like a quick hack. See some past discussion here
(and adding Mike to this thread):

https://lore.kernel.org/all/YYUChdTeXP%2FOQUwS@arm.com/

> Maybe adding a new memblock allocation function that allows this flag to
> be passed as just a flag can be used to avoid creating these kmemleak
> objects for CMA allocations?

That's an option. If there's too much churn to add a flag, an
alternative is to use the bottom bit of 'end' to set the noleaktrace
flag.

Yet another idea is to avoid the kmemleak callback on all the 'phys'
memblock allocations. We can add the callback to the higher level
memblock_alloc() which returns a VA but the lower level 'phys' variants
could simply avoid it. However, I think we still need the
MEMBLOCK_ALLOC_NOLEAKTRACE flag for the kasan shadow allocation. Well,
given that this flag is not widely used, we can add explicit
kmemleak_ignore() calls in those four places.

I think the latter, if it works, would be the least intrusive.

-- 
Catalin