linux-kernel - Re: [PATCH v3] mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection

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: <Yqn53TimQq33BanG@xz-m1.local>
Date:   Wed, 15 Jun 2022 11:25:17 -0400
From:   Peter Xu <peterx@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Peter Collingbourne <pcc@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nadav Amit <nadav.amit@...il.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Yang Shi <shy828301@...il.com>,
        Hugh Dickins <hughd@...gle.com>,
        Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [PATCH v3] mm/mprotect: try avoiding write faults for exclusive
 anonymous pages when changing protection

On Tue, Jun 14, 2022 at 11:36:29AM +0200, David Hildenbrand wrote:
> Similar to our MM_CP_DIRTY_ACCT handling for shared, writable mappings, we
> can try mapping anonymous pages in a private writable mapping writable if
> they are exclusive, the PTE is already dirty, and no special handling
> applies. Mapping the anonymous page writable is essentially the same thing
> the write fault handler would do in this case.
> 
> Special handling is required for uffd-wp and softdirty tracking, so take
> care of that properly. Also, leave PROT_NONE handling alone for now;
> in the future, we could similarly extend the logic in do_numa_page() or
> use pte_mk_savedwrite() here.
> 
> While this improves mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE)
> performance, it should also be a valuable optimization for uffd-wp, when
> un-protecting.
> 
> This has been previously suggested by Peter Collingbourne in [1],
> relevant in the context of the Scudo memory allocator, before we had
> PageAnonExclusive.
> 
> This commit doesn't add the same handling for PMDs (i.e., anonymous THP,
> anonymous hugetlb); benchmark results from Andrea indicate that there
> are minor performance gains, so it's might still be valuable to streamline
> that logic for all anonymous pages in the future.
> 
> As we now also set MM_CP_DIRTY_ACCT for private mappings, let's rename
> it to MM_CP_TRY_CHANGE_WRITABLE, to make it clearer what's actually
> happening.

I'm personally not sure why DIRTY_ACCT cannot be applied to private
mappings; it sounds not only for shared but a common thing.  I also don't
know whether "change writable" could be misread too anyway. Say, we're
never changing RO->RW mappings with this flag, but only try to unprotect
the page proactively when proper, from that POV Nadav's suggestion seems
slightly better on using "unprotect".

No strong opinion, the patch looks correct to me, and thanks for providing
the new test results,

Acked-by: Peter Xu <peterx@...hat.com>

-- 
Peter Xu