linux-kernel - Re: [PATCH v1 0/2] Fixes for kmemleak tracking with CMA regions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y8neaPB2y689WKOf@google.com>
Date:   Thu, 19 Jan 2023 16:20:56 -0800
From:   Isaac Manjarres <isaacmanjarres@...gle.com>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Saravana Kannan <saravanak@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        kernel-team@...roid.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 0/2] Fixes for kmemleak tracking with CMA regions

On Wed, Jan 18, 2023 at 05:16:46PM +0000, Catalin Marinas wrote:
> Hi Isaac,
> 
> Please cc me on kmemleak patches. I only noticed when Andrew picket them
> up.
Will do, sorry about that.
> 
> What I don't understand is why kmemleak scans such CMA regions. The only
> reason for a kmemleak_ignore_phys() call in cma_declare_contiguous_nid()
> is because the kmemleak_alloc_phys() hook was called on the
> memblock_alloc_range_nid() path, so we don't want this scanned.
The reason is because kmemleak_ignore_phys() is only called within
cma_declare_contiguous_nid(), which is not called for every CMA region.

For instance, CMA regions which are specified through the devicetree
and not constrained to a fixed address are allocated through
early_init_dt_alloc_reserved_memory_arch(), which eventually calls
kmemleak_alloc_phys() through memblock_phys_alloc_range().

When the CMA region is constrained to a particular address, it is allocated
through early_init_dt_reserve_memory(), which is followed up by a call to
kmemleak_alloc_phys() due to this commit:
https://lore.kernel.org/all/20211123090641.3654006-1-calvinzhang.cool@gmail.com/T/#u

I'm not sure if that commit is appropriate, given that reserved regions
that still have their direct mappings intact may be used for DMA, which
isn't appropriate for kmemleak scanning.

In any one of these cases, the kmemleak object is never removed, nor is
kmemleak told to ignore it, so it ends up scanning it later.

> Do you have a backtrace?
Yes:

[   61.155981][   T97] Unable to handle kernel paging request at virtual address ...
[   61.156241][   T97] Hardware name: Oriole EVT 1.1 (DT)
[   61.156243][   T97] pstate: 204000c5 (nzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   61.156246][   T97] pc : scan_block+0xbc/0x3c8
[   61.156253][   T97] lr : scan_block+0xac/0x3c8
[   61.156291][   T97] Call trace:
[   61.156293][   T97]  scan_block+0xbc/0x3c8
[   61.156296][   T97]  scan_gray_list+0x130/0x23c
[   61.156299][   T97]  kmemleak_scan+0x408/0x71c
[   61.156302][   T97]  kmemleak_scan_thread+0xc0/0xf0
[   61.156306][   T97]  kthread+0x114/0x15c
[   61.156311][   T97]  ret_from_fork+0x10/0x20

when I looked at the PTE from the page table walk, I saw that the virtual
address mapped to the physical address within a CMA region, and that the
page was unmapped (because of CONFIG_DEBUG_PAGEALLOC).

> kmemleak would only scan such objects if it knows about them. So I think
> it's only the case where CMA does a memblock allocation. The
> kmemleak_ignore_phys() should tell kmemleak not to touch this region but
> it's probably better to just free it altogether (i.e. replace the ignore
> with the free kmemleak callback). Would this be sufficient for your
> scenario?
I agree that freeing the kmemleak object is a better strategy. However,
replacing the call to kmemleak_ignore_phys() wouldn't be sufficient,
as there are other scenarios that would still leave behind kmemleak
objects to be scanned. That's why I ended up freeing the kmemleak object
in a path that is common for all CMA areas.

> I may be missing something but I don't get why kmemleak needs to be
> informed only to tell kmemleak shortly after to remove them from its
> list of objects.
That's a good point, and I agree that it's pointless; ideally whatever
way the CMA region is allocated would not inform kmemleak, and we
wouldn't have to deal with kmemleak.

We could use a flag like MEMBLOCK_ALLOC_NOLEAKTRACE to tell
memblock to skip the call to kmemleak_alloc_phys(), but that wouldn't
work as is, since MEMBLOCK_ALLOC_NOLEAKTRACE is meant to be used as the
"end" parameter for memblock_alloc() and friends. For CMA regions "end"
can be used (e.g. the case where the CMA region is supposed to be
within a certain range).

Perhaps we should change memblock to take MEMBLOCK_ALLOC_NOLEAKTRACE as
a flag under a "flags" argument instead?

--Isaac