linux-kernel - Re: [PATCH v7 1/1] kasan: Avoid sleepable page allocation from atomic context

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] [day] [month] [year] [list]

Message-ID: <aCKSjnQdzaRvgZzo@harry>
Date: Tue, 13 May 2025 09:30:06 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Alexander Gordeev <agordeev@...ux.ibm.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Andrey Ryabinin <ryabinin.a.a@...il.com>,
        Daniel Axtens <dja@...ens.net>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, kasan-dev@...glegroups.com,
        linux-s390@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v7 1/1] kasan: Avoid sleepable page allocation from
 atomic context

On Mon, May 12, 2025 at 06:31:30PM +0200, Alexander Gordeev wrote:
> On Tue, May 13, 2025 at 12:33:35AM +0900, Harry Yoo wrote:
> > Thanks for the update, but I don't think nr_populated is sufficient
> > here. If nr_populated in the last iteration is smaller than its value
> > in any previous iteration, it could lead to a memory leak.
> > 
> > That's why I suggested (PAGE_SIZE / sizeof(data.pages[0])).
> > ...but on second thought maybe touching the whole array is not
> > efficient either.
> 
> Yes, I did not like it and wanted to limit the number of pages,
> but did not realize that using nr_populated still could produce
> leaks. In addition I could simply do:
> 
> 	max_populted = max(max_populted, nr_populated);
> 	...
> 	free_pages_bulk(data.pages, max_populated);

Yeah that could work, but given that it already confused you,
I think we should focus on fixing the bug and defer further
improvements later, since it will be backported to -stable.

> > If this ends up making things complicated probably we should just
> > merge v6 instead (v6 looks good)? micro-optimizing vmalloc shadow memory
> > population doesn't seem worth it if it comes at the cost of complexity :)
> 
> v6 is okay, except that in v7 I use break instead of return:
> 
> 	ret = apply_to_page_range(...);
> 	if (ret)
> 		break;
> 
> and as result can call the final:
> 
> 	free_page((unsigned long)data.pages);

Uh, I didn't realize that while reviewing.

I think at this stage (-rc6) Andrew will prefer a fixup patch on top of
v6. I think this [1] could fix it, but could you please verify it's
correct and send a fixup patch (as a reply to v6)?

[1] https://lore.kernel.org/mm-commits/aCKJYHPL_3xAewUB@hyeyoo

-- 
Cheers,
Harry / Hyeonggon