linux-kernel - Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect

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]

Date:   Wed, 20 Jul 2022 20:00:19 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Nadav Amit <namit@...are.com>
Cc:     Linux MM <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Xu <peterx@...hat.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>
Subject: Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on
 writeprotect

On 20.07.22 19:36, Nadav Amit wrote:
> On Jul 20, 2022, at 2:42 AM, David Hildenbrand <david@...hat.com> wrote:
> 
>> ⚠ External Email
>>
>> On 18.07.22 14:01, Nadav Amit wrote:
>>> From: Nadav Amit <namit@...are.com>
>>>
>>> When userfaultfd makes a PTE writable, it can now change the PTE
>>> directly, in some cases, without going triggering a page-fault first.
>>> Yet, doing so might leave the PTE that was write-unprotected as old and
>>> clean. At least on x86, this would cause a >500 cycles overhead when the
>>> PTE is first accessed.
>>>
>>> Use MM_CP_WILL_NEED to set the PTE as young and dirty when userfaultfd
>>> gets a hint that the page is likely to be used. Avoid changing the PTE
>>> to young and dirty in other cases to avoid excessive writeback and
>>> messing with the page reclamation logic.
>>>
>>> Cc: Andrea Arcangeli <aarcange@...hat.com>
>>> Cc: Andrew Cooper <andrew.cooper3@...rix.com>
>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>> Cc: Andy Lutomirski <luto@...nel.org>
>>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>>> Cc: David Hildenbrand <david@...hat.com>
>>> Cc: Peter Xu <peterx@...hat.com>
>>> Cc: Peter Zijlstra <peterz@...radead.org>
>>> Cc: Thomas Gleixner <tglx@...utronix.de>
>>> Cc: Will Deacon <will@...nel.org>
>>> Cc: Yu Zhao <yuzhao@...gle.com>
>>> Cc: Nick Piggin <npiggin@...il.com>
>>> ---
>>> include/linux/mm.h | 2 ++
>>> mm/mprotect.c | 9 ++++++++-
>>> mm/userfaultfd.c | 8 ++++++--
>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>> index 9cc02a7e503b..4afd75ce5875 100644
>>> --- a/include/linux/mm.h
>>> +++ b/include/linux/mm.h
>>> @@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
>>> /* Whether this change is for write protecting */
>>> #define MM_CP_UFFD_WP (1UL << 2) /* do wp */
>>> #define MM_CP_UFFD_WP_RESOLVE (1UL << 3) /* Resolve wp */
>>> +/* Whether to try to mark entries as dirty as they are to be written */
>>> +#define MM_CP_WILL_NEED (1UL << 4)
>>> #define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \
>>> MM_CP_UFFD_WP_RESOLVE)
>>>
>>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>>> index 996a97e213ad..34c2dfb68c42 100644
>>> --- a/mm/mprotect.c
>>> +++ b/mm/mprotect.c
>>> @@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
>>> bool prot_numa = cp_flags & MM_CP_PROT_NUMA;
>>> bool uffd_wp = cp_flags & MM_CP_UFFD_WP;
>>> bool uffd_wp_resolve = cp_flags & MM_CP_UFFD_WP_RESOLVE;
>>> + bool will_need = cp_flags & MM_CP_WILL_NEED;
>>>
>>> tlb_change_page_size(tlb, PAGE_SIZE);
>>>
>>> @@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
>>> ptent = pte_clear_uffd_wp(ptent);
>>> }
>>>
>>> + if (will_need)
>>> + ptent = pte_mkyoung(ptent);
>>> +
>>> /*
>>> * In some writable, shared mappings, we might want
>>> * to catch actual write access -- see
>>> @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
>>> */
>>> if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) &&
>>> !pte_write(ptent) &&
>>
>>
>> Why would we want to check if we can set something writable if it
>> already *is* writable? That doesn't make sense to me.
> 
> We check !pte_write(). What am I missing in your question?

My patch review skills have seen better days. I thought you'd be
removing the pte_write() check ... :( Tired eyes ...

> 
> Having said that, I do notice now that pte_mkdirty() should not be done
> only this condition is fulfilled. Instead we should just have
> something like:
> 
>                        if (will_need) {
>                                ptent = pte_mkyoung(ptent);
>                                if (pte_write(ptent))
>                                        ptent = pte_mkdirty(ptent);
>                        }

As can_change_pte_writable() will fail if it stumbles over a !pte_dirty
page in current code ... so I assume you would have that code before the
actual pte_mkwrite() logic, correct?

-- 
Thanks,

David / dhildenb