linux-kernel - Re: [PATCH] mm: remove redundant clear page when CONFIG_INIT_ON_ALLOC_DEFAULT

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: <ZP8MR6N8512YDU/j@casper.infradead.org>
Date:   Mon, 11 Sep 2023 13:47:03 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Michal Hocko <mhocko@...e.com>
Cc:     "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Zhaoyang Huang <huangzhaoyang@...il.com>, ke.wang@...soc.com
Subject: Re: [PATCH] mm: remove redundant clear page when
 CONFIG_INIT_ON_ALLOC_DEFAULT_ON configured

On Mon, Sep 11, 2023 at 02:12:25PM +0200, Michal Hocko wrote:
> On Mon 11-09-23 18:49:06, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@...soc.com>
> > 
> > There will be redundant clear page within vma_alloc_zeroed_movable_folio
> > when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on. Remove it by judging related
> > configs.
> 
> Thanks for spotting this. I suspect this is a fix based on a code review
> rather than a real performance issue, right? It is always good to
> mention that. From a very quick look it seems that many architectures
> just definte vma_alloc_zeroed_movable_folio to use __GFP_ZERO so they
> are not affected by this. This means that only a subset of architectures
> are really affected. This is an important information as well.
> Finally I think it would be more appropriate to mention that the double
> initialization is done when init_on_alloc is enabled rather than
> referring to the above config option which only controls whether the
> functionality is enabled by default.

This may well be an unsaafe change to make.  We're not just zeroing the
page, we're calling clear_user_highpage() which tells the architecture
which virtual address the page will be mapped at.  It could be that
skipping the zeroing ("because the page is already zero") isn't enough;
there will be traces of the former contents of some page in the D-cache
for this address.

Or it might just be an optimisation.  The description of clear_user_page()
isn't entirely clear; the port may be relying on clear_user_page()
to have flushed the dcache aliases.

At this point, I don't think this patch is worth the risk.  My mind is
changable on this, but I think we'd need buy-in from ARM, SH and Xtensa
(who directly define clear_user_highpage()) as well as Arc, csky, ia64,
m68k, mips, nios2, parisc, powerpc, sparc who all seem to have non-trivial
clear_user_page() implementations.