linux-kernel - Re: [RFC 03/20] mm/mprotect: do not flush on permission promotion

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: <e7ed0b21-f63d-21fc-f519-cc2cf0d465a2@citrix.com>
Date:   Sun, 31 Jan 2021 11:30:08 +0000
From:   Andrew Cooper <andrew.cooper3@...rix.com>
To:     Andy Lutomirski <luto@...nel.org>,
        Nadav Amit <nadav.amit@...il.com>
CC:     Linux-MM <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Nick Piggin <npiggin@...il.com>, X86 ML <x86@...nel.org>
Subject: Re: [RFC 03/20] mm/mprotect: do not flush on permission promotion

On 31/01/2021 02:59, Andy Lutomirski wrote:
>>>> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
>>>> index 8c87a2e0b660..a617dc0a9b06 100644
>>>> --- a/arch/x86/include/asm/tlbflush.h
>>>> +++ b/arch/x86/include/asm/tlbflush.h
>>>> @@ -255,6 +255,50 @@ static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,
>>>>
>>>> extern void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch);
>>>>
>>>> +static inline bool pte_may_need_flush(pte_t oldpte, pte_t newpte)
>>>> +{
>>>> +       const pteval_t ignore_mask = _PAGE_SOFTW1 | _PAGE_SOFTW2 |
>>>> +                                    _PAGE_SOFTW3 | _PAGE_ACCESSED;
>>> Why is accessed ignored?  Surely clearing the accessed bit needs a
>>> flush if the old PTE is present.
>> I am just following the current scheme in the kernel (x86):
>>
>> int ptep_clear_flush_young(struct vm_area_struct *vma,
>>                            unsigned long address, pte_t *ptep)
>> {
>>         /*
>>          * On x86 CPUs, clearing the accessed bit without a TLB flush
>>          * doesn't cause data corruption. [ It could cause incorrect
>>          * page aging and the (mistaken) reclaim of hot pages, but the
>>          * chance of that should be relatively low. ]
>>          *
> If anyone ever implements the optimization of skipping the flush when
> unmapping a !accessed page, then this will cause data corruption.
>
> If nothing else, this deserves a nice comment in the new code.

Architecturally, access bits get as part of a pagewalk, and before the
TLB entry is made.  Once an access bit has been set, you know for
certain that a mapping for the address is, or was, present in a TLB
somewhere.

I can't help but feel that this "optimisation" is shooting itself in the
foot.  It does cause incorrect page aging, and TLBs are getting bigger,
so the chance of reclamating hot pages is getting worse and worse.

~Andrew