lkml.org 
[lkml]   [2022]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v1 1/2] mm/userfaultfd: rely on vma->vm_page_prot in uffd_wp_range()
    From
    On 23.12.22 16:56, David Hildenbrand wrote:
    > uffd_wp_range() currently calculates page protection manually using
    > vm_get_page_prot(). This will ignore any other reason for active
    > writenotify: one mechanism applicable to shmem is softdirty tracking.
    >
    > For example, the following sequence
    >
    > 1) Write to mapped shmem page
    > 2) Clear softdirty
    > 3) Register uffd-wp covering the mapped page
    > 4) Unregister uffd-wp covering the mapped page
    > 5) Write to page again
    >
    > will not set the modified page softdirty, because uffd_wp_range() will
    > ignore that writenotify is required for softdirty tracking and simply map
    > the page writable again using change_protection(). Similarly, instead of
    > unregistering, protecting followed by un-protecting the page using
    > uffd-wp would result in the same situation.
    >
    > Now that we enable writenotify whenever enabling uffd-wp on a VMA,
    > vma->vm_page_prot will already properly reflect our requirements: the
    > default is to write-protect all PTEs. However, for shared mappings we
    > would now not remap the PTEs writable if possible when unprotecting, just
    > like for private mappings (COW). To compensate, set
    > MM_CP_TRY_CHANGE_WRITABLE just like mprotect() does to try mapping
    > individual PTEs writable.
    >
    > For private mappings, this change implies that we will now always try
    > setting PTEs writable when un-protecting, just like when upgrading write
    > permissions using mprotect(), which is an improvement.
    >
    > For shared mappings, we will only set PTEs writable if
    > can_change_pte_writable()/can_change_pmd_writable() indicates that it's
    > ok. For ordinary shmem, this will be the case when PTEs are dirty, which
    > should usually be the case -- otherwise we could special-case shmem in
    > can_change_pte_writable()/can_change_pmd_writable() easily, because
    > shmem itself doesn't require writenotify.
    >
    > Note that hugetlb does not yet implement MM_CP_TRY_CHANGE_WRITABLE, so we
    > won't try setting PTEs writable when unprotecting or when unregistering
    > uffd-wp. This can be added later on top by implementing
    > MM_CP_TRY_CHANGE_WRITABLE.
    >
    > While commit ffd05793963a ("userfaultfd: wp: support write protection for
    > userfault vma range") introduced that code, it should only be applicable
    > to uffd-wp on shared mappings -- shmem (hugetlb does not support softdirty
    > tracking). I don't think this corner cases justifies to cc stable. Let's
    > just handle it correctly and prepare for change_protection() cleanups.
    >
    > Fixes: b1f9e876862d ("mm/uffd: enable write protection for shmem & hugetlbfs")
    > Signed-off-by: David Hildenbrand <david@redhat.com>
    > ---
    > mm/userfaultfd.c | 18 +++++++++++++-----
    > 1 file changed, 13 insertions(+), 5 deletions(-)
    >
    > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
    > index 0499907b6f1a..351e8d6b398b 100644
    > --- a/mm/userfaultfd.c
    > +++ b/mm/userfaultfd.c
    > @@ -727,17 +727,25 @@ ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,
    > void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *dst_vma,
    > unsigned long start, unsigned long len, bool enable_wp)
    > {
    > + unsigned int mm_cp_flags;
    > struct mmu_gather tlb;
    > - pgprot_t newprot;
    >
    > if (enable_wp)
    > - newprot = vm_get_page_prot(dst_vma->vm_flags & ~(VM_WRITE));
    > + mm_cp_flags = MM_CP_UFFD_WP;
    > else
    > - newprot = vm_get_page_prot(dst_vma->vm_flags);
    > + mm_cp_flags = MM_CP_UFFD_WP_RESOLVE;
    >
    > + /*
    > + * vma->vm_page_prot already reflects that uffd-wp is enabled for this
    > + * VMA (see userfaultfd_set_vm_flags()) and that all PTEs are supposed
    > + * to be write-protected as default whenever protection changes.
    > + * Try upgrading write permissions manually.
    > + */
    > + if (vma_wants_manual_pte_write_upgrade(dst_vma))
    > + mm_cp_flags |= MM_CP_TRY_CHANGE_WRITABLE;
    > tlb_gather_mmu(&tlb, dst_mm);
    > - change_protection(&tlb, dst_vma, start, start + len, newprot,
    > - enable_wp ? MM_CP_UFFD_WP : MM_CP_UFFD_WP_RESOLVE);
    > + change_protection(&tlb, dst_vma, start, start + len, vma->vm_page_prot,
    > + mm_cp_flags);
    > tlb_finish_mmu(&tlb);
    > }
    >

    The following optimization makes sense:

    From 779b36768328d99dbcc96fbba7be8b50536ce350 Mon Sep 17 00:00:00 2001
    From: David Hildenbrand <david@redhat.com>
    Date: Sat, 24 Dec 2022 15:02:36 +0100
    Subject: [PATCH] fixup: mm/userfaultfd: enable writenotify while
    userfaultfd-wp is enabled for a VMA

    No need for additional harmless checks if we're wr-protecting either way.

    Signed-off-by: David Hildenbrand <david@redhat.com>
    ---
    mm/userfaultfd.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
    index be7ee9d82e72..1ac1de527719 100644
    --- a/mm/userfaultfd.c
    +++ b/mm/userfaultfd.c
    @@ -741,7 +741,7 @@ void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *dst_vma,
    * to be write-protected as default whenever protection changes.
    * Try upgrading write permissions manually.
    */
    - if (vma_wants_manual_pte_write_upgrade(dst_vma))
    + if (!enable_wp && vma_wants_manual_pte_write_upgrade(dst_vma))
    mm_cp_flags |= MM_CP_TRY_CHANGE_WRITABLE;
    tlb_gather_mmu(&tlb, dst_mm);
    change_protection(&tlb, dst_vma, start, start + len, mm_cp_flags);
    --
    2.38.1

    --
    Thanks,

    David / dhildenb

    \
     
     \ /
      Last update: 2023-03-26 23:19    [W:3.165 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site